From 0d5f9113a307371f5cdb14cea25fe631ebf884ba Mon Sep 17 00:00:00 2001
From: Remco Vermeulen <rvermeulen@users.noreply.github.com>
Date: Tue, 21 Jul 2020 12:36:18 +0000
Subject: [PATCH 01/10] Extract ldap injection sink into importable library

---
 .../Security/CWE/CWE-090/LdapInjectionLib.qll |  80 +------------
 .../code/java/security/LdapInjection.qll      | 107 ++++++++++++++++++
 2 files changed, 108 insertions(+), 79 deletions(-)
 create mode 100644 java/ql/src/semmle/code/java/security/LdapInjection.qll

diff --git a/java/ql/src/Security/CWE/CWE-090/LdapInjectionLib.qll b/java/ql/src/Security/CWE/CWE-090/LdapInjectionLib.qll
index d2131dadb80..a603b2bad5e 100644
--- a/java/ql/src/Security/CWE/CWE-090/LdapInjectionLib.qll
+++ b/java/ql/src/Security/CWE/CWE-090/LdapInjectionLib.qll
@@ -1,10 +1,7 @@
 import java
 import semmle.code.java.dataflow.FlowSources
 import DataFlow
-import semmle.code.java.frameworks.Jndi
-import semmle.code.java.frameworks.UnboundId
-import semmle.code.java.frameworks.SpringLdap
-import semmle.code.java.frameworks.ApacheLdap
+import semmle.code.java.security.LdapInjection
 
 /**
  * A taint-tracking configuration for unvalidated user input that is used to construct LDAP queries.
@@ -44,81 +41,6 @@ class LdapInjectionFlowConfig extends TaintTracking::Configuration {
   }
 }
 
-/**
- * JNDI sink for LDAP injection vulnerabilities, i.e. 1st (DN) or 2nd (filter) argument to
- * `search` method from `DirContext`.
- */
-predicate jndiLdapInjectionSinkMethod(Method m, int index) {
-  m.getDeclaringType().getAnAncestor() instanceof TypeDirContext and
-  m.hasName("search") and
-  index in [0 .. 1]
-}
-
-/**
- * UnboundID sink for LDAP injection vulnerabilities,
- * i.e. LDAPConnection.search, LDAPConnection.asyncSearch or LDAPConnection.searchForEntry method.
- */
-predicate unboundIdLdapInjectionSinkMethod(Method m, int index) {
-  exists(Parameter param | m.getParameter(index) = param and not param.isVarargs() |
-    m instanceof MethodUnboundIdLDAPConnectionSearch or
-    m instanceof MethodUnboundIdLDAPConnectionAsyncSearch or
-    m instanceof MethodUnboundIdLDAPConnectionSearchForEntry
-  )
-}
-
-/**
- * Spring LDAP sink for LDAP injection vulnerabilities,
- * i.e. LdapTemplate.authenticate, LdapTemplate.find* or LdapTemplate.search* method.
- */
-predicate springLdapInjectionSinkMethod(Method m, int index) {
-  // LdapTemplate.authenticate, LdapTemplate.find* or LdapTemplate.search* method
-  (
-    m instanceof MethodSpringLdapTemplateAuthenticate or
-    m instanceof MethodSpringLdapTemplateFind or
-    m instanceof MethodSpringLdapTemplateFindOne or
-    m instanceof MethodSpringLdapTemplateSearch or
-    m instanceof MethodSpringLdapTemplateSearchForContext or
-    m instanceof MethodSpringLdapTemplateSearchForObject
-  ) and
-  (
-    // Parameter index is 1 (DN or query) or 2 (filter) if method is not authenticate
-    index in [0 .. 1] and
-    not m instanceof MethodSpringLdapTemplateAuthenticate
-    or
-    // But it's not the last parameter in case of authenticate method (last param is password)
-    index in [0 .. 1] and
-    index < m.getNumberOfParameters() - 1 and
-    m instanceof MethodSpringLdapTemplateAuthenticate
-  )
-}
-
-/** Apache LDAP API sink for LDAP injection vulnerabilities, i.e. LdapConnection.search method. */
-predicate apacheLdapInjectionSinkMethod(Method m, int index) {
-  exists(Parameter param | m.getParameter(index) = param and not param.isVarargs() |
-    m.getDeclaringType().getAnAncestor() instanceof TypeApacheLdapConnection and
-    m.hasName("search")
-  )
-}
-
-/** Holds if parameter at index `index` in method `m` is LDAP injection sink. */
-predicate ldapInjectionSinkMethod(Method m, int index) {
-  jndiLdapInjectionSinkMethod(m, index) or
-  unboundIdLdapInjectionSinkMethod(m, index) or
-  springLdapInjectionSinkMethod(m, index) or
-  apacheLdapInjectionSinkMethod(m, index)
-}
-
-/** A data flow sink for unvalidated user input that is used to construct LDAP queries. */
-class LdapInjectionSink extends DataFlow::ExprNode {
-  LdapInjectionSink() {
-    exists(MethodAccess ma, Method m, int index |
-      ma.getMethod() = m and
-      ma.getArgument(index) = this.getExpr() and
-      ldapInjectionSinkMethod(m, index)
-    )
-  }
-}
-
 /**
  * Holds if `n1` to `n2` is a dataflow step that converts between `String` and `LdapName`,
  * i.e. `new LdapName(tainted)`.
diff --git a/java/ql/src/semmle/code/java/security/LdapInjection.qll b/java/ql/src/semmle/code/java/security/LdapInjection.qll
new file mode 100644
index 00000000000..6c1de4c8617
--- /dev/null
+++ b/java/ql/src/semmle/code/java/security/LdapInjection.qll
@@ -0,0 +1,107 @@
+/** Provides classes to reason about LDAP injection attacks. */
+
+import java
+import semmle.code.java.dataflow.DataFlow
+import semmle.code.java.frameworks.Jndi
+import semmle.code.java.frameworks.UnboundId
+import semmle.code.java.frameworks.SpringLdap
+import semmle.code.java.frameworks.ApacheLdap
+
+/** A data flow sink for unvalidated user input that is used to construct LDAP queries. */
+abstract class LdapInjectionSink extends DataFlow::Node { }
+
+private predicate jndiLdapInjectionSinkMethod(Method m, int index) {
+  m.getDeclaringType().getAnAncestor() instanceof TypeDirContext and
+  m.hasName("search") and
+  index in [0 .. 1]
+}
+
+/**
+ * JNDI sink for LDAP injection vulnerabilities, i.e. 1st (DN) or 2nd (filter) argument to
+ * `search` method from `DirContext`.
+ */
+private class JndiLdapInjectionSink extends LdapInjectionSink {
+  JndiLdapInjectionSink() {
+    exists(MethodAccess ma, Method m, int index |
+      ma.getMethod() = m and
+      ma.getArgument(index) = this.asExpr() and
+      jndiLdapInjectionSinkMethod(m, index)
+    )
+  }
+}
+
+private predicate unboundIdLdapInjectionSinkMethod(Method m, int index) {
+  exists(Parameter param | m.getParameter(index) = param and not param.isVarargs() |
+    m instanceof MethodUnboundIdLDAPConnectionSearch or
+    m instanceof MethodUnboundIdLDAPConnectionAsyncSearch or
+    m instanceof MethodUnboundIdLDAPConnectionSearchForEntry
+  )
+}
+
+/**
+ * UnboundID sink for LDAP injection vulnerabilities,
+ * i.e. LDAPConnection.search, LDAPConnection.asyncSearch or LDAPConnection.searchForEntry method.
+ */
+private class UnboundedIdLdapInjectionSink extends LdapInjectionSink {
+  UnboundedIdLdapInjectionSink() {
+    exists(MethodAccess ma, Method m, int index |
+      ma.getMethod() = m and
+      ma.getArgument(index) = this.asExpr() and
+      unboundIdLdapInjectionSinkMethod(m, index)
+    )
+  }
+}
+
+private predicate springLdapInjectionSinkMethod(Method m, int index) {
+  // LdapTemplate.authenticate, LdapTemplate.find* or LdapTemplate.search* method
+  (
+    m instanceof MethodSpringLdapTemplateAuthenticate or
+    m instanceof MethodSpringLdapTemplateFind or
+    m instanceof MethodSpringLdapTemplateFindOne or
+    m instanceof MethodSpringLdapTemplateSearch or
+    m instanceof MethodSpringLdapTemplateSearchForContext or
+    m instanceof MethodSpringLdapTemplateSearchForObject
+  ) and
+  (
+    // Parameter index is 1 (DN or query) or 2 (filter) if method is not authenticate
+    index in [0 .. 1] and
+    not m instanceof MethodSpringLdapTemplateAuthenticate
+    or
+    // But it's not the last parameter in case of authenticate method (last param is password)
+    index in [0 .. 1] and
+    index < m.getNumberOfParameters() - 1 and
+    m instanceof MethodSpringLdapTemplateAuthenticate
+  )
+}
+
+/**
+ * Spring LDAP sink for LDAP injection vulnerabilities,
+ * i.e. LdapTemplate.authenticate, LdapTemplate.find* or LdapTemplate.search* method.
+ */
+private class SpringLdapInjectionSink extends LdapInjectionSink {
+  SpringLdapInjectionSink() {
+    exists(MethodAccess ma, Method m, int index |
+      ma.getMethod() = m and
+      ma.getArgument(index) = this.asExpr() and
+      springLdapInjectionSinkMethod(m, index)
+    )
+  }
+}
+
+private predicate apacheLdapInjectionSinkMethod(Method m, int index) {
+  exists(Parameter param | m.getParameter(index) = param and not param.isVarargs() |
+    m.getDeclaringType().getAnAncestor() instanceof TypeApacheLdapConnection and
+    m.hasName("search")
+  )
+}
+
+/** Apache LDAP API sink for LDAP injection vulnerabilities, i.e. LdapConnection.search method. */
+private class ApacheLdapInjectionSink extends LdapInjectionSink {
+  ApacheLdapInjectionSink() {
+    exists(MethodAccess ma, Method m, int index |
+      ma.getMethod() = m and
+      ma.getArgument(index) = this.asExpr() and
+      apacheLdapInjectionSinkMethod(m, index)
+    )
+  }
+}

From 57e7411c0a8d52607fce91419c366f658791b924 Mon Sep 17 00:00:00 2001
From: Remco Vermeulen <rvermeulen@users.noreply.github.com>
Date: Tue, 21 Jul 2020 14:51:58 +0200
Subject: [PATCH 02/10] Extract Ldap injection sanitizers to importable lib

This includes a new abstract class that represents all the Ldap injection
santizers and can be used to add additional santizers through
extension.
---
 .../src/Security/CWE/CWE-090/LdapInjectionLib.qll   |  4 +---
 .../src/semmle/code/java/security/LdapInjection.qll | 13 +++++++++++++
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/java/ql/src/Security/CWE/CWE-090/LdapInjectionLib.qll b/java/ql/src/Security/CWE/CWE-090/LdapInjectionLib.qll
index a603b2bad5e..759bbeea1af 100644
--- a/java/ql/src/Security/CWE/CWE-090/LdapInjectionLib.qll
+++ b/java/ql/src/Security/CWE/CWE-090/LdapInjectionLib.qll
@@ -13,9 +13,7 @@ class LdapInjectionFlowConfig extends TaintTracking::Configuration {
 
   override predicate isSink(DataFlow::Node sink) { sink instanceof LdapInjectionSink }
 
-  override predicate isSanitizer(DataFlow::Node node) {
-    node.getType() instanceof PrimitiveType or node.getType() instanceof BoxedType
-  }
+  override predicate isSanitizer(DataFlow::Node node) { node instanceof LdapInjectionSanitizer }
 
   override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) {
     ldapNameStep(node1, node2) or
diff --git a/java/ql/src/semmle/code/java/security/LdapInjection.qll b/java/ql/src/semmle/code/java/security/LdapInjection.qll
index 6c1de4c8617..ef764c9c14b 100644
--- a/java/ql/src/semmle/code/java/security/LdapInjection.qll
+++ b/java/ql/src/semmle/code/java/security/LdapInjection.qll
@@ -10,6 +10,9 @@ import semmle.code.java.frameworks.ApacheLdap
 /** A data flow sink for unvalidated user input that is used to construct LDAP queries. */
 abstract class LdapInjectionSink extends DataFlow::Node { }
 
+/** A class that identifies sanitizers that prevent LDAP injection attacks. */
+abstract class LdapInjectionSanitizer extends DataFlow::Node { }
+
 private predicate jndiLdapInjectionSinkMethod(Method m, int index) {
   m.getDeclaringType().getAnAncestor() instanceof TypeDirContext and
   m.hasName("search") and
@@ -105,3 +108,13 @@ private class ApacheLdapInjectionSink extends LdapInjectionSink {
     )
   }
 }
+
+/** A sanitizer that clears the taint on primitive types. */
+private class PrimitiveTypeLdapSanitizer extends LdapInjectionSanitizer {
+  PrimitiveTypeLdapSanitizer() { this.getType() instanceof PrimitiveType }
+}
+
+/** A sanitizer that clears the taint on boxed primitive types. */
+private class BoxedTypeLdapSanitizer extends LdapInjectionSanitizer {
+  BoxedTypeLdapSanitizer() { this.getType() instanceof BoxedType }
+}

From 2c42d3cca5d0556d47a87ff62a58537598cd2a40 Mon Sep 17 00:00:00 2001
From: Remco Vermeulen <rvermeulen@users.noreply.github.com>
Date: Wed, 22 Jul 2020 14:52:08 +0200
Subject: [PATCH 03/10] Extract additional taint steps

This is done for logical cohesion. We already have the capability of
extending additional taint steps by extending
`TaintTracking::AdditionalTaintStep`.
---
 .../Security/CWE/CWE-090/LdapInjectionLib.qll | 309 -----------------
 .../code/java/security/LdapInjection.qll      | 313 +++++++++++++++++-
 2 files changed, 312 insertions(+), 310 deletions(-)

diff --git a/java/ql/src/Security/CWE/CWE-090/LdapInjectionLib.qll b/java/ql/src/Security/CWE/CWE-090/LdapInjectionLib.qll
index 759bbeea1af..7f95a8b6c1a 100644
--- a/java/ql/src/Security/CWE/CWE-090/LdapInjectionLib.qll
+++ b/java/ql/src/Security/CWE/CWE-090/LdapInjectionLib.qll
@@ -14,313 +14,4 @@ class LdapInjectionFlowConfig extends TaintTracking::Configuration {
   override predicate isSink(DataFlow::Node sink) { sink instanceof LdapInjectionSink }
 
   override predicate isSanitizer(DataFlow::Node node) { node instanceof LdapInjectionSanitizer }
-
-  override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) {
-    ldapNameStep(node1, node2) or
-    ldapNameAddAllStep(node1, node2) or
-    ldapNameGetCloneStep(node1, node2) or
-    filterStep(node1, node2) or
-    filterToStringStep(node1, node2) or
-    unboundIdSearchRequestStep(node1, node2) or
-    unboundIdSearchRequestDuplicateStep(node1, node2) or
-    unboundIdSearchRequestSetStep(node1, node2) or
-    ldapQueryStep(node1, node2) or
-    ldapQueryBaseStep(node1, node2) or
-    ldapQueryBuilderStep(node1, node2) or
-    hardcodedFilterStep(node1, node2) or
-    springLdapFilterToStringStep(node1, node2) or
-    ldapNameBuilderStep(node1, node2) or
-    ldapNameBuilderBuildStep(node1, node2) or
-    ldapUtilsStep(node1, node2) or
-    apacheSearchRequestStep(node1, node2) or
-    apacheSearchRequestGetStep(node1, node2) or
-    apacheLdapDnStep(node1, node2) or
-    apacheLdapDnGetStep(node1, node2)
-  }
-}
-
-/**
- * Holds if `n1` to `n2` is a dataflow step that converts between `String` and `LdapName`,
- * i.e. `new LdapName(tainted)`.
- */
-predicate ldapNameStep(ExprNode n1, ExprNode n2) {
-  exists(ConstructorCall cc | cc.getConstructedType() instanceof TypeLdapName |
-    n1.asExpr() = cc.getAnArgument() and
-    n2.asExpr() = cc
-  )
-}
-
-/**
- * Holds if `n1` to `n2` is a dataflow step that converts between `List<Rdn>` and `LdapName`,
- * i.e. `new LdapName().addAll(tainted)`.
- */
-predicate ldapNameAddAllStep(ExprNode n1, ExprNode n2) {
-  exists(MethodAccess ma |
-    n1.asExpr() = ma.getAnArgument() and
-    (n2.asExpr() = ma or n2.asExpr() = ma.getQualifier())
-  |
-    ma.getMethod() instanceof MethodLdapNameAddAll
-  )
-}
-
-/**
- * Holds if `n1` to `n2` is a dataflow step that converts between `LdapName` and `LdapName` or
- * `String`, i.e. `taintedLdapName.clone()`, `taintedLdapName.getAll()`,
- * `taintedLdapName.getRdns()` or `taintedLdapName.toString()`.
- */
-predicate ldapNameGetCloneStep(ExprNode n1, ExprNode n2) {
-  exists(MethodAccess ma, Method m |
-    n1.asExpr() = ma.getQualifier() and
-    n2.asExpr() = ma and
-    ma.getMethod() = m
-  |
-    m instanceof MethodLdapNameClone or
-    m instanceof MethodLdapNameGetAll or
-    m instanceof MethodLdapNameGetRdns or
-    m instanceof MethodLdapNameToString
-  )
-}
-
-/**
- * Holds if `n1` to `n2` is a dataflow step that converts between `String` and UnboundID `Filter`,
- * i.e. `Filter.create*(tainted)`.
- */
-predicate filterStep(ExprNode n1, ExprNode n2) {
-  exists(MethodAccess ma, Method m |
-    n1.asExpr() = ma.getAnArgument() and
-    n2.asExpr() = ma and
-    ma.getMethod() = m
-  |
-    m instanceof MethodUnboundIdFilterCreate or
-    m instanceof MethodUnboundIdFilterCreateANDFilter or
-    m instanceof MethodUnboundIdFilterCreateNOTFilter or
-    m instanceof MethodUnboundIdFilterCreateORFilter or
-    m instanceof MethodUnboundIdFilterSimplifyFilter
-  )
-}
-
-/**
- * Holds if `n1` to `n2` is a dataflow step that converts between UnboundID `Filter` and `String`,
- * i.e. `taintedFilter.toString()` or `taintedFilter.toString(buffer)`.
- */
-predicate filterToStringStep(ExprNode n1, ExprNode n2) {
-  exists(MethodAccess ma, Method m |
-    n1.asExpr() = ma.getQualifier() and
-    (n2.asExpr() = ma or n2.asExpr() = ma.getAnArgument())
-  |
-    ma.getMethod() = m and
-    m.getDeclaringType() instanceof TypeUnboundIdLdapFilter and
-    (m.hasName("toString") or m.hasName("toNormalizedString"))
-  )
-}
-
-/**
- * Holds if `n1` to `n2` is a dataflow step that converts between `String` and UnboundID
- * `SearchRequest`, i.e. `new SearchRequest(tainted)`.
- */
-predicate unboundIdSearchRequestStep(ExprNode n1, ExprNode n2) {
-  exists(ConstructorCall cc, int index, Parameter param |
-    cc.getConstructedType() instanceof TypeUnboundIdSearchRequest
-  |
-    n1.asExpr() = cc.getArgument(index) and
-    n2.asExpr() = cc and
-    cc.getConstructor().getParameter(index) = param and
-    not param.isVarargs()
-  )
-}
-
-/**
- * Holds if `n1` to `n2` is a dataflow step that converts between UnboundID `SearchRequest`
- * and UnboundID `SearchRequest`, i.e. `taintedSearchRequest.duplicate()`.
- */
-predicate unboundIdSearchRequestDuplicateStep(ExprNode n1, ExprNode n2) {
-  exists(MethodAccess ma, Method m | n1.asExpr() = ma.getQualifier() and n2.asExpr() = ma |
-    ma.getMethod() = m and
-    m.getDeclaringType().getAnAncestor() instanceof TypeUnboundIdReadOnlySearchRequest and
-    m.hasName("duplicate")
-  )
-}
-
-/**
- * Holds if `n1` to `n2` is a dataflow step that converts between DN or filter and UnboundID
- * `SearchRequest`, i.e. `searchRequest.setBaseDN(tainted)` or `searchRequest.setFilter(tainted)`.
- */
-predicate unboundIdSearchRequestSetStep(ExprNode n1, ExprNode n2) {
-  exists(MethodAccess ma, Method m |
-    n1.asExpr() = ma.getAnArgument() and
-    n2.asExpr() = ma.getQualifier() and
-    ma.getMethod() = m
-  |
-    m instanceof MethodUnboundIdSearchRequestSetBaseDN or
-    m instanceof MethodUnboundIdSearchRequestSetFilter
-  )
-}
-
-/**
- * Holds if `n1` to `n2` is a dataflow step that converts between `String` and Spring `LdapQuery`,
- * i.e. `LdapQueryBuilder.query().filter(tainted)` or `LdapQueryBuilder.query().base(tainted)`.
- */
-predicate ldapQueryStep(ExprNode n1, ExprNode n2) {
-  exists(MethodAccess ma, Method m, int index |
-    n1.asExpr() = ma.getArgument(index) and
-    n2.asExpr() = ma and
-    ma.getMethod() = m and
-    index = 0
-  |
-    m instanceof MethodSpringLdapQueryBuilderFilter or
-    m instanceof MethodSpringLdapQueryBuilderBase
-  )
-}
-
-/**
- * Holds if `n1` to `n2` is a dataflow step that converts between Spring `LdapQueryBuilder` and
- * `Name`, i.e. `taintedLdapQueryBuilder.base()`.
- */
-predicate ldapQueryBaseStep(ExprNode n1, ExprNode n2) {
-  exists(MethodAccess ma, Method m |
-    n1.asExpr() = ma.getQualifier() and
-    n2.asExpr() = ma and
-    ma.getMethod() = m
-  |
-    m instanceof MethodSpringLdapQueryBuilderBase and
-    m.getNumberOfParameters() = 0
-  )
-}
-
-/**
- * Holds if `n1` to `n2` is a dataflow step that converts between Spring `LdapQueryBuilder`,
- * `ConditionCriteria` or `ContainerCriteria`, i.e. when the query is built, for example
- * `query().base(tainted).where("objectclass").is("person")`.
- */
-predicate ldapQueryBuilderStep(ExprNode n1, ExprNode n2) {
-  exists(MethodAccess ma, Method m |
-    n1.asExpr() = ma.getQualifier() and
-    n2.asExpr() = ma and
-    ma.getMethod() = m
-  |
-    (
-      m.getDeclaringType() instanceof TypeSpringLdapQueryBuilder or
-      m.getDeclaringType() instanceof TypeSpringConditionCriteria or
-      m.getDeclaringType() instanceof TypeSpringContainerCriteria
-    ) and
-    (
-      m.getReturnType() instanceof TypeSpringLdapQueryBuilder or
-      m.getReturnType() instanceof TypeSpringConditionCriteria or
-      m.getReturnType() instanceof TypeSpringContainerCriteria
-    )
-  )
-}
-
-/**
- * Holds if `n1` to `n2` is a dataflow step that converts between `String` and Spring
- * `HardcodedFilter`, i.e. `new HardcodedFilter(tainted)`.
- */
-predicate hardcodedFilterStep(ExprNode n1, ExprNode n2) {
-  exists(ConstructorCall cc | cc.getConstructedType() instanceof TypeSpringHardcodedFilter |
-    n1.asExpr() = cc.getAnArgument() and
-    n2.asExpr() = cc
-  )
-}
-
-/**
- * Holds if `n1` to `n2` is a dataflow step that converts between Spring `Filter` and
- * `String`, i.e. `taintedFilter.toString()`, `taintedFilter.encode()` or
- * `taintedFilter.encode(buffer)`.
- */
-predicate springLdapFilterToStringStep(ExprNode n1, ExprNode n2) {
-  exists(MethodAccess ma, Method m |
-    n1.asExpr() = ma.getQualifier() and
-    (n2.asExpr() = ma or n2.asExpr() = ma.getAnArgument()) and
-    ma.getMethod() = m
-  |
-    m.getDeclaringType().getAnAncestor() instanceof TypeSpringLdapFilter and
-    (m.hasName("encode") or m.hasName("toString"))
-  )
-}
-
-/**
- * Holds if `n1` to `n2` is a dataflow step that converts between `String` and Spring
- * `LdapNameBuilder`, i.e. `LdapNameBuilder.newInstance(tainted)` or
- * `LdapNameBuilder.newInstance().add(tainted)`.
- */
-predicate ldapNameBuilderStep(ExprNode n1, ExprNode n2) {
-  exists(MethodAccess ma, Method m |
-    n1.asExpr() = ma.getAnArgument() and
-    (n2.asExpr() = ma or n2.asExpr() = ma.getQualifier()) and
-    ma.getMethod() = m and
-    m.getNumberOfParameters() = 1
-  |
-    m instanceof MethodSpringLdapNameBuilderNewInstance or
-    m instanceof MethodSpringLdapNameBuilderAdd
-  )
-}
-
-/**
- * Holds if `n1` to `n2` is a dataflow step that converts between tainted Spring `LdapNameBuilder`
- * and `LdapName`, `LdapNameBuilder.build()`.
- */
-predicate ldapNameBuilderBuildStep(ExprNode n1, ExprNode n2) {
-  exists(MethodAccess ma | n1.asExpr() = ma.getQualifier() and n2.asExpr() = ma |
-    ma.getMethod() instanceof MethodSpringLdapNameBuilderBuild
-  )
-}
-
-/**
- * Holds if `n1` to `n2` is a dataflow step that converts between `String` and `LdapName` via
- * Spring `LdapUtils.newLdapName`, i.e. `LdapUtils.newLdapName(tainted)`.
- */
-predicate ldapUtilsStep(ExprNode n1, ExprNode n2) {
-  exists(MethodAccess ma | n1.asExpr() = ma.getAnArgument() and n2.asExpr() = ma |
-    ma.getMethod() instanceof MethodSpringLdapUtilsNewLdapName
-  )
-}
-
-/**
- * Holds if `n1` to `n2` is a dataflow step that converts between `String` and Apache LDAP API
- * `SearchRequest`, i.e. `searchRequest.setFilter(tainted)` or `searchRequest.setBase(tainted)`.
- */
-predicate apacheSearchRequestStep(ExprNode n1, ExprNode n2) {
-  exists(MethodAccess ma, Method m |
-    n1.asExpr() = ma.getAnArgument() and
-    n2.asExpr() = ma.getQualifier()
-  |
-    ma.getMethod() = m and
-    m.getDeclaringType().getAnAncestor() instanceof TypeApacheSearchRequest and
-    (m.hasName("setFilter") or m.hasName("setBase"))
-  )
-}
-
-/**
- * Holds if `n1` to `n2` is a dataflow step that converts between Apache LDAP API `SearchRequest`
- * and filter or DN i.e. `tainterSearchRequest.getFilter()` or `taintedSearchRequest.getBase()`.
- */
-predicate apacheSearchRequestGetStep(ExprNode n1, ExprNode n2) {
-  exists(MethodAccess ma, Method m | n1.asExpr() = ma.getQualifier() and n2.asExpr() = ma |
-    ma.getMethod() = m and
-    m.getDeclaringType().getAnAncestor() instanceof TypeApacheSearchRequest and
-    (m.hasName("getFilter") or m.hasName("getBase"))
-  )
-}
-
-/**
- * Holds if `n1` to `n2` is a dataflow step that converts between `String` and Apache LDAP API
- * `Dn`, i.e. `new Dn(tainted)`.
- */
-predicate apacheLdapDnStep(ExprNode n1, ExprNode n2) {
-  exists(ConstructorCall cc | cc.getConstructedType() instanceof TypeApacheDn |
-    n1.asExpr() = cc.getAnArgument() and
-    n2.asExpr() = cc
-  )
-}
-
-/**
- * Holds if `n1` to `n2` is a dataflow step that converts between Apache LDAP API `Dn`
- * and `String` i.e. `taintedDn.getName()`, `taintedDn.getNormName()` or `taintedDn.toString()`.
- */
-predicate apacheLdapDnGetStep(ExprNode n1, ExprNode n2) {
-  exists(MethodAccess ma, Method m | n1.asExpr() = ma.getQualifier() and n2.asExpr() = ma |
-    ma.getMethod() = m and
-    m.getDeclaringType().getAnAncestor() instanceof TypeApacheDn and
-    (m.hasName("getName") or m.hasName("getNormName") or m.hasName("toString"))
-  )
 }
diff --git a/java/ql/src/semmle/code/java/security/LdapInjection.qll b/java/ql/src/semmle/code/java/security/LdapInjection.qll
index ef764c9c14b..4862dcf68e3 100644
--- a/java/ql/src/semmle/code/java/security/LdapInjection.qll
+++ b/java/ql/src/semmle/code/java/security/LdapInjection.qll
@@ -1,7 +1,7 @@
 /** Provides classes to reason about LDAP injection attacks. */
 
 import java
-import semmle.code.java.dataflow.DataFlow
+import semmle.code.java.dataflow.TaintTracking
 import semmle.code.java.frameworks.Jndi
 import semmle.code.java.frameworks.UnboundId
 import semmle.code.java.frameworks.SpringLdap
@@ -118,3 +118,314 @@ private class PrimitiveTypeLdapSanitizer extends LdapInjectionSanitizer {
 private class BoxedTypeLdapSanitizer extends LdapInjectionSanitizer {
   BoxedTypeLdapSanitizer() { this.getType() instanceof BoxedType }
 }
+
+/**
+ * Holds if `n1` to `n2` is a dataflow step that converts between `String` and `LdapName`,
+ * i.e. `new LdapName(tainted)`.
+ */
+private predicate ldapNameStep(DataFlow::ExprNode n1, DataFlow::ExprNode n2) {
+  exists(ConstructorCall cc | cc.getConstructedType() instanceof TypeLdapName |
+    n1.asExpr() = cc.getAnArgument() and
+    n2.asExpr() = cc
+  )
+}
+
+/**
+ * Holds if `n1` to `n2` is a dataflow step that converts between `List<Rdn>` and `LdapName`,
+ * i.e. `new LdapName().addAll(tainted)`.
+ */
+private predicate ldapNameAddAllStep(DataFlow::ExprNode n1, DataFlow::ExprNode n2) {
+  exists(MethodAccess ma |
+    n1.asExpr() = ma.getAnArgument() and
+    (n2.asExpr() = ma or n2.asExpr() = ma.getQualifier())
+  |
+    ma.getMethod() instanceof MethodLdapNameAddAll
+  )
+}
+
+/**
+ * Holds if `n1` to `n2` is a dataflow step that converts between `LdapName` and `LdapName` or
+ * `String`, i.e. `taintedLdapName.clone()`, `taintedLdapName.getAll()`,
+ * `taintedLdapName.getRdns()` or `taintedLdapName.toString()`.
+ */
+private predicate ldapNameGetCloneStep(DataFlow::ExprNode n1, DataFlow::ExprNode n2) {
+  exists(MethodAccess ma, Method m |
+    n1.asExpr() = ma.getQualifier() and
+    n2.asExpr() = ma and
+    ma.getMethod() = m
+  |
+    m instanceof MethodLdapNameClone or
+    m instanceof MethodLdapNameGetAll or
+    m instanceof MethodLdapNameGetRdns or
+    m instanceof MethodLdapNameToString
+  )
+}
+
+/**
+ * Holds if `n1` to `n2` is a dataflow step that converts between `String` and UnboundID `Filter`,
+ * i.e. `Filter.create*(tainted)`.
+ */
+private predicate filterStep(DataFlow::ExprNode n1, DataFlow::ExprNode n2) {
+  exists(MethodAccess ma, Method m |
+    n1.asExpr() = ma.getAnArgument() and
+    n2.asExpr() = ma and
+    ma.getMethod() = m
+  |
+    m instanceof MethodUnboundIdFilterCreate or
+    m instanceof MethodUnboundIdFilterCreateANDFilter or
+    m instanceof MethodUnboundIdFilterCreateNOTFilter or
+    m instanceof MethodUnboundIdFilterCreateORFilter or
+    m instanceof MethodUnboundIdFilterSimplifyFilter
+  )
+}
+
+/**
+ * Holds if `n1` to `n2` is a dataflow step that converts between UnboundID `Filter` and `String`,
+ * i.e. `taintedFilter.toString()` or `taintedFilter.toString(buffer)`.
+ */
+private predicate filterToStringStep(DataFlow::ExprNode n1, DataFlow::ExprNode n2) {
+  exists(MethodAccess ma, Method m |
+    n1.asExpr() = ma.getQualifier() and
+    (n2.asExpr() = ma or n2.asExpr() = ma.getAnArgument())
+  |
+    ma.getMethod() = m and
+    m.getDeclaringType() instanceof TypeUnboundIdLdapFilter and
+    (m.hasName("toString") or m.hasName("toNormalizedString"))
+  )
+}
+
+/**
+ * Holds if `n1` to `n2` is a dataflow step that converts between `String` and UnboundID
+ * `SearchRequest`, i.e. `new SearchRequest(tainted)`.
+ */
+private predicate unboundIdSearchRequestStep(DataFlow::ExprNode n1, DataFlow::ExprNode n2) {
+  exists(ConstructorCall cc, int index, Parameter param |
+    cc.getConstructedType() instanceof TypeUnboundIdSearchRequest
+  |
+    n1.asExpr() = cc.getArgument(index) and
+    n2.asExpr() = cc and
+    cc.getConstructor().getParameter(index) = param and
+    not param.isVarargs()
+  )
+}
+
+/**
+ * Holds if `n1` to `n2` is a dataflow step that converts between UnboundID `SearchRequest`
+ * and UnboundID `SearchRequest`, i.e. `taintedSearchRequest.duplicate()`.
+ */
+private predicate unboundIdSearchRequestDuplicateStep(DataFlow::ExprNode n1, DataFlow::ExprNode n2) {
+  exists(MethodAccess ma, Method m | n1.asExpr() = ma.getQualifier() and n2.asExpr() = ma |
+    ma.getMethod() = m and
+    m.getDeclaringType().getAnAncestor() instanceof TypeUnboundIdReadOnlySearchRequest and
+    m.hasName("duplicate")
+  )
+}
+
+/**
+ * Holds if `n1` to `n2` is a dataflow step that converts between DN or filter and UnboundID
+ * `SearchRequest`, i.e. `searchRequest.setBaseDN(tainted)` or `searchRequest.setFilter(tainted)`.
+ */
+private predicate unboundIdSearchRequestSetStep(DataFlow::ExprNode n1, DataFlow::ExprNode n2) {
+  exists(MethodAccess ma, Method m |
+    n1.asExpr() = ma.getAnArgument() and
+    n2.asExpr() = ma.getQualifier() and
+    ma.getMethod() = m
+  |
+    m instanceof MethodUnboundIdSearchRequestSetBaseDN or
+    m instanceof MethodUnboundIdSearchRequestSetFilter
+  )
+}
+
+/**
+ * Holds if `n1` to `n2` is a dataflow step that converts between `String` and Spring `LdapQuery`,
+ * i.e. `LdapQueryBuilder.query().filter(tainted)` or `LdapQueryBuilder.query().base(tainted)`.
+ */
+private predicate ldapQueryStep(DataFlow::ExprNode n1, DataFlow::ExprNode n2) {
+  exists(MethodAccess ma, Method m, int index |
+    n1.asExpr() = ma.getArgument(index) and
+    n2.asExpr() = ma and
+    ma.getMethod() = m and
+    index = 0
+  |
+    m instanceof MethodSpringLdapQueryBuilderFilter or
+    m instanceof MethodSpringLdapQueryBuilderBase
+  )
+}
+
+/**
+ * Holds if `n1` to `n2` is a dataflow step that converts between Spring `LdapQueryBuilder` and
+ * `Name`, i.e. `taintedLdapQueryBuilder.base()`.
+ */
+private predicate ldapQueryBaseStep(DataFlow::ExprNode n1, DataFlow::ExprNode n2) {
+  exists(MethodAccess ma, Method m |
+    n1.asExpr() = ma.getQualifier() and
+    n2.asExpr() = ma and
+    ma.getMethod() = m
+  |
+    m instanceof MethodSpringLdapQueryBuilderBase and
+    m.getNumberOfParameters() = 0
+  )
+}
+
+/**
+ * Holds if `n1` to `n2` is a dataflow step that converts between Spring `LdapQueryBuilder`,
+ * `ConditionCriteria` or `ContainerCriteria`, i.e. when the query is built, for example
+ * `query().base(tainted).where("objectclass").is("person")`.
+ */
+private predicate ldapQueryBuilderStep(DataFlow::ExprNode n1, DataFlow::ExprNode n2) {
+  exists(MethodAccess ma, Method m |
+    n1.asExpr() = ma.getQualifier() and
+    n2.asExpr() = ma and
+    ma.getMethod() = m
+  |
+    (
+      m.getDeclaringType() instanceof TypeSpringLdapQueryBuilder or
+      m.getDeclaringType() instanceof TypeSpringConditionCriteria or
+      m.getDeclaringType() instanceof TypeSpringContainerCriteria
+    ) and
+    (
+      m.getReturnType() instanceof TypeSpringLdapQueryBuilder or
+      m.getReturnType() instanceof TypeSpringConditionCriteria or
+      m.getReturnType() instanceof TypeSpringContainerCriteria
+    )
+  )
+}
+
+/**
+ * Holds if `n1` to `n2` is a dataflow step that converts between `String` and Spring
+ * `HardcodedFilter`, i.e. `new HardcodedFilter(tainted)`.
+ */
+private predicate hardcodedFilterStep(DataFlow::ExprNode n1, DataFlow::ExprNode n2) {
+  exists(ConstructorCall cc | cc.getConstructedType() instanceof TypeSpringHardcodedFilter |
+    n1.asExpr() = cc.getAnArgument() and
+    n2.asExpr() = cc
+  )
+}
+
+/**
+ * Holds if `n1` to `n2` is a dataflow step that converts between Spring `Filter` and
+ * `String`, i.e. `taintedFilter.toString()`, `taintedFilter.encode()` or
+ * `taintedFilter.encode(buffer)`.
+ */
+private predicate springLdapFilterToStringStep(DataFlow::ExprNode n1, DataFlow::ExprNode n2) {
+  exists(MethodAccess ma, Method m |
+    n1.asExpr() = ma.getQualifier() and
+    (n2.asExpr() = ma or n2.asExpr() = ma.getAnArgument()) and
+    ma.getMethod() = m
+  |
+    m.getDeclaringType().getAnAncestor() instanceof TypeSpringLdapFilter and
+    (m.hasName("encode") or m.hasName("toString"))
+  )
+}
+
+/**
+ * Holds if `n1` to `n2` is a dataflow step that converts between `String` and Spring
+ * `LdapNameBuilder`, i.e. `LdapNameBuilder.newInstance(tainted)` or
+ * `LdapNameBuilder.newInstance().add(tainted)`.
+ */
+private predicate ldapNameBuilderStep(DataFlow::ExprNode n1, DataFlow::ExprNode n2) {
+  exists(MethodAccess ma, Method m |
+    n1.asExpr() = ma.getAnArgument() and
+    (n2.asExpr() = ma or n2.asExpr() = ma.getQualifier()) and
+    ma.getMethod() = m and
+    m.getNumberOfParameters() = 1
+  |
+    m instanceof MethodSpringLdapNameBuilderNewInstance or
+    m instanceof MethodSpringLdapNameBuilderAdd
+  )
+}
+
+/**
+ * Holds if `n1` to `n2` is a dataflow step that converts between tainted Spring `LdapNameBuilder`
+ * and `LdapName`, `LdapNameBuilder.build()`.
+ */
+private predicate ldapNameBuilderBuildStep(DataFlow::ExprNode n1, DataFlow::ExprNode n2) {
+  exists(MethodAccess ma | n1.asExpr() = ma.getQualifier() and n2.asExpr() = ma |
+    ma.getMethod() instanceof MethodSpringLdapNameBuilderBuild
+  )
+}
+
+/**
+ * Holds if `n1` to `n2` is a dataflow step that converts between `String` and `LdapName` via
+ * Spring `LdapUtils.newLdapName`, i.e. `LdapUtils.newLdapName(tainted)`.
+ */
+private predicate ldapUtilsStep(DataFlow::ExprNode n1, DataFlow::ExprNode n2) {
+  exists(MethodAccess ma | n1.asExpr() = ma.getAnArgument() and n2.asExpr() = ma |
+    ma.getMethod() instanceof MethodSpringLdapUtilsNewLdapName
+  )
+}
+
+/**
+ * Holds if `n1` to `n2` is a dataflow step that converts between `String` and Apache LDAP API
+ * `SearchRequest`, i.e. `searchRequest.setFilter(tainted)` or `searchRequest.setBase(tainted)`.
+ */
+private predicate apacheSearchRequestStep(DataFlow::ExprNode n1, DataFlow::ExprNode n2) {
+  exists(MethodAccess ma, Method m |
+    n1.asExpr() = ma.getAnArgument() and
+    n2.asExpr() = ma.getQualifier()
+  |
+    ma.getMethod() = m and
+    m.getDeclaringType().getAnAncestor() instanceof TypeApacheSearchRequest and
+    (m.hasName("setFilter") or m.hasName("setBase"))
+  )
+}
+
+/**
+ * Holds if `n1` to `n2` is a dataflow step that converts between Apache LDAP API `SearchRequest`
+ * and filter or DN i.e. `tainterSearchRequest.getFilter()` or `taintedSearchRequest.getBase()`.
+ */
+private predicate apacheSearchRequestGetStep(DataFlow::ExprNode n1, DataFlow::ExprNode n2) {
+  exists(MethodAccess ma, Method m | n1.asExpr() = ma.getQualifier() and n2.asExpr() = ma |
+    ma.getMethod() = m and
+    m.getDeclaringType().getAnAncestor() instanceof TypeApacheSearchRequest and
+    (m.hasName("getFilter") or m.hasName("getBase"))
+  )
+}
+
+/**
+ * Holds if `n1` to `n2` is a dataflow step that converts between `String` and Apache LDAP API
+ * `Dn`, i.e. `new Dn(tainted)`.
+ */
+private predicate apacheLdapDnStep(DataFlow::ExprNode n1, DataFlow::ExprNode n2) {
+  exists(ConstructorCall cc | cc.getConstructedType() instanceof TypeApacheDn |
+    n1.asExpr() = cc.getAnArgument() and
+    n2.asExpr() = cc
+  )
+}
+
+/**
+ * Holds if `n1` to `n2` is a dataflow step that converts between Apache LDAP API `Dn`
+ * and `String` i.e. `taintedDn.getName()`, `taintedDn.getNormName()` or `taintedDn.toString()`.
+ */
+private predicate apacheLdapDnGetStep(DataFlow::ExprNode n1, DataFlow::ExprNode n2) {
+  exists(MethodAccess ma, Method m | n1.asExpr() = ma.getQualifier() and n2.asExpr() = ma |
+    ma.getMethod() = m and
+    m.getDeclaringType().getAnAncestor() instanceof TypeApacheDn and
+    (m.hasName("getName") or m.hasName("getNormName") or m.hasName("toString"))
+  )
+}
+
+class LdapInjectionAdditionalTaintStep extends TaintTracking::AdditionalTaintStep {
+  override predicate step(DataFlow::Node node1, DataFlow::Node node2) {
+    ldapNameStep(node1, node2) or
+    ldapNameAddAllStep(node1, node2) or
+    ldapNameGetCloneStep(node1, node2) or
+    filterStep(node1, node2) or
+    filterToStringStep(node1, node2) or
+    unboundIdSearchRequestStep(node1, node2) or
+    unboundIdSearchRequestDuplicateStep(node1, node2) or
+    unboundIdSearchRequestSetStep(node1, node2) or
+    ldapQueryStep(node1, node2) or
+    ldapQueryBaseStep(node1, node2) or
+    ldapQueryBuilderStep(node1, node2) or
+    hardcodedFilterStep(node1, node2) or
+    springLdapFilterToStringStep(node1, node2) or
+    ldapNameBuilderStep(node1, node2) or
+    ldapNameBuilderBuildStep(node1, node2) or
+    ldapUtilsStep(node1, node2) or
+    apacheSearchRequestStep(node1, node2) or
+    apacheSearchRequestGetStep(node1, node2) or
+    apacheLdapDnStep(node1, node2) or
+    apacheLdapDnGetStep(node1, node2)
+  }
+}

From 3320061178e0e14a890dac6d72a2f280d93844d3 Mon Sep 17 00:00:00 2001
From: Remco Vermeulen <rvermeulen@users.noreply.github.com>
Date: Wed, 22 Jul 2020 16:03:52 +0200
Subject: [PATCH 04/10] Add and adjust QL docs for classes and predicates

---
 java/ql/src/semmle/code/java/security/LdapInjection.qll | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/java/ql/src/semmle/code/java/security/LdapInjection.qll b/java/ql/src/semmle/code/java/security/LdapInjection.qll
index 4862dcf68e3..d8cfa431358 100644
--- a/java/ql/src/semmle/code/java/security/LdapInjection.qll
+++ b/java/ql/src/semmle/code/java/security/LdapInjection.qll
@@ -10,9 +10,10 @@ import semmle.code.java.frameworks.ApacheLdap
 /** A data flow sink for unvalidated user input that is used to construct LDAP queries. */
 abstract class LdapInjectionSink extends DataFlow::Node { }
 
-/** A class that identifies sanitizers that prevent LDAP injection attacks. */
+/** A sanitizer that prevents LDAP injection attacks. */
 abstract class LdapInjectionSanitizer extends DataFlow::Node { }
 
+/** Holds if the JNDI method parameter at index is susceptible to a LDAP injection attack. */
 private predicate jndiLdapInjectionSinkMethod(Method m, int index) {
   m.getDeclaringType().getAnAncestor() instanceof TypeDirContext and
   m.hasName("search") and
@@ -33,6 +34,7 @@ private class JndiLdapInjectionSink extends LdapInjectionSink {
   }
 }
 
+/** Holds if the UnboundID method parameter at `index` is susceptible to a LDAP injection attack. */
 private predicate unboundIdLdapInjectionSinkMethod(Method m, int index) {
   exists(Parameter param | m.getParameter(index) = param and not param.isVarargs() |
     m instanceof MethodUnboundIdLDAPConnectionSearch or
@@ -55,6 +57,7 @@ private class UnboundedIdLdapInjectionSink extends LdapInjectionSink {
   }
 }
 
+/** Holds if the Spring method parameter at `index` is susceptible to a LDAP injection attack. */
 private predicate springLdapInjectionSinkMethod(Method m, int index) {
   // LdapTemplate.authenticate, LdapTemplate.find* or LdapTemplate.search* method
   (
@@ -91,6 +94,7 @@ private class SpringLdapInjectionSink extends LdapInjectionSink {
   }
 }
 
+/** Holds if the Apache LDAP API method parameter at `index` is susceptible to a LDAP injection attack. */
 private predicate apacheLdapInjectionSinkMethod(Method m, int index) {
   exists(Parameter param | m.getParameter(index) = param and not param.isVarargs() |
     m.getDeclaringType().getAnAncestor() instanceof TypeApacheLdapConnection and
@@ -405,6 +409,7 @@ private predicate apacheLdapDnGetStep(DataFlow::ExprNode n1, DataFlow::ExprNode
   )
 }
 
+/** A set of additional taint steps to consider when taint tracking LDAP related data flows. */
 class LdapInjectionAdditionalTaintStep extends TaintTracking::AdditionalTaintStep {
   override predicate step(DataFlow::Node node1, DataFlow::Node node2) {
     ldapNameStep(node1, node2) or

From 0c09d66d43d60d257492d3c9b121b9e1e7d93b1f Mon Sep 17 00:00:00 2001
From: Remco Vermeulen <rvermeulen@users.noreply.github.com>
Date: Wed, 5 Aug 2020 16:53:50 +0200
Subject: [PATCH 05/10] Consolidate different sinks into a default sink.

---
 .../code/java/security/LdapInjection.qll      | 72 +++++--------------
 1 file changed, 19 insertions(+), 53 deletions(-)

diff --git a/java/ql/src/semmle/code/java/security/LdapInjection.qll b/java/ql/src/semmle/code/java/security/LdapInjection.qll
index d8cfa431358..1da19516037 100644
--- a/java/ql/src/semmle/code/java/security/LdapInjection.qll
+++ b/java/ql/src/semmle/code/java/security/LdapInjection.qll
@@ -13,6 +13,25 @@ abstract class LdapInjectionSink extends DataFlow::Node { }
 /** A sanitizer that prevents LDAP injection attacks. */
 abstract class LdapInjectionSanitizer extends DataFlow::Node { }
 
+/** Default sink for LDAP injection vulnerabilities. */
+private class DefaultLdapInjectionSink extends LdapInjectionSink {
+  DefaultLdapInjectionSink() {
+    exists(MethodAccess ma, Method m, int index |
+      ma.getMethod() = m and
+      ma.getArgument(index) = this.asExpr() and
+      ldapInjectionSinkMethod(m, index)
+    )
+  }
+}
+
+/** Holds if the method parameter at index is susceptible to a LDAP injection attack. */
+private predicate ldapInjectionSinkMethod(Method m, int index) {
+  jndiLdapInjectionSinkMethod(m, index) or
+  unboundIdLdapInjectionSinkMethod(m, index) or
+  springLdapInjectionSinkMethod(m, index) or
+  apacheLdapInjectionSinkMethod(m, index)
+}
+
 /** Holds if the JNDI method parameter at index is susceptible to a LDAP injection attack. */
 private predicate jndiLdapInjectionSinkMethod(Method m, int index) {
   m.getDeclaringType().getAnAncestor() instanceof TypeDirContext and
@@ -20,20 +39,6 @@ private predicate jndiLdapInjectionSinkMethod(Method m, int index) {
   index in [0 .. 1]
 }
 
-/**
- * JNDI sink for LDAP injection vulnerabilities, i.e. 1st (DN) or 2nd (filter) argument to
- * `search` method from `DirContext`.
- */
-private class JndiLdapInjectionSink extends LdapInjectionSink {
-  JndiLdapInjectionSink() {
-    exists(MethodAccess ma, Method m, int index |
-      ma.getMethod() = m and
-      ma.getArgument(index) = this.asExpr() and
-      jndiLdapInjectionSinkMethod(m, index)
-    )
-  }
-}
-
 /** Holds if the UnboundID method parameter at `index` is susceptible to a LDAP injection attack. */
 private predicate unboundIdLdapInjectionSinkMethod(Method m, int index) {
   exists(Parameter param | m.getParameter(index) = param and not param.isVarargs() |
@@ -43,20 +48,6 @@ private predicate unboundIdLdapInjectionSinkMethod(Method m, int index) {
   )
 }
 
-/**
- * UnboundID sink for LDAP injection vulnerabilities,
- * i.e. LDAPConnection.search, LDAPConnection.asyncSearch or LDAPConnection.searchForEntry method.
- */
-private class UnboundedIdLdapInjectionSink extends LdapInjectionSink {
-  UnboundedIdLdapInjectionSink() {
-    exists(MethodAccess ma, Method m, int index |
-      ma.getMethod() = m and
-      ma.getArgument(index) = this.asExpr() and
-      unboundIdLdapInjectionSinkMethod(m, index)
-    )
-  }
-}
-
 /** Holds if the Spring method parameter at `index` is susceptible to a LDAP injection attack. */
 private predicate springLdapInjectionSinkMethod(Method m, int index) {
   // LdapTemplate.authenticate, LdapTemplate.find* or LdapTemplate.search* method
@@ -80,20 +71,6 @@ private predicate springLdapInjectionSinkMethod(Method m, int index) {
   )
 }
 
-/**
- * Spring LDAP sink for LDAP injection vulnerabilities,
- * i.e. LdapTemplate.authenticate, LdapTemplate.find* or LdapTemplate.search* method.
- */
-private class SpringLdapInjectionSink extends LdapInjectionSink {
-  SpringLdapInjectionSink() {
-    exists(MethodAccess ma, Method m, int index |
-      ma.getMethod() = m and
-      ma.getArgument(index) = this.asExpr() and
-      springLdapInjectionSinkMethod(m, index)
-    )
-  }
-}
-
 /** Holds if the Apache LDAP API method parameter at `index` is susceptible to a LDAP injection attack. */
 private predicate apacheLdapInjectionSinkMethod(Method m, int index) {
   exists(Parameter param | m.getParameter(index) = param and not param.isVarargs() |
@@ -102,17 +79,6 @@ private predicate apacheLdapInjectionSinkMethod(Method m, int index) {
   )
 }
 
-/** Apache LDAP API sink for LDAP injection vulnerabilities, i.e. LdapConnection.search method. */
-private class ApacheLdapInjectionSink extends LdapInjectionSink {
-  ApacheLdapInjectionSink() {
-    exists(MethodAccess ma, Method m, int index |
-      ma.getMethod() = m and
-      ma.getArgument(index) = this.asExpr() and
-      apacheLdapInjectionSinkMethod(m, index)
-    )
-  }
-}
-
 /** A sanitizer that clears the taint on primitive types. */
 private class PrimitiveTypeLdapSanitizer extends LdapInjectionSanitizer {
   PrimitiveTypeLdapSanitizer() { this.getType() instanceof PrimitiveType }

From a1411407c18c51ef9ec2857d0af974e277d4f63b Mon Sep 17 00:00:00 2001
From: Remco Vermeulen <rvermeulen@users.noreply.github.com>
Date: Wed, 5 Aug 2020 17:07:05 +0200
Subject: [PATCH 06/10] Consolidate sanitizers into default sanitizer

---
 .../semmle/code/java/security/LdapInjection.qll    | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/java/ql/src/semmle/code/java/security/LdapInjection.qll b/java/ql/src/semmle/code/java/security/LdapInjection.qll
index 1da19516037..2102c285a31 100644
--- a/java/ql/src/semmle/code/java/security/LdapInjection.qll
+++ b/java/ql/src/semmle/code/java/security/LdapInjection.qll
@@ -79,14 +79,12 @@ private predicate apacheLdapInjectionSinkMethod(Method m, int index) {
   )
 }
 
-/** A sanitizer that clears the taint on primitive types. */
-private class PrimitiveTypeLdapSanitizer extends LdapInjectionSanitizer {
-  PrimitiveTypeLdapSanitizer() { this.getType() instanceof PrimitiveType }
-}
-
-/** A sanitizer that clears the taint on boxed primitive types. */
-private class BoxedTypeLdapSanitizer extends LdapInjectionSanitizer {
-  BoxedTypeLdapSanitizer() { this.getType() instanceof BoxedType }
+/** A sanitizer that clears the taint on (boxed) primitive types */
+private class DefaultLdapSanitizer extends LdapInjectionSanitizer {
+  DefaultLdapSanitizer() {
+    this.getType() instanceof PrimitiveType or
+    this.getType() instanceof BoxedType
+  }
 }
 
 /**

From 7f7ad88deafa1eeeb85e2a66cd6f156139fc4b34 Mon Sep 17 00:00:00 2001
From: Remco Vermeulen <rvermeulen@users.noreply.github.com>
Date: Thu, 6 Aug 2020 11:35:03 +0200
Subject: [PATCH 07/10] Limit LdapAdditionalTaintStep to Ldap configuration

---
 .../Security/CWE/CWE-090/LdapInjectionLib.qll   |  4 ++++
 .../semmle/code/java/security/LdapInjection.qll | 17 ++++++++++++++++-
 2 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/java/ql/src/Security/CWE/CWE-090/LdapInjectionLib.qll b/java/ql/src/Security/CWE/CWE-090/LdapInjectionLib.qll
index 7f95a8b6c1a..2304ec1cd02 100644
--- a/java/ql/src/Security/CWE/CWE-090/LdapInjectionLib.qll
+++ b/java/ql/src/Security/CWE/CWE-090/LdapInjectionLib.qll
@@ -14,4 +14,8 @@ class LdapInjectionFlowConfig extends TaintTracking::Configuration {
   override predicate isSink(DataFlow::Node sink) { sink instanceof LdapInjectionSink }
 
   override predicate isSanitizer(DataFlow::Node node) { node instanceof LdapInjectionSanitizer }
+
+  override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
+    any(LdapInjectionAdditionalTaintStep a).step(pred, succ)
+  }
 }
diff --git a/java/ql/src/semmle/code/java/security/LdapInjection.qll b/java/ql/src/semmle/code/java/security/LdapInjection.qll
index 2102c285a31..30d1edb1304 100644
--- a/java/ql/src/semmle/code/java/security/LdapInjection.qll
+++ b/java/ql/src/semmle/code/java/security/LdapInjection.qll
@@ -13,6 +13,21 @@ abstract class LdapInjectionSink extends DataFlow::Node { }
 /** A sanitizer that prevents LDAP injection attacks. */
 abstract class LdapInjectionSanitizer extends DataFlow::Node { }
 
+private newtype TUnit = TMkUnit()
+
+class Unit extends TUnit {
+  string toString() { result = "unit" }
+}
+
+/**
+ * A unit class for adding additional taint steps.
+ *
+ * Extend this class to add additional taint steps that should apply to the LdapInjectionFlowConfig.
+ */
+class LdapInjectionAdditionalTaintStep extends Unit {
+  abstract predicate step(DataFlow::Node node1, DataFlow::Node node2);
+}
+
 /** Default sink for LDAP injection vulnerabilities. */
 private class DefaultLdapInjectionSink extends LdapInjectionSink {
   DefaultLdapInjectionSink() {
@@ -374,7 +389,7 @@ private predicate apacheLdapDnGetStep(DataFlow::ExprNode n1, DataFlow::ExprNode
 }
 
 /** A set of additional taint steps to consider when taint tracking LDAP related data flows. */
-class LdapInjectionAdditionalTaintStep extends TaintTracking::AdditionalTaintStep {
+private class DefaultLdapInjectionAdditionalTaintStep extends LdapInjectionAdditionalTaintStep {
   override predicate step(DataFlow::Node node1, DataFlow::Node node2) {
     ldapNameStep(node1, node2) or
     ldapNameAddAllStep(node1, node2) or

From 5a819422c1f369864f1bfce9820887636c54c658 Mon Sep 17 00:00:00 2001
From: Remco Vermeulen <rvermeulen@users.noreply.github.com>
Date: Thu, 6 Aug 2020 12:02:34 +0200
Subject: [PATCH 08/10] Reuse `Unit` class from `TaintTracking`

---
 java/ql/src/semmle/code/java/security/LdapInjection.qll | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/java/ql/src/semmle/code/java/security/LdapInjection.qll b/java/ql/src/semmle/code/java/security/LdapInjection.qll
index 30d1edb1304..203256bd4dd 100644
--- a/java/ql/src/semmle/code/java/security/LdapInjection.qll
+++ b/java/ql/src/semmle/code/java/security/LdapInjection.qll
@@ -13,18 +13,12 @@ abstract class LdapInjectionSink extends DataFlow::Node { }
 /** A sanitizer that prevents LDAP injection attacks. */
 abstract class LdapInjectionSanitizer extends DataFlow::Node { }
 
-private newtype TUnit = TMkUnit()
-
-class Unit extends TUnit {
-  string toString() { result = "unit" }
-}
-
 /**
  * A unit class for adding additional taint steps.
  *
  * Extend this class to add additional taint steps that should apply to the LdapInjectionFlowConfig.
  */
-class LdapInjectionAdditionalTaintStep extends Unit {
+class LdapInjectionAdditionalTaintStep extends TaintTracking::Unit {
   abstract predicate step(DataFlow::Node node1, DataFlow::Node node2);
 }
 

From 408db412dc65a804ac6ed9a53cd500736450a0bf Mon Sep 17 00:00:00 2001
From: Remco Vermeulen <rvermeulen@users.noreply.github.com>
Date: Thu, 6 Aug 2020 13:29:02 +0200
Subject: [PATCH 09/10] Add missing predicate qldoc

---
 java/ql/src/semmle/code/java/security/LdapInjection.qll | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/java/ql/src/semmle/code/java/security/LdapInjection.qll b/java/ql/src/semmle/code/java/security/LdapInjection.qll
index 203256bd4dd..c2a25e43cd6 100644
--- a/java/ql/src/semmle/code/java/security/LdapInjection.qll
+++ b/java/ql/src/semmle/code/java/security/LdapInjection.qll
@@ -19,6 +19,10 @@ 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 {
+  /**
+   * Holds if the step from `node1` to `node2` should be considered a taint
+   * step for the LdapInjectionFlowConfig configuration.
+   */
   abstract predicate step(DataFlow::Node node1, DataFlow::Node node2);
 }
 

From 3ae3a879d2491d46032e2f00fb346b1ee7688976 Mon Sep 17 00:00:00 2001
From: Remco Vermeulen <rvermeulen@users.noreply.github.com>
Date: Thu, 6 Aug 2020 23:00:03 +0200
Subject: [PATCH 10/10] Fix qldoc grammar and style mistakes

Co-authored-by: intrigus-lgtm <60750685+intrigus-lgtm@users.noreply.github.com>
---
 .../semmle/code/java/security/LdapInjection.qll  | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/java/ql/src/semmle/code/java/security/LdapInjection.qll b/java/ql/src/semmle/code/java/security/LdapInjection.qll
index c2a25e43cd6..45e7d548734 100644
--- a/java/ql/src/semmle/code/java/security/LdapInjection.qll
+++ b/java/ql/src/semmle/code/java/security/LdapInjection.qll
@@ -16,12 +16,12 @@ abstract class LdapInjectionSanitizer extends DataFlow::Node { }
 /**
  * A unit class for adding additional taint steps.
  *
- * Extend this class to add additional taint steps that should apply to the LdapInjectionFlowConfig.
+ * Extend this class to add additional taint steps that should apply to the `LdapInjectionFlowConfig`.
  */
 class LdapInjectionAdditionalTaintStep extends TaintTracking::Unit {
   /**
    * Holds if the step from `node1` to `node2` should be considered a taint
-   * step for the LdapInjectionFlowConfig configuration.
+   * step for the `LdapInjectionFlowConfig` configuration.
    */
   abstract predicate step(DataFlow::Node node1, DataFlow::Node node2);
 }
@@ -37,7 +37,7 @@ private class DefaultLdapInjectionSink extends LdapInjectionSink {
   }
 }
 
-/** Holds if the method parameter at index is susceptible to a LDAP injection attack. */
+/** Holds if the method parameter at `index` is susceptible to an LDAP injection attack. */
 private predicate ldapInjectionSinkMethod(Method m, int index) {
   jndiLdapInjectionSinkMethod(m, index) or
   unboundIdLdapInjectionSinkMethod(m, index) or
@@ -45,14 +45,14 @@ private predicate ldapInjectionSinkMethod(Method m, int index) {
   apacheLdapInjectionSinkMethod(m, index)
 }
 
-/** Holds if the JNDI method parameter at index is susceptible to a LDAP injection attack. */
+/** Holds if the JNDI method parameter at `index` is susceptible to an LDAP injection attack. */
 private predicate jndiLdapInjectionSinkMethod(Method m, int index) {
   m.getDeclaringType().getAnAncestor() instanceof TypeDirContext and
   m.hasName("search") and
   index in [0 .. 1]
 }
 
-/** Holds if the UnboundID method parameter at `index` is susceptible to a LDAP injection attack. */
+/** Holds if the UnboundID method parameter at `index` is susceptible to an LDAP injection attack. */
 private predicate unboundIdLdapInjectionSinkMethod(Method m, int index) {
   exists(Parameter param | m.getParameter(index) = param and not param.isVarargs() |
     m instanceof MethodUnboundIdLDAPConnectionSearch or
@@ -61,7 +61,7 @@ private predicate unboundIdLdapInjectionSinkMethod(Method m, int index) {
   )
 }
 
-/** Holds if the Spring method parameter at `index` is susceptible to a LDAP injection attack. */
+/** Holds if the Spring method parameter at `index` is susceptible to an LDAP injection attack. */
 private predicate springLdapInjectionSinkMethod(Method m, int index) {
   // LdapTemplate.authenticate, LdapTemplate.find* or LdapTemplate.search* method
   (
@@ -84,7 +84,7 @@ private predicate springLdapInjectionSinkMethod(Method m, int index) {
   )
 }
 
-/** Holds if the Apache LDAP API method parameter at `index` is susceptible to a LDAP injection attack. */
+/** Holds if the Apache LDAP API method parameter at `index` is susceptible to an LDAP injection attack. */
 private predicate apacheLdapInjectionSinkMethod(Method m, int index) {
   exists(Parameter param | m.getParameter(index) = param and not param.isVarargs() |
     m.getDeclaringType().getAnAncestor() instanceof TypeApacheLdapConnection and
@@ -92,7 +92,7 @@ private predicate apacheLdapInjectionSinkMethod(Method m, int index) {
   )
 }
 
-/** A sanitizer that clears the taint on (boxed) primitive types */
+/** A sanitizer that clears the taint on (boxed) primitive types. */
 private class DefaultLdapSanitizer extends LdapInjectionSanitizer {
   DefaultLdapSanitizer() {
     this.getType() instanceof PrimitiveType or