Skip to content

Commit 7dbdba2

Browse files
atorralbaatorralba
committed
Consider search methods with unsafe SearchControls
1 parent 2613e58 commit 7dbdba2

File tree

3 files changed

+263
-28
lines changed

3 files changed

+263
-28
lines changed

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

Lines changed: 124 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
import java
44
import semmle.code.java.dataflow.DataFlow
5+
import semmle.code.java.dataflow.DataFlow2
56
import semmle.code.java.dataflow.ExternalFlow
67
import semmle.code.java.frameworks.Jndi
78
import semmle.code.java.frameworks.SpringLdap
@@ -29,18 +30,14 @@ private class DefaultJndiInjectionSink extends JndiInjectionSink {
2930
}
3031

3132
/**
32-
* A method that does a JNDI lookup but needs a specific parameter set to `true` to be vulnerable
33+
* A method that does a JNDI lookup when it receives a specific argument set to `true`.
3334
*/
3435
private class ConditionedJndiInjectionSink extends JndiInjectionSink, DataFlow::ExprNode {
3536
ConditionedJndiInjectionSink() {
3637
exists(MethodAccess ma, Method m |
3738
ma.getMethod() = m and
38-
ma.getAnArgument() = this.asExpr() and
39-
m.getSourceDeclaration()
40-
.getDeclaringType()
41-
.getASourceSupertype*()
42-
.hasQualifiedName(["org.springframework.ldap.core", "org.springframework.ldap"],
43-
"LdapOperations")
39+
ma.getArgument(0) = this.asExpr() and
40+
m.getDeclaringType().getASourceSupertype*() instanceof TypeLdapOperations
4441
|
4542
m.hasName("search") and
4643
ma.getArgument(3).(CompileTimeConstantExpr).getBooleanValue() = true
@@ -51,6 +48,19 @@ private class ConditionedJndiInjectionSink extends JndiInjectionSink, DataFlow::
5148
}
5249
}
5350

51+
/**
52+
* A method that does a JNDI lookup when it receives a `SearchControls` argument with `setReturningObjFlag` = `true`
53+
*/
54+
private class UnsafeSearchControlsSink extends JndiInjectionSink {
55+
UnsafeSearchControlsSink() {
56+
exists(UnsafeSearchControlsConf conf, MethodAccess ma |
57+
conf.hasFlowTo(DataFlow::exprNode(ma.getAnArgument()))
58+
|
59+
this.asExpr() = ma.getArgument(0)
60+
)
61+
}
62+
}
63+
5464
/**
5565
* Tainted value passed to env `Hashtable` as the provider URL, i.e.
5666
* `env.put(Context.PROVIDER_URL, tainted)` or `env.setProperty(Context.PROVIDER_URL, tainted)`.
@@ -61,7 +71,7 @@ private class ProviderUrlJndiInjectionSink extends JndiInjectionSink, DataFlow::
6171
ma.getMethod() = m and
6272
ma.getArgument(1) = this.getExpr()
6373
|
64-
m.getDeclaringType().getAnAncestor() instanceof TypeHashtable and
74+
m.getDeclaringType().getASourceSupertype*() instanceof TypeHashtable and
6575
(m.hasName("put") or m.hasName("setProperty")) and
6676
(
6777
ma.getArgument(0).(CompileTimeConstantExpr).getStringValue() = "java.naming.provider.url"
@@ -81,12 +91,12 @@ private class DefaultJndiInjectionSinkModel extends SinkModelCsv {
8191
override predicate row(string row) {
8292
row =
8393
[
84-
"javax.naming;InitialContext;true;lookup;;;Argument[0];jndi-injection",
85-
"javax.naming;InitialContext;true;lookupLink;;;Argument[0];jndi-injection",
86-
"javax.naming;InitialContext;true;doLookup;;;Argument[0];jndi-injection",
87-
"javax.naming;InitialContext;true;rename;;;Argument[0];jndi-injection",
88-
"javax.naming;InitialContext;true;list;;;Argument[0];jndi-injection",
89-
"javax.naming;InitialContext;true;listBindings;;;Argument[0];jndi-injection",
94+
"javax.naming;Context;true;lookup;;;Argument[0];jndi-injection",
95+
"javax.naming;Context;true;lookupLink;;;Argument[0];jndi-injection",
96+
"javax.naming;Context;true;doLookup;;;Argument[0];jndi-injection",
97+
"javax.naming;Context;true;rename;;;Argument[0];jndi-injection",
98+
"javax.naming;Context;true;list;;;Argument[0];jndi-injection",
99+
"javax.naming;Context;true;listBindings;;;Argument[0];jndi-injection",
90100
"javax.management.remote;JMXConnector;true;connect;;;Argument[-1];jndi-injection",
91101
"javax.management.remote;JMXConnectorFactory;false;connect;;;Argument[0];jndi-injection",
92102
// Spring
@@ -127,10 +137,92 @@ private class DefaultJndiInjectionSinkModel extends SinkModelCsv {
127137
}
128138
}
129139

140+
/**
141+
* Find flows between a `SearchControls` object with `setReturningObjFlag` = `true`
142+
* and an argument of a `LdapOperations.search` or `DirContext.search` call.
143+
*/
144+
private class UnsafeSearchControlsConf extends DataFlow2::Configuration {
145+
UnsafeSearchControlsConf() { this = "UnsafeSearchControlsConf" }
146+
147+
override predicate isSource(DataFlow2::Node source) { source instanceof UnsafeSearchControls }
148+
149+
override predicate isSink(DataFlow2::Node sink) { sink instanceof UnsafeSearchControlsArgument }
150+
}
151+
152+
/**
153+
* An argument of type `SearchControls` of a a `LdapOperations.search` or `DirContext.search` call.
154+
*/
155+
private class UnsafeSearchControlsArgument extends DataFlow2::ExprNode {
156+
UnsafeSearchControlsArgument() {
157+
exists(MethodAccess ma, Method m |
158+
ma.getMethod() = m and
159+
ma.getAnArgument() = this.asExpr() and
160+
this.asExpr().getType() instanceof TypeSearchControls and
161+
m.hasName("search")
162+
|
163+
m.getDeclaringType().getASourceSupertype*() instanceof TypeLdapOperations or
164+
m.getDeclaringType().getASourceSupertype*() instanceof TypeDirContext
165+
)
166+
}
167+
}
168+
169+
/**
170+
* A `SearchControls` object with `setReturningObjFlag` = `true`.
171+
*/
172+
private class UnsafeSearchControls extends DataFlow2::ExprNode {
173+
UnsafeSearchControls() {
174+
exists(MethodAccess ma |
175+
ma.getMethod() instanceof SetReturningObjFlagMethod and
176+
ma.getArgument(0).(CompileTimeConstantExpr).getBooleanValue() = true and
177+
this.asExpr() = ma.getQualifier()
178+
)
179+
or
180+
exists(ConstructorCall cc |
181+
cc.getConstructedType() instanceof TypeSearchControls and
182+
cc.getArgument(4).(CompileTimeConstantExpr).getBooleanValue() = true and
183+
this.asExpr() = cc
184+
)
185+
}
186+
}
187+
188+
/**
189+
* The method `SearchControls.setReturningObjFlag`.
190+
*/
191+
private class SetReturningObjFlagMethod extends Method {
192+
SetReturningObjFlagMethod() {
193+
this.getDeclaringType() instanceof TypeSearchControls and
194+
this.hasName("setReturningObjFlag")
195+
}
196+
}
197+
198+
/**
199+
* The class `javax.naming.directory.SearchControls`
200+
*/
201+
private class TypeSearchControls extends Class {
202+
TypeSearchControls() { this.hasQualifiedName("javax.naming.directory", "SearchControls") }
203+
}
204+
205+
/**
206+
* The interface `org.springframework.ldap.core.LdapOperations` or
207+
* `org.springframework.ldap.LdapOperations`
208+
*/
209+
private class TypeLdapOperations extends Interface {
210+
TypeLdapOperations() {
211+
this.hasQualifiedName(["org.springframework.ldap.core", "org.springframework.ldap"],
212+
"LdapOperations")
213+
}
214+
}
215+
216+
/** The class `java.util.Hashtable`. */
217+
private class TypeHashtable extends Class {
218+
TypeHashtable() { this.getSourceDeclaration().hasQualifiedName("java.util", "Hashtable") }
219+
}
220+
130221
/** A set of additional taint steps to consider when taint tracking JNDI injection related data flows. */
131222
private class DefaultJndiInjectionAdditionalTaintStep extends JndiInjectionAdditionalTaintStep {
132223
override predicate step(DataFlow::Node node1, DataFlow::Node node2) {
133224
nameStep(node1, node2) or
225+
nameAddStep(node1, node2) or
134226
jmxServiceUrlStep(node1, node2) or
135227
jmxConnectorStep(node1, node2) or
136228
rmiConnectorStep(node1, node2)
@@ -151,6 +243,24 @@ private predicate nameStep(DataFlow::ExprNode n1, DataFlow::ExprNode n2) {
151243
)
152244
}
153245

246+
/**
247+
* Holds if `n1` to `n2` is a dataflow step that converts between `String` and `CompositeName` or
248+
* `CompoundName`, i.e. `new CompositeName().add(tainted)` or `new CompoundName().add(tainted)`.
249+
*/
250+
private predicate nameAddStep(DataFlow::ExprNode n1, DataFlow::ExprNode n2) {
251+
exists(Method m, MethodAccess ma |
252+
ma.getMethod() = m and
253+
m.hasName("add") and
254+
(
255+
m.getDeclaringType() instanceof TypeCompositeName or
256+
m.getDeclaringType() instanceof TypeCompoundName
257+
)
258+
|
259+
n1.asExpr() = ma.getAnArgument() and
260+
n2.asExpr() = ma
261+
)
262+
}
263+
154264
/**
155265
* Holds if `n1` to `n2` is a dataflow step that converts between `String` and `JMXServiceURL`,
156266
* i.e. `new JMXServiceURL(tainted)`.
@@ -184,8 +294,3 @@ private predicate rmiConnectorStep(DataFlow::ExprNode n1, DataFlow::ExprNode n2)
184294
n2.asExpr() = cc
185295
)
186296
}
187-
188-
/** The class `java.util.Hashtable`. */
189-
private class TypeHashtable extends Class {
190-
TypeHashtable() { this.getSourceDeclaration().hasQualifiedName("java.util", "Hashtable") }
191-
}

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

Lines changed: 57 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -11,13 +11,15 @@
1111
import javax.naming.InitialContext;
1212
import javax.naming.Name;
1313
import javax.naming.NamingException;
14+
import javax.naming.directory.DirContext;
1415
import javax.naming.directory.InitialDirContext;
1516
import javax.naming.directory.SearchControls;
1617
import javax.naming.ldap.InitialLdapContext;
1718

1819
import org.springframework.jndi.JndiTemplate;
1920
import org.springframework.ldap.core.AttributesMapper;
2021
import org.springframework.ldap.core.ContextMapper;
22+
import org.springframework.ldap.core.DirContextProcessor;
2123
import org.springframework.ldap.core.LdapTemplate;
2224
import org.springframework.ldap.core.NameClassPairCallbackHandler;
2325
import org.springframework.stereotype.Controller;
@@ -47,9 +49,9 @@ public void testInitialContextBad1(@RequestParam String nameStr) throws NamingEx
4749
}
4850

4951
@RequestMapping
50-
public void testInitialDirContextBad1(@RequestParam String nameStr) throws NamingException {
52+
public void testDirContextBad1(@RequestParam String nameStr) throws NamingException {
5153
Name name = new CompoundName(nameStr, new Properties());
52-
InitialDirContext ctx = new InitialDirContext();
54+
DirContext ctx = new InitialDirContext();
5355

5456
ctx.lookup(nameStr); // $hasJndiInjection
5557
ctx.lookupLink(nameStr); // $hasJndiInjection
@@ -62,6 +64,19 @@ public void testInitialDirContextBad1(@RequestParam String nameStr) throws Namin
6264
ctx.rename(name, null); // $hasJndiInjection
6365
ctx.list(name); // $hasJndiInjection
6466
ctx.listBindings(name); // $hasJndiInjection
67+
68+
SearchControls searchControls = new SearchControls();
69+
searchControls.setReturningObjFlag(true);
70+
ctx.search(nameStr, "", searchControls); // $hasJndiInjection
71+
ctx.search(nameStr, "", new Object[] {}, searchControls); // $hasJndiInjection
72+
73+
SearchControls searchControls2 = new SearchControls(1, 0, 0, null, true, false);
74+
ctx.search(nameStr, "", searchControls2); // $hasJndiInjection
75+
ctx.search(nameStr, "", new Object[] {}, searchControls2); // $hasJndiInjection
76+
77+
SearchControls searchControls3 = new SearchControls(1, 0, 0, null, false, false);
78+
ctx.search(nameStr, "", searchControls3); // Safe
79+
ctx.search(nameStr, "", new Object[] {}, searchControls3); // Safe
6580
}
6681

6782
@RequestMapping
@@ -93,7 +108,7 @@ public void testSpringJndiTemplateBad1(@RequestParam String nameStr) throws Nami
93108
@RequestMapping
94109
public void testSpringLdapTemplateBad1(@RequestParam String nameStr) throws NamingException {
95110
LdapTemplate ctx = new LdapTemplate();
96-
Name name = new CompositeName(nameStr);
111+
Name name = new CompositeName().add(nameStr);
97112

98113
ctx.lookup(nameStr); // $hasJndiInjection
99114
ctx.lookupContext(nameStr); // $hasJndiInjection
@@ -104,11 +119,45 @@ public void testSpringLdapTemplateBad1(@RequestParam String nameStr) throws Nami
104119
ctx.unbind(nameStr, true); // $hasJndiInjection
105120

106121
ctx.search(nameStr, "", 0, true, null); // $hasJndiInjection
107-
ctx.search(nameStr, "", 0, new String[] {}, (ContextMapper<Object>) new Object()); // $hasJndiInjection
108-
ctx.search(nameStr, "", 0, (ContextMapper<Object>) new Object()); // $hasJndiInjection
109-
ctx.search(nameStr, "", (ContextMapper) new Object()); // $hasJndiInjection
110-
111-
ctx.searchForObject(nameStr, "", (ContextMapper) new Object()); // $hasJndiInjection
122+
ctx.search(nameStr, "", 0, new String[] {}, (ContextMapper<Object>) null); // $hasJndiInjection
123+
ctx.search(nameStr, "", 0, (ContextMapper<Object>) null); // $hasJndiInjection
124+
ctx.search(nameStr, "", (ContextMapper<Object>) null); // $hasJndiInjection
125+
126+
SearchControls searchControls = new SearchControls();
127+
searchControls.setReturningObjFlag(true);
128+
ctx.search(nameStr, "", searchControls, (AttributesMapper<Object>) null); // $hasJndiInjection
129+
ctx.search(nameStr, "", searchControls, (AttributesMapper<Object>) null, // $hasJndiInjection
130+
(DirContextProcessor) null);
131+
ctx.search(nameStr, "", searchControls, (ContextMapper<Object>) null); // $hasJndiInjection
132+
ctx.search(nameStr, "", searchControls, (ContextMapper<Object>) null, // $hasJndiInjection
133+
(DirContextProcessor) null);
134+
ctx.search(nameStr, "", searchControls, (NameClassPairCallbackHandler) null); // $hasJndiInjection
135+
ctx.search(nameStr, "", searchControls, (NameClassPairCallbackHandler) null, // $hasJndiInjection
136+
(DirContextProcessor) null);
137+
138+
SearchControls searchControls2 = new SearchControls(1, 0, 0, null, true, false);
139+
ctx.search(nameStr, "", searchControls2, (AttributesMapper<Object>) null); // $hasJndiInjection
140+
ctx.search(nameStr, "", searchControls2, (AttributesMapper<Object>) null, // $hasJndiInjection
141+
(DirContextProcessor) null);
142+
ctx.search(nameStr, "", searchControls2, (ContextMapper<Object>) null); // $hasJndiInjection
143+
ctx.search(nameStr, "", searchControls2, (ContextMapper<Object>) null, // $hasJndiInjection
144+
(DirContextProcessor) null);
145+
ctx.search(nameStr, "", searchControls2, (NameClassPairCallbackHandler) null); // $hasJndiInjection
146+
ctx.search(nameStr, "", searchControls2, (NameClassPairCallbackHandler) null, // $hasJndiInjection
147+
(DirContextProcessor) null);
148+
149+
SearchControls searchControls3 = new SearchControls(1, 0, 0, null, false, false);
150+
ctx.search(nameStr, "", searchControls3, (AttributesMapper<Object>) null); // Safe
151+
ctx.search(nameStr, "", searchControls3, (AttributesMapper<Object>) null, // Safe
152+
(DirContextProcessor) null);
153+
ctx.search(nameStr, "", searchControls3, (ContextMapper<Object>) null); // Safe
154+
ctx.search(nameStr, "", searchControls3, (ContextMapper<Object>) null, // Safe
155+
(DirContextProcessor) null);
156+
ctx.search(nameStr, "", searchControls3, (NameClassPairCallbackHandler) null); // Safe
157+
ctx.search(nameStr, "", searchControls3, (NameClassPairCallbackHandler) null, // Safe
158+
(DirContextProcessor) null);
159+
160+
ctx.searchForObject(nameStr, "", (ContextMapper<Object>) null); // $hasJndiInjection
112161
}
113162

114163
@RequestMapping

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

Lines changed: 82 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,84 @@
11
package org.springframework.ldap.core;
22

3-
public interface LdapOperations {}
3+
import java.util.*;
4+
5+
import javax.naming.Name;
6+
import javax.naming.directory.SearchControls;
7+
8+
import org.springframework.ldap.filter.Filter;
9+
10+
import org.springframework.ldap.query.LdapQuery;
11+
12+
public interface LdapOperations {
13+
void authenticate(LdapQuery query, String password);
14+
15+
boolean authenticate(Name base, String filter, String password);
16+
17+
<T> List<T> find(Name base, Filter filter, SearchControls searchControls, final Class<T> clazz);
18+
19+
<T> List<T> find(LdapQuery query, Class<T> clazz);
20+
21+
<T> T findOne(LdapQuery query, Class<T> clazz);
22+
23+
void search(String base, String filter, int searchScope, boolean returningObjFlag,
24+
NameClassPairCallbackHandler handler);
25+
26+
void search(final String base, final String filter, final SearchControls controls,
27+
NameClassPairCallbackHandler handler);
28+
29+
void search(final String base, final String filter, final SearchControls controls,
30+
NameClassPairCallbackHandler handler, DirContextProcessor processor);
31+
32+
void search(String base, String filter, NameClassPairCallbackHandler handler);
33+
34+
<T> List<T> search(String base, String filter, int searchScope, String[] attrs,
35+
AttributesMapper<T> mapper);
36+
37+
<T> List<T> search(String base, String filter, int searchScope, AttributesMapper<T> mapper);
38+
39+
<T> List<T> search(String base, String filter, AttributesMapper<T> mapper);
40+
41+
<T> List<T> search(String base, String filter, int searchScope, String[] attrs,
42+
ContextMapper<T> mapper);
43+
44+
<T> List<T> search(String base, String filter, int searchScope, ContextMapper<T> mapper);
45+
46+
<T> List<T> search(String base, String filter, ContextMapper<T> mapper);
47+
48+
<T> List<T> search(String base, String filter, SearchControls controls,
49+
ContextMapper<T> mapper);
50+
51+
<T> List<T> search(String base, String filter, SearchControls controls,
52+
AttributesMapper<T> mapper);
53+
54+
<T> List<T> search(String base, String filter, SearchControls controls,
55+
AttributesMapper<T> mapper, DirContextProcessor processor);
56+
57+
<T> List<T> search(String base, String filter, SearchControls controls, ContextMapper<T> mapper,
58+
DirContextProcessor processor);
59+
60+
DirContextOperations searchForContext(LdapQuery query);
61+
62+
<T> T searchForObject(Name base, String filter, ContextMapper<T> mapper);
63+
64+
<T> T searchForObject(String base, String filter, ContextMapper<T> mapper);
65+
66+
<T> T searchForObject(String base, String filter, SearchControls searchControls,
67+
ContextMapper<T> mapper);
68+
69+
Object lookup(final String dn);
70+
71+
DirContextOperations lookupContext(String dn);
72+
73+
<T> T findByDn(Name dn, final Class<T> clazz);
74+
75+
void rename(final Name oldDn, final Name newDn);
76+
77+
List<String> list(final Name base);
78+
79+
List<String> listBindings(final Name base);
80+
81+
void unbind(final String dn);
82+
83+
void unbind(final String dn, boolean recursive);
84+
}

0 commit comments

Comments
 (0)