Skip to content

Commit 5c8ab1f

Browse files
authored
Merge pull request #20956 from owen-mc/java/improve-regex-sanitizer
Java: improve regex sanitizer for `java/ssrf`
2 parents cdd8aa4 + e710c15 commit 5c8ab1f

File tree

6 files changed

+339
-256
lines changed

6 files changed

+339
-256
lines changed
Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: minorAnalysis
3+
---
4+
* A sanitizer has been added to `java/ssrf` to remove alerts when a regular expression check is used to verify that the value is safe.

java/ql/lib/semmle/code/java/frameworks/Regex.qll

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,11 +4,46 @@ module;
44

55
import java
66

7+
/** The class `java.util.regex.Matcher`. */
8+
class TypeRegexMatcher extends Class {
9+
TypeRegexMatcher() { this.hasQualifiedName("java.util.regex", "Matcher") }
10+
}
11+
12+
/**
13+
* The `matches` method of `java.util.regex.Matcher`.
14+
*/
15+
class MatcherMatchesMethod extends Method {
16+
MatcherMatchesMethod() {
17+
this.getDeclaringType() instanceof TypeRegexMatcher and
18+
this.hasName("matches")
19+
}
20+
}
21+
722
/** The class `java.util.regex.Pattern`. */
823
class TypeRegexPattern extends Class {
924
TypeRegexPattern() { this.hasQualifiedName("java.util.regex", "Pattern") }
1025
}
1126

27+
/**
28+
* The `matches` method of `java.util.regex.Pattern`.
29+
*/
30+
class PatternMatchesMethod extends Method {
31+
PatternMatchesMethod() {
32+
this.getDeclaringType() instanceof TypeRegexPattern and
33+
this.hasName("matches")
34+
}
35+
}
36+
37+
/**
38+
* The `matcher` method of `java.util.regex.Pattern`.
39+
*/
40+
class PatternMatcherMethod extends Method {
41+
PatternMatcherMethod() {
42+
this.getDeclaringType() instanceof TypeRegexPattern and
43+
this.hasName("matcher")
44+
}
45+
}
46+
1247
/** The `quote` method of the `java.util.regex.Pattern` class. */
1348
class PatternQuoteMethod extends Method {
1449
PatternQuoteMethod() {

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

Lines changed: 3 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -166,22 +166,7 @@ private class HostComparisonSanitizer extends RequestForgerySanitizer {
166166
}
167167

168168
/**
169-
* A qualifier in a call to a `.matches()` method that is a sanitizer for URL redirects.
170-
*
171-
* Matches any method call where the method is named `matches`.
172-
*/
173-
private predicate isMatchesSanitizer(Guard guard, Expr e, boolean branch) {
174-
guard =
175-
any(MethodCall method |
176-
method.getMethod().getName() = "matches" and
177-
e = method.getQualifier() and
178-
branch = true
179-
)
180-
}
181-
182-
/**
183-
* A qualifier in a call to `.matches()` that is a sanitizer for URL redirects.
169+
* A comparison with a regular expression that is a sanitizer for URL redirects.
184170
*/
185-
private class MatchesSanitizer extends RequestForgerySanitizer {
186-
MatchesSanitizer() { this = DataFlow::BarrierGuard<isMatchesSanitizer/3>::getABarrierNode() }
187-
}
171+
private class RegexpCheckRequestForgerySanitizer extends RequestForgerySanitizer instanceof RegexpCheckBarrier
172+
{ }

java/ql/lib/semmle/code/java/security/Sanitizers.qll

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,9 @@ overlay[local?]
33
module;
44

55
import java
6+
private import semmle.code.java.controlflow.Guards
67
private import semmle.code.java.dataflow.DataFlow
8+
private import semmle.code.java.frameworks.Regex
79

810
/**
911
* A node whose type is a simple type unlikely to carry taint, such as primitives and their boxed counterparts,
@@ -29,3 +31,44 @@ class SimpleTypeSanitizer extends DataFlow::Node {
2931
this.getType() instanceof EnumType
3032
}
3133
}
34+
35+
/**
36+
* Holds if `guard` holds with branch `branch` if `e` matches a regular expression.
37+
*
38+
* This is overapproximate: we do not attempt to reason about the correctness of the regexp.
39+
*
40+
* Use this if you want to define a derived `DataFlow::BarrierGuard` without
41+
* make the type recursive. Otherwise use `RegexpCheckBarrier`.
42+
*/
43+
predicate regexpMatchGuardChecks(Guard guard, Expr e, boolean branch) {
44+
exists(Method method, MethodCall mc |
45+
method = mc.getMethod() and
46+
guard = mc and
47+
branch = true
48+
|
49+
// `String.matches` and other `matches` methods.
50+
method.getName() = "matches" and
51+
e = mc.getQualifier()
52+
or
53+
method instanceof PatternMatchesMethod and
54+
e = mc.getArgument(1)
55+
or
56+
method instanceof MatcherMatchesMethod and
57+
exists(MethodCall matcherCall |
58+
matcherCall.getMethod() instanceof PatternMatcherMethod and
59+
e = matcherCall.getArgument(0) and
60+
DataFlow::localExprFlow(matcherCall, mc.getQualifier())
61+
)
62+
)
63+
}
64+
65+
/**
66+
* A check against a regular expression, considered as a barrier guard.
67+
*
68+
* This is overapproximate: we do not attempt to reason about the correctness of the regexp.
69+
*/
70+
class RegexpCheckBarrier extends DataFlow::Node {
71+
RegexpCheckBarrier() {
72+
this = DataFlow::BarrierGuard<regexpMatchGuardChecks/3>::getABarrierNode()
73+
}
74+
}

0 commit comments

Comments
 (0)