Skip to content

Commit be7a8ec

Browse files
owen-mcCopilot
owen-mc
and
Copilot
committed
Java: Require non-null host allowlist matches
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent df832a4 commit be7a8ec

6 files changed

Lines changed: 143 additions & 63 deletions

File tree

‎java/ql/lib/semmle/code/java/security/ConstantValueSanitizer.qll‎

Lines changed: 70 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ private import codeql.typeflow.UniversalFlow as UniversalFlow
77
private import semmle.code.java.Collections
88
private import semmle.code.java.controlflow.Guards
99
private import semmle.code.java.dataflow.internal.BaseSSA as Base
10+
private import semmle.code.java.dataflow.NullGuards
1011
private import semmle.code.java.dataflow.TaintTracking
1112
private import semmle.code.java.dataflow.TypeFlow
1213
private import semmle.code.java.dispatch.VirtualDispatch
@@ -154,6 +155,29 @@ private predicate collectionWithPossibleMutation(FlowNode n) {
154155
)
155156
}
156157

158+
/** Holds if `n` is a collection constructed by an API that rejects null elements. */
159+
private predicate nullRejectingCollectionCreation(FlowNode n) {
160+
exists(Call c, Callable tgt |
161+
n.asExpr() = c and
162+
c.getCallee().getSourceDeclaration() = tgt
163+
|
164+
tgt.hasQualifiedName("java.util", ["List", "Set"], ["copyOf", "of"])
165+
or
166+
tgt.hasQualifiedName("com.google.common.collect", ["ImmutableList", "ImmutableSet"],
167+
["copyOf", "of"])
168+
)
169+
}
170+
171+
private module NullRejectingCollectionProp implements BaseUniversalFlow::NullaryPropertySig {
172+
predicate hasPropertyBase(FlowNode n) { nullRejectingCollectionCreation(n) }
173+
174+
predicate barrier = collectionWithPossibleMutation/1;
175+
}
176+
177+
/** Holds if the given collection cannot contain null. */
178+
private predicate nullRejectingCollection =
179+
BaseUniversalFlow::FlowNullary<NullRejectingCollectionProp>::hasProperty/1;
180+
157181
/**
158182
* A collection constructor that constructs an empty mutable collection.
159183
*/
@@ -238,18 +262,26 @@ private MethodCall normalizeCaseCall(Expr arg) {
238262
* Holds if the guard `g` ensures that the expression `e` is one of a set of
239263
* known constants upon evaluating to `branch`.
240264
*/
241-
private predicate constantCollectionContainsCheck(Guard g, Expr e, boolean branch) {
242-
exists(MethodCall mc, Method m, FlowNode n, Expr checked |
243-
g = mc and
265+
private predicate constantCollectionContainsCall(
266+
MethodCall contains, FlowNode collection, Expr checked
267+
) {
268+
exists(MethodCall mc, Method m, FlowNode n |
269+
contains = mc and
244270
mc.getMethod().getSourceDeclaration().overridesOrInstantiates*(m) and
245271
m.hasQualifiedName("java.util", "Collection", "contains") and
246272
n.asExpr() = mc.getQualifier() and
247273
constantCollection(n) and
248274
checked = mc.getAnArgument() and
249-
branch = true
250-
|
251-
checked = e or
252-
checked = normalizeCaseCall(e)
275+
collection = n
276+
)
277+
}
278+
279+
private predicate constantCollectionContainsCheck(Guard g, Expr e, boolean branch) {
280+
exists(MethodCall contains, FlowNode collection, Expr checked |
281+
constantCollectionContainsCall(contains, collection, checked) and
282+
g = contains and
283+
branch = true and
284+
(checked = e or checked = normalizeCaseCall(e))
253285
)
254286
}
255287

@@ -282,6 +314,37 @@ predicate constantValueGuardChecks(Guard g, Expr e, boolean branch) {
282314
constantCollectionContainsCheck(g, e, branch)
283315
}
284316

317+
/**
318+
* Holds if `g` ensures that `e` is a non-null member of a set of compile-time constants upon
319+
* evaluating to `branch`.
320+
*/
321+
predicate nonNullConstantValueGuardChecks(Guard g, Expr e, boolean branch) {
322+
constantStringEqualityCheck(g, e, branch)
323+
or
324+
exists(MethodCall contains, FlowNode collection, Expr checked |
325+
constantCollectionContainsCall(contains, collection, checked) and
326+
g = contains and
327+
branch = true and
328+
(
329+
checked = e and
330+
(
331+
nullRejectingCollection(collection)
332+
or
333+
e = clearlyNotNullExpr()
334+
)
335+
or
336+
checked = normalizeCaseCall(e) and
337+
(
338+
nullRejectingCollection(collection)
339+
or
340+
checked.(MethodCall).getQualifier() = e
341+
or
342+
e = clearlyNotNullExpr()
343+
)
344+
)
345+
)
346+
}
347+
285348
/**
286349
* A check against compile-time constants, sanitizing taint by restricting a value to a known set.
287350
*/

‎java/ql/lib/semmle/code/java/security/RequestForgery.qll‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -151,7 +151,7 @@ private predicate isHostComparisonSanitizer(Guard guard, Expr e, boolean branch)
151151
)
152152
or
153153
exists(Expr checked |
154-
constantValueGuardChecks(guard, checked, branch) and
154+
nonNullConstantValueGuardChecks(guard, checked, branch) and
155155
e = getCheckedHost(checked).getQualifier()
156156
)
157157
}

‎java/ql/test/query-tests/security/CWE-601/semmle/tests/HostComparisonRedirect.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -221,10 +221,10 @@ public void normalizedConstantCollection(HttpServletRequest request, HttpServlet
221221
}
222222

223223
public void nullContainingCollection(HttpServletRequest request, HttpServletResponse response) throws Exception {
224-
URI uri = new URI(request.getParameter("url")); // $ MISSING: Source
224+
URI uri = new URI(request.getParameter("url")); // $ Source
225225
String host = uri.getHost();
226226
if (Arrays.asList("example.com", null).contains(host)) {
227-
response.sendRedirect(uri.toString()); // $ MISSING: Alert
227+
response.sendRedirect(uri.toString()); // $ Alert
228228
}
229229
}
230230

‎java/ql/test/query-tests/security/CWE-601/semmle/tests/UrlRedirect.expected‎

Lines changed: 32 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -14,9 +14,10 @@
1414
| HostComparisonRedirect.java:157:35:157:48 | toString(...) | HostComparisonRedirect.java:155:27:155:53 | getParameter(...) : String | HostComparisonRedirect.java:157:35:157:48 | toString(...) | Untrusted URL redirection depends on a $@. | HostComparisonRedirect.java:155:27:155:53 | getParameter(...) | user-provided value |
1515
| HostComparisonRedirect.java:185:39:185:52 | toString(...) | HostComparisonRedirect.java:178:27:178:53 | getParameter(...) : String | HostComparisonRedirect.java:185:39:185:52 | toString(...) | Untrusted URL redirection depends on a $@. | HostComparisonRedirect.java:178:27:178:53 | getParameter(...) | user-provided value |
1616
| HostComparisonRedirect.java:227:35:227:48 | toString(...) | HostComparisonRedirect.java:224:27:224:53 | getParameter(...) : String | HostComparisonRedirect.java:227:35:227:48 | toString(...) | Untrusted URL redirection depends on a $@. | HostComparisonRedirect.java:224:27:224:53 | getParameter(...) | user-provided value |
17-
| HostComparisonRedirect.java:235:35:235:48 | toString(...) | HostComparisonRedirect.java:232:27:232:53 | getParameter(...) : String | HostComparisonRedirect.java:235:35:235:48 | toString(...) | Untrusted URL redirection depends on a $@. | HostComparisonRedirect.java:232:27:232:53 | getParameter(...) | user-provided value |
18-
| HostComparisonRedirect.java:261:35:261:48 | toString(...) | HostComparisonRedirect.java:257:27:257:53 | getParameter(...) : String | HostComparisonRedirect.java:261:35:261:48 | toString(...) | Untrusted URL redirection depends on a $@. | HostComparisonRedirect.java:257:27:257:53 | getParameter(...) | user-provided value |
19-
| HostComparisonRedirect.java:270:35:270:48 | toString(...) | HostComparisonRedirect.java:266:27:266:53 | getParameter(...) : String | HostComparisonRedirect.java:270:35:270:48 | toString(...) | Untrusted URL redirection depends on a $@. | HostComparisonRedirect.java:266:27:266:53 | getParameter(...) | user-provided value |
17+
| HostComparisonRedirect.java:243:35:243:48 | toString(...) | HostComparisonRedirect.java:240:27:240:53 | getParameter(...) : String | HostComparisonRedirect.java:243:35:243:48 | toString(...) | Untrusted URL redirection depends on a $@. | HostComparisonRedirect.java:240:27:240:53 | getParameter(...) | user-provided value |
18+
| HostComparisonRedirect.java:251:35:251:48 | toString(...) | HostComparisonRedirect.java:248:27:248:53 | getParameter(...) : String | HostComparisonRedirect.java:251:35:251:48 | toString(...) | Untrusted URL redirection depends on a $@. | HostComparisonRedirect.java:248:27:248:53 | getParameter(...) | user-provided value |
19+
| HostComparisonRedirect.java:277:35:277:48 | toString(...) | HostComparisonRedirect.java:273:27:273:53 | getParameter(...) : String | HostComparisonRedirect.java:277:35:277:48 | toString(...) | Untrusted URL redirection depends on a $@. | HostComparisonRedirect.java:273:27:273:53 | getParameter(...) | user-provided value |
20+
| HostComparisonRedirect.java:286:35:286:48 | toString(...) | HostComparisonRedirect.java:282:27:282:53 | getParameter(...) : String | HostComparisonRedirect.java:286:35:286:48 | toString(...) | Untrusted URL redirection depends on a $@. | HostComparisonRedirect.java:282:27:282:53 | getParameter(...) | user-provided value |
2021
| MicronautUrlRedirect.java:14:38:14:55 | create(...) | MicronautUrlRedirect.java:12:39:12:63 | target : String | MicronautUrlRedirect.java:14:38:14:55 | create(...) | Untrusted URL redirection depends on a $@. | MicronautUrlRedirect.java:12:39:12:63 | target | user-provided value |
2122
| MicronautUrlRedirect.java:28:47:28:64 | create(...) | MicronautUrlRedirect.java:27:45:27:69 | target : String | MicronautUrlRedirect.java:28:47:28:64 | create(...) | Untrusted URL redirection depends on a $@. | MicronautUrlRedirect.java:27:45:27:69 | target | user-provided value |
2223
| MicronautUrlRedirect.java:33:38:33:55 | create(...) | MicronautUrlRedirect.java:32:44:32:68 | target : String | MicronautUrlRedirect.java:33:38:33:55 | create(...) | Untrusted URL redirection depends on a $@. | MicronautUrlRedirect.java:32:44:32:68 | target | user-provided value |
@@ -73,15 +74,18 @@ edges
7374
| HostComparisonRedirect.java:224:19:224:54 | new URI(...) : URI | HostComparisonRedirect.java:227:35:227:37 | uri : URI | provenance | |
7475
| HostComparisonRedirect.java:224:27:224:53 | getParameter(...) : String | HostComparisonRedirect.java:224:19:224:54 | new URI(...) : URI | provenance | Src:MaD:6 MaD:8 |
7576
| HostComparisonRedirect.java:227:35:227:37 | uri : URI | HostComparisonRedirect.java:227:35:227:48 | toString(...) | provenance | MaD:10 |
76-
| HostComparisonRedirect.java:232:19:232:54 | new URI(...) : URI | HostComparisonRedirect.java:235:35:235:37 | uri : URI | provenance | |
77-
| HostComparisonRedirect.java:232:27:232:53 | getParameter(...) : String | HostComparisonRedirect.java:232:19:232:54 | new URI(...) : URI | provenance | Src:MaD:6 MaD:8 |
78-
| HostComparisonRedirect.java:235:35:235:37 | uri : URI | HostComparisonRedirect.java:235:35:235:48 | toString(...) | provenance | MaD:10 |
79-
| HostComparisonRedirect.java:257:19:257:54 | new URI(...) : URI | HostComparisonRedirect.java:261:35:261:37 | uri : URI | provenance | |
80-
| HostComparisonRedirect.java:257:27:257:53 | getParameter(...) : String | HostComparisonRedirect.java:257:19:257:54 | new URI(...) : URI | provenance | Src:MaD:6 MaD:8 |
81-
| HostComparisonRedirect.java:261:35:261:37 | uri : URI | HostComparisonRedirect.java:261:35:261:48 | toString(...) | provenance | MaD:10 |
82-
| HostComparisonRedirect.java:266:19:266:54 | new URI(...) : URI | HostComparisonRedirect.java:270:35:270:37 | uri : URI | provenance | |
83-
| HostComparisonRedirect.java:266:27:266:53 | getParameter(...) : String | HostComparisonRedirect.java:266:19:266:54 | new URI(...) : URI | provenance | Src:MaD:6 MaD:8 |
84-
| HostComparisonRedirect.java:270:35:270:37 | uri : URI | HostComparisonRedirect.java:270:35:270:48 | toString(...) | provenance | MaD:10 |
77+
| HostComparisonRedirect.java:240:19:240:54 | new URI(...) : URI | HostComparisonRedirect.java:243:35:243:37 | uri : URI | provenance | |
78+
| HostComparisonRedirect.java:240:27:240:53 | getParameter(...) : String | HostComparisonRedirect.java:240:19:240:54 | new URI(...) : URI | provenance | Src:MaD:6 MaD:8 |
79+
| HostComparisonRedirect.java:243:35:243:37 | uri : URI | HostComparisonRedirect.java:243:35:243:48 | toString(...) | provenance | MaD:10 |
80+
| HostComparisonRedirect.java:248:19:248:54 | new URI(...) : URI | HostComparisonRedirect.java:251:35:251:37 | uri : URI | provenance | |
81+
| HostComparisonRedirect.java:248:27:248:53 | getParameter(...) : String | HostComparisonRedirect.java:248:19:248:54 | new URI(...) : URI | provenance | Src:MaD:6 MaD:8 |
82+
| HostComparisonRedirect.java:251:35:251:37 | uri : URI | HostComparisonRedirect.java:251:35:251:48 | toString(...) | provenance | MaD:10 |
83+
| HostComparisonRedirect.java:273:19:273:54 | new URI(...) : URI | HostComparisonRedirect.java:277:35:277:37 | uri : URI | provenance | |
84+
| HostComparisonRedirect.java:273:27:273:53 | getParameter(...) : String | HostComparisonRedirect.java:273:19:273:54 | new URI(...) : URI | provenance | Src:MaD:6 MaD:8 |
85+
| HostComparisonRedirect.java:277:35:277:37 | uri : URI | HostComparisonRedirect.java:277:35:277:48 | toString(...) | provenance | MaD:10 |
86+
| HostComparisonRedirect.java:282:19:282:54 | new URI(...) : URI | HostComparisonRedirect.java:286:35:286:37 | uri : URI | provenance | |
87+
| HostComparisonRedirect.java:282:27:282:53 | getParameter(...) : String | HostComparisonRedirect.java:282:19:282:54 | new URI(...) : URI | provenance | Src:MaD:6 MaD:8 |
88+
| HostComparisonRedirect.java:286:35:286:37 | uri : URI | HostComparisonRedirect.java:286:35:286:48 | toString(...) | provenance | MaD:10 |
8589
| MicronautUrlRedirect.java:12:39:12:63 | target : String | MicronautUrlRedirect.java:14:49:14:54 | target : String | provenance | |
8690
| MicronautUrlRedirect.java:14:49:14:54 | target : String | MicronautUrlRedirect.java:14:38:14:55 | create(...) | provenance | MaD:9 Sink:MaD:2 |
8791
| MicronautUrlRedirect.java:27:45:27:69 | target : String | MicronautUrlRedirect.java:28:58:28:63 | target : String | provenance | |
@@ -168,18 +172,22 @@ nodes
168172
| HostComparisonRedirect.java:224:27:224:53 | getParameter(...) : String | semmle.label | getParameter(...) : String |
169173
| HostComparisonRedirect.java:227:35:227:37 | uri : URI | semmle.label | uri : URI |
170174
| HostComparisonRedirect.java:227:35:227:48 | toString(...) | semmle.label | toString(...) |
171-
| HostComparisonRedirect.java:232:19:232:54 | new URI(...) : URI | semmle.label | new URI(...) : URI |
172-
| HostComparisonRedirect.java:232:27:232:53 | getParameter(...) : String | semmle.label | getParameter(...) : String |
173-
| HostComparisonRedirect.java:235:35:235:37 | uri : URI | semmle.label | uri : URI |
174-
| HostComparisonRedirect.java:235:35:235:48 | toString(...) | semmle.label | toString(...) |
175-
| HostComparisonRedirect.java:257:19:257:54 | new URI(...) : URI | semmle.label | new URI(...) : URI |
176-
| HostComparisonRedirect.java:257:27:257:53 | getParameter(...) : String | semmle.label | getParameter(...) : String |
177-
| HostComparisonRedirect.java:261:35:261:37 | uri : URI | semmle.label | uri : URI |
178-
| HostComparisonRedirect.java:261:35:261:48 | toString(...) | semmle.label | toString(...) |
179-
| HostComparisonRedirect.java:266:19:266:54 | new URI(...) : URI | semmle.label | new URI(...) : URI |
180-
| HostComparisonRedirect.java:266:27:266:53 | getParameter(...) : String | semmle.label | getParameter(...) : String |
181-
| HostComparisonRedirect.java:270:35:270:37 | uri : URI | semmle.label | uri : URI |
182-
| HostComparisonRedirect.java:270:35:270:48 | toString(...) | semmle.label | toString(...) |
175+
| HostComparisonRedirect.java:240:19:240:54 | new URI(...) : URI | semmle.label | new URI(...) : URI |
176+
| HostComparisonRedirect.java:240:27:240:53 | getParameter(...) : String | semmle.label | getParameter(...) : String |
177+
| HostComparisonRedirect.java:243:35:243:37 | uri : URI | semmle.label | uri : URI |
178+
| HostComparisonRedirect.java:243:35:243:48 | toString(...) | semmle.label | toString(...) |
179+
| HostComparisonRedirect.java:248:19:248:54 | new URI(...) : URI | semmle.label | new URI(...) : URI |
180+
| HostComparisonRedirect.java:248:27:248:53 | getParameter(...) : String | semmle.label | getParameter(...) : String |
181+
| HostComparisonRedirect.java:251:35:251:37 | uri : URI | semmle.label | uri : URI |
182+
| HostComparisonRedirect.java:251:35:251:48 | toString(...) | semmle.label | toString(...) |
183+
| HostComparisonRedirect.java:273:19:273:54 | new URI(...) : URI | semmle.label | new URI(...) : URI |
184+
| HostComparisonRedirect.java:273:27:273:53 | getParameter(...) : String | semmle.label | getParameter(...) : String |
185+
| HostComparisonRedirect.java:277:35:277:37 | uri : URI | semmle.label | uri : URI |
186+
| HostComparisonRedirect.java:277:35:277:48 | toString(...) | semmle.label | toString(...) |
187+
| HostComparisonRedirect.java:282:19:282:54 | new URI(...) : URI | semmle.label | new URI(...) : URI |
188+
| HostComparisonRedirect.java:282:27:282:53 | getParameter(...) : String | semmle.label | getParameter(...) : String |
189+
| HostComparisonRedirect.java:286:35:286:37 | uri : URI | semmle.label | uri : URI |
190+
| HostComparisonRedirect.java:286:35:286:48 | toString(...) | semmle.label | toString(...) |
183191
| MicronautUrlRedirect.java:12:39:12:63 | target : String | semmle.label | target : String |
184192
| MicronautUrlRedirect.java:14:38:14:55 | create(...) | semmle.label | create(...) |
185193
| MicronautUrlRedirect.java:14:49:14:54 | target : String | semmle.label | target : String |

‎java/ql/test/query-tests/security/CWE-918/HostComparison.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -220,10 +220,10 @@ public void normalizedConstantCollection(HttpServletRequest request) throws Exce
220220
}
221221

222222
public void nullContainingCollection(HttpServletRequest request) throws Exception {
223-
URI uri = new URI(request.getParameter("url")); // $ MISSING: Source
223+
URI uri = new URI(request.getParameter("url")); // $ Source
224224
String host = uri.getHost();
225225
if (Arrays.asList("example.com", null).contains(host)) {
226-
uri.toURL().openConnection(); // $ MISSING: Alert
226+
uri.toURL().openConnection(); // $ Alert
227227
}
228228
}
229229

0 commit comments

Comments
 (0)