Skip to content

Commit d0eec1e

Browse files
haby0smowton
haby0
authored and
smowton
committed
Add CWE-552-UnsafeUrlForward
1 parent 662852b commit d0eec1e

File tree

9 files changed

+330
-0
lines changed

9 files changed

+330
-0
lines changed

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

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -191,6 +191,18 @@ 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") and
201+
getNumberOfParameters() = 1 and
202+
getParameter(0).getType() instanceof TypeString
203+
}
204+
}
205+
194206
/**
195207
* The method `sendRedirect(String)` declared in `javax.servlet.http.HttpServletResponse`.
196208
*/

‎java/ql/src/experimental/Security/CWE/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/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qhelp

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
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 entering 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+
In <code>bad1</code> method and <code>bad2</code> method and <code>bad3</code> method and
21+
<code>bad4</code> method and <code>bad5</code> method and <code>bad6</code> method, shows an HTTP request parameter being used directly in a URL forward
22+
without validating the input, which may cause file leakage. In <code>good1</code> method,
23+
ordinary forwarding requests are shown, which will not cause file leakage.
24+
</p>
25+
26+
<sample src="UnsafeUrlForward.java" />
27+
28+
</example>
29+
<references>
30+
<li>File Disclosure: <a href="https://vulncat.fortify.com/en/detail?id=desc.dataflow.java.file_disclosure_spring">Unsafe Url Forward</a>.</li>
31+
</references>
32+
</qhelp>

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

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
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 DataFlow::PathGraph
17+
18+
class UnsafeUrlForwardFlowConfig extends TaintTracking::Configuration {
19+
UnsafeUrlForwardFlowConfig() { this = "UnsafeUrlForwardFlowConfig" }
20+
21+
override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
22+
23+
override predicate isSink(DataFlow::Node sink) { sink instanceof UnsafeUrlForwardSink }
24+
25+
override predicate isSanitizer(DataFlow::Node node) {
26+
node.getType() instanceof BoxedType
27+
or
28+
node.getType() instanceof PrimitiveType
29+
or
30+
exists(AddExpr ae |
31+
ae.getRightOperand() = node.asExpr() and
32+
(
33+
not ae.getLeftOperand().(CompileTimeConstantExpr).getStringValue().matches("/WEB-INF/%")
34+
and
35+
not ae.getLeftOperand().(CompileTimeConstantExpr).getStringValue() = "forward:"
36+
)
37+
)
38+
}
39+
}
40+
41+
from DataFlow::PathNode source, DataFlow::PathNode sink, UnsafeUrlForwardFlowConfig conf
42+
where conf.hasFlowPath(source, sink)
43+
select sink.getNode(), source, sink, "Potentially untrusted URL forward due to $@.",
44+
source.getNode(), "user-provided value"

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

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,76 @@
1+
import java
2+
import DataFlow
3+
import semmle.code.java.dataflow.FlowSources
4+
import semmle.code.java.frameworks.Servlets
5+
6+
/**
7+
* A concatenate expression using the string `forward:` on the left.
8+
*
9+
* E.g: `"forward:" + url`
10+
*/
11+
class ForwardBuilderExpr extends AddExpr {
12+
ForwardBuilderExpr() {
13+
this.getLeftOperand().(CompileTimeConstantExpr).getStringValue() = "forward:"
14+
}
15+
}
16+
17+
/**
18+
* A call to `StringBuilder.append` or `StringBuffer.append` method, and the parameter value is `"forward:"`.
19+
*
20+
* E.g: `StringBuilder.append("forward:")`
21+
*/
22+
class ForwardAppendCall extends MethodAccess {
23+
ForwardAppendCall() {
24+
this.getMethod().hasName("append") and
25+
this.getMethod().getDeclaringType() instanceof StringBuildingType and
26+
this.getArgument(0).(CompileTimeConstantExpr).getStringValue() = "forward:"
27+
}
28+
}
29+
30+
abstract class UnsafeUrlForwardSink extends DataFlow::Node { }
31+
32+
/** A Unsafe url forward sink from getRequestDispatcher method. */
33+
private class RequestDispatcherSink extends UnsafeUrlForwardSink {
34+
RequestDispatcherSink() {
35+
exists(MethodAccess ma |
36+
ma.getMethod() instanceof ServletRequestGetRequestDispatcherMethod and
37+
ma.getArgument(0) = this.asExpr()
38+
)
39+
}
40+
}
41+
42+
/** A Unsafe url forward sink from spring controller method. */
43+
private class SpringUrlForwardSink extends UnsafeUrlForwardSink {
44+
SpringUrlForwardSink() {
45+
exists(ForwardBuilderExpr rbe |
46+
rbe.getRightOperand() = this.asExpr() and
47+
any(SpringRequestMappingMethod sqmm).polyCalls*(this.getEnclosingCallable())
48+
)
49+
or
50+
exists(MethodAccess ma, ForwardAppendCall rac |
51+
DataFlow2::localExprFlow(rac.getQualifier(), ma.getQualifier()) and
52+
ma.getMethod().hasName("append") and
53+
ma.getArgument(0) = this.asExpr() and
54+
any(SpringRequestMappingMethod sqmm).polyCalls*(this.getEnclosingCallable())
55+
)
56+
or
57+
exists(ClassInstanceExpr cie |
58+
cie.getConstructedType().hasQualifiedName("org.springframework.web.servlet", "ModelAndView") and
59+
(
60+
exists(ForwardBuilderExpr rbe |
61+
rbe = cie.getArgument(0) and rbe.getRightOperand() = this.asExpr()
62+
)
63+
or
64+
cie.getArgument(0) = this.asExpr()
65+
)
66+
)
67+
or
68+
exists(MethodAccess ma |
69+
ma.getMethod().hasName("setViewName") and
70+
ma.getMethod()
71+
.getDeclaringType()
72+
.hasQualifiedName("org.springframework.web.servlet", "ModelAndView") and
73+
ma.getArgument(0) = this.asExpr()
74+
)
75+
}
76+
}

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

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
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+
#select
24+
| 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 |
25+
| 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 |
26+
| 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 |
27+
| 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 |
28+
| 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 |
29+
| 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 |
30+
| 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.2.3/

0 commit comments

Comments
 (0)