Skip to content

Java: CWE-552 Query to detect unsafe request dispatcher usage #7286

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
smowton merged 17 commits into from
Jan 18, 2022
Merged

Java: CWE-552 Query to detect unsafe request dispatcher usage #7286

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
8bcffc2
Query to detect unsafe request dispatcher usage
luchua-bc Dec 2, 2021
29ce0e9
Add sanitizer for virtual method calls
luchua-bc Dec 15, 2021
263dbd3
Optimize the query
luchua-bc Jan 12, 2022
cd9a485
Refactor NullOrEmptyCheckGuard
atorralba Jan 13, 2022
81feaae
Refactor PathMatchGuard
atorralba Jan 13, 2022
b6886b8
Move code to qll file
atorralba Jan 13, 2022
877c529
Remove the deprecated library keyword
luchua-bc Jan 14, 2022
136fefb
Apply suggestions from code review
atorralba Jan 14, 2022
fb1287d
Use dominance instead of getParent
atorralba Jan 14, 2022
eb1806c
Split PathMatchGuard into three guards
atorralba Jan 14, 2022
a2c98ba
Reordering
atorralba Jan 14, 2022
978ef15
Update method names
luchua-bc Jan 16, 2022
4797fce
Update use cases and qldoc
luchua-bc Jan 16, 2022
a3d65a8
Update recommendation in qldoc and make examples more comprehendible
luchua-bc Jan 18, 2022
748008a
Remove dangling reference to UnsafeRequestPath.java
smowton Jan 18, 2022
d744cf9
Clean up guard logic:
smowton Jan 18, 2022
1e32514
Avoid using `this` for a non-extending supertype, and remove needless…
smowton Jan 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Refactor PathMatchGuard
  • Loading branch information
@atorralba
atorralba committed Jan 13, 2022
commit 81feaaec0270c7b9635d539eaa7f3877d34e9340
156 changes: 57 additions & 99 deletions java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,15 +19,15 @@ import semmle.code.java.dataflow.NullGuards
import DataFlow::PathGraph

/**
* Holds if `ma` is a call to a method that checks exact match of string, probably a whitelisted one.
* Holds if `ma` is a call to a method that checks exact match of string.
*/
predicate isExactStringPathMatch(MethodAccess ma) {
ma.getMethod().getDeclaringType() instanceof TypeString and
ma.getMethod().getName() = ["equals", "equalsIgnoreCase"]
}

/**
* Holds if `ma` is a call to a method that checks a path string, probably a whitelisted one.
* Holds if `ma` is a call to a method that checks a path string.
*/
predicate isStringPathMatch(MethodAccess ma) {
ma.getMethod().getDeclaringType() instanceof TypeString and
Expand All @@ -36,46 +36,44 @@ predicate isStringPathMatch(MethodAccess ma) {
}

/**
* Holds if `ma` is a call to a method of `java.nio.file.Path` that checks a path, probably
* a whitelisted one.
* Holds if `ma` is a call to a method of `java.nio.file.Path` that checks a path.
*/
predicate isFilePathMatch(MethodAccess ma) {
ma.getMethod().getDeclaringType() instanceof TypePath and
ma.getMethod().getName() = "startsWith"
}

/**
* Holds if `ma` is a call to a method that checks an input doesn't match using the `!`
* logical negation expression.
* Holds if `ma` protects against path traversal, by either:
* * looking for the literal `..`
* * performing path normalization
*/
predicate checkNoPathMatch(MethodAccess ma) {
exists(LogNotExpr lne |
(isStringPathMatch(ma) or isFilePathMatch(ma)) and
lne.getExpr() = ma
)
}

/**
* Holds if `ma` is a call to a method that checks special characters `..` used in path traversal.
*/
predicate isPathTraversalCheck(MethodAccess ma) {
predicate isPathTraversalCheck(MethodAccess ma, Expr checked) {
ma.getMethod().getDeclaringType() instanceof TypeString and
ma.getMethod().hasName(["contains", "indexOf"]) and
ma.getAnArgument().(CompileTimeConstantExpr).getStringValue() = ".."
ma.getAnArgument().(CompileTimeConstantExpr).getStringValue() = ".." and
ma.(Guard).controls(checked.getBasicBlock(), false)
or
ma.getMethod() instanceof PathNormalizeMethod and
checked = ma
}

/**
* Holds if `ma` is a call to a method that decodes a URL string or check URL encoding.
* Holds if `ma` protects against double URL encoding, by either:
* * looking for the literal `%`
* * performing URL decoding
*/
predicate isPathDecoding(MethodAccess ma) {
predicate isURLEncodingCheck(MethodAccess ma, Expr checked) {
// Search the special character `%` used in url encoding
ma.getMethod().getDeclaringType() instanceof TypeString and
ma.getMethod().hasName(["contains", "indexOf"]) and
ma.getAnArgument().(CompileTimeConstantExpr).getStringValue() = "%"
ma.getAnArgument().(CompileTimeConstantExpr).getStringValue() = "%" and
ma.(Guard).controls(checked.getBasicBlock(), false)
or
// Call to `URLDecoder` assuming the implementation handles double encoding correctly
ma.getMethod().getDeclaringType().hasQualifiedName("java.net", "URLDecoder") and
ma.getMethod().hasName("decode")
ma.getMethod().hasName("decode") and
checked = ma
}

/** The Java method `normalize` of `java.nio.file.Path`. */
Expand All @@ -86,90 +84,54 @@ class PathNormalizeMethod extends Method {
}
}

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

private predicate isAllowListCheck(MethodAccess ma) {
(isStringPathMatch(ma) or isFilePathMatch(ma)) and
not isDisallowedWord(ma.getAnArgument())
}

private predicate isDisallowListCheck(MethodAccess ma) {
(isStringPathMatch(ma) or isFilePathMatch(ma)) and
isDisallowedWord(ma.getAnArgument())
}

/**
* Sanitizer to check the following scenarios in a web application:
* A guard that checks a path with the following methods:
* 1. Exact string match
* 2. String startsWith or match check with path traversal validation
* 3. String not startsWith or not match check with decoding processing
* 4. java.nio.file.Path startsWith check having path normalization
* 2. Path matches allowed values (needs to protect against path traversal)
* 3. Path matches disallowed values (needs to protect against URL encoding)
*/
private class PathMatchGuard extends DataFlow::BarrierGuard {
PathMatchGuard() {
isExactStringPathMatch(this)
or
isStringPathMatch(this) and
not checkNoPathMatch(this) and
exists(MethodAccess tma |
isPathTraversalCheck(tma) and
DataFlow::localExprFlow(this.(MethodAccess).getQualifier(), tma.getQualifier())
)
or
checkNoPathMatch(this) and
exists(MethodAccess dma |
isPathDecoding(dma) and
DataFlow::localExprFlow(dma, this.(MethodAccess).getQualifier())
)
or
isFilePathMatch(this) and
exists(MethodAccess pma |
pma.getMethod() instanceof PathNormalizeMethod and
DataFlow::localExprFlow(pma, this.(MethodAccess).getQualifier())
)
isExactStringPathMatch(this) or isStringPathMatch(this) or isFilePathMatch(this)
}

override predicate checks(Expr e, boolean branch) {
e = this.(MethodAccess).getQualifier() and
(
branch = true and not checkNoPathMatch(this)
isExactStringPathMatch(this) and
branch = true
or
branch = false and checkNoPathMatch(this)
)
}
}

/**
* Holds if `ma` is a call to a method that checks string content, which means an input string is not
* blindly trusted and helps to reduce FPs.
*/
predicate checkStringContent(MethodAccess ma, Expr expr) {
ma.getMethod().getDeclaringType() instanceof TypeString and
ma.getMethod()
.hasName([
"charAt", "getBytes", "getChars", "length", "replace", "replaceAll", "replaceFirst",
"substring"
]) and
expr = ma.getQualifier()
or
(
ma.getMethod().getDeclaringType() instanceof TypeStringBuffer or
ma.getMethod().getDeclaringType() instanceof TypeStringBuilder
) and
expr = ma.getAnArgument()
}

private class StringOperationSanitizer extends DataFlow::Node {
StringOperationSanitizer() { exists(MethodAccess ma | checkStringContent(ma, this.asExpr())) }
}

private class NullOrEmptyCheckGuard extends DataFlow::BarrierGuard {
NullOrEmptyCheckGuard() {
this = nullGuard(_, _, _)
or
exists(MethodAccess ma |
cb.getCondition() = ma and
ma.getMethod().getDeclaringType() instanceof TypeString and
ma.getMethod().hasName("equals") and
ma.getArgument(0).(CompileTimeConstantExpr).getStringValue() = "" and
this = ma
isAllowListCheck(this) and
exists(MethodAccess ma, Expr checked | isPathTraversalCheck(ma, checked) |
DataFlow::localExprFlow(checked, e)
or
ma.getParent*().(BinaryExpr) = this.(MethodAccess).getParent*()
) and
branch = true
or
isDisallowListCheck(this) and
exists(MethodAccess ma, Expr checked | isURLEncodingCheck(ma, checked) |
DataFlow::localExprFlow(checked, e)
or
ma.getParent*().(BinaryExpr) = this.(MethodAccess).getParent*()
) and
branch = false
)
}

override predicate checks(Expr e, boolean branch) {
exists(SsaVariable ssa | this = nullGuard(ssa, branch, true) and e = ssa.getAFirstUse())
or
e = this.(MethodAccess).getQualifier() and
branch = true
}
}

class UnsafeUrlForwardFlowConfig extends TaintTracking::Configuration {
Expand All @@ -189,14 +151,10 @@ class UnsafeUrlForwardFlowConfig extends TaintTracking::Configuration {

override predicate isSink(DataFlow::Node sink) { sink instanceof UnsafeUrlForwardSink }

override predicate isSanitizer(DataFlow::Node node) {
node instanceof UnsafeUrlForwardSanitizer or
node instanceof StringOperationSanitizer
}
override predicate isSanitizer(DataFlow::Node node) { node instanceof UnsafeUrlForwardSanitizer }

override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
guard instanceof PathMatchGuard or
guard instanceof NullOrEmptyCheckGuard
guard instanceof PathMatchGuard
}

override DataFlow::FlowFeature getAFeature() {
Expand Down
6 changes: 3 additions & 3 deletions java/ql/test/experimental/query-tests/security/CWE-552/UnsafeServletRequestDispatch.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ protected void doHead2(HttpServletRequest request, HttpServletResponse response)
// GOOD: Request dispatcher with path traversal check
protected void doHead3(HttpServletRequest request, HttpServletResponse response)
throws ServletException, IOException {
String path = request.getParameter("path");
String path = request.getParameter("path");

if (path.startsWith(BASE_PATH) && !path.contains("..")) {
request.getServletContext().getRequestDispatcher(path).include(request, response);
Expand All @@ -100,7 +100,7 @@ protected void doHead4(HttpServletRequest request, HttpServletResponse response)
}
}

// GOOD: Request dispatcher with negation check and path normalization
// BAD: Request dispatcher with negation check and path normalization, but without URL decoding
protected void doHead5(HttpServletRequest request, HttpServletResponse response)
throws ServletException, IOException {
String path = request.getParameter("path");
Expand All @@ -111,7 +111,7 @@ protected void doHead5(HttpServletRequest request, HttpServletResponse response)
}
}

// GOOD: Request dispatcher with path traversal check and url decoding
// GOOD: Request dispatcher with path traversal check and URL decoding
protected void doHead6(HttpServletRequest request, HttpServletResponse response)
throws ServletException, IOException {
String path = request.getParameter("path");
Expand Down
12 changes: 12 additions & 0 deletions java/ql/test/experimental/query-tests/security/CWE-552/UnsafeUrlForward.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,11 @@ edges
| UnsafeServletRequestDispatch.java:23:22:23:54 | getParameter(...) : String | UnsafeServletRequestDispatch.java:32:51:32:59 | returnURL |
| UnsafeServletRequestDispatch.java:42:22:42:54 | getParameter(...) : String | UnsafeServletRequestDispatch.java:48:56:48:64 | returnURL |
| UnsafeServletRequestDispatch.java:71:17:71:44 | getParameter(...) : String | UnsafeServletRequestDispatch.java:76:53:76:56 | path |
| UnsafeServletRequestDispatch.java:106:17:106:44 | getParameter(...) : String | UnsafeServletRequestDispatch.java:107:53:107:56 | path : String |
| UnsafeServletRequestDispatch.java:107:24:107:57 | resolve(...) : Path | UnsafeServletRequestDispatch.java:107:24:107:69 | normalize(...) : Path |
| UnsafeServletRequestDispatch.java:107:24:107:69 | normalize(...) : Path | UnsafeServletRequestDispatch.java:110:53:110:65 | requestedPath : Path |
| UnsafeServletRequestDispatch.java:107:53:107:56 | path : String | UnsafeServletRequestDispatch.java:107:24:107:57 | resolve(...) : Path |
| UnsafeServletRequestDispatch.java:110:53:110:65 | requestedPath : Path | UnsafeServletRequestDispatch.java:110:53:110:76 | toString(...) |
| UnsafeUrlForward.java:13:27:13:36 | url : String | UnsafeUrlForward.java:14:27:14:29 | url |
| UnsafeUrlForward.java:18:27:18:36 | url : String | UnsafeUrlForward.java:20:28:20:30 | url |
| UnsafeUrlForward.java:25:21:25:30 | url : String | UnsafeUrlForward.java:26:23:26:25 | url |
Expand All @@ -20,6 +25,12 @@ nodes
| UnsafeServletRequestDispatch.java:48:56:48:64 | returnURL | semmle.label | returnURL |
| UnsafeServletRequestDispatch.java:71:17:71:44 | getParameter(...) : String | semmle.label | getParameter(...) : String |
| UnsafeServletRequestDispatch.java:76:53:76:56 | path | semmle.label | path |
| UnsafeServletRequestDispatch.java:106:17:106:44 | getParameter(...) : String | semmle.label | getParameter(...) : String |
| UnsafeServletRequestDispatch.java:107:24:107:57 | resolve(...) : Path | semmle.label | resolve(...) : Path |
| UnsafeServletRequestDispatch.java:107:24:107:69 | normalize(...) : Path | semmle.label | normalize(...) : Path |
| UnsafeServletRequestDispatch.java:107:53:107:56 | path : String | semmle.label | path : String |
| UnsafeServletRequestDispatch.java:110:53:110:65 | requestedPath : Path | semmle.label | requestedPath : Path |
| UnsafeServletRequestDispatch.java:110:53:110:76 | toString(...) | semmle.label | toString(...) |
| UnsafeUrlForward.java:13:27:13:36 | url : String | semmle.label | url : String |
| UnsafeUrlForward.java:14:27:14:29 | url | semmle.label | url |
| UnsafeUrlForward.java:18:27:18:36 | url : String | semmle.label | url : String |
Expand All @@ -41,6 +52,7 @@ subpaths
| UnsafeServletRequestDispatch.java:32:51:32:59 | returnURL | UnsafeServletRequestDispatch.java:23:22:23:54 | getParameter(...) : String | UnsafeServletRequestDispatch.java:32:51:32:59 | returnURL | Potentially untrusted URL forward due to $@. | UnsafeServletRequestDispatch.java:23:22:23:54 | getParameter(...) | user-provided value |
| UnsafeServletRequestDispatch.java:48:56:48:64 | returnURL | UnsafeServletRequestDispatch.java:42:22:42:54 | getParameter(...) : String | UnsafeServletRequestDispatch.java:48:56:48:64 | returnURL | Potentially untrusted URL forward due to $@. | UnsafeServletRequestDispatch.java:42:22:42:54 | getParameter(...) | user-provided value |
| UnsafeServletRequestDispatch.java:76:53:76:56 | path | UnsafeServletRequestDispatch.java:71:17:71:44 | getParameter(...) : String | UnsafeServletRequestDispatch.java:76:53:76:56 | path | Potentially untrusted URL forward due to $@. | UnsafeServletRequestDispatch.java:71:17:71:44 | getParameter(...) | user-provided value |
| UnsafeServletRequestDispatch.java:110:53:110:76 | toString(...) | UnsafeServletRequestDispatch.java:106:17:106:44 | getParameter(...) : String | UnsafeServletRequestDispatch.java:110:53:110:76 | toString(...) | Potentially untrusted URL forward due to $@. | UnsafeServletRequestDispatch.java:106:17:106:44 | getParameter(...) | user-provided value |
| UnsafeUrlForward.java:14:27:14:29 | url | UnsafeUrlForward.java:13:27:13:36 | url : String | UnsafeUrlForward.java:14:27:14:29 | url | Potentially untrusted URL forward due to $@. | UnsafeUrlForward.java:13:27:13:36 | url | user-provided value |
| UnsafeUrlForward.java:20:28:20:30 | url | UnsafeUrlForward.java:18:27:18:36 | url : String | UnsafeUrlForward.java:20:28:20:30 | url | Potentially untrusted URL forward due to $@. | UnsafeUrlForward.java:18:27:18:36 | url | user-provided value |
| UnsafeUrlForward.java:26:23:26:25 | url | UnsafeUrlForward.java:25:21:25:30 | url : String | UnsafeUrlForward.java:26:23:26:25 | url | Potentially untrusted URL forward due to $@. | UnsafeUrlForward.java:25:21:25:30 | url | user-provided value |
Expand Down