Skip to content

Commit afea933

Browse files
ggolawskiggolawski
committed
Fix the case where user-controlled input is passed as URL to env Hashtable
1 parent df9921f commit afea933

File tree

3 files changed

+57
-50
lines changed

3 files changed

+57
-50
lines changed

‎java/ql/src/experimental/Security/CWE/CWE-074/JndiInjectionLib.qll

Lines changed: 28 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -24,11 +24,15 @@ class JndiInjectionFlowConfig extends TaintTracking::Configuration {
2424
nameStep(node1, node2) or
2525
jmxServiceUrlStep(node1, node2) or
2626
jmxConnectorStep(node1, node2) or
27-
rmiConnectorStep(node1, node2) or
28-
providerUrlEnvStep(node1, node2)
27+
rmiConnectorStep(node1, node2)
2928
}
3029
}
3130

31+
/** The class `java.util.Hashtable`. */
32+
class TypeHashtable extends Class {
33+
TypeHashtable() { this.getSourceDeclaration().hasQualifiedName("java.util", "Hashtable") }
34+
}
35+
3236
/** The class `javax.naming.directory.SearchControls`. */
3337
class TypeSearchControls extends Class {
3438
TypeSearchControls() { this.hasQualifiedName("javax.naming.directory", "SearchControls") }
@@ -100,7 +104,7 @@ predicate jndiSinkMethod(Method m, int index) {
100104
*/
101105
predicate springJndiTemplateSinkMethod(Method m, int index) {
102106
m.getDeclaringType() instanceof TypeSpringJndiTemplate and
103-
(m.hasName("lookup") or m.hasName("setEnvironment")) and
107+
m.hasName("lookup") and
104108
index = 0
105109
}
106110

@@ -155,13 +159,33 @@ predicate jmxConnectorFactorySinkMethod(Method m, int index) {
155159
index = 0
156160
}
157161

162+
/**
163+
* Tainted value passed to env `Hashtable` as the prodiver UDL, i.e.
164+
* `env.put(Context.PROVIDER_URL, tainted)` or `env.setProperty(Context.PROVIDER_URL, tainted)`.
165+
*/
166+
predicate providerUrlEnv(MethodAccess ma, Method m, int index) {
167+
m.getDeclaringType().getAnAncestor() instanceof TypeHashtable and
168+
(m.hasName("put") or m.hasName("setProperty")) and
169+
(
170+
ma.getArgument(0).(CompileTimeConstantExpr).getStringValue() = "java.naming.provider.url"
171+
or
172+
exists(Field f |
173+
ma.getArgument(0) = f.getAnAccess() and
174+
f.hasName("PROVIDER_URL") and
175+
f.getDeclaringType() instanceof TypeNamingContext
176+
)
177+
) and
178+
index = 1
179+
}
180+
158181
/** Holds if parameter at index `index` in method `m` is JNDI injection sink. */
159182
predicate jndiInjectionSinkMethod(MethodAccess ma, Method m, int index) {
160183
jndiSinkMethod(m, index) or
161184
springJndiTemplateSinkMethod(m, index) or
162185
springLdapTemplateSinkMethod(ma, m, index) or
163186
shiroSinkMethod(m, index) or
164-
jmxConnectorFactorySinkMethod(m, index)
187+
jmxConnectorFactorySinkMethod(m, index) or
188+
providerUrlEnv(ma, m, index)
165189
}
166190

167191
/** A data flow sink for unvalidated user input that is used in JNDI lookup. */
@@ -179,13 +203,6 @@ class JndiInjectionSink extends DataFlow::ExprNode {
179203
m.getDeclaringType().getAnAncestor() instanceof TypeJMXConnector and
180204
m.hasName("connect")
181205
)
182-
or
183-
exists(ConstructorCall cc |
184-
cc.getConstructedType().getAnAncestor() instanceof TypeInitialContext or
185-
cc.getConstructedType() instanceof TypeSpringJndiTemplate
186-
|
187-
cc.getArgument(0) = this.getExpr()
188-
)
189206
}
190207
}
191208

@@ -236,27 +253,3 @@ predicate rmiConnectorStep(ExprNode n1, ExprNode n2) {
236253
n2.asExpr() = cc
237254
)
238255
}
239-
240-
/**
241-
* Holds if `n1` to `n2` is a dataflow step that converts between `String` and
242-
* `Hashtable` or `Properties`, i.e. `env.put(Context.PROVIDER_URL, tainted)` or
243-
* `env.setProperty(Context.PROVIDER_URL, tainted)`.
244-
*/
245-
predicate providerUrlEnvStep(ExprNode n1, ExprNode n2) {
246-
exists(MethodAccess ma, Method m |
247-
n1.asExpr() = ma.getArgument(1) and n2.asExpr() = ma.getQualifier()
248-
|
249-
ma.getMethod() = m and
250-
m.getDeclaringType().getAnAncestor() instanceof TypeProperty and
251-
(m.hasName("put") or m.hasName("setProperty")) and
252-
(
253-
ma.getArgument(0).(CompileTimeConstantExpr).getStringValue() = "java.naming.provider.url"
254-
or
255-
exists(Field f |
256-
ma.getArgument(0) = f.getAnAccess() and
257-
f.hasName("PROVIDER_URL") and
258-
f.getDeclaringType() instanceof TypeNamingContext
259-
)
260-
)
261-
)
262-
}

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

Lines changed: 15 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -49,11 +49,11 @@ edges
4949
| JndiInjection.java:106:41:106:68 | nameStr : String | JndiInjection.java:110:16:110:22 | nameStr |
5050
| JndiInjection.java:113:37:113:63 | urlStr : String | JndiInjection.java:114:33:114:57 | new JMXServiceURL(...) |
5151
| JndiInjection.java:113:37:113:63 | urlStr : String | JndiInjection.java:118:5:118:13 | connector |
52-
| JndiInjection.java:121:27:121:53 | urlStr : String | JndiInjection.java:125:24:125:26 | env |
53-
| JndiInjection.java:128:27:128:53 | urlStr : String | JndiInjection.java:132:27:132:29 | env |
54-
| JndiInjection.java:135:52:135:78 | urlStr : String | JndiInjection.java:139:22:139:26 | props |
55-
| JndiInjection.java:142:52:142:78 | urlStr : String | JndiInjection.java:146:22:146:26 | props |
56-
| JndiInjection.java:149:52:149:78 | urlStr : String | JndiInjection.java:154:29:154:33 | props |
52+
| JndiInjection.java:121:27:121:53 | urlStr : String | JndiInjection.java:124:35:124:40 | urlStr |
53+
| JndiInjection.java:128:27:128:53 | urlStr : String | JndiInjection.java:131:41:131:46 | urlStr |
54+
| JndiInjection.java:135:52:135:78 | urlStr : String | JndiInjection.java:138:37:138:42 | urlStr |
55+
| JndiInjection.java:142:52:142:78 | urlStr : String | JndiInjection.java:145:51:145:56 | urlStr |
56+
| JndiInjection.java:149:52:149:78 | urlStr : String | JndiInjection.java:152:51:152:56 | urlStr |
5757
nodes
5858
| JndiInjection.java:26:38:26:65 | nameStr : String | semmle.label | nameStr : String |
5959
| JndiInjection.java:30:16:30:22 | nameStr | semmle.label | nameStr |
@@ -113,15 +113,15 @@ nodes
113113
| JndiInjection.java:114:33:114:57 | new JMXServiceURL(...) | semmle.label | new JMXServiceURL(...) |
114114
| JndiInjection.java:118:5:118:13 | connector | semmle.label | connector |
115115
| JndiInjection.java:121:27:121:53 | urlStr : String | semmle.label | urlStr : String |
116-
| JndiInjection.java:125:24:125:26 | env | semmle.label | env |
116+
| JndiInjection.java:124:35:124:40 | urlStr | semmle.label | urlStr |
117117
| JndiInjection.java:128:27:128:53 | urlStr : String | semmle.label | urlStr : String |
118-
| JndiInjection.java:132:27:132:29 | env | semmle.label | env |
118+
| JndiInjection.java:131:41:131:46 | urlStr | semmle.label | urlStr |
119119
| JndiInjection.java:135:52:135:78 | urlStr : String | semmle.label | urlStr : String |
120-
| JndiInjection.java:139:22:139:26 | props | semmle.label | props |
120+
| JndiInjection.java:138:37:138:42 | urlStr | semmle.label | urlStr |
121121
| JndiInjection.java:142:52:142:78 | urlStr : String | semmle.label | urlStr : String |
122-
| JndiInjection.java:146:22:146:26 | props | semmle.label | props |
122+
| JndiInjection.java:145:51:145:56 | urlStr | semmle.label | urlStr |
123123
| JndiInjection.java:149:52:149:78 | urlStr : String | semmle.label | urlStr : String |
124-
| JndiInjection.java:154:29:154:33 | props | semmle.label | props |
124+
| JndiInjection.java:152:51:152:56 | urlStr | semmle.label | urlStr |
125125
#select
126126
| JndiInjection.java:30:16:30:22 | nameStr | JndiInjection.java:26:38:26:65 | nameStr : String | JndiInjection.java:30:16:30:22 | nameStr | JNDI lookup might include name from $@. | JndiInjection.java:26:38:26:65 | nameStr | this user input |
127127
| JndiInjection.java:31:20:31:26 | nameStr | JndiInjection.java:26:38:26:65 | nameStr : String | JndiInjection.java:31:20:31:26 | nameStr | JNDI lookup might include name from $@. | JndiInjection.java:26:38:26:65 | nameStr | this user input |
@@ -173,8 +173,8 @@ nodes
173173
| JndiInjection.java:110:16:110:22 | nameStr | JndiInjection.java:106:41:106:68 | nameStr : String | JndiInjection.java:110:16:110:22 | nameStr | JNDI lookup might include name from $@. | JndiInjection.java:106:41:106:68 | nameStr | this user input |
174174
| JndiInjection.java:114:33:114:57 | new JMXServiceURL(...) | JndiInjection.java:113:37:113:63 | urlStr : String | JndiInjection.java:114:33:114:57 | new JMXServiceURL(...) | JNDI lookup might include name from $@. | JndiInjection.java:113:37:113:63 | urlStr | this user input |
175175
| JndiInjection.java:118:5:118:13 | connector | JndiInjection.java:113:37:113:63 | urlStr : String | JndiInjection.java:118:5:118:13 | connector | JNDI lookup might include name from $@. | JndiInjection.java:113:37:113:63 | urlStr | this user input |
176-
| JndiInjection.java:125:24:125:26 | env | JndiInjection.java:121:27:121:53 | urlStr : String | JndiInjection.java:125:24:125:26 | env | JNDI lookup might include name from $@. | JndiInjection.java:121:27:121:53 | urlStr | this user input |
177-
| JndiInjection.java:132:27:132:29 | env | JndiInjection.java:128:27:128:53 | urlStr : String | JndiInjection.java:132:27:132:29 | env | JNDI lookup might include name from $@. | JndiInjection.java:128:27:128:53 | urlStr | this user input |
178-
| JndiInjection.java:139:22:139:26 | props | JndiInjection.java:135:52:135:78 | urlStr : String | JndiInjection.java:139:22:139:26 | props | JNDI lookup might include name from $@. | JndiInjection.java:135:52:135:78 | urlStr | this user input |
179-
| JndiInjection.java:146:22:146:26 | props | JndiInjection.java:142:52:142:78 | urlStr : String | JndiInjection.java:146:22:146:26 | props | JNDI lookup might include name from $@. | JndiInjection.java:142:52:142:78 | urlStr | this user input |
180-
| JndiInjection.java:154:29:154:33 | props | JndiInjection.java:149:52:149:78 | urlStr : String | JndiInjection.java:154:29:154:33 | props | JNDI lookup might include name from $@. | JndiInjection.java:149:52:149:78 | urlStr | this user input |
176+
| JndiInjection.java:124:35:124:40 | urlStr | JndiInjection.java:121:27:121:53 | urlStr : String | JndiInjection.java:124:35:124:40 | urlStr | JNDI lookup might include name from $@. | JndiInjection.java:121:27:121:53 | urlStr | this user input |
177+
| JndiInjection.java:131:41:131:46 | urlStr | JndiInjection.java:128:27:128:53 | urlStr : String | JndiInjection.java:131:41:131:46 | urlStr | JNDI lookup might include name from $@. | JndiInjection.java:128:27:128:53 | urlStr | this user input |
178+
| JndiInjection.java:138:37:138:42 | urlStr | JndiInjection.java:135:52:135:78 | urlStr : String | JndiInjection.java:138:37:138:42 | urlStr | JNDI lookup might include name from $@. | JndiInjection.java:135:52:135:78 | urlStr | this user input |
179+
| JndiInjection.java:145:51:145:56 | urlStr | JndiInjection.java:142:52:142:78 | urlStr : String | JndiInjection.java:145:51:145:56 | urlStr | JNDI lookup might include name from $@. | JndiInjection.java:142:52:142:78 | urlStr | this user input |
180+
| JndiInjection.java:152:51:152:56 | urlStr | JndiInjection.java:149:52:149:78 | urlStr : String | JndiInjection.java:152:51:152:56 | urlStr | JNDI lookup might include name from $@. | JndiInjection.java:149:52:149:78 | urlStr | this user input |

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

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -174,4 +174,18 @@ public void testSpringLdapTemplateOk1(@RequestParam String nameStr) throws Namin
174174

175175
ctx.searchForObject(nameStr, "", new SearchControls(), (ContextMapper) new Object());
176176
}
177+
178+
public void testEnvOk1(@RequestParam String urlStr) throws NamingException {
179+
Hashtable<String, String> env = new Hashtable<String, String>();
180+
env.put(Context.INITIAL_CONTEXT_FACTORY, "com.sun.jndi.rmi.registry.RegistryContextFactory");
181+
env.put(Context.SECURITY_PRINCIPAL, urlStr);
182+
new InitialContext(env);
183+
}
184+
185+
public void testEnvOk2(@RequestParam String urlStr) throws NamingException {
186+
Hashtable<String, String> env = new Hashtable<String, String>();
187+
env.put(Context.INITIAL_CONTEXT_FACTORY, "com.sun.jndi.rmi.registry.RegistryContextFactory");
188+
env.put("java.naming.security.principal", urlStr);
189+
new InitialContext(env);
190+
}
177191
}

0 commit comments

Comments
 (0)