Merge pull request #4184 from aschackmull/java/cleanup-queryinjection

Approved by aibaars

java/ql/src/Security/CWE/CWE-089/SqlInjectionLib.qll

@@ -4,22 +4,6 @@ import java
 import semmle.code.java.dataflow.FlowSources
 import semmle.code.java.security.QueryInjection
 
-/** A sink for MongoDB injection vulnerabilities. */
-class MongoDbInjectionSink extends QueryInjectionSink {
-  MongoDbInjectionSink() {
-    exists(MethodAccess call |
-      call.getMethod().getDeclaringType().hasQualifiedName("com.mongodb", "BasicDBObject") and
-      call.getMethod().hasName("parse") and
-      this.asExpr() = call.getArgument(0)
-    )
-    or
-    exists(CastExpr c |
-      c.getExpr() = this.asExpr() and
-      c.getTypeExpr().getType().(RefType).hasQualifiedName("com.mongodb", "DBObject")
-    )
-  }
-}
-
 private class QueryInjectionFlowConfig extends TaintTracking::Configuration {
   QueryInjectionFlowConfig() { this = "SqlInjectionLib::QueryInjectionFlowConfig" }
 
@@ -34,7 +18,7 @@ private class QueryInjectionFlowConfig extends TaintTracking::Configuration {
   }
 
   override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) {
-    mongoJsonStep(node1, node2)
+    any(AdditionalQueryInjectionTaintStep s).step(node1, node2)
   }
 }
 
@@ -47,12 +31,3 @@ predicate queryTaintedBy(
 ) {
   exists(QueryInjectionFlowConfig conf | conf.hasFlowPath(source, sink) and sink.getNode() = query)
 }
-
-predicate mongoJsonStep(DataFlow::Node node1, DataFlow::Node node2) {
-  exists(MethodAccess ma |
-    ma.getMethod().getDeclaringType().hasQualifiedName("com.mongodb.util", "JSON") and
-    ma.getMethod().hasName("parse") and
-    ma.getArgument(0) = node1.asExpr() and
-    ma = node2.asExpr()
-  )
-}

java/ql/src/Security/CWE/CWE-089/SqlTaintedLocal.ql

@@ -27,7 +27,7 @@ class LocalUserInputToQueryInjectionFlowConfig extends TaintTracking::Configurat
   }
 
   override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) {
-    mongoJsonStep(node1, node2)
+    any(AdditionalQueryInjectionTaintStep s).step(node1, node2)
   }
 }
 

java/ql/src/java.qll

@@ -3,6 +3,7 @@
 import Customizations
 import semmle.code.FileSystem
 import semmle.code.Location
+import semmle.code.Unit
 import semmle.code.java.Annotation
 import semmle.code.java.CompilationUnit
 import semmle.code.java.ControlFlowGraph

java/ql/src/semmle/code/Unit.qll

@@ -0,0 +1,10 @@
+/** Provides the `Unit` class. */
+
+/** The unit type. */
+private newtype TUnit = TMkUnit()
+
+/** The trivial type with a single element. */
+class Unit extends TUnit {
+  /** Gets a textual representation of this element. */
+  string toString() { result = "unit" }
+}

java/ql/src/semmle/code/java/dataflow/internal/TaintTrackingUtil.qll

@@ -54,12 +54,6 @@ predicate localAdditionalTaintStep(DataFlow::Node src, DataFlow::Node sink) {
   )
 }
 
-private newtype TUnit = TMkUnit()
-
-class Unit extends TUnit {
-  string toString() { result = "unit" }
-}
-
 /**
  * A unit class for adding additional taint steps.
  *

java/ql/src/semmle/code/java/security/LdapInjection.qll

@@ -18,7 +18,7 @@ abstract class LdapInjectionSanitizer extends DataFlow::Node { }
  *
  * Extend this class to add additional taint steps that should apply to the `LdapInjectionFlowConfig`.
  */
-class LdapInjectionAdditionalTaintStep extends TaintTracking::Unit {
+class LdapInjectionAdditionalTaintStep extends Unit {
   /**
    * Holds if the step from `node1` to `node2` should be considered a taint
    * step for the `LdapInjectionFlowConfig` configuration.

java/ql/src/semmle/code/java/security/QueryInjection.qll

@@ -13,6 +13,20 @@ import semmle.code.java.frameworks.Hibernate
 /** A sink for database query language injection vulnerabilities. */
 abstract class QueryInjectionSink extends DataFlow::Node { }
 
+/**
+ * A unit class for adding additional taint steps.
+ *
+ * Extend this class to add additional taint steps that should apply to the SQL
+ * injection taint configuration.
+ */
+class AdditionalQueryInjectionTaintStep extends Unit {
+  /**
+   * Holds if the step from `node1` to `node2` should be considered a taint
+   * step for SQL injection taint configurations.
+   */
+  abstract predicate step(DataFlow::Node node1, DataFlow::Node node2);
+}
+
 /** A sink for SQL injection vulnerabilities. */
 private class SqlInjectionSink extends QueryInjectionSink {
   SqlInjectionSink() {
@@ -49,3 +63,30 @@ private class PersistenceQueryInjectionSink extends QueryInjectionSink {
     )
   }
 }
+
+/** A sink for MongoDB injection vulnerabilities. */
+private class MongoDbInjectionSink extends QueryInjectionSink {
+  MongoDbInjectionSink() {
+    exists(MethodAccess call |
+      call.getMethod().getDeclaringType().hasQualifiedName("com.mongodb", "BasicDBObject") and
+      call.getMethod().hasName("parse") and
+      this.asExpr() = call.getArgument(0)
+    )
+    or
+    exists(CastExpr c |
+      c.getExpr() = this.asExpr() and
+      c.getTypeExpr().getType().(RefType).hasQualifiedName("com.mongodb", "DBObject")
+    )
+  }
+}
+
+private class MongoJsonStep extends AdditionalQueryInjectionTaintStep {
+  override predicate step(DataFlow::Node node1, DataFlow::Node node2) {
+    exists(MethodAccess ma |
+      ma.getMethod().getDeclaringType().hasQualifiedName("com.mongodb.util", "JSON") and
+      ma.getMethod().hasName("parse") and
+      ma.getArgument(0) = node1.asExpr() and
+      ma = node2.asExpr()
+    )
+  }
+}

java/ql/src/semmle/code/java/security/XSS.qll

@@ -20,7 +20,7 @@ abstract class XssSanitizer extends DataFlow::Node { }
  * Extend this class to add additional taint steps that should apply to the XSS
  * taint configuration.
  */
-abstract class XssAdditionalTaintStep extends TaintTracking2::Unit {
+class XssAdditionalTaintStep extends Unit {
   /**
    * Holds if the step from `node1` to `node2` should be considered a taint
    * step for XSS taint configurations.