Skip to content

Commit 3a4deee

Browse files
theinfosecguytheinfosecguy
committed
Java: Add Spring MVC URL redirect sinks
1 parent b9f8dee commit 3a4deee

5 files changed

Lines changed: 255 additions & 7 deletions

File tree

‎java/ql/lib/change-notes/2026-08-17-spring-url-redirect-sinks.md‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
---
2+
category: minorAnalysis
3+
---
4+
* The `java/unvalidated-url-redirection` query now detects untrusted URLs used in Spring MVC
5+
`RedirectView` objects and `redirect:` view names, including view names constructed in helper
6+
methods called by request handlers.

‎java/ql/lib/semmle/code/java/security/UrlRedirect.qll‎

Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,9 @@ import semmle.code.java.frameworks.Servlets
88
import semmle.code.java.frameworks.ApacheHttp
99
private import semmle.code.java.dataflow.ExternalFlow
1010
private import semmle.code.java.dataflow.FlowSinks
11+
private import semmle.code.java.dataflow.StringPrefixes
1112
private import semmle.code.java.frameworks.JaxWS
13+
private import semmle.code.java.frameworks.spring.SpringController
1214
private import semmle.code.java.security.RequestForgery
1315

1416
/** A URL redirection sink. */
@@ -50,5 +52,87 @@ private class ApacheUrlRedirectSink extends UrlRedirectSink {
5052
}
5153
}
5254

55+
/**
56+
* An expression appended to a Spring `"redirect:"` view-name prefix from a request handler or a
57+
* helper called by one.
58+
*/
59+
private class SpringUrlRedirectPrefixSink extends UrlRedirectSink {
60+
SpringUrlRedirectPrefixSink() {
61+
isSpringMvcViewResult(this.asExpr()) and
62+
appendedToRedirectPrefix(this)
63+
}
64+
}
65+
66+
pragma[nomagic]
67+
private predicate appendedToRedirectPrefix(DataFlow::ExprNode exprNode) {
68+
exists(SpringRedirectPrefix prefix | exprNode.asExpr() = prefix.getAnAppendedExpression())
69+
}
70+
71+
private class SpringRedirectPrefix extends InterestingPrefix {
72+
SpringRedirectPrefix() { this.getStringValue() = "redirect:" }
73+
74+
override int getOffset() { result = 0 }
75+
}
76+
77+
private predicate contributesToReturn(Expr value) {
78+
exists(ReturnStmt ret |
79+
ret.getEnclosingCallable() = value.getEnclosingCallable() and
80+
(
81+
value.getParent*() = ret.getExpr()
82+
or
83+
DataFlow::localFlow(DataFlow::exprNode(value), DataFlow::exprNode(ret.getExpr()))
84+
)
85+
)
86+
}
87+
88+
/** Holds if `value` contributes to a view returned from a Spring MVC request handler. */
89+
pragma[nomagic]
90+
private predicate isSpringMvcViewResult(Expr value) {
91+
contributesToReturn(value) and
92+
value.getEnclosingCallable() instanceof SpringRequestMappingMethod and
93+
not value.getEnclosingCallable().(SpringRequestMappingMethod).isResponseBody()
94+
or
95+
contributesToReturn(value) and
96+
exists(MethodCall call |
97+
call.getCallee().getSourceDeclaration() = value.getEnclosingCallable() and
98+
isSpringMvcViewResult(call)
99+
)
100+
}
101+
102+
private class SpringRedirectViewType extends RefType {
103+
SpringRedirectViewType() {
104+
this.getASupertype*().hasQualifiedName("org.springframework.web.servlet.view", "RedirectView")
105+
}
106+
}
107+
108+
/** A URL passed to a Spring `RedirectView` constructor. */
109+
private class SpringRedirectViewSink extends UrlRedirectSink {
110+
SpringRedirectViewSink() {
111+
exists(ClassInstanceExpr newRedirectView |
112+
newRedirectView.getConstructedType() instanceof SpringRedirectViewType and
113+
isSpringMvcViewResult(newRedirectView) and
114+
this.asExpr() = newRedirectView.getArgument(0)
115+
)
116+
}
117+
}
118+
119+
/** A URL passed to `setUrl` on a Spring `RedirectView` that is returned by a request handler. */
120+
private class SpringRedirectViewSetUrlSink extends UrlRedirectSink {
121+
SpringRedirectViewSetUrlSink() {
122+
exists(MethodCall setUrl |
123+
setUrl.getMethod().hasName("setUrl") and
124+
setUrl.getMethod().getNumberOfParameters() = 1 and
125+
setUrl
126+
.getMethod()
127+
.getDeclaringType()
128+
.getASupertype*()
129+
.hasQualifiedName("org.springframework.web.servlet.view", "AbstractUrlBasedView") and
130+
setUrl.getQualifier().getType() instanceof SpringRedirectViewType and
131+
isSpringMvcViewResult(setUrl.getQualifier()) and
132+
this.asExpr() = setUrl.getArgument(0)
133+
)
134+
}
135+
}
136+
53137
private class DefaultUrlRedirectSanitizer extends UrlRedirectSanitizer instanceof RequestForgerySanitizer
54138
{ }

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

Lines changed: 125 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,125 @@
1+
package test.cwe601.cwe.examples;
2+
3+
import javax.servlet.http.HttpServletRequest;
4+
import org.springframework.stereotype.Controller;
5+
import org.springframework.web.bind.annotation.GetMapping;
6+
import org.springframework.web.bind.annotation.ResponseBody;
7+
import org.springframework.web.bind.annotation.RestController;
8+
import org.springframework.web.servlet.ModelAndView;
9+
import org.springframework.web.servlet.view.RedirectView;
10+
11+
class BaseController {
12+
protected String redirect(String path) {
13+
return "redirect:" + path; // $ Alert
14+
}
15+
16+
protected String ordinaryView(String path) {
17+
return "view:" + path;
18+
}
19+
20+
protected String discardedRedirect(String path) {
21+
return "redirect:" + path;
22+
}
23+
}
24+
25+
class CustomRedirectView extends RedirectView {
26+
CustomRedirectView(String url) {
27+
super(url);
28+
}
29+
}
30+
31+
@Controller
32+
public class SpringUrlRedirect extends BaseController {
33+
@GetMapping("/case1")
34+
public String directViewName(HttpServletRequest request) {
35+
String next = request.getHeader("referer"); // $ Source
36+
return "redirect:" + next; // $ Alert
37+
}
38+
39+
@GetMapping("/case2")
40+
public ModelAndView modelAndView(HttpServletRequest request) {
41+
String next = request.getHeader("referer"); // $ Source
42+
return new ModelAndView("redirect:" + next); // $ Alert
43+
}
44+
45+
@GetMapping("/case3")
46+
public RedirectView redirectView(HttpServletRequest request) {
47+
String next = request.getHeader("referer"); // $ Source
48+
return new RedirectView(next); // $ Alert
49+
}
50+
51+
@GetMapping("/case4")
52+
public String helperViewName(HttpServletRequest request) {
53+
String next = request.getHeader("referer"); // $ Source
54+
return redirect(next);
55+
}
56+
57+
@GetMapping("/case5")
58+
public RedirectView configuredRedirectView(HttpServletRequest request) {
59+
String next = request.getParameter("next"); // $ Source
60+
RedirectView view = new RedirectView();
61+
view.setUrl(next); // $ Alert
62+
return view;
63+
}
64+
65+
@GetMapping("/case6")
66+
public RedirectView overloadedRedirectView(HttpServletRequest request) {
67+
String next = request.getParameter("next"); // $ Source
68+
return new RedirectView(next, true); // $ Alert
69+
}
70+
71+
@GetMapping("/case7")
72+
public RedirectView customRedirectView(HttpServletRequest request) {
73+
String next = request.getParameter("next"); // $ Source
74+
return new CustomRedirectView(next); // $ Alert
75+
}
76+
77+
@GetMapping("/safe-constant")
78+
public String constantViewName() {
79+
return "redirect:/account";
80+
}
81+
82+
@GetMapping("/safe-path")
83+
public String fixedPath(HttpServletRequest request) {
84+
String tab = request.getParameter("tab");
85+
return "redirect:/account?tab=" + tab;
86+
}
87+
88+
@GetMapping("/safe-model-and-view")
89+
public ModelAndView ordinaryModelAndView(HttpServletRequest request) {
90+
String view = request.getParameter("view");
91+
return new ModelAndView(ordinaryView(view));
92+
}
93+
94+
@GetMapping("/safe-discarded")
95+
public String discardedRedirectValue(HttpServletRequest request) {
96+
discardedRedirect(request.getParameter("next"));
97+
return "home";
98+
}
99+
100+
@GetMapping("/safe-validated")
101+
public RedirectView validatedRedirectView(HttpServletRequest request) {
102+
String next = request.getParameter("next");
103+
if ("https://example.com/account".equals(next)) {
104+
return new RedirectView("https://example.com/account");
105+
}
106+
return new RedirectView("https://example.com/account");
107+
}
108+
}
109+
110+
@Controller
111+
class ResponseBodyController {
112+
@ResponseBody
113+
@GetMapping("/body")
114+
public String responseBody(HttpServletRequest request) {
115+
return "redirect:" + request.getParameter("value");
116+
}
117+
}
118+
119+
@RestController
120+
class JsonController {
121+
@GetMapping("/json")
122+
public String responseBody(HttpServletRequest request) {
123+
return "redirect:" + request.getParameter("value");
124+
}
125+
}

‎java/ql/test/query-tests/security/CWE-601/semmle/tests/UrlRedirect.expected‎

Lines changed: 39 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,22 +1,55 @@
11
#select
2+
| SpringUrlRedirect.java:13:30:13:33 | path | SpringUrlRedirect.java:53:23:53:50 | getHeader(...) : String | SpringUrlRedirect.java:13:30:13:33 | path | Untrusted URL redirection depends on a $@. | SpringUrlRedirect.java:53:23:53:50 | getHeader(...) | user-provided value |
3+
| SpringUrlRedirect.java:36:30:36:33 | next | SpringUrlRedirect.java:35:23:35:50 | getHeader(...) : String | SpringUrlRedirect.java:36:30:36:33 | next | Untrusted URL redirection depends on a $@. | SpringUrlRedirect.java:35:23:35:50 | getHeader(...) | user-provided value |
4+
| SpringUrlRedirect.java:42:47:42:50 | next | SpringUrlRedirect.java:41:23:41:50 | getHeader(...) : String | SpringUrlRedirect.java:42:47:42:50 | next | Untrusted URL redirection depends on a $@. | SpringUrlRedirect.java:41:23:41:50 | getHeader(...) | user-provided value |
5+
| SpringUrlRedirect.java:48:33:48:36 | next | SpringUrlRedirect.java:47:23:47:50 | getHeader(...) : String | SpringUrlRedirect.java:48:33:48:36 | next | Untrusted URL redirection depends on a $@. | SpringUrlRedirect.java:47:23:47:50 | getHeader(...) | user-provided value |
6+
| SpringUrlRedirect.java:61:21:61:24 | next | SpringUrlRedirect.java:59:23:59:50 | getParameter(...) : String | SpringUrlRedirect.java:61:21:61:24 | next | Untrusted URL redirection depends on a $@. | SpringUrlRedirect.java:59:23:59:50 | getParameter(...) | user-provided value |
7+
| SpringUrlRedirect.java:68:33:68:36 | next | SpringUrlRedirect.java:67:23:67:50 | getParameter(...) : String | SpringUrlRedirect.java:68:33:68:36 | next | Untrusted URL redirection depends on a $@. | SpringUrlRedirect.java:67:23:67:50 | getParameter(...) | user-provided value |
8+
| SpringUrlRedirect.java:74:39:74:42 | next | SpringUrlRedirect.java:73:23:73:50 | getParameter(...) : String | SpringUrlRedirect.java:74:39:74:42 | next | Untrusted URL redirection depends on a $@. | SpringUrlRedirect.java:73:23:73:50 | getParameter(...) | user-provided value |
29
| UrlRedirect2.java:27:25:27:54 | getParameter(...) | UrlRedirect2.java:27:25:27:54 | getParameter(...) | UrlRedirect2.java:27:25:27:54 | getParameter(...) | Untrusted URL redirection depends on a $@. | UrlRedirect2.java:27:25:27:54 | getParameter(...) | user-provided value |
310
| UrlRedirect.java:23:25:23:54 | getParameter(...) | UrlRedirect.java:23:25:23:54 | getParameter(...) | UrlRedirect.java:23:25:23:54 | getParameter(...) | Untrusted URL redirection depends on a $@. | UrlRedirect.java:23:25:23:54 | getParameter(...) | user-provided value |
411
| UrlRedirect.java:32:25:32:67 | weakCleanup(...) | UrlRedirect.java:32:37:32:66 | getParameter(...) : String | UrlRedirect.java:32:25:32:67 | weakCleanup(...) | Untrusted URL redirection depends on a $@. | UrlRedirect.java:32:37:32:66 | getParameter(...) | user-provided value |
512
| UrlRedirect.java:39:34:39:63 | getParameter(...) | UrlRedirect.java:39:34:39:63 | getParameter(...) | UrlRedirect.java:39:34:39:63 | getParameter(...) | Untrusted URL redirection depends on a $@. | UrlRedirect.java:39:34:39:63 | getParameter(...) | user-provided value |
613
| UrlRedirect.java:42:43:42:72 | getParameter(...) | UrlRedirect.java:42:43:42:72 | getParameter(...) | UrlRedirect.java:42:43:42:72 | getParameter(...) | Untrusted URL redirection depends on a $@. | UrlRedirect.java:42:43:42:72 | getParameter(...) | user-provided value |
714
| mad/Test.java:14:22:14:38 | (...)... | mad/Test.java:9:16:9:41 | getParameter(...) : String | mad/Test.java:14:22:14:38 | (...)... | Untrusted URL redirection depends on a $@. | mad/Test.java:9:16:9:41 | getParameter(...) | user-provided value |
815
edges
9-
| UrlRedirect.java:32:37:32:66 | getParameter(...) : String | UrlRedirect.java:32:25:32:67 | weakCleanup(...) | provenance | Src:MaD:2 MaD:3 |
10-
| UrlRedirect.java:32:37:32:66 | getParameter(...) : String | UrlRedirect.java:45:28:45:39 | input : String | provenance | Src:MaD:2 |
16+
| SpringUrlRedirect.java:12:31:12:41 | path : String | SpringUrlRedirect.java:13:30:13:33 | path | provenance | |
17+
| SpringUrlRedirect.java:35:23:35:50 | getHeader(...) : String | SpringUrlRedirect.java:36:30:36:33 | next | provenance | Src:MaD:2 |
18+
| SpringUrlRedirect.java:41:23:41:50 | getHeader(...) : String | SpringUrlRedirect.java:42:47:42:50 | next | provenance | Src:MaD:2 |
19+
| SpringUrlRedirect.java:47:23:47:50 | getHeader(...) : String | SpringUrlRedirect.java:48:33:48:36 | next | provenance | Src:MaD:2 |
20+
| SpringUrlRedirect.java:53:23:53:50 | getHeader(...) : String | SpringUrlRedirect.java:54:25:54:28 | next : String | provenance | Src:MaD:2 |
21+
| SpringUrlRedirect.java:54:25:54:28 | next : String | SpringUrlRedirect.java:12:31:12:41 | path : String | provenance | |
22+
| SpringUrlRedirect.java:59:23:59:50 | getParameter(...) : String | SpringUrlRedirect.java:61:21:61:24 | next | provenance | Src:MaD:3 |
23+
| SpringUrlRedirect.java:67:23:67:50 | getParameter(...) : String | SpringUrlRedirect.java:68:33:68:36 | next | provenance | Src:MaD:3 |
24+
| SpringUrlRedirect.java:73:23:73:50 | getParameter(...) : String | SpringUrlRedirect.java:74:39:74:42 | next | provenance | Src:MaD:3 |
25+
| UrlRedirect.java:32:37:32:66 | getParameter(...) : String | UrlRedirect.java:32:25:32:67 | weakCleanup(...) | provenance | Src:MaD:3 MaD:4 |
26+
| UrlRedirect.java:32:37:32:66 | getParameter(...) : String | UrlRedirect.java:45:28:45:39 | input : String | provenance | Src:MaD:3 |
1127
| UrlRedirect.java:45:28:45:39 | input : String | UrlRedirect.java:46:10:46:14 | input : String | provenance | |
12-
| UrlRedirect.java:46:10:46:14 | input : String | UrlRedirect.java:46:10:46:40 | replaceAll(...) : String | provenance | MaD:3 |
13-
| mad/Test.java:9:16:9:41 | getParameter(...) : String | mad/Test.java:14:31:14:38 | source(...) : String | provenance | Src:MaD:2 |
28+
| UrlRedirect.java:46:10:46:14 | input : String | UrlRedirect.java:46:10:46:40 | replaceAll(...) : String | provenance | MaD:4 |
29+
| mad/Test.java:9:16:9:41 | getParameter(...) : String | mad/Test.java:14:31:14:38 | source(...) : String | provenance | Src:MaD:3 |
1430
| mad/Test.java:14:31:14:38 | source(...) : String | mad/Test.java:14:22:14:38 | (...)... | provenance | Sink:MaD:1 |
1531
models
1632
| 1 | Sink: org.kohsuke.stapler; HttpResponses; true; redirectTo; (String); ; Argument[0]; url-redirection; ai-manual |
17-
| 2 | Source: javax.servlet; ServletRequest; false; getParameter; (String); ; ReturnValue; remote; manual |
18-
| 3 | Summary: java.lang; String; false; replaceAll; ; ; Argument[this]; ReturnValue; taint; manual |
33+
| 2 | Source: javax.servlet.http; HttpServletRequest; false; getHeader; (String); ; ReturnValue; remote; manual |
34+
| 3 | Source: javax.servlet; ServletRequest; false; getParameter; (String); ; ReturnValue; remote; manual |
35+
| 4 | Summary: java.lang; String; false; replaceAll; ; ; Argument[this]; ReturnValue; taint; manual |
1936
nodes
37+
| SpringUrlRedirect.java:12:31:12:41 | path : String | semmle.label | path : String |
38+
| SpringUrlRedirect.java:13:30:13:33 | path | semmle.label | path |
39+
| SpringUrlRedirect.java:35:23:35:50 | getHeader(...) : String | semmle.label | getHeader(...) : String |
40+
| SpringUrlRedirect.java:36:30:36:33 | next | semmle.label | next |
41+
| SpringUrlRedirect.java:41:23:41:50 | getHeader(...) : String | semmle.label | getHeader(...) : String |
42+
| SpringUrlRedirect.java:42:47:42:50 | next | semmle.label | next |
43+
| SpringUrlRedirect.java:47:23:47:50 | getHeader(...) : String | semmle.label | getHeader(...) : String |
44+
| SpringUrlRedirect.java:48:33:48:36 | next | semmle.label | next |
45+
| SpringUrlRedirect.java:53:23:53:50 | getHeader(...) : String | semmle.label | getHeader(...) : String |
46+
| SpringUrlRedirect.java:54:25:54:28 | next : String | semmle.label | next : String |
47+
| SpringUrlRedirect.java:59:23:59:50 | getParameter(...) : String | semmle.label | getParameter(...) : String |
48+
| SpringUrlRedirect.java:61:21:61:24 | next | semmle.label | next |
49+
| SpringUrlRedirect.java:67:23:67:50 | getParameter(...) : String | semmle.label | getParameter(...) : String |
50+
| SpringUrlRedirect.java:68:33:68:36 | next | semmle.label | next |
51+
| SpringUrlRedirect.java:73:23:73:50 | getParameter(...) : String | semmle.label | getParameter(...) : String |
52+
| SpringUrlRedirect.java:74:39:74:42 | next | semmle.label | next |
2053
| UrlRedirect2.java:27:25:27:54 | getParameter(...) | semmle.label | getParameter(...) |
2154
| UrlRedirect.java:23:25:23:54 | getParameter(...) | semmle.label | getParameter(...) |
2255
| UrlRedirect.java:32:25:32:67 | weakCleanup(...) | semmle.label | weakCleanup(...) |

‎java/ql/test/query-tests/security/CWE-601/semmle/tests/options‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
//semmle-extractor-options: --javac-args -cp ${testdir}/../../../../../stubs/servlet-api-2.4:${testdir}/../../../../../stubs/stapler-1.263:${testdir}/../../../../../stubs/javax-servlet-2.5:${testdir}/../../../../../stubs/apache-commons-jelly-1.0.1:${testdir}/../../../../../stubs/apache-commons-fileupload-1.4:${testdir}/../../../../../stubs/saxon-xqj-9.x:${testdir}/../../../../../stubs/apache-commons-beanutils:${testdir}/../../../../../stubs/dom4j-2.1.1:${testdir}/../../../../../stubs/apache-commons-lang:${testdir}/../../../../../stubs/jaxen-1.2.0
1+
//semmle-extractor-options: --javac-args -cp ${testdir}/../../../../../stubs/servlet-api-2.4:${testdir}/../../../../../stubs/stapler-1.263:${testdir}/../../../../../stubs/javax-servlet-2.5:${testdir}/../../../../../stubs/apache-commons-jelly-1.0.1:${testdir}/../../../../../stubs/apache-commons-fileupload-1.4:${testdir}/../../../../../stubs/saxon-xqj-9.x:${testdir}/../../../../../stubs/apache-commons-beanutils:${testdir}/../../../../../stubs/dom4j-2.1.1:${testdir}/../../../../../stubs/apache-commons-lang:${testdir}/../../../../../stubs/jaxen-1.2.0:${testdir}/../../../../../stubs/springframework-5.8.x

0 commit comments

Comments
 (0)