Skip to content

Commit 8bcffc2

Browse files
luchua-bcluchua-bc
committed
Query to detect unsafe request dispatcher usage
1 parent 9f8326a commit 8bcffc2

File tree

11 files changed

+543
-21
lines changed

11 files changed

+543
-21
lines changed

‎java/ql/lib/semmle/code/java/frameworks/Servlets.qll

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -347,3 +347,27 @@ predicate isRequestGetParamMethod(MethodAccess ma) {
347347
ma.getMethod() instanceof ServletRequestGetParameterMapMethod or
348348
ma.getMethod() instanceof HttpServletRequestGetQueryStringMethod
349349
}
350+
351+
/** The Java EE RequestDispatcher. */
352+
library class RequestDispatcher extends RefType {
353+
RequestDispatcher() {
354+
this.hasQualifiedName(["javax.servlet", "jakarta.servlet"], "RequestDispatcher") or
355+
this.hasQualifiedName("javax.portlet", "PortletRequestDispatcher")
356+
}
357+
}
358+
359+
/** The `getRequestDispatcher` method. */
360+
library class GetRequestDispatcherMethod extends Method {
361+
GetRequestDispatcherMethod() {
362+
this.getReturnType() instanceof RequestDispatcher and
363+
this.getName() = "getRequestDispatcher"
364+
}
365+
}
366+
367+
/** The request dispatch method. */
368+
library class RequestDispatchMethod extends Method {
369+
RequestDispatchMethod() {
370+
this.getDeclaringType() instanceof RequestDispatcher and
371+
this.hasName(["forward", "include"])
372+
}
373+
}

‎java/ql/src/experimental/Security/CWE/CWE-552/UnsafeRequestPath.java

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
public class UnsafeRequestPath implements Filter {
2+
private static final String BASE_PATH = "/pages";
3+
4+
@Override
5+
public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain)
6+
throws IOException, ServletException {
7+
8+
{
9+
// BAD: Request dispatcher from servlet path without check
10+
String path = ((HttpServletRequest) request).getServletPath();
11+
// A sample payload "/%57EB-INF/web.xml" can bypass this `startsWith` check
12+
if (path != null && !path.startsWith("/WEB-INF")) {
13+
request.getRequestDispatcher(path).forward(request, response);
14+
} else {
15+
chain.doFilter(request, response);
16+
}
17+
}
18+
19+
{
20+
// GOOD: Request dispatcher from servlet path with path traversal check
21+
String path = ((HttpServletRequest) request).getServletPath();
22+
23+
if (path.startsWith(BASE_PATH) && !path.contains("..")) {
24+
request.getRequestDispatcher(path).forward(request, response);
25+
} else {
26+
chain.doFilter(request, response);
27+
}
28+
}
29+
}
30+
}

‎java/ql/src/experimental/Security/CWE/CWE-552/UnsafeServletRequestDispatch.java

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,82 @@
1+
public class UnsafeServletRequestDispatch extends HttpServlet {
2+
private static final String BASE_PATH = "/pages";
3+
4+
@Override
5+
protected void doGet(HttpServletRequest request, HttpServletResponse response)
6+
throws ServletException, IOException {
7+
{
8+
// GOOD: whitelisted URI
9+
if (action.equals("Login")) {
10+
ServletContext sc = cfg.getServletContext();
11+
RequestDispatcher rd = sc.getRequestDispatcher("/Login.jsp");
12+
rd.forward(request, response);
13+
}
14+
}
15+
16+
{
17+
// BAD: Request dispatcher constructed from `ServletContext` without input validation
18+
String returnURL = request.getParameter("returnURL");
19+
ServletConfig cfg = getServletConfig();
20+
21+
ServletContext sc = cfg.getServletContext();
22+
RequestDispatcher rd = sc.getRequestDispatcher(returnURL);
23+
rd.forward(request, response);
24+
}
25+
26+
{
27+
// BAD: Request dispatcher without path traversal check
28+
String path = request.getParameter("path");
29+
30+
// A sample payload "/pages/welcome.jsp/../WEB-INF/web.xml" can bypass the `startsWith` check
31+
// The payload "/pages/welcome.jsp/../../%57EB-INF/web.xml" can bypass the check as well since RequestDispatcher will decode `%57` as `W`
32+
if (path.startsWith(BASE_PATH)) {
33+
request.getServletContext().getRequestDispatcher(path).include(request, response);
34+
}
35+
}
36+
}
37+
38+
{
39+
// GOOD: Request dispatcher with path traversal check
40+
String path = request.getParameter("path");
41+
42+
if (path.startsWith(BASE_PATH) && !path.contains("..")) {
43+
request.getServletContext().getRequestDispatcher(path).include(request, response);
44+
}
45+
}
46+
47+
{
48+
// GOOD: Request dispatcher with path normalization
49+
String path = request.getParameter("path");
50+
Path requestedPath = Paths.get(BASE_PATH).resolve(path).normalize();
51+
52+
// /pages/welcome.jsp/../../WEB-INF/web.xml becomes /WEB-INF/web.xml
53+
// /pages/welcome.jsp/../../%57EB-INF/web.xml becomes /%57EB-INF/web.xml
54+
if (requestedPath.startsWith(BASE_PATH)) {
55+
request.getServletContext().getRequestDispatcher(requestedPath.toString()).forward(request, response);
56+
}
57+
}
58+
59+
{
60+
// BAD: Request dispatcher with improper negation check and without url decoding
61+
String path = request.getParameter("path");
62+
Path requestedPath = Paths.get(BASE_PATH).resolve(path).normalize();
63+
64+
if (!requestedPath.startsWith("/WEB-INF") && !requestedPath.startsWith("/META-INF")) {
65+
request.getServletContext().getRequestDispatcher(requestedPath.toString()).forward(request, response);
66+
}
67+
}
68+
69+
{
70+
// GOOD: Request dispatcher with path traversal check and url decoding
71+
String path = request.getParameter("path");
72+
boolean hasEncoding = path.contains("%");
73+
while (hasEncoding) {
74+
path = URLDecoder.decode(path, "UTF-8");
75+
hasEncoding = path.contains("%");
76+
}
77+
78+
if (!path.startsWith("/WEB-INF/") && !path.contains("..")) {
79+
request.getServletContext().getRequestDispatcher(path).include(request, response);
80+
}
81+
}
82+
}

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

Lines changed: 22 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5,27 +5,45 @@
55

66

77
<overview>
8-
<p>Constructing a server-side redirect path with user input could allow an attacker to download application binaries
8+
<p>Constructing a server-side redirect path with user input could allow an attacker to download application binaries
99
(including application classes or jar files) or view arbitrary files within protected directories.</p>
1010

1111
</overview>
1212
<recommendation>
1313

14-
<p>In order to prevent untrusted URL forwarding, it is recommended to avoid concatenating user input directly into the forwarding URL.</p>
14+
<p>Unsanitized user provided data must not be used to construct the path for URL forwarding. In order to prevent
15+
untrusted URL forwarding, it is recommended to avoid concatenating user input directly into the forwarding URL.
16+
</p>
1517

1618
</recommendation>
1719
<example>
1820

1921
<p>The following examples show the bad case and the good case respectively.
2022
The <code>bad</code> methods show an HTTP request parameter being used directly in a URL forward
21-
without validating the input, which may cause file leakage. In <code>good1</code> method,
23+
without validating the input, which may cause file leakage. In <code>good1</code> method,
2224
ordinary forwarding requests are shown, which will not cause file leakage.
2325
</p>
2426

2527
<sample src="UnsafeUrlForward.java" />
2628

29+
<p>The following examples show an HTTP request parameter or request path being used directly in a
30+
request dispatcher of Java EE without validating the input, which allows sensitive file exposure
31+
attacks. It also shows how to remedy the problem by validating the user input.
32+
</p>
33+
34+
<sample src="UnsafeServletRequestDispatch.java" />
35+
<sample src="UnsafeRequestPath.java" />
36+
2737
</example>
2838
<references>
29-
<li>File Disclosure: <a href="https://vulncat.fortify.com/en/detail?id=desc.dataflow.java.file_disclosure_spring">Unsafe Url Forward</a>.</li>
39+
<li>File Disclosure:
40+
<a href="https://vulncat.fortify.com/en/detail?id=desc.dataflow.java.file_disclosure_spring">Unsafe Url Forward</a>.
41+
</li>
42+
<li>Jakarta Javadoc:
43+
<a href="https://jakarta.ee/specifications/webprofile/9/apidocs/jakarta/servlet/servletrequest#getRequestDispatcher-java.lang.String-">Security vulnerability with unsafe usage of RequestDispatcher</a>.
44+
</li>
45+
<li>Micro Focus:
46+
<a href="https://vulncat.fortify.com/en/detail?id=desc.dataflow.java.file_disclosure_j2ee">File Disclosure: J2EE</a>
47+
</li>
3048
</references>
3149
</qhelp>

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

Lines changed: 133 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,11 @@
11
/**
2-
* @name Unsafe url forward from remote source
3-
* @description URL forward based on unvalidated user-input
2+
* @name Unsafe url forward or dispatch from remote source
3+
* @description URL forward or dispatch based on unvalidated user-input
44
* may cause file information disclosure.
55
* @kind path-problem
66
* @problem.severity error
77
* @precision high
8-
* @id java/unsafe-url-forward
8+
* @id java/unsafe-url-forward-dispatch
99
* @tags security
1010
* external/cwe-552
1111
*/
@@ -14,20 +14,137 @@ import java
1414
import UnsafeUrlForward
1515
import semmle.code.java.dataflow.FlowSources
1616
import semmle.code.java.frameworks.Servlets
17+
import semmle.code.java.controlflow.Guards
1718
import DataFlow::PathGraph
1819

19-
private class StartsWithSanitizer extends DataFlow::BarrierGuard {
20-
StartsWithSanitizer() {
21-
this.(MethodAccess).getMethod().hasName("startsWith") and
22-
this.(MethodAccess).getMethod().getDeclaringType() instanceof TypeString and
23-
this.(MethodAccess).getMethod().getNumberOfParameters() = 1
24-
}
20+
/**
21+
* Holds if `ma` is a method call of matching with a path string, probably a whitelisted one.
22+
*/
23+
predicate isStringPathMatch(MethodAccess ma) {
24+
ma.getMethod().getDeclaringType() instanceof TypeString and
25+
ma.getMethod().getName() = ["startsWith", "matches", "regionMatches"]
26+
}
27+
28+
/**
29+
* Holds if `ma` is a method call of `java.nio.file.Path` which matches with another
30+
* path, probably a whitelisted one.
31+
*/
32+
predicate isFilePathMatch(MethodAccess ma) {
33+
ma.getMethod().getDeclaringType() instanceof TypePath and
34+
ma.getMethod().getName() = "startsWith"
35+
}
36+
37+
/**
38+
* Holds if `ma` is a method call that checks an input doesn't match using the `!`
39+
* logical negation expression.
40+
*/
41+
predicate checkNoPathMatch(MethodAccess ma) {
42+
exists(LogNotExpr lne |
43+
(isStringPathMatch(ma) or isFilePathMatch(ma)) and
44+
lne.getExpr() = ma
45+
)
46+
}
47+
48+
/**
49+
* Holds if `ma` is a method call to check special characters `..` used in path traversal.
50+
*/
51+
predicate isPathTraversalCheck(MethodAccess ma) {
52+
ma.getMethod().getDeclaringType() instanceof TypeString and
53+
ma.getMethod().hasName(["contains", "indexOf"]) and
54+
ma.getAnArgument().(CompileTimeConstantExpr).getStringValue() = ".."
55+
}
56+
57+
/**
58+
* Holds if `ma` is a method call to decode a url string or check url encoding.
59+
*/
60+
predicate isPathDecoding(MethodAccess ma) {
61+
// Search the special character `%` used in url encoding
62+
ma.getMethod().getDeclaringType() instanceof TypeString and
63+
ma.getMethod().hasName(["contains", "indexOf"]) and
64+
ma.getAnArgument().(CompileTimeConstantExpr).getStringValue() = "%"
65+
or
66+
// Call to `URLDecoder` assuming the implementation handles double encoding correctly
67+
ma.getMethod().getDeclaringType().hasQualifiedName("java.net", "URLDecoder") and
68+
ma.getMethod().hasName("decode")
69+
}
2570

26-
override predicate checks(Expr e, boolean branch) {
27-
e = this.(MethodAccess).getQualifier() and branch = true
71+
private class PathMatchSanitizer extends DataFlow::Node {
72+
PathMatchSanitizer() {
73+
exists(MethodAccess ma |
74+
(
75+
isStringPathMatch(ma) and
76+
exists(MethodAccess ma2 |
77+
isPathTraversalCheck(ma2) and
78+
ma.getQualifier().(VarAccess).getVariable().getAnAccess() = ma2.getQualifier()
79+
)
80+
or
81+
isFilePathMatch(ma)
82+
) and
83+
(
84+
not checkNoPathMatch(ma)
85+
or
86+
// non-match check needs decoding e.g. !path.startsWith("/WEB-INF/") won't detect /%57EB-INF/web.xml, which will be decoded and served by RequestDispatcher
87+
checkNoPathMatch(ma) and
88+
exists(MethodAccess ma2 |
89+
isPathDecoding(ma2) and
90+
ma.getQualifier().(VarAccess).getVariable().getAnAccess() = ma2.getQualifier()
91+
)
92+
) and
93+
this.asExpr() = ma.getQualifier()
94+
)
2895
}
2996
}
3097

98+
/**
99+
* Holds if `ma` is a method call to check string content, which means an input string is not
100+
* blindly trusted and helps to reduce FPs.
101+
*/
102+
predicate checkStringContent(MethodAccess ma, Expr expr) {
103+
ma.getMethod().getDeclaringType() instanceof TypeString and
104+
ma.getMethod()
105+
.hasName([
106+
"charAt", "contains", "equals", "equalsIgnoreCase", "getBytes", "getChars", "indexOf",
107+
"lastIndexOf", "length", "matches", "regionMatches", "replace", "replaceAll",
108+
"replaceFirst", "substring"
109+
]) and
110+
expr = ma.getQualifier()
111+
or
112+
(
113+
ma.getMethod().getDeclaringType() instanceof TypeStringBuffer or
114+
ma.getMethod().getDeclaringType() instanceof TypeStringBuilder
115+
) and
116+
expr = ma.getAnArgument()
117+
}
118+
119+
private class StringOperationSanitizer extends DataFlow::Node {
120+
StringOperationSanitizer() { exists(MethodAccess ma | checkStringContent(ma, this.asExpr())) }
121+
}
122+
123+
/**
124+
* Holds if `expr` is an expression returned from null or empty string check.
125+
*/
126+
predicate isNullOrEmptyCheck(Expr expr) {
127+
exists(ConditionBlock cb, ReturnStmt rt |
128+
cb.controls(rt.getBasicBlock(), true) and
129+
(
130+
cb.getCondition().(EQExpr).getAnOperand() instanceof NullLiteral // if (path == null)
131+
or
132+
// if (path.equals(""))
133+
exists(MethodAccess ma |
134+
cb.getCondition() = ma and
135+
ma.getMethod().getDeclaringType() instanceof TypeString and
136+
ma.getMethod().hasName("equals") and
137+
ma.getArgument(0).(CompileTimeConstantExpr).getStringValue() = ""
138+
)
139+
) and
140+
expr.getParent+() = rt
141+
)
142+
}
143+
144+
private class NullOrEmptyCheckSanitizer extends DataFlow::Node {
145+
NullOrEmptyCheckSanitizer() { isNullOrEmptyCheck(this.asExpr()) }
146+
}
147+
31148
class UnsafeUrlForwardFlowConfig extends TaintTracking::Configuration {
32149
UnsafeUrlForwardFlowConfig() { this = "UnsafeUrlForwardFlowConfig" }
33150

@@ -45,11 +162,12 @@ class UnsafeUrlForwardFlowConfig extends TaintTracking::Configuration {
45162

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

48-
override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
49-
guard instanceof StartsWithSanitizer
165+
override predicate isSanitizer(DataFlow::Node node) {
166+
node instanceof UnsafeUrlForwardSanitizer or
167+
node instanceof PathMatchSanitizer or
168+
node instanceof StringOperationSanitizer or
169+
node instanceof NullOrEmptyCheckSanitizer
50170
}
51-
52-
override predicate isSanitizer(DataFlow::Node node) { node instanceof UnsafeUrlForwardSanitizer }
53171
}
54172

55173
from DataFlow::PathNode source, DataFlow::PathNode sink, UnsafeUrlForwardFlowConfig conf

0 commit comments

Comments
 (0)