Skip to content

Commit 42b6b26

Browse files
atorralbaatorralba
committed
Decouple JndiInjection.qll to reuse the taint tracking configuration
1 parent b8ea833 commit 42b6b26

File tree

9 files changed

+133
-142
lines changed

9 files changed

+133
-142
lines changed

‎java/change-notes/2021-05-20-jndi-injection-query.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,2 +1,2 @@
11
lgtm,codescanning
2-
* The query "JNDI lookup with user-controlled name" (`java/jndi-injection`) has been promoted from experimental to the main query pack. Its results will now appear by default. This query was originally [submitted as an experimental query by @ggolawski](https://github.com/github/codeql/pull/3288)
2+
* The query "JNDI lookup with user-controlled name" (`java/jndi-injection`) has been promoted from experimental to the main query pack. Its results will now appear by default. This query was originally [submitted as an experimental query by @ggolawski](https://github.com/github/codeql/pull/3288).

‎java/ql/src/Security/CWE/CWE-074/JndiInjection.ql

Lines changed: 1 addition & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -11,29 +11,9 @@
1111
*/
1212

1313
import java
14-
import semmle.code.java.dataflow.FlowSources
15-
import semmle.code.java.security.JndiInjection
14+
import semmle.code.java.security.JndiInjectionQuery
1615
import DataFlow::PathGraph
1716

18-
/**
19-
* A taint-tracking configuration for unvalidated user input that is used in JNDI lookup.
20-
*/
21-
class JndiInjectionFlowConfig extends TaintTracking::Configuration {
22-
JndiInjectionFlowConfig() { this = "JndiInjectionFlowConfig" }
23-
24-
override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
25-
26-
override predicate isSink(DataFlow::Node sink) { sink instanceof JndiInjectionSink }
27-
28-
override predicate isSanitizer(DataFlow::Node node) {
29-
node.getType() instanceof PrimitiveType or node.getType() instanceof BoxedType
30-
}
31-
32-
override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) {
33-
any(JndiInjectionAdditionalTaintStep c).step(node1, node2)
34-
}
35-
}
36-
3717
from DataFlow::PathNode source, DataFlow::PathNode sink, JndiInjectionFlowConfig conf
3818
where conf.hasFlowPath(source, sink)
3919
select sink.getNode(), source, sink, "JNDI lookup might include name from $@.", source.getNode(),

‎java/ql/src/semmle/code/java/frameworks/Jndi.qll

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,11 @@ class TypeDirContext extends Interface {
2727
TypeDirContext() { this.hasQualifiedName("javax.naming.directory", "DirContext") }
2828
}
2929

30+
/** The class `javax.naming.directory.SearchControls` */
31+
class TypeSearchControls extends Class {
32+
TypeSearchControls() { this.hasQualifiedName("javax.naming.directory", "SearchControls") }
33+
}
34+
3035
/** The class `javax.naming.ldap.LdapName`. */
3136
class TypeLdapName extends Class {
3237
TypeLdapName() { this.hasQualifiedName("javax.naming.ldap", "LdapName") }

‎java/ql/src/semmle/code/java/frameworks/SpringLdap.qll

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,17 @@ class TypeSpringLdapUtils extends Class {
5959
TypeSpringLdapUtils() { this.hasQualifiedName("org.springframework.ldap.support", "LdapUtils") }
6060
}
6161

62+
/**
63+
* The interface `org.springframework.ldap.core.LdapOperations` or
64+
* `org.springframework.ldap.LdapOperations`
65+
*/
66+
class TypeLdapOperations extends Interface {
67+
TypeLdapOperations() {
68+
this.hasQualifiedName(["org.springframework.ldap.core", "org.springframework.ldap"],
69+
"LdapOperations")
70+
}
71+
}
72+
6273
/*--- Methods ---*/
6374
/**
6475
* A method with the name `authenticate` declared in

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

Lines changed: 10 additions & 100 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,10 @@
11
/** Provides classes to reason about JNDI injection vulnerabilities. */
22

33
import java
4-
import semmle.code.java.dataflow.DataFlow
5-
import semmle.code.java.dataflow.DataFlow2
6-
import semmle.code.java.dataflow.ExternalFlow
7-
import semmle.code.java.frameworks.Jndi
8-
import semmle.code.java.frameworks.SpringLdap
4+
private import semmle.code.java.dataflow.DataFlow
5+
private import semmle.code.java.dataflow.ExternalFlow
6+
private import semmle.code.java.frameworks.Jndi
7+
private import semmle.code.java.frameworks.SpringLdap
98

109
/** A data flow sink for unvalidated user input that is used in JNDI lookup. */
1110
abstract class JndiInjectionSink extends DataFlow::Node { }
@@ -48,19 +47,6 @@ private class ConditionedJndiInjectionSink extends JndiInjectionSink, DataFlow::
4847
}
4948
}
5049

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-
6450
/**
6551
* Tainted value passed to env `Hashtable` as the provider URL by calling
6652
* `env.put(Context.PROVIDER_URL, tainted)` or `env.setProperty(Context.PROVIDER_URL, tainted)`.
@@ -93,10 +79,10 @@ private class DefaultJndiInjectionSinkModel extends SinkModelCsv {
9379
[
9480
"javax.naming;Context;true;lookup;;;Argument[0];jndi-injection",
9581
"javax.naming;Context;true;lookupLink;;;Argument[0];jndi-injection",
96-
"javax.naming;Context;true;doLookup;;;Argument[0];jndi-injection",
9782
"javax.naming;Context;true;rename;;;Argument[0];jndi-injection",
9883
"javax.naming;Context;true;list;;;Argument[0];jndi-injection",
9984
"javax.naming;Context;true;listBindings;;;Argument[0];jndi-injection",
85+
"javax.naming;InitialContext;true;doLookup;;;Argument[0];jndi-injection",
10086
"javax.management.remote;JMXConnector;true;connect;;;Argument[-1];jndi-injection",
10187
"javax.management.remote;JMXConnectorFactory;false;connect;;;Argument[0];jndi-injection",
10288
// Spring
@@ -137,87 +123,6 @@ private class DefaultJndiInjectionSinkModel extends SinkModelCsv {
137123
}
138124
}
139125

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-
221126
/** A set of additional taint steps to consider when taint tracking JNDI injection related data flows. */
222127
private class DefaultJndiInjectionAdditionalTaintStep extends JndiInjectionAdditionalTaintStep {
223128
override predicate step(DataFlow::Node node1, DataFlow::Node node2) {
@@ -294,3 +199,8 @@ private predicate rmiConnectorStep(DataFlow::ExprNode n1, DataFlow::ExprNode n2)
294199
n2.asExpr() = cc
295200
)
296201
}
202+
203+
/** The class `java.util.Hashtable`. */
204+
private class TypeHashtable extends Class {
205+
TypeHashtable() { this.getSourceDeclaration().hasQualifiedName("java.util", "Hashtable") }
206+
}

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

Lines changed: 100 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,100 @@
1+
import java
2+
import semmle.code.java.dataflow.FlowSources
3+
import semmle.code.java.frameworks.Jndi
4+
import semmle.code.java.frameworks.SpringLdap
5+
import semmle.code.java.security.JndiInjection
6+
7+
/**
8+
* A taint-tracking configuration for unvalidated user input that is used in JNDI lookup.
9+
*/
10+
class JndiInjectionFlowConfig extends TaintTracking::Configuration {
11+
JndiInjectionFlowConfig() { this = "JndiInjectionFlowConfig" }
12+
13+
override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
14+
15+
override predicate isSink(DataFlow::Node sink) { sink instanceof JndiInjectionSink }
16+
17+
override predicate isSanitizer(DataFlow::Node node) {
18+
node.getType() instanceof PrimitiveType or node.getType() instanceof BoxedType
19+
}
20+
21+
override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) {
22+
any(JndiInjectionAdditionalTaintStep c).step(node1, node2)
23+
}
24+
}
25+
26+
/**
27+
* A method that does a JNDI lookup when it receives a `SearchControls` argument with `setReturningObjFlag` = `true`
28+
*/
29+
private class UnsafeSearchControlsSink extends JndiInjectionSink {
30+
UnsafeSearchControlsSink() {
31+
exists(UnsafeSearchControlsConf conf, MethodAccess ma |
32+
conf.hasFlowTo(DataFlow::exprNode(ma.getAnArgument()))
33+
|
34+
this.asExpr() = ma.getArgument(0)
35+
)
36+
}
37+
}
38+
39+
/**
40+
* Find flows between a `SearchControls` object with `setReturningObjFlag` = `true`
41+
* and an argument of a `LdapOperations.search` or `DirContext.search` call.
42+
*/
43+
private class UnsafeSearchControlsConf extends DataFlow2::Configuration {
44+
UnsafeSearchControlsConf() { this = "UnsafeSearchControlsConf" }
45+
46+
override predicate isSource(DataFlow2::Node source) { source instanceof UnsafeSearchControls }
47+
48+
override predicate isSink(DataFlow2::Node sink) { sink instanceof UnsafeSearchControlsArgument }
49+
}
50+
51+
/**
52+
* An argument of type `SearchControls` of a a `LdapOperations.search` or `DirContext.search` call.
53+
*/
54+
private class UnsafeSearchControlsArgument extends DataFlow2::ExprNode {
55+
UnsafeSearchControlsArgument() {
56+
exists(MethodAccess ma, Method m |
57+
ma.getMethod() = m and
58+
ma.getAnArgument() = this.asExpr() and
59+
this.asExpr().getType() instanceof TypeSearchControls and
60+
m.hasName("search")
61+
|
62+
m.getDeclaringType().getASourceSupertype*() instanceof TypeLdapOperations or
63+
m.getDeclaringType().getASourceSupertype*() instanceof TypeDirContext
64+
)
65+
}
66+
}
67+
68+
/**
69+
* A `SearchControls` object with `setReturningObjFlag` = `true`.
70+
*/
71+
private class UnsafeSearchControls extends DataFlow2::ExprNode {
72+
UnsafeSearchControls() {
73+
exists(MethodAccess ma |
74+
ma.getMethod() instanceof SetReturningObjFlagMethod and
75+
ma.getArgument(0).(CompileTimeConstantExpr).getBooleanValue() = true and
76+
this.asExpr() = ma.getQualifier()
77+
)
78+
or
79+
exists(ConstructorCall cc |
80+
cc.getConstructedType() instanceof TypeSearchControls and
81+
cc.getArgument(4).(CompileTimeConstantExpr).getBooleanValue() = true and
82+
this.asExpr() = cc
83+
)
84+
}
85+
}
86+
87+
/**
88+
* The method `SearchControls.setReturningObjFlag`.
89+
*/
90+
private class SetReturningObjFlagMethod extends Method {
91+
SetReturningObjFlagMethod() {
92+
this.getDeclaringType() instanceof TypeSearchControls and
93+
this.hasName("setReturningObjFlag")
94+
}
95+
}
96+
97+
/** The class `java.util.Hashtable`. */
98+
private class TypeHashtable extends Class {
99+
TypeHashtable() { this.getSourceDeclaration().hasQualifiedName("java.util", "Hashtable") }
100+
}

‎java/ql/test/experimental/query-tests/security/CWE-074-JndiInjection/options

Lines changed: 0 additions & 1 deletion
This file was deleted.

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

Lines changed: 4 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1,33 +1,19 @@
11
import java
22
import semmle.code.java.dataflow.TaintTracking
33
import semmle.code.java.dataflow.FlowSources
4-
import semmle.code.java.security.JndiInjection
4+
import semmle.code.java.security.JndiInjectionQuery
55
import TestUtilities.InlineExpectationsTest
66

7-
class Conf extends TaintTracking::Configuration {
8-
Conf() { this = "test:cwe:jndiinjection" }
9-
10-
override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
11-
12-
override predicate isSink(DataFlow::Node sink) { sink instanceof JndiInjectionSink }
13-
14-
override predicate isSanitizer(DataFlow::Node node) {
15-
node.getType() instanceof PrimitiveType or node.getType() instanceof BoxedType
16-
}
17-
18-
override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) {
19-
any(JndiInjectionAdditionalTaintStep c).step(node1, node2)
20-
}
21-
}
22-
237
class HasJndiInjectionTest extends InlineExpectationsTest {
248
HasJndiInjectionTest() { this = "HasJndiInjectionTest" }
259

2610
override string getARelevantTag() { result = "hasJndiInjection" }
2711

2812
override predicate hasActualResult(Location location, string element, string tag, string value) {
2913
tag = "hasJndiInjection" and
30-
exists(DataFlow::Node src, DataFlow::Node sink, Conf conf | conf.hasFlow(src, sink) |
14+
exists(DataFlow::Node src, DataFlow::Node sink, JndiInjectionFlowConfig conf |
15+
conf.hasFlow(src, sink)
16+
|
3117
sink.getLocation() = location and
3218
element = sink.toString() and
3319
value = ""

‎java/ql/test/query-tests/security/CWE-074/options

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
//semmle-extractor-options: --javac-args -cp ${testdir}/../../../stubs/springframework-5.2.3:${testdir}/../../../stubs/shiro-core-1.5.2:${testdir}/../../../stubs/spring-ldap-2.3.2
1+
//semmle-extractor-options: --javac-args -cp ${testdir}/../../../stubs/springframework-5.3.8:${testdir}/../../../stubs/shiro-core-1.5.2:${testdir}/../../../stubs/spring-ldap-2.3.2

0 commit comments

Comments
 (0)