Consider search methods with unsafe SearchControls

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

@@ -2,6 +2,7 @@
 
 import java
 import semmle.code.java.dataflow.DataFlow
+import semmle.code.java.dataflow.DataFlow2
 import semmle.code.java.dataflow.ExternalFlow
 import semmle.code.java.frameworks.Jndi
 import semmle.code.java.frameworks.SpringLdap
@@ -29,18 +30,14 @@ private class DefaultJndiInjectionSink extends JndiInjectionSink {
 }
 
 /**
- * A method that does a JNDI lookup but needs a specific parameter set to `true` to be vulnerable
+ * A method that does a JNDI lookup when it receives a specific argument set to `true`.
  */
 private class ConditionedJndiInjectionSink extends JndiInjectionSink, DataFlow::ExprNode {
   ConditionedJndiInjectionSink() {
     exists(MethodAccess ma, Method m |
       ma.getMethod() = m and
-      ma.getAnArgument() = this.asExpr() and
-      m.getSourceDeclaration()
-          .getDeclaringType()
-          .getASourceSupertype*()
-          .hasQualifiedName(["org.springframework.ldap.core", "org.springframework.ldap"],
-            "LdapOperations")
+      ma.getArgument(0) = this.asExpr() and
+      m.getDeclaringType().getASourceSupertype*() instanceof TypeLdapOperations
     |
       m.hasName("search") and
       ma.getArgument(3).(CompileTimeConstantExpr).getBooleanValue() = true
@@ -51,6 +48,19 @@ private class ConditionedJndiInjectionSink extends JndiInjectionSink, DataFlow::
   }
 }
 
+/**
+ * A method that does a JNDI lookup when it receives a `SearchControls` argument with `setReturningObjFlag` = `true`
+ */
+private class UnsafeSearchControlsSink extends JndiInjectionSink {
+  UnsafeSearchControlsSink() {
+    exists(UnsafeSearchControlsConf conf, MethodAccess ma |
+      conf.hasFlowTo(DataFlow::exprNode(ma.getAnArgument()))
+    |
+      this.asExpr() = ma.getArgument(0)
+    )
+  }
+}
+
 /**
  * Tainted value passed to env `Hashtable` as the provider URL, i.e.
  * `env.put(Context.PROVIDER_URL, tainted)` or `env.setProperty(Context.PROVIDER_URL, tainted)`.
@@ -61,7 +71,7 @@ private class ProviderUrlJndiInjectionSink extends JndiInjectionSink, DataFlow::
       ma.getMethod() = m and
       ma.getArgument(1) = this.getExpr()
     |
-      m.getDeclaringType().getAnAncestor() instanceof TypeHashtable and
+      m.getDeclaringType().getASourceSupertype*() instanceof TypeHashtable and
       (m.hasName("put") or m.hasName("setProperty")) and
       (
         ma.getArgument(0).(CompileTimeConstantExpr).getStringValue() = "java.naming.provider.url"
@@ -81,12 +91,12 @@ private class DefaultJndiInjectionSinkModel extends SinkModelCsv {
   override predicate row(string row) {
     row =
       [
-        "javax.naming;InitialContext;true;lookup;;;Argument[0];jndi-injection",
-        "javax.naming;InitialContext;true;lookupLink;;;Argument[0];jndi-injection",
-        "javax.naming;InitialContext;true;doLookup;;;Argument[0];jndi-injection",
-        "javax.naming;InitialContext;true;rename;;;Argument[0];jndi-injection",
-        "javax.naming;InitialContext;true;list;;;Argument[0];jndi-injection",
-        "javax.naming;InitialContext;true;listBindings;;;Argument[0];jndi-injection",
+        "javax.naming;Context;true;lookup;;;Argument[0];jndi-injection",
+        "javax.naming;Context;true;lookupLink;;;Argument[0];jndi-injection",
+        "javax.naming;Context;true;doLookup;;;Argument[0];jndi-injection",
+        "javax.naming;Context;true;rename;;;Argument[0];jndi-injection",
+        "javax.naming;Context;true;list;;;Argument[0];jndi-injection",
+        "javax.naming;Context;true;listBindings;;;Argument[0];jndi-injection",
         "javax.management.remote;JMXConnector;true;connect;;;Argument[-1];jndi-injection",
         "javax.management.remote;JMXConnectorFactory;false;connect;;;Argument[0];jndi-injection",
         // Spring
@@ -127,10 +137,92 @@ private class DefaultJndiInjectionSinkModel extends SinkModelCsv {
   }
 }
 
+/**
+ * Find flows between a `SearchControls` object with `setReturningObjFlag` = `true`
+ * and an argument of a `LdapOperations.search` or `DirContext.search` call.
+ */
+private class UnsafeSearchControlsConf extends DataFlow2::Configuration {
+  UnsafeSearchControlsConf() { this = "UnsafeSearchControlsConf" }
+
+  override predicate isSource(DataFlow2::Node source) { source instanceof UnsafeSearchControls }
+
+  override predicate isSink(DataFlow2::Node sink) { sink instanceof UnsafeSearchControlsArgument }
+}
+
+/**
+ * An argument of type `SearchControls` of a a `LdapOperations.search` or `DirContext.search` call.
+ */
+private class UnsafeSearchControlsArgument extends DataFlow2::ExprNode {
+  UnsafeSearchControlsArgument() {
+    exists(MethodAccess ma, Method m |
+      ma.getMethod() = m and
+      ma.getAnArgument() = this.asExpr() and
+      this.asExpr().getType() instanceof TypeSearchControls and
+      m.hasName("search")
+    |
+      m.getDeclaringType().getASourceSupertype*() instanceof TypeLdapOperations or
+      m.getDeclaringType().getASourceSupertype*() instanceof TypeDirContext
+    )
+  }
+}
+
+/**
+ * A `SearchControls` object with `setReturningObjFlag` = `true`.
+ */
+private class UnsafeSearchControls extends DataFlow2::ExprNode {
+  UnsafeSearchControls() {
+    exists(MethodAccess ma |
+      ma.getMethod() instanceof SetReturningObjFlagMethod and
+      ma.getArgument(0).(CompileTimeConstantExpr).getBooleanValue() = true and
+      this.asExpr() = ma.getQualifier()
+    )
+    or
+    exists(ConstructorCall cc |
+      cc.getConstructedType() instanceof TypeSearchControls and
+      cc.getArgument(4).(CompileTimeConstantExpr).getBooleanValue() = true and
+      this.asExpr() = cc
+    )
+  }
+}
+
+/**
+ * The method `SearchControls.setReturningObjFlag`.
+ */
+private class SetReturningObjFlagMethod extends Method {
+  SetReturningObjFlagMethod() {
+    this.getDeclaringType() instanceof TypeSearchControls and
+    this.hasName("setReturningObjFlag")
+  }
+}
+
+/**
+ * The class `javax.naming.directory.SearchControls`
+ */
+private class TypeSearchControls extends Class {
+  TypeSearchControls() { this.hasQualifiedName("javax.naming.directory", "SearchControls") }
+}
+
+/**
+ * The interface `org.springframework.ldap.core.LdapOperations` or
+ * `org.springframework.ldap.LdapOperations`
+ */
+private class TypeLdapOperations extends Interface {
+  TypeLdapOperations() {
+    this.hasQualifiedName(["org.springframework.ldap.core", "org.springframework.ldap"],
+      "LdapOperations")
+  }
+}
+
+/** The class `java.util.Hashtable`. */
+private class TypeHashtable extends Class {
+  TypeHashtable() { this.getSourceDeclaration().hasQualifiedName("java.util", "Hashtable") }
+}
+
 /** A set of additional taint steps to consider when taint tracking JNDI injection related data flows. */
 private class DefaultJndiInjectionAdditionalTaintStep extends JndiInjectionAdditionalTaintStep {
   override predicate step(DataFlow::Node node1, DataFlow::Node node2) {
     nameStep(node1, node2) or
+    nameAddStep(node1, node2) or
     jmxServiceUrlStep(node1, node2) or
     jmxConnectorStep(node1, node2) or
     rmiConnectorStep(node1, node2)
@@ -151,6 +243,24 @@ private predicate nameStep(DataFlow::ExprNode n1, DataFlow::ExprNode n2) {
   )
 }
 
+/**
+ * Holds if `n1` to `n2` is a dataflow step that converts between `String` and `CompositeName` or
+ * `CompoundName`, i.e. `new CompositeName().add(tainted)` or `new CompoundName().add(tainted)`.
+ */
+private predicate nameAddStep(DataFlow::ExprNode n1, DataFlow::ExprNode n2) {
+  exists(Method m, MethodAccess ma |
+    ma.getMethod() = m and
+    m.hasName("add") and
+    (
+      m.getDeclaringType() instanceof TypeCompositeName or
+      m.getDeclaringType() instanceof TypeCompoundName
+    )
+  |
+    n1.asExpr() = ma.getAnArgument() and
+    n2.asExpr() = ma
+  )
+}
+
 /**
  * Holds if `n1` to `n2` is a dataflow step that converts between `String` and `JMXServiceURL`,
  * i.e. `new JMXServiceURL(tainted)`.
@@ -184,8 +294,3 @@ private predicate rmiConnectorStep(DataFlow::ExprNode n1, DataFlow::ExprNode n2)
     n2.asExpr() = cc
   )
 }
-
-/** The class `java.util.Hashtable`. */
-private class TypeHashtable extends Class {
-  TypeHashtable() { this.getSourceDeclaration().hasQualifiedName("java.util", "Hashtable") }
-}

java/ql/test/query-tests/security/CWE-074/JndiInjectionTest.java

@@ -11,6 +11,7 @@ import javax.naming.Context;
 import javax.naming.InitialContext;
 import javax.naming.Name;
 import javax.naming.NamingException;
+import javax.naming.directory.DirContext;
 import javax.naming.directory.InitialDirContext;
 import javax.naming.directory.SearchControls;
 import javax.naming.ldap.InitialLdapContext;
@@ -18,6 +19,7 @@ import javax.naming.ldap.InitialLdapContext;
 import org.springframework.jndi.JndiTemplate;
 import org.springframework.ldap.core.AttributesMapper;
 import org.springframework.ldap.core.ContextMapper;
+import org.springframework.ldap.core.DirContextProcessor;
 import org.springframework.ldap.core.LdapTemplate;
 import org.springframework.ldap.core.NameClassPairCallbackHandler;
 import org.springframework.stereotype.Controller;
@@ -47,9 +49,9 @@ public class JndiInjectionTest {
   }
 
   @RequestMapping
-  public void testInitialDirContextBad1(@RequestParam String nameStr) throws NamingException {
+  public void testDirContextBad1(@RequestParam String nameStr) throws NamingException {
     Name name = new CompoundName(nameStr, new Properties());
-    InitialDirContext ctx = new InitialDirContext();
+    DirContext ctx = new InitialDirContext();
 
     ctx.lookup(nameStr); // $hasJndiInjection
     ctx.lookupLink(nameStr); // $hasJndiInjection
@@ -62,6 +64,19 @@ public class JndiInjectionTest {
     ctx.rename(name, null); // $hasJndiInjection
     ctx.list(name); // $hasJndiInjection
     ctx.listBindings(name); // $hasJndiInjection
+
+    SearchControls searchControls = new SearchControls();
+    searchControls.setReturningObjFlag(true);
+    ctx.search(nameStr, "", searchControls); // $hasJndiInjection
+    ctx.search(nameStr, "", new Object[] {}, searchControls); // $hasJndiInjection
+
+    SearchControls searchControls2 = new SearchControls(1, 0, 0, null, true, false);
+    ctx.search(nameStr, "", searchControls2); // $hasJndiInjection
+    ctx.search(nameStr, "", new Object[] {}, searchControls2); // $hasJndiInjection
+
+    SearchControls searchControls3 = new SearchControls(1, 0, 0, null, false, false);
+    ctx.search(nameStr, "", searchControls3); // Safe
+    ctx.search(nameStr, "", new Object[] {}, searchControls3); // Safe
   }
 
   @RequestMapping
@@ -93,7 +108,7 @@ public class JndiInjectionTest {
   @RequestMapping
   public void testSpringLdapTemplateBad1(@RequestParam String nameStr) throws NamingException {
     LdapTemplate ctx = new LdapTemplate();
-    Name name = new CompositeName(nameStr);
+    Name name = new CompositeName().add(nameStr);
 
     ctx.lookup(nameStr); // $hasJndiInjection
     ctx.lookupContext(nameStr); // $hasJndiInjection
@@ -104,11 +119,45 @@ public class JndiInjectionTest {
     ctx.unbind(nameStr, true); // $hasJndiInjection
 
     ctx.search(nameStr, "", 0, true, null); // $hasJndiInjection
-    ctx.search(nameStr, "", 0, new String[] {}, (ContextMapper<Object>) new Object()); // $hasJndiInjection
-    ctx.search(nameStr, "", 0, (ContextMapper<Object>) new Object()); // $hasJndiInjection
-    ctx.search(nameStr, "", (ContextMapper) new Object()); // $hasJndiInjection
+    ctx.search(nameStr, "", 0, new String[] {}, (ContextMapper<Object>) null); // $hasJndiInjection
+    ctx.search(nameStr, "", 0, (ContextMapper<Object>) null); // $hasJndiInjection
+    ctx.search(nameStr, "", (ContextMapper<Object>) null); // $hasJndiInjection
 
-    ctx.searchForObject(nameStr, "", (ContextMapper) new Object()); // $hasJndiInjection
+    SearchControls searchControls = new SearchControls();
+    searchControls.setReturningObjFlag(true);
+    ctx.search(nameStr, "", searchControls, (AttributesMapper<Object>) null); // $hasJndiInjection
+    ctx.search(nameStr, "", searchControls, (AttributesMapper<Object>) null, // $hasJndiInjection
+        (DirContextProcessor) null);
+    ctx.search(nameStr, "", searchControls, (ContextMapper<Object>) null); // $hasJndiInjection
+    ctx.search(nameStr, "", searchControls, (ContextMapper<Object>) null, // $hasJndiInjection
+        (DirContextProcessor) null);
+    ctx.search(nameStr, "", searchControls, (NameClassPairCallbackHandler) null); // $hasJndiInjection
+    ctx.search(nameStr, "", searchControls, (NameClassPairCallbackHandler) null, // $hasJndiInjection
+        (DirContextProcessor) null);
+
+    SearchControls searchControls2 = new SearchControls(1, 0, 0, null, true, false);
+    ctx.search(nameStr, "", searchControls2, (AttributesMapper<Object>) null); // $hasJndiInjection
+    ctx.search(nameStr, "", searchControls2, (AttributesMapper<Object>) null, // $hasJndiInjection
+        (DirContextProcessor) null);
+    ctx.search(nameStr, "", searchControls2, (ContextMapper<Object>) null); // $hasJndiInjection
+    ctx.search(nameStr, "", searchControls2, (ContextMapper<Object>) null, // $hasJndiInjection
+        (DirContextProcessor) null);
+    ctx.search(nameStr, "", searchControls2, (NameClassPairCallbackHandler) null); // $hasJndiInjection
+    ctx.search(nameStr, "", searchControls2, (NameClassPairCallbackHandler) null, // $hasJndiInjection
+        (DirContextProcessor) null);
+
+    SearchControls searchControls3 = new SearchControls(1, 0, 0, null, false, false);
+    ctx.search(nameStr, "", searchControls3, (AttributesMapper<Object>) null); // Safe
+    ctx.search(nameStr, "", searchControls3, (AttributesMapper<Object>) null, // Safe
+        (DirContextProcessor) null);
+    ctx.search(nameStr, "", searchControls3, (ContextMapper<Object>) null); // Safe
+    ctx.search(nameStr, "", searchControls3, (ContextMapper<Object>) null, // Safe
+        (DirContextProcessor) null);
+    ctx.search(nameStr, "", searchControls3, (NameClassPairCallbackHandler) null); // Safe
+    ctx.search(nameStr, "", searchControls3, (NameClassPairCallbackHandler) null, // Safe
+        (DirContextProcessor) null);
+
+    ctx.searchForObject(nameStr, "", (ContextMapper<Object>) null); // $hasJndiInjection
   }
 
   @RequestMapping

java/ql/test/stubs/spring-ldap-2.3.2/org/springframework/ldap/core/LdapOperations.java

@@ -1,3 +1,84 @@
 package org.springframework.ldap.core;
 
-public interface LdapOperations {}
+import java.util.*;
+
+import javax.naming.Name;
+import javax.naming.directory.SearchControls;
+
+import org.springframework.ldap.filter.Filter;
+
+import org.springframework.ldap.query.LdapQuery;
+
+public interface LdapOperations {
+    void authenticate(LdapQuery query, String password);
+
+    boolean authenticate(Name base, String filter, String password);
+
+    <T> List<T> find(Name base, Filter filter, SearchControls searchControls, final Class<T> clazz);
+
+    <T> List<T> find(LdapQuery query, Class<T> clazz);
+
+    <T> T findOne(LdapQuery query, Class<T> clazz);
+
+    void search(String base, String filter, int searchScope, boolean returningObjFlag,
+            NameClassPairCallbackHandler handler);
+
+    void search(final String base, final String filter, final SearchControls controls,
+            NameClassPairCallbackHandler handler);
+
+    void search(final String base, final String filter, final SearchControls controls,
+            NameClassPairCallbackHandler handler, DirContextProcessor processor);
+
+    void search(String base, String filter, NameClassPairCallbackHandler handler);
+
+    <T> List<T> search(String base, String filter, int searchScope, String[] attrs,
+            AttributesMapper<T> mapper);
+
+    <T> List<T> search(String base, String filter, int searchScope, AttributesMapper<T> mapper);
+
+    <T> List<T> search(String base, String filter, AttributesMapper<T> mapper);
+
+    <T> List<T> search(String base, String filter, int searchScope, String[] attrs,
+            ContextMapper<T> mapper);
+
+    <T> List<T> search(String base, String filter, int searchScope, ContextMapper<T> mapper);
+
+    <T> List<T> search(String base, String filter, ContextMapper<T> mapper);
+
+    <T> List<T> search(String base, String filter, SearchControls controls,
+            ContextMapper<T> mapper);
+
+    <T> List<T> search(String base, String filter, SearchControls controls,
+            AttributesMapper<T> mapper);
+
+    <T> List<T> search(String base, String filter, SearchControls controls,
+            AttributesMapper<T> mapper, DirContextProcessor processor);
+
+    <T> List<T> search(String base, String filter, SearchControls controls, ContextMapper<T> mapper,
+            DirContextProcessor processor);
+
+    DirContextOperations searchForContext(LdapQuery query);
+
+    <T> T searchForObject(Name base, String filter, ContextMapper<T> mapper);
+
+    <T> T searchForObject(String base, String filter, ContextMapper<T> mapper);
+
+    <T> T searchForObject(String base, String filter, SearchControls searchControls,
+            ContextMapper<T> mapper);
+
+    Object lookup(final String dn);
+
+    DirContextOperations lookupContext(String dn);
+
+    <T> T findByDn(Name dn, final Class<T> clazz);
+
+    void rename(final Name oldDn, final Name newDn);
+
+    List<String> list(final Name base);
+
+    List<String> listBindings(final Name base);
+
+    void unbind(final String dn);
+
+    void unbind(final String dn, boolean recursive);
+}