Skip to content

Commit 71f540a

Browse files
smowtonsmowton
authored
Merge pull request github#5844 from haby0/SpringRedirects
[Java] CWE-601 Spring url redirection detect
2 parents 1d12082 + e46de44 commit 71f540a

File tree

12 files changed

+637
-0
lines changed

12 files changed

+637
-0
lines changed

‎java/ql/src/experimental/Security/CWE/CWE-601/SpringUrlRedirect.java

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
import javax.servlet.http.HttpServletResponse;
2+
import org.springframework.stereotype.Controller;
3+
import org.springframework.web.bind.annotation.GetMapping;
4+
import org.springframework.web.servlet.ModelAndView;
5+
import org.springframework.web.servlet.view.RedirectView;
6+
7+
@Controller
8+
public class SpringUrlRedirect {
9+
10+
private final static String VALID_REDIRECT = "http://127.0.0.1";
11+
12+
@GetMapping("url1")
13+
public RedirectView bad1(String redirectUrl, HttpServletResponse response) throws Exception {
14+
RedirectView rv = new RedirectView();
15+
rv.setUrl(redirectUrl);
16+
return rv;
17+
}
18+
19+
@GetMapping("url2")
20+
public String bad2(String redirectUrl) {
21+
String url = "redirect:" + redirectUrl;
22+
return url;
23+
}
24+
25+
@GetMapping("url3")
26+
public RedirectView bad3(String redirectUrl) {
27+
RedirectView rv = new RedirectView(redirectUrl);
28+
return rv;
29+
}
30+
31+
@GetMapping("url4")
32+
public ModelAndView bad4(String redirectUrl) {
33+
return new ModelAndView("redirect:" + redirectUrl);
34+
}
35+
36+
@GetMapping("url5")
37+
public RedirectView good1(String redirectUrl) {
38+
RedirectView rv = new RedirectView();
39+
if (redirectUrl.startsWith(VALID_REDIRECT)){
40+
rv.setUrl(redirectUrl);
41+
}else {
42+
rv.setUrl(VALID_REDIRECT);
43+
}
44+
return rv;
45+
}
46+
}

‎java/ql/src/experimental/Security/CWE/CWE-601/SpringUrlRedirect.qhelp

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
6+
7+
<overview>
8+
<p>Directly incorporating user input into a URL redirect request without validating the input
9+
can facilitate phishing attacks. In these attacks, unsuspecting users can be redirected to a
10+
malicious site that looks very similar to the real site they intend to visit, but which is
11+
controlled by the attacker.</p>
12+
13+
</overview>
14+
<recommendation>
15+
16+
<p>To guard against untrusted URL redirection, it is advisable to avoid putting user input
17+
directly into a redirect URL. Instead, maintain a list of authorized
18+
redirects on the server; then choose from that list based on the user input provided.</p>
19+
20+
</recommendation>
21+
<example>
22+
23+
<p>The following examples show the bad case and the good case respectively.
24+
In <code>bad1</code> method and <code>bad2</code> method and <code>bad3</code> method and
25+
<code>bad4</code> method, shows an HTTP request parameter being used directly in a URL redirect
26+
without validating the input, which facilitates phishing attacks. In <code>good1</code> method,
27+
shows how to solve this problem by verifying whether the user input is a known fixed string beginning.
28+
</p>
29+
30+
<sample src="SpringUrlRedirect.java" />
31+
32+
</example>
33+
<references>
34+
<li>A Guide To Spring Redirects: <a href="https://www.baeldung.com/spring-redirect-and-forward">Spring Redirects</a>.</li>
35+
<li>Url redirection - attack and defense: <a href="https://www.virtuesecurity.com/kb/url-redirection-attack-and-defense/">Url Redirection</a>.</li>
36+
</references>
37+
</qhelp>

‎java/ql/src/experimental/Security/CWE/CWE-601/SpringUrlRedirect.ql

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,66 @@
1+
/**
2+
* @name Spring url redirection from remote source
3+
* @description Spring url redirection based on unvalidated user-input
4+
* may cause redirection to malicious web sites.
5+
* @kind path-problem
6+
* @problem.severity error
7+
* @precision high
8+
* @id java/spring-unvalidated-url-redirection
9+
* @tags security
10+
* external/cwe-601
11+
*/
12+
13+
import java
14+
import SpringUrlRedirect
15+
import semmle.code.java.dataflow.FlowSources
16+
import DataFlow::PathGraph
17+
18+
private class StartsWithSanitizer extends DataFlow::BarrierGuard {
19+
StartsWithSanitizer() {
20+
this.(MethodAccess).getMethod().hasName("startsWith") and
21+
this.(MethodAccess).getMethod().getDeclaringType() instanceof TypeString and
22+
this.(MethodAccess).getMethod().getNumberOfParameters() = 1
23+
}
24+
25+
override predicate checks(Expr e, boolean branch) {
26+
e = this.(MethodAccess).getQualifier() and branch = true
27+
}
28+
}
29+
30+
class SpringUrlRedirectFlowConfig extends TaintTracking::Configuration {
31+
SpringUrlRedirectFlowConfig() { this = "SpringUrlRedirectFlowConfig" }
32+
33+
override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
34+
35+
override predicate isSink(DataFlow::Node sink) { sink instanceof SpringUrlRedirectSink }
36+
37+
override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
38+
guard instanceof StartsWithSanitizer
39+
}
40+
41+
override predicate isSanitizer(DataFlow::Node node) {
42+
// Exclude the case where the left side of the concatenated string is not `redirect:`.
43+
// E.g: `String url = "/path?token=" + request.getParameter("token");`
44+
// Note this is quite a broad sanitizer (it will also sanitize the right-hand side of `url = "http://" + request.getParameter("token")`);
45+
// Consider making this stricter in future.
46+
exists(AddExpr ae |
47+
ae.getRightOperand() = node.asExpr() and
48+
not ae instanceof RedirectBuilderExpr
49+
)
50+
or
51+
exists(MethodAccess ma, int index |
52+
ma.getMethod().hasName("format") and
53+
ma.getMethod().getDeclaringType() instanceof TypeString and
54+
ma.getArgument(index) = node.asExpr() and
55+
(
56+
index != 0 and
57+
not ma.getArgument(0).(CompileTimeConstantExpr).getStringValue().regexpMatch("^%s.*")
58+
)
59+
)
60+
}
61+
}
62+
63+
from DataFlow::PathNode source, DataFlow::PathNode sink, SpringUrlRedirectFlowConfig conf
64+
where conf.hasFlowPath(source, sink)
65+
select sink.getNode(), source, sink, "Potentially untrusted URL redirection due to $@.",
66+
source.getNode(), "user-provided value"

‎java/ql/src/experimental/Security/CWE/CWE-601/SpringUrlRedirect.qll

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

‎java/ql/test/experimental/query-tests/security/CWE-601/SpringUrlRedirect.expected

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
edges
2+
| SpringUrlRedirect.java:13:30:13:47 | redirectUrl : String | SpringUrlRedirect.java:15:19:15:29 | redirectUrl |
3+
| SpringUrlRedirect.java:20:24:20:41 | redirectUrl : String | SpringUrlRedirect.java:21:36:21:46 | redirectUrl |
4+
| SpringUrlRedirect.java:26:30:26:47 | redirectUrl : String | SpringUrlRedirect.java:27:44:27:54 | redirectUrl |
5+
| SpringUrlRedirect.java:32:30:32:47 | redirectUrl : String | SpringUrlRedirect.java:33:47:33:57 | redirectUrl |
6+
| SpringUrlRedirect.java:37:24:37:41 | redirectUrl : String | SpringUrlRedirect.java:40:29:40:39 | redirectUrl |
7+
| SpringUrlRedirect.java:45:24:45:41 | redirectUrl : String | SpringUrlRedirect.java:48:30:48:40 | redirectUrl |
8+
| SpringUrlRedirect.java:53:24:53:41 | redirectUrl : String | SpringUrlRedirect.java:54:30:54:66 | format(...) |
9+
| SpringUrlRedirect.java:58:24:58:41 | redirectUrl : String | SpringUrlRedirect.java:59:30:59:76 | format(...) |
10+
nodes
11+
| SpringUrlRedirect.java:13:30:13:47 | redirectUrl : String | semmle.label | redirectUrl : String |
12+
| SpringUrlRedirect.java:15:19:15:29 | redirectUrl | semmle.label | redirectUrl |
13+
| SpringUrlRedirect.java:20:24:20:41 | redirectUrl : String | semmle.label | redirectUrl : String |
14+
| SpringUrlRedirect.java:21:36:21:46 | redirectUrl | semmle.label | redirectUrl |
15+
| SpringUrlRedirect.java:26:30:26:47 | redirectUrl : String | semmle.label | redirectUrl : String |
16+
| SpringUrlRedirect.java:27:44:27:54 | redirectUrl | semmle.label | redirectUrl |
17+
| SpringUrlRedirect.java:32:30:32:47 | redirectUrl : String | semmle.label | redirectUrl : String |
18+
| SpringUrlRedirect.java:33:47:33:57 | redirectUrl | semmle.label | redirectUrl |
19+
| SpringUrlRedirect.java:37:24:37:41 | redirectUrl : String | semmle.label | redirectUrl : String |
20+
| SpringUrlRedirect.java:40:29:40:39 | redirectUrl | semmle.label | redirectUrl |
21+
| SpringUrlRedirect.java:45:24:45:41 | redirectUrl : String | semmle.label | redirectUrl : String |
22+
| SpringUrlRedirect.java:48:30:48:40 | redirectUrl | semmle.label | redirectUrl |
23+
| SpringUrlRedirect.java:53:24:53:41 | redirectUrl : String | semmle.label | redirectUrl : String |
24+
| SpringUrlRedirect.java:54:30:54:66 | format(...) | semmle.label | format(...) |
25+
| SpringUrlRedirect.java:58:24:58:41 | redirectUrl : String | semmle.label | redirectUrl : String |
26+
| SpringUrlRedirect.java:59:30:59:76 | format(...) | semmle.label | format(...) |
27+
#select
28+
| SpringUrlRedirect.java:15:19:15:29 | redirectUrl | SpringUrlRedirect.java:13:30:13:47 | redirectUrl : String | SpringUrlRedirect.java:15:19:15:29 | redirectUrl | Potentially untrusted URL redirection due to $@. | SpringUrlRedirect.java:13:30:13:47 | redirectUrl | user-provided value |
29+
| SpringUrlRedirect.java:21:36:21:46 | redirectUrl | SpringUrlRedirect.java:20:24:20:41 | redirectUrl : String | SpringUrlRedirect.java:21:36:21:46 | redirectUrl | Potentially untrusted URL redirection due to $@. | SpringUrlRedirect.java:20:24:20:41 | redirectUrl | user-provided value |
30+
| SpringUrlRedirect.java:27:44:27:54 | redirectUrl | SpringUrlRedirect.java:26:30:26:47 | redirectUrl : String | SpringUrlRedirect.java:27:44:27:54 | redirectUrl | Potentially untrusted URL redirection due to $@. | SpringUrlRedirect.java:26:30:26:47 | redirectUrl | user-provided value |
31+
| SpringUrlRedirect.java:33:47:33:57 | redirectUrl | SpringUrlRedirect.java:32:30:32:47 | redirectUrl : String | SpringUrlRedirect.java:33:47:33:57 | redirectUrl | Potentially untrusted URL redirection due to $@. | SpringUrlRedirect.java:32:30:32:47 | redirectUrl | user-provided value |
32+
| SpringUrlRedirect.java:40:29:40:39 | redirectUrl | SpringUrlRedirect.java:37:24:37:41 | redirectUrl : String | SpringUrlRedirect.java:40:29:40:39 | redirectUrl | Potentially untrusted URL redirection due to $@. | SpringUrlRedirect.java:37:24:37:41 | redirectUrl | user-provided value |
33+
| SpringUrlRedirect.java:48:30:48:40 | redirectUrl | SpringUrlRedirect.java:45:24:45:41 | redirectUrl : String | SpringUrlRedirect.java:48:30:48:40 | redirectUrl | Potentially untrusted URL redirection due to $@. | SpringUrlRedirect.java:45:24:45:41 | redirectUrl | user-provided value |
34+
| SpringUrlRedirect.java:54:30:54:66 | format(...) | SpringUrlRedirect.java:53:24:53:41 | redirectUrl : String | SpringUrlRedirect.java:54:30:54:66 | format(...) | Potentially untrusted URL redirection due to $@. | SpringUrlRedirect.java:53:24:53:41 | redirectUrl | user-provided value |
35+
| SpringUrlRedirect.java:59:30:59:76 | format(...) | SpringUrlRedirect.java:58:24:58:41 | redirectUrl : String | SpringUrlRedirect.java:59:30:59:76 | format(...) | Potentially untrusted URL redirection due to $@. | SpringUrlRedirect.java:58:24:58:41 | redirectUrl | user-provided value |

‎java/ql/test/experimental/query-tests/security/CWE-601/SpringUrlRedirect.java

Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,83 @@
1+
import javax.servlet.http.HttpServletResponse;
2+
import org.springframework.stereotype.Controller;
3+
import org.springframework.web.bind.annotation.GetMapping;
4+
import org.springframework.web.servlet.ModelAndView;
5+
import org.springframework.web.servlet.view.RedirectView;
6+
7+
@Controller
8+
public class SpringUrlRedirect {
9+
10+
private final static String VALID_REDIRECT = "http://127.0.0.1";
11+
12+
@GetMapping("url1")
13+
public RedirectView bad1(String redirectUrl, HttpServletResponse response) throws Exception {
14+
RedirectView rv = new RedirectView();
15+
rv.setUrl(redirectUrl);
16+
return rv;
17+
}
18+
19+
@GetMapping("url2")
20+
public String bad2(String redirectUrl) {
21+
String url = "redirect:" + redirectUrl;
22+
return url;
23+
}
24+
25+
@GetMapping("url3")
26+
public RedirectView bad3(String redirectUrl) {
27+
RedirectView rv = new RedirectView(redirectUrl);
28+
return rv;
29+
}
30+
31+
@GetMapping("url4")
32+
public ModelAndView bad4(String redirectUrl) {
33+
return new ModelAndView("redirect:" + redirectUrl);
34+
}
35+
36+
@GetMapping("url5")
37+
public String bad5(String redirectUrl) {
38+
StringBuffer stringBuffer = new StringBuffer();
39+
stringBuffer.append("redirect:");
40+
stringBuffer.append(redirectUrl);
41+
return stringBuffer.toString();
42+
}
43+
44+
@GetMapping("url6")
45+
public String bad6(String redirectUrl) {
46+
StringBuilder stringBuilder = new StringBuilder();
47+
stringBuilder.append("redirect:");
48+
stringBuilder.append(redirectUrl);
49+
return stringBuilder.toString();
50+
}
51+
52+
@GetMapping("url7")
53+
public String bad7(String redirectUrl) {
54+
return "redirect:" + String.format("%s/?aaa", redirectUrl);
55+
}
56+
57+
@GetMapping("url8")
58+
public String bad8(String redirectUrl, String token) {
59+
return "redirect:" + String.format(redirectUrl + "?token=%s", token);
60+
}
61+
62+
@GetMapping("url9")
63+
public RedirectView good1(String redirectUrl) {
64+
RedirectView rv = new RedirectView();
65+
if (redirectUrl.startsWith(VALID_REDIRECT)){
66+
rv.setUrl(redirectUrl);
67+
}else {
68+
rv.setUrl(VALID_REDIRECT);
69+
}
70+
return rv;
71+
}
72+
73+
@GetMapping("url10")
74+
public ModelAndView good2(String token) {
75+
String url = "/edit?token=" + token;
76+
return new ModelAndView("redirect:" + url);
77+
}
78+
79+
@GetMapping("url11")
80+
public String good3(String status) {
81+
return "redirect:" + String.format("/stories/search/criteria?status=%s", status);
82+
}
83+
}

‎java/ql/test/experimental/query-tests/security/CWE-601/SpringUrlRedirect.qlref

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

‎java/ql/test/experimental/query-tests/security/CWE-601/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)