Skip to content

Commit 233a334

Browse files
smowtonsmowton
authored
Merge pull request github#6240 from haby0/java/UnsafeUrlForward
[Java] CWE-552: Unsafe url forward
2 parents 7015be7 + 057d0fb commit 233a334

File tree

9 files changed

+312
-3
lines changed

9 files changed

+312
-3
lines changed

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

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,7 @@ library class HttpServletRequestGetQueryStringMethod extends Method {
7474
/**
7575
* The method `getPathInfo()` declared in `javax.servlet.http.HttpServletRequest`.
7676
*/
77-
library class HttpServletRequestGetPathMethod extends Method {
77+
class HttpServletRequestGetPathMethod extends Method {
7878
HttpServletRequestGetPathMethod() {
7979
getDeclaringType() instanceof HttpServletRequest and
8080
hasName("getPathInfo") and
@@ -120,7 +120,7 @@ library class HttpServletRequestGetHeaderNamesMethod extends Method {
120120
/**
121121
* The method `getRequestURL()` declared in `javax.servlet.http.HttpServletRequest`.
122122
*/
123-
library class HttpServletRequestGetRequestURLMethod extends Method {
123+
class HttpServletRequestGetRequestURLMethod extends Method {
124124
HttpServletRequestGetRequestURLMethod() {
125125
getDeclaringType() instanceof HttpServletRequest and
126126
hasName("getRequestURL") and
@@ -131,7 +131,7 @@ library class HttpServletRequestGetRequestURLMethod extends Method {
131131
/**
132132
* The method `getRequestURI()` declared in `javax.servlet.http.HttpServletRequest`.
133133
*/
134-
library class HttpServletRequestGetRequestURIMethod extends Method {
134+
class HttpServletRequestGetRequestURIMethod extends Method {
135135
HttpServletRequestGetRequestURIMethod() {
136136
getDeclaringType() instanceof HttpServletRequest and
137137
hasName("getRequestURI") and
@@ -191,6 +191,16 @@ class HttpServletResponseSendErrorMethod extends Method {
191191
}
192192
}
193193

194+
/**
195+
* The method `getRequestDispatcher(String)` declared in `javax.servlet.http.HttpServletRequest` or `javax.servlet.ServletRequest`.
196+
*/
197+
class ServletRequestGetRequestDispatcherMethod extends Method {
198+
ServletRequestGetRequestDispatcherMethod() {
199+
getDeclaringType() instanceof ServletRequest and
200+
hasName("getRequestDispatcher")
201+
}
202+
}
203+
194204
/**
195205
* The method `sendRedirect(String)` declared in `javax.servlet.http.HttpServletResponse`.
196206
*/

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

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
import java.io.IOException;
2+
import javax.servlet.ServletException;
3+
import javax.servlet.http.HttpServletRequest;
4+
import javax.servlet.http.HttpServletResponse;
5+
import org.springframework.stereotype.Controller;
6+
import org.springframework.web.bind.annotation.GetMapping;
7+
import org.springframework.web.servlet.ModelAndView;
8+
9+
@Controller
10+
public class UnsafeUrlForward {
11+
12+
@GetMapping("/bad1")
13+
public ModelAndView bad1(String url) {
14+
return new ModelAndView(url);
15+
}
16+
17+
@GetMapping("/bad2")
18+
public void bad2(String url, HttpServletRequest request, HttpServletResponse response) {
19+
try {
20+
request.getRequestDispatcher("/WEB-INF/jsp/" + url + ".jsp").include(request, response);
21+
} catch (ServletException e) {
22+
e.printStackTrace();
23+
} catch (IOException e) {
24+
e.printStackTrace();
25+
}
26+
}
27+
28+
@GetMapping("/good1")
29+
public void good1(String url, HttpServletRequest request, HttpServletResponse response) {
30+
try {
31+
request.getRequestDispatcher("/index.jsp?token=" + url).forward(request, response);
32+
} catch (ServletException e) {
33+
e.printStackTrace();
34+
} catch (IOException e) {
35+
e.printStackTrace();
36+
}
37+
}
38+
}

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

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
6+
7+
<overview>
8+
<p>Constructing a server-side redirect path with user input could allow an attacker to download application binaries
9+
(including application classes or jar files) or view arbitrary files within protected directories.</p>
10+
11+
</overview>
12+
<recommendation>
13+
14+
<p>In order to prevent untrusted URL forwarding, it is recommended to avoid concatenating user input directly into the forwarding URL.</p>
15+
16+
</recommendation>
17+
<example>
18+
19+
<p>The following examples show the bad case and the good case respectively.
20+
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,
22+
ordinary forwarding requests are shown, which will not cause file leakage.
23+
</p>
24+
25+
<sample src="UnsafeUrlForward.java" />
26+
27+
</example>
28+
<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>
30+
</references>
31+
</qhelp>

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

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
/**
2+
* @name Unsafe url forward from remote source
3+
* @description URL forward based on unvalidated user-input
4+
* may cause file information disclosure.
5+
* @kind path-problem
6+
* @problem.severity error
7+
* @precision high
8+
* @id java/unsafe-url-forward
9+
* @tags security
10+
* external/cwe-552
11+
*/
12+
13+
import java
14+
import UnsafeUrlForward
15+
import semmle.code.java.dataflow.FlowSources
16+
import semmle.code.java.frameworks.Servlets
17+
import DataFlow::PathGraph
18+
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+
}
25+
26+
override predicate checks(Expr e, boolean branch) {
27+
e = this.(MethodAccess).getQualifier() and branch = true
28+
}
29+
}
30+
31+
class UnsafeUrlForwardFlowConfig extends TaintTracking::Configuration {
32+
UnsafeUrlForwardFlowConfig() { this = "UnsafeUrlForwardFlowConfig" }
33+
34+
override predicate isSource(DataFlow::Node source) {
35+
source instanceof RemoteFlowSource and
36+
not exists(MethodAccess ma, Method m | ma.getMethod() = m |
37+
(
38+
m instanceof HttpServletRequestGetRequestURIMethod or
39+
m instanceof HttpServletRequestGetRequestURLMethod or
40+
m instanceof HttpServletRequestGetPathMethod
41+
) and
42+
ma = source.asExpr()
43+
)
44+
}
45+
46+
override predicate isSink(DataFlow::Node sink) { sink instanceof UnsafeUrlForwardSink }
47+
48+
override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
49+
guard instanceof StartsWithSanitizer
50+
}
51+
52+
override predicate isSanitizer(DataFlow::Node node) { node instanceof UnsafeUrlForwardSanitizer }
53+
}
54+
55+
from DataFlow::PathNode source, DataFlow::PathNode sink, UnsafeUrlForwardFlowConfig conf
56+
where conf.hasFlowPath(source, sink)
57+
select sink.getNode(), source, sink, "Potentially untrusted URL forward due to $@.",
58+
source.getNode(), "user-provided value"

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

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,72 @@
1+
import java
2+
import DataFlow
3+
import semmle.code.java.dataflow.FlowSources
4+
import semmle.code.java.frameworks.Servlets
5+
import semmle.code.java.frameworks.spring.SpringWeb
6+
import semmle.code.java.security.RequestForgery
7+
private import semmle.code.java.dataflow.StringPrefixes
8+
9+
/** A sanitizer for unsafe url forward vulnerabilities. */
10+
abstract class UnsafeUrlForwardSanitizer extends DataFlow::Node { }
11+
12+
private class PrimitiveSanitizer extends UnsafeUrlForwardSanitizer {
13+
PrimitiveSanitizer() {
14+
this.getType() instanceof PrimitiveType or
15+
this.getType() instanceof BoxedType or
16+
this.getType() instanceof NumberType
17+
}
18+
}
19+
20+
private class SanitizingPrefix extends InterestingPrefix {
21+
SanitizingPrefix() {
22+
not this.getStringValue().matches("/WEB-INF/%") and
23+
not this.getStringValue() = "forward:"
24+
}
25+
26+
override int getOffset() { result = 0 }
27+
}
28+
29+
private class FollowsSanitizingPrefix extends UnsafeUrlForwardSanitizer {
30+
FollowsSanitizingPrefix() { this.asExpr() = any(SanitizingPrefix fp).getAnAppendedExpression() }
31+
}
32+
33+
abstract class UnsafeUrlForwardSink extends DataFlow::Node { }
34+
35+
/** An argument to `ServletRequest.getRequestDispatcher`. */
36+
private class RequestDispatcherSink extends UnsafeUrlForwardSink {
37+
RequestDispatcherSink() {
38+
exists(MethodAccess ma |
39+
ma.getMethod() instanceof ServletRequestGetRequestDispatcherMethod and
40+
ma.getArgument(0) = this.asExpr()
41+
)
42+
}
43+
}
44+
45+
/** An argument to `new ModelAndView` or `ModelAndView.setViewName`. */
46+
private class SpringModelAndViewSink extends UnsafeUrlForwardSink {
47+
SpringModelAndViewSink() {
48+
exists(ClassInstanceExpr cie |
49+
cie.getConstructedType() instanceof ModelAndView and
50+
cie.getArgument(0) = this.asExpr()
51+
)
52+
or
53+
exists(SpringModelAndViewSetViewNameCall smavsvnc | smavsvnc.getArgument(0) = this.asExpr())
54+
}
55+
}
56+
57+
private class ForwardPrefix extends InterestingPrefix {
58+
ForwardPrefix() { this.getStringValue() = "forward:" }
59+
60+
override int getOffset() { result = 0 }
61+
}
62+
63+
/**
64+
* An expression appended (perhaps indirectly) to `"forward:"`, and which
65+
* is reachable from a Spring entry point.
66+
*/
67+
private class SpringUrlForwardSink extends UnsafeUrlForwardSink {
68+
SpringUrlForwardSink() {
69+
any(SpringRequestMappingMethod sqmm).polyCalls*(this.getEnclosingCallable()) and
70+
this.asExpr() = any(ForwardPrefix fp).getAnAppendedExpression()
71+
}
72+
}

‎java/ql/test/experimental/query-tests/security/CWE-552/UnsafeUrlForward.expected

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
edges
2+
| UnsafeUrlForward.java:13:27:13:36 | url : String | UnsafeUrlForward.java:14:27:14:29 | url |
3+
| UnsafeUrlForward.java:18:27:18:36 | url : String | UnsafeUrlForward.java:20:28:20:30 | url |
4+
| UnsafeUrlForward.java:25:21:25:30 | url : String | UnsafeUrlForward.java:26:23:26:25 | url |
5+
| UnsafeUrlForward.java:30:27:30:36 | url : String | UnsafeUrlForward.java:31:48:31:63 | ... + ... |
6+
| UnsafeUrlForward.java:30:27:30:36 | url : String | UnsafeUrlForward.java:31:61:31:63 | url |
7+
| UnsafeUrlForward.java:36:19:36:28 | url : String | UnsafeUrlForward.java:38:33:38:35 | url |
8+
| UnsafeUrlForward.java:47:19:47:28 | url : String | UnsafeUrlForward.java:49:33:49:62 | ... + ... |
9+
nodes
10+
| UnsafeUrlForward.java:13:27:13:36 | url : String | semmle.label | url : String |
11+
| UnsafeUrlForward.java:14:27:14:29 | url | semmle.label | url |
12+
| UnsafeUrlForward.java:18:27:18:36 | url : String | semmle.label | url : String |
13+
| UnsafeUrlForward.java:20:28:20:30 | url | semmle.label | url |
14+
| UnsafeUrlForward.java:25:21:25:30 | url : String | semmle.label | url : String |
15+
| UnsafeUrlForward.java:26:23:26:25 | url | semmle.label | url |
16+
| UnsafeUrlForward.java:30:27:30:36 | url : String | semmle.label | url : String |
17+
| UnsafeUrlForward.java:31:48:31:63 | ... + ... | semmle.label | ... + ... |
18+
| UnsafeUrlForward.java:31:61:31:63 | url | semmle.label | url |
19+
| UnsafeUrlForward.java:36:19:36:28 | url : String | semmle.label | url : String |
20+
| UnsafeUrlForward.java:38:33:38:35 | url | semmle.label | url |
21+
| UnsafeUrlForward.java:47:19:47:28 | url : String | semmle.label | url : String |
22+
| UnsafeUrlForward.java:49:33:49:62 | ... + ... | semmle.label | ... + ... |
23+
subpaths
24+
#select
25+
| 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 |
26+
| 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 |
27+
| 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 |
28+
| UnsafeUrlForward.java:31:48:31:63 | ... + ... | UnsafeUrlForward.java:30:27:30:36 | url : String | UnsafeUrlForward.java:31:48:31:63 | ... + ... | Potentially untrusted URL forward due to $@. | UnsafeUrlForward.java:30:27:30:36 | url | user-provided value |
29+
| UnsafeUrlForward.java:31:61:31:63 | url | UnsafeUrlForward.java:30:27:30:36 | url : String | UnsafeUrlForward.java:31:61:31:63 | url | Potentially untrusted URL forward due to $@. | UnsafeUrlForward.java:30:27:30:36 | url | user-provided value |
30+
| UnsafeUrlForward.java:38:33:38:35 | url | UnsafeUrlForward.java:36:19:36:28 | url : String | UnsafeUrlForward.java:38:33:38:35 | url | Potentially untrusted URL forward due to $@. | UnsafeUrlForward.java:36:19:36:28 | url | user-provided value |
31+
| UnsafeUrlForward.java:49:33:49:62 | ... + ... | UnsafeUrlForward.java:47:19:47:28 | url : String | UnsafeUrlForward.java:49:33:49:62 | ... + ... | Potentially untrusted URL forward due to $@. | UnsafeUrlForward.java:47:19:47:28 | url | user-provided value |

‎java/ql/test/experimental/query-tests/security/CWE-552/UnsafeUrlForward.java

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,67 @@
1+
import java.io.IOException;
2+
import javax.servlet.ServletException;
3+
import javax.servlet.http.HttpServletRequest;
4+
import javax.servlet.http.HttpServletResponse;
5+
import org.springframework.stereotype.Controller;
6+
import org.springframework.web.bind.annotation.GetMapping;
7+
import org.springframework.web.servlet.ModelAndView;
8+
9+
@Controller
10+
public class UnsafeUrlForward {
11+
12+
@GetMapping("/bad1")
13+
public ModelAndView bad1(String url) {
14+
return new ModelAndView(url);
15+
}
16+
17+
@GetMapping("/bad2")
18+
public ModelAndView bad2(String url) {
19+
ModelAndView modelAndView = new ModelAndView();
20+
modelAndView.setViewName(url);
21+
return modelAndView;
22+
}
23+
24+
@GetMapping("/bad3")
25+
public String bad3(String url) {
26+
return "forward:" + url + "/swagger-ui/index.html";
27+
}
28+
29+
@GetMapping("/bad4")
30+
public ModelAndView bad4(String url) {
31+
ModelAndView modelAndView = new ModelAndView("forward:" + url);
32+
return modelAndView;
33+
}
34+
35+
@GetMapping("/bad5")
36+
public void bad5(String url, HttpServletRequest request, HttpServletResponse response) {
37+
try {
38+
request.getRequestDispatcher(url).include(request, response);
39+
} catch (ServletException e) {
40+
e.printStackTrace();
41+
} catch (IOException e) {
42+
e.printStackTrace();
43+
}
44+
}
45+
46+
@GetMapping("/bad6")
47+
public void bad6(String url, HttpServletRequest request, HttpServletResponse response) {
48+
try {
49+
request.getRequestDispatcher("/WEB-INF/jsp/" + url + ".jsp").include(request, response);
50+
} catch (ServletException e) {
51+
e.printStackTrace();
52+
} catch (IOException e) {
53+
e.printStackTrace();
54+
}
55+
}
56+
57+
@GetMapping("/good1")
58+
public void good1(String url, HttpServletRequest request, HttpServletResponse response) {
59+
try {
60+
request.getRequestDispatcher("/index.jsp?token=" + url).forward(request, response);
61+
} catch (ServletException e) {
62+
e.printStackTrace();
63+
} catch (IOException e) {
64+
e.printStackTrace();
65+
}
66+
}
67+
}

‎java/ql/test/experimental/query-tests/security/CWE-552/UnsafeUrlForward.qlref

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
experimental/Security/CWE/CWE-552/UnsafeUrlForward.ql

‎java/ql/test/experimental/query-tests/security/CWE-552/options

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
//semmle-extractor-options: --javac-args -cp ${testdir}/../../../../stubs/servlet-api-2.4:${testdir}/../../../../stubs/springframework-5.3.8/

0 commit comments

Comments
 (0)