Skip to content

Commit eb1806c

Browse files
atorralbaatorralba
committed
Split PathMatchGuard into three guards
1 parent fb1287d commit eb1806c

File tree

1 file changed

+94
-80
lines changed

1 file changed

+94
-80
lines changed

‎java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qll

Lines changed: 94 additions & 80 deletions
Original file line numberDiff line numberDiff line change
@@ -34,14 +34,6 @@ private class FollowsSanitizingPrefix extends UnsafeUrlForwardSanitizer {
3434
/** A barrier guard that protects against URL forward vulnerabilities. */
3535
abstract class UnsafeUrlForwardBarrierGuard extends DataFlow::BarrierGuard { }
3636

37-
/**
38-
* Holds if `ma` is a call to a method that checks exact match of string.
39-
*/
40-
private predicate isExactStringPathMatch(MethodAccess ma) {
41-
ma.getMethod().getDeclaringType() instanceof TypeString and
42-
ma.getMethod().getName() = ["equals", "equalsIgnoreCase"]
43-
}
44-
4537
/**
4638
* Holds if `ma` is a call to a method that checks a path string.
4739
*/
@@ -59,105 +51,127 @@ private predicate isFilePathMatch(MethodAccess ma) {
5951
ma.getMethod().getName() = "startsWith"
6052
}
6153

62-
/**
63-
* Holds if `ma` protects against path traversal, by either:
64-
* * looking for the literal `..`
65-
* * performing path normalization
66-
*/
67-
private predicate isPathTraversalCheck(MethodAccess ma, Expr checked) {
68-
ma.getMethod().getDeclaringType() instanceof TypeString and
69-
ma.getMethod().hasName(["contains", "indexOf"]) and
70-
ma.getAnArgument().(CompileTimeConstantExpr).getStringValue() = ".." and
71-
ma.(Guard).controls(checked.getBasicBlock(), false)
72-
or
73-
ma.getMethod() instanceof PathNormalizeMethod and
74-
checked = ma
54+
/** A complementary guard that protects against path traversal, by looking for the literal `..`. */
55+
private class PathTraversalGuard extends Guard instanceof MethodAccess {
56+
Expr checked;
57+
58+
PathTraversalGuard() {
59+
this.getMethod().getDeclaringType() instanceof TypeString and
60+
this.getMethod().hasName(["contains", "indexOf"]) and
61+
this.getAnArgument().(CompileTimeConstantExpr).getStringValue() = ".." and
62+
this.controls(checked.getBasicBlock(), false)
63+
}
64+
65+
predicate checks(Expr e) { checked = e }
7566
}
7667

77-
/**
78-
* Holds if `ma` protects against double URL encoding, by either:
79-
* * looking for the literal `%`
80-
* * performing URL decoding
81-
*/
82-
private predicate isURLEncodingCheck(MethodAccess ma, Expr checked) {
83-
// Search the special character `%` used in url encoding
84-
ma.getMethod().getDeclaringType() instanceof TypeString and
85-
ma.getMethod().hasName(["contains", "indexOf"]) and
86-
ma.getAnArgument().(CompileTimeConstantExpr).getStringValue() = "%" and
87-
ma.(Guard).controls(checked.getBasicBlock(), false)
88-
or
89-
// Call to `URLDecoder` assuming the implementation handles double encoding correctly
90-
ma.getMethod().getDeclaringType().hasQualifiedName("java.net", "URLDecoder") and
91-
ma.getMethod().hasName("decode") and
92-
checked = ma
68+
/** A complementary sanitizer that protects against path traversal using path normalization. */
69+
private class PathNormalizeSanitizer extends MethodAccess {
70+
PathNormalizeSanitizer() {
71+
this.getMethod().getDeclaringType().hasQualifiedName("java.nio.file", "Path") and
72+
this.getMethod().hasName("normalize")
73+
}
74+
}
75+
76+
/** A complementary guard that protects against double URL encoding, by looking for the literal `%`. */
77+
private class UrlEncodingGuard extends Guard instanceof MethodAccess {
78+
Expr checked;
79+
80+
UrlEncodingGuard() {
81+
this.getMethod().getDeclaringType() instanceof TypeString and
82+
this.getMethod().hasName(["contains", "indexOf"]) and
83+
this.getAnArgument().(CompileTimeConstantExpr).getStringValue() = "%" and
84+
this.controls(checked.getBasicBlock(), false)
85+
}
86+
87+
predicate checks(Expr e) { checked = e }
9388
}
9489

95-
/** The Java method `normalize` of `java.nio.file.Path`. */
96-
private class PathNormalizeMethod extends Method {
97-
PathNormalizeMethod() {
98-
this.getDeclaringType().hasQualifiedName("java.nio.file", "Path") and
99-
this.hasName("normalize")
90+
/** A complementary sanitizer that protects against double URL encoding using URL decoding. */
91+
private class UrlDecodeSanitizer extends MethodAccess {
92+
UrlDecodeSanitizer() {
93+
this.getMethod().getDeclaringType().hasQualifiedName("java.net", "URLDecoder") and
94+
this.getMethod().hasName("decode")
10095
}
10196
}
10297

10398
private predicate isDisallowedWord(CompileTimeConstantExpr word) {
10499
word.getStringValue().matches(["%WEB-INF%", "%META-INF%", "%..%"])
105100
}
106101

107-
private predicate isAllowListCheck(MethodAccess ma) {
108-
(isStringPathMatch(ma) or isFilePathMatch(ma)) and
109-
not isDisallowedWord(ma.getAnArgument())
110-
}
102+
/**
103+
* A guard that considers safe a string being exactly compared to a trusted value.
104+
*/
105+
private class ExactStringPathMatchGuard extends UnsafeUrlForwardBarrierGuard instanceof MethodAccess {
106+
ExactStringPathMatchGuard() {
107+
this.getMethod().getDeclaringType() instanceof TypeString and
108+
this.getMethod().getName() = ["equals", "equalsIgnoreCase"]
109+
}
111110

112-
private predicate isDisallowListCheck(MethodAccess ma) {
113-
(isStringPathMatch(ma) or isFilePathMatch(ma)) and
114-
isDisallowedWord(ma.getAnArgument())
111+
override predicate checks(Expr e, boolean branch) {
112+
e = this.(MethodAccess).getQualifier() and
113+
branch = true
114+
}
115115
}
116116

117117
/**
118-
* A guard that checks a path with the following methods:
119-
* 1. Exact string match
120-
* 2. Path matches allowed values (needs to protect against path traversal)
121-
* 3. Path matches disallowed values (needs to protect against URL encoding)
118+
* A guard that considers safe a string being matched against an allowlist of partial trusted values.
119+
* This requires additional protection against path traversal, either another guard (`PathTraversalGuard`)
120+
* or a sanitizer (`PathNormalizeSanitizer`).
122121
*/
123-
private class PathMatchGuard extends UnsafeUrlForwardBarrierGuard {
124-
PathMatchGuard() {
125-
isExactStringPathMatch(this) or isAllowListCheck(this) or isDisallowListCheck(this)
122+
private class AllowListCheckGuard extends UnsafeUrlForwardBarrierGuard instanceof MethodAccess {
123+
AllowListCheckGuard() {
124+
(isStringPathMatch(this) or isFilePathMatch(this)) and
125+
not isDisallowedWord(this.getAnArgument())
126126
}
127127

128128
override predicate checks(Expr e, boolean branch) {
129129
e = this.(MethodAccess).getQualifier() and
130+
branch = true and
130131
(
131-
isExactStringPathMatch(this) and
132-
branch = true
132+
// Either the path normalization sanitizer comes before the guard
133+
exists(PathNormalizeSanitizer sanitizer | DataFlow::localExprFlow(sanitizer, e))
133134
or
134-
// When using an allowlist, that is, checking for known safe paths
135-
// (for example, `path.startsWith(BASE_PATH)`)
136-
// the application needs to protect against path traversal bypasses.
137-
isAllowListCheck(this) and
138-
exists(MethodAccess ma, Expr checked | isPathTraversalCheck(ma, checked) |
139-
// Either the path traversal check comes before the guard
140-
DataFlow::localExprFlow(checked, e) or
135+
// or the path traversal check comes before the guard
136+
exists(PathTraversalGuard guard |
137+
guard.checks(any(Expr checked | DataFlow::localExprFlow(checked, e))) or
141138
// or both checks are in the same condition
142139
// (for example, `path.startsWith(BASE_PATH) && !path.contains("..")`)
143-
ma.(Guard).controls(this.getBasicBlock(), _) or
144-
this.controls(ma.getBasicBlock(), branch)
145-
) and
146-
branch = true
140+
guard.controls(this.getBasicBlock().(ConditionBlock), false) or
141+
this.controls(guard.getBasicBlock().(ConditionBlock), branch)
142+
)
143+
)
144+
}
145+
}
146+
147+
/**
148+
* A guard that considers safe a string being matched against a blocklist of known dangerous values.
149+
* This requires additional protection against path traversal, either another guard (`UrlEncodingGuard`)
150+
* or a sanitizer (`UrlDecodeSanitizer`).
151+
*/
152+
private class BlockListCheckGuard extends UnsafeUrlForwardBarrierGuard instanceof MethodAccess {
153+
BlockListCheckGuard() {
154+
(isStringPathMatch(this) or isFilePathMatch(this)) and
155+
isDisallowedWord(this.getAnArgument())
156+
}
157+
158+
override predicate checks(Expr e, boolean branch) {
159+
e = this.(MethodAccess).getQualifier() and
160+
branch = false and
161+
(
162+
// Either the URL decode sanitization comes before the guard
163+
exists(UrlDecodeSanitizer sanitizer | DataFlow::localExprFlow(sanitizer, e))
147164
or
148-
// When using a blocklist, that is, checking for known bad patterns in the path,
149-
// (for example, `path.startsWith("/WEB-INF/")` or `path.contains("..")`)
150-
// the application needs to protect against double URL encoding bypasses.
151-
isDisallowListCheck(this) and
152-
exists(MethodAccess ma, Expr checked | isURLEncodingCheck(ma, checked) |
153-
// Either the URL encoding check comes before the guard
154-
DataFlow::localExprFlow(checked, e) or
165+
// or the URL encoding check comes before the guard
166+
exists(UrlEncodingGuard guard |
167+
guard.checks(any(Expr checked | DataFlow::localExprFlow(checked, e)))
168+
or
155169
// or both checks are in the same condition
156170
// (for example, `!path.contains("..") && !path.contains("%")`)
157-
ma.(Guard).controls(this.getBasicBlock(), _) or
158-
this.controls(ma.getBasicBlock(), branch)
159-
) and
160-
branch = false
171+
guard.controls(this.getBasicBlock().(ConditionBlock), false)
172+
or
173+
this.controls(guard.getBasicBlock().(ConditionBlock), branch)
174+
)
161175
)
162176
}
163177
}

0 commit comments

Comments
 (0)