Merge pull request #1 from smowton/smowton/admin/rewrite-xquery

Rewrite XQuery injection to use an additional taint step instead of multiple configurations

Author: haby0

java/ql/src/Security/CWE/CWE-652/XQueryInjection.ql

@@ -15,25 +15,21 @@ import semmle.code.java.dataflow.FlowSources
 import XQueryInjectionLib
 import DataFlow::PathGraph
 
-class XQueryInjectionConfig extends DataFlow::Configuration {
+class XQueryInjectionConfig extends TaintTracking::Configuration {
   XQueryInjectionConfig() { this = "XQueryInjectionConfig" }
 
-  override predicate isSource(DataFlow::Node source) { source instanceof XQueryInjectionSource }
+  override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
 
-  override predicate isSink(DataFlow::Node sink) { sink instanceof XQueryInjectionSink }
+  override predicate isSink(DataFlow::Node sink) {
+    sink.asExpr() = any(XQueryExecuteCall execute).getPreparedExpression()
+  }
 
-  override predicate isBarrier(DataFlow::Node node) {
-    exists(MethodAccess ma, Method m, BindParameterRemoteFlowConf conf, DataFlow::Node node1 |
-      m = ma.getMethod()
-    |
-      node.asExpr() = ma and
-      m.hasName("bindString") and
-      m.getDeclaringType()
-          .getASourceSupertype*()
-          .hasQualifiedName("javax.xml.xquery", "XQDynamicContext") and
-      ma.getArgument(1) = node1.asExpr() and
-      conf.hasFlowTo(node1)
-    )
+  /**
+   * Conveys taint from the input to a `prepareExpression` call to the returned prepared expression.
+   */
+  override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
+    exists(XQueryParserCall parser |
+      pred.asExpr() = parser.getInput() and succ.asExpr() = parser)
   }
 }
 

java/ql/src/Security/CWE/CWE-652/XQueryInjectionLib.qll

@@ -1,7 +1,4 @@
 import java
-import semmle.code.java.dataflow.FlowSources
-import semmle.code.java.dataflow.TaintTracking2
-import DataFlow::PathGraph
 
 /** A call to `XQConnection.prepareExpression`. */
 class XQueryParserCall extends MethodAccess {
@@ -14,77 +11,25 @@ class XQueryParserCall extends MethodAccess {
       m.hasName("prepareExpression")
     )
   }
-  /** Returns the first parameter of the `bindString` method. */
-  Expr getInput() { result = this.getArgument(0) }
-}
-
-/** A call to `XQDynamicContext.bindString`. */
-class XQueryBindStringCall extends MethodAccess {
-  XQueryBindStringCall() {
-    exists(Method m |
-      this.getMethod() = m and
-      m.getDeclaringType()
-          .getASourceSupertype*()
-          .hasQualifiedName("javax.xml.xquery", "XQDynamicContext") and
-      m.hasName("bindString")
-    )
-  }
-  /** Returns the second parameter of the `bindString` method. */
-  Expr getInput() { result = this.getArgument(1) }
-}
-
-/** Used to determine whether to call the `prepareExpression` method, and the first parameter value can be remotely controlled. */
-class ParserParameterRemoteFlowConf extends TaintTracking2::Configuration {
-  ParserParameterRemoteFlowConf() { this = "ParserParameterRemoteFlowConf" }
 
-  override predicate isSource(DataFlow::Node src) { src instanceof RemoteFlowSource }
-
-  override predicate isSink(DataFlow::Node sink) {
-    exists(XQueryParserCall xqpc | xqpc.getSink() = sink.asExpr())
-  }
-}
-
-/** Used to determine whether to call the `bindString` method, and the second parameter value can be controlled remotely. */
-class BindParameterRemoteFlowConf extends TaintTracking2::Configuration {
-  BindParameterRemoteFlowConf() { this = "BindParameterRemoteFlowConf" }
-
-  override predicate isSource(DataFlow::Node src) { src instanceof RemoteFlowSource }
-
-  override predicate isSink(DataFlow::Node sink) {
-    exists(XQueryBindStringCall xqbsc | xqbsc.getSink() = sink.asExpr())
-  }
+  /**
+   * Returns the first parameter of the `prepareExpression` method, which provides
+   * the string, stream or reader to be compiled into a prepared expression.
+   */
+  Expr getInput() { result = this.getArgument(0) }
 }
 
-/**
- * A data flow source for XQuery injection vulnerability.
- *  1. `prepareExpression` call as sink.
- *  2. Determine whether the `var1` parameter of `prepareExpression` method can be controlled remotely.
- */
-class XQueryInjectionSource extends DataFlow::ExprNode {
-  XQueryInjectionSource() {
-    exists(MethodAccess ma, Method m, ParserParameterRemoteFlowConf conf, DataFlow::Node node |
-      m = ma.getMethod()
-    |
-      m.hasName("prepareExpression") and
-      m.getDeclaringType()
-          .getASourceSupertype*()
-          .hasQualifiedName("javax.xml.xquery", "XQConnection") and
-      asExpr() = ma and
-      node.asExpr() = ma.getArgument(0) and
-      conf.hasFlowTo(node)
+/** A call to `XQPreparedExpression.executeQuery`. */
+class XQueryExecuteCall extends MethodAccess {
+  XQueryExecuteCall() {
+    exists(Method m | this.getMethod() = m and
+    m.hasName("executeQuery") and
+    m.getDeclaringType()
+        .getASourceSupertype*()
+        .hasQualifiedName("javax.xml.xquery", "XQPreparedExpression")
     )
   }
-}
 
-/** A data flow sink for XQuery injection vulnerability. */
-class XQueryInjectionSink extends DataFlow::Node {
-  XQueryInjectionSink() {
-    exists(MethodAccess ma, Method m | m = ma.getMethod() |
-      m.hasName("executeQuery") and
-      m.getDeclaringType()
-          .getASourceSupertype*()
-          .hasQualifiedName("javax.xml.xquery", "XQPreparedExpression") and
-      asExpr() = ma.getQualifier()
-    )
-  }
+  /** Return this prepared expression. */
+  Expr getPreparedExpression() { result = this.getQualifier() }
 }