diff --git a/java/ql/lib/change-notes/2026-08-17-spring-url-redirect-sinks.md b/java/ql/lib/change-notes/2026-08-17-spring-url-redirect-sinks.md new file mode 100644 index 000000000000..7e9200ff16b7 --- /dev/null +++ b/java/ql/lib/change-notes/2026-08-17-spring-url-redirect-sinks.md @@ -0,0 +1,6 @@ +--- +category: minorAnalysis +--- +* The `java/unvalidated-url-redirection` query now detects untrusted URLs used in Spring MVC + `RedirectView` objects and `redirect:` view names, including view names constructed in helper + methods called by request handlers. diff --git a/java/ql/lib/semmle/code/java/security/UrlRedirect.qll b/java/ql/lib/semmle/code/java/security/UrlRedirect.qll index be6addfa2529..13d1911b66b0 100644 --- a/java/ql/lib/semmle/code/java/security/UrlRedirect.qll +++ b/java/ql/lib/semmle/code/java/security/UrlRedirect.qll @@ -8,7 +8,10 @@ import semmle.code.java.frameworks.Servlets import semmle.code.java.frameworks.ApacheHttp private import semmle.code.java.dataflow.ExternalFlow private import semmle.code.java.dataflow.FlowSinks +private import semmle.code.java.dataflow.StringPrefixes +private import semmle.code.java.dataflow.TaintTracking private import semmle.code.java.frameworks.JaxWS +private import semmle.code.java.frameworks.spring.SpringController private import semmle.code.java.security.RequestForgery /** A URL redirection sink. */ @@ -50,5 +53,131 @@ private class ApacheUrlRedirectSink extends UrlRedirectSink { } } +/** An expression appended to a Spring `"redirect:"` view-name returned by a request handler. */ +private class SpringUrlRedirectPrefixSink extends UrlRedirectSink { + SpringUrlRedirectPrefixSink() { + appendedToRedirectPrefix(this) and + ( + isSpringMvcReturnedString(this.asExpr()) + or + isSpringModelAndViewName(this.asExpr()) + ) + } +} + +/** A call to a helper that returns a Spring `"redirect:"` view name. */ +private class SpringUrlRedirectHelperSink extends UrlRedirectSink { + SpringUrlRedirectHelperSink() { + exists(MethodCall call | + this.asExpr() = call and + isSpringMvcViewResult(call) and + returnsRedirectViewName(call.getCallee().getSourceDeclaration()) + ) + } +} + +pragma[nomagic] +private predicate appendedToRedirectPrefix(DataFlow::ExprNode exprNode) { + exists(SpringRedirectPrefix prefix | exprNode.asExpr() = prefix.getAnAppendedExpression()) +} + +private class SpringRedirectPrefix extends InterestingPrefix { + SpringRedirectPrefix() { this.getStringValue() = "redirect:" } + + override int getOffset() { result = 0 } +} + +private predicate contributesToReturn(Expr value) { + exists(ReturnStmt ret | + ret.getEnclosingCallable() = value.getEnclosingCallable() and + DataFlow::localFlow(DataFlow::exprNode(value), DataFlow::exprNode(ret.getExpr())) + ) +} + +/** Holds if `value` contributes string content to the return value of its callable. */ +private predicate contributesToReturnedString(Expr value) { + exists(ReturnStmt ret | + ret.getEnclosingCallable() = value.getEnclosingCallable() and + TaintTracking::localTaint(DataFlow::exprNode(value), DataFlow::exprNode(ret.getExpr())) + ) +} + +/** Holds if `value` contributes to a view returned from a Spring MVC request handler. */ +pragma[nomagic] +private predicate isSpringMvcViewResult(Expr value) { + contributesToReturn(value) and + value.getEnclosingCallable() instanceof SpringRequestMappingMethod and + not value.getEnclosingCallable().(SpringRequestMappingMethod).isResponseBody() +} + +/** Holds if `value` contributes string content to a view name returned by a request handler. */ +private predicate isSpringMvcReturnedString(Expr value) { + contributesToReturnedString(value) and + value.getEnclosingCallable() instanceof SpringRequestMappingMethod and + not value.getEnclosingCallable().(SpringRequestMappingMethod).isResponseBody() +} + +/** Holds if `value` contributes to the view name of a returned `ModelAndView`. */ +private predicate isSpringModelAndViewName(Expr value) { + exists(ClassInstanceExpr newModelAndView | + newModelAndView + .getConstructedType() + .hasQualifiedName("org.springframework.web.servlet", "ModelAndView") and + TaintTracking::localTaint(DataFlow::exprNode(value), + DataFlow::exprNode(newModelAndView.getArgument(0))) and + isSpringMvcViewResult(newModelAndView) + ) +} + +/** Holds if `callable` returns a view name constructed with the Spring `"redirect:"` prefix. */ +pragma[nomagic] +private predicate returnsRedirectViewName(Callable callable) { + exists(DataFlow::ExprNode appended, ReturnStmt ret | + appendedToRedirectPrefix(appended) and + appended.asExpr().getEnclosingCallable() = callable and + ret.getEnclosingCallable() = callable and + TaintTracking::localTaint(appended, DataFlow::exprNode(ret.getExpr())) + ) + or + exists(MethodCall call | + call.getEnclosingCallable() = callable and + contributesToReturn(call) and + returnsRedirectViewName(call.getCallee().getSourceDeclaration()) + ) +} + +/** A URL passed to a Spring `RedirectView` constructor. */ +private class SpringRedirectViewSink extends UrlRedirectSink { + SpringRedirectViewSink() { + exists(ClassInstanceExpr newRedirectView | + newRedirectView + .getConstructedType() + .hasQualifiedName("org.springframework.web.servlet.view", "RedirectView") and + isSpringMvcViewResult(newRedirectView) and + this.asExpr() = newRedirectView.getArgument(0) + ) + } +} + +/** A URL passed to `setUrl` on a Spring `RedirectView` that is returned by a request handler. */ +private class SpringRedirectViewSetUrlSink extends UrlRedirectSink { + SpringRedirectViewSetUrlSink() { + exists(MethodCall setUrl | + setUrl + .getMethod() + .getSourceDeclaration() + .hasQualifiedName("org.springframework.web.servlet.view", "AbstractUrlBasedView", "setUrl") and + setUrl + .getQualifier() + .getType() + .(RefType) + .getASupertype*() + .hasQualifiedName("org.springframework.web.servlet.view", "RedirectView") and + isSpringMvcViewResult(setUrl.getQualifier()) and + this.asExpr() = setUrl.getArgument(0) + ) + } +} + private class DefaultUrlRedirectSanitizer extends UrlRedirectSanitizer instanceof RequestForgerySanitizer { } diff --git a/java/ql/test/query-tests/security/CWE-601/semmle/tests/SpringUrlRedirect.java b/java/ql/test/query-tests/security/CWE-601/semmle/tests/SpringUrlRedirect.java new file mode 100644 index 000000000000..d2dbb7933c78 --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-601/semmle/tests/SpringUrlRedirect.java @@ -0,0 +1,161 @@ +package test.cwe601.cwe.examples; + +import javax.servlet.http.HttpServletRequest; +import org.springframework.stereotype.Controller; +import org.springframework.web.bind.annotation.GetMapping; +import org.springframework.web.bind.annotation.ResponseBody; +import org.springframework.web.bind.annotation.RestController; +import org.springframework.web.servlet.ModelAndView; +import org.springframework.web.servlet.view.RedirectView; + +class BaseController { + protected String redirect(String path) { + return "redirect:" + path; + } + + protected String ordinaryView(String path) { + return "view:" + path; + } + + protected String discardedRedirect(String path) { + return "redirect:" + path; + } +} + +class LabeledRedirectView extends RedirectView { + LabeledRedirectView(String label, String url) { + super(url); + } + + void setUrl(Object label) { + } +} + +@Controller +public class SpringUrlRedirect extends BaseController { + @GetMapping("/case1") + public String directViewName(HttpServletRequest request) { + String next = request.getHeader("referer"); // $ Source + return "redirect:" + next; // $ Alert + } + + @GetMapping("/case2") + public ModelAndView modelAndView(HttpServletRequest request) { + String next = request.getHeader("referer"); // $ Source + return new ModelAndView("redirect:" + next); // $ Alert + } + + @GetMapping("/case3") + public RedirectView redirectView(HttpServletRequest request) { + String next = request.getHeader("referer"); // $ Source + return new RedirectView(next); // $ Alert + } + + @GetMapping("/case4") + public String helperViewName(HttpServletRequest request) { + String next = request.getHeader("referer"); // $ Source + return redirect(next); // $ Alert + } + + @GetMapping("/case5") + public RedirectView configuredRedirectView(HttpServletRequest request) { + String next = request.getParameter("next"); // $ Source + RedirectView view = new RedirectView(); + view.setUrl(next); // $ Alert + return view; + } + + @GetMapping("/case6") + public RedirectView overloadedRedirectView(HttpServletRequest request) { + String next = request.getParameter("next"); // $ Source + return new RedirectView(next, true); // $ Alert + } + + @GetMapping("/safe-constant") + public String constantViewName() { + return "redirect:/account"; + } + + @GetMapping("/safe-path") + public String fixedPath(HttpServletRequest request) { + String tab = request.getParameter("tab"); + return "redirect:/account?tab=" + tab; + } + + @GetMapping("/safe-model-and-view") + public ModelAndView ordinaryModelAndView(HttpServletRequest request) { + String view = request.getParameter("view"); + return new ModelAndView(ordinaryView(view)); + } + + @GetMapping("/safe-model-value") + public ModelAndView redirectInModel(HttpServletRequest request) { + String value = "redirect:" + request.getParameter("value"); + return new ModelAndView("home", "value", value); + } + + @GetMapping("/safe-discarded") + public String discardedRedirectValue(HttpServletRequest request) { + discardedRedirect(request.getParameter("next")); + return "home"; + } + + @GetMapping("/safe-redirect-view") + public RedirectView constantRedirectView() { + return new RedirectView("https://example.com/account"); + } + + @GetMapping("/safe-subclass-label") + public RedirectView customRedirectView(HttpServletRequest request) { + String label = request.getParameter("label"); + return new LabeledRedirectView(label, "/account"); + } + + @GetMapping("/safe-set-url-overload") + public RedirectView customSetUrl(HttpServletRequest request) { + LabeledRedirectView view = new LabeledRedirectView("label", "/account"); + Object label = request.getParameter("label"); + view.setUrl(label); + return view; + } +} + +@Controller +class ResponseBodyController { + @ResponseBody + @GetMapping("/body") + public String responseBody(HttpServletRequest request) { + return "redirect:" + request.getParameter("value"); + } +} + +@RestController +class JsonController { + @GetMapping("/json") + public String responseBody(HttpServletRequest request) { + return "redirect:" + request.getParameter("value"); + } +} + +class SharedRedirectHelper { + protected String redirectShared(String path) { + return "redirect:" + path; + } +} + +@Controller +class MvcViewUser extends SharedRedirectHelper { + @GetMapping("/constant-view") + public String view() { + return redirectShared("/account"); + } +} + +@Controller +class ResponseBodyUser extends SharedRedirectHelper { + @ResponseBody + @GetMapping("/body-user") + public String body(HttpServletRequest request) { + return redirectShared(request.getParameter("value")); + } +} diff --git a/java/ql/test/query-tests/security/CWE-601/semmle/tests/UrlRedirect.expected b/java/ql/test/query-tests/security/CWE-601/semmle/tests/UrlRedirect.expected index fdd602c616bf..a1d45bdab204 100644 --- a/java/ql/test/query-tests/security/CWE-601/semmle/tests/UrlRedirect.expected +++ b/java/ql/test/query-tests/security/CWE-601/semmle/tests/UrlRedirect.expected @@ -1,4 +1,10 @@ #select +| SpringUrlRedirect.java:39:30:39:33 | next | SpringUrlRedirect.java:38:23:38:50 | getHeader(...) : String | SpringUrlRedirect.java:39:30:39:33 | next | Untrusted URL redirection depends on a $@. | SpringUrlRedirect.java:38:23:38:50 | getHeader(...) | user-provided value | +| SpringUrlRedirect.java:45:47:45:50 | next | SpringUrlRedirect.java:44:23:44:50 | getHeader(...) : String | SpringUrlRedirect.java:45:47:45:50 | next | Untrusted URL redirection depends on a $@. | SpringUrlRedirect.java:44:23:44:50 | getHeader(...) | user-provided value | +| SpringUrlRedirect.java:51:33:51:36 | next | SpringUrlRedirect.java:50:23:50:50 | getHeader(...) : String | SpringUrlRedirect.java:51:33:51:36 | next | Untrusted URL redirection depends on a $@. | SpringUrlRedirect.java:50:23:50:50 | getHeader(...) | user-provided value | +| SpringUrlRedirect.java:57:16:57:29 | redirect(...) | SpringUrlRedirect.java:56:23:56:50 | getHeader(...) : String | SpringUrlRedirect.java:57:16:57:29 | redirect(...) | Untrusted URL redirection depends on a $@. | SpringUrlRedirect.java:56:23:56:50 | getHeader(...) | user-provided value | +| SpringUrlRedirect.java:64:21:64:24 | next | SpringUrlRedirect.java:62:23:62:50 | getParameter(...) : String | SpringUrlRedirect.java:64:21:64:24 | next | Untrusted URL redirection depends on a $@. | SpringUrlRedirect.java:62:23:62:50 | getParameter(...) | user-provided value | +| SpringUrlRedirect.java:71:33:71:36 | next | SpringUrlRedirect.java:70:23:70:50 | getParameter(...) : String | SpringUrlRedirect.java:71:33:71:36 | next | Untrusted URL redirection depends on a $@. | SpringUrlRedirect.java:70:23:70:50 | getParameter(...) | user-provided value | | 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 | | 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 | | 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 | @@ -6,17 +12,42 @@ | 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 | | 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 | edges -| UrlRedirect.java:32:37:32:66 | getParameter(...) : String | UrlRedirect.java:32:25:32:67 | weakCleanup(...) | provenance | Src:MaD:2 MaD:3 | -| UrlRedirect.java:32:37:32:66 | getParameter(...) : String | UrlRedirect.java:45:28:45:39 | input : String | provenance | Src:MaD:2 | +| SpringUrlRedirect.java:12:31:12:41 | path : String | SpringUrlRedirect.java:13:16:13:33 | ... + ... : String | provenance | | +| SpringUrlRedirect.java:38:23:38:50 | getHeader(...) : String | SpringUrlRedirect.java:39:30:39:33 | next | provenance | Src:MaD:2 | +| SpringUrlRedirect.java:44:23:44:50 | getHeader(...) : String | SpringUrlRedirect.java:45:47:45:50 | next | provenance | Src:MaD:2 | +| SpringUrlRedirect.java:50:23:50:50 | getHeader(...) : String | SpringUrlRedirect.java:51:33:51:36 | next | provenance | Src:MaD:2 | +| SpringUrlRedirect.java:56:23:56:50 | getHeader(...) : String | SpringUrlRedirect.java:57:25:57:28 | next : String | provenance | Src:MaD:2 | +| SpringUrlRedirect.java:57:25:57:28 | next : String | SpringUrlRedirect.java:12:31:12:41 | path : String | provenance | | +| SpringUrlRedirect.java:57:25:57:28 | next : String | SpringUrlRedirect.java:57:16:57:29 | redirect(...) | provenance | | +| SpringUrlRedirect.java:62:23:62:50 | getParameter(...) : String | SpringUrlRedirect.java:64:21:64:24 | next | provenance | Src:MaD:3 | +| SpringUrlRedirect.java:70:23:70:50 | getParameter(...) : String | SpringUrlRedirect.java:71:33:71:36 | next | provenance | Src:MaD:3 | +| UrlRedirect.java:32:37:32:66 | getParameter(...) : String | UrlRedirect.java:32:25:32:67 | weakCleanup(...) | provenance | Src:MaD:3 MaD:4 | +| UrlRedirect.java:32:37:32:66 | getParameter(...) : String | UrlRedirect.java:45:28:45:39 | input : String | provenance | Src:MaD:3 | | UrlRedirect.java:45:28:45:39 | input : String | UrlRedirect.java:46:10:46:14 | input : String | provenance | | -| UrlRedirect.java:46:10:46:14 | input : String | UrlRedirect.java:46:10:46:40 | replaceAll(...) : String | provenance | MaD:3 | -| mad/Test.java:9:16:9:41 | getParameter(...) : String | mad/Test.java:14:31:14:38 | source(...) : String | provenance | Src:MaD:2 | +| UrlRedirect.java:46:10:46:14 | input : String | UrlRedirect.java:46:10:46:40 | replaceAll(...) : String | provenance | MaD:4 | +| mad/Test.java:9:16:9:41 | getParameter(...) : String | mad/Test.java:14:31:14:38 | source(...) : String | provenance | Src:MaD:3 | | mad/Test.java:14:31:14:38 | source(...) : String | mad/Test.java:14:22:14:38 | (...)... | provenance | Sink:MaD:1 | models | 1 | Sink: org.kohsuke.stapler; HttpResponses; true; redirectTo; (String); ; Argument[0]; url-redirection; ai-manual | -| 2 | Source: javax.servlet; ServletRequest; false; getParameter; (String); ; ReturnValue; remote; manual | -| 3 | Summary: java.lang; String; false; replaceAll; ; ; Argument[this]; ReturnValue; taint; manual | +| 2 | Source: javax.servlet.http; HttpServletRequest; false; getHeader; (String); ; ReturnValue; remote; manual | +| 3 | Source: javax.servlet; ServletRequest; false; getParameter; (String); ; ReturnValue; remote; manual | +| 4 | Summary: java.lang; String; false; replaceAll; ; ; Argument[this]; ReturnValue; taint; manual | nodes +| SpringUrlRedirect.java:12:31:12:41 | path : String | semmle.label | path : String | +| SpringUrlRedirect.java:13:16:13:33 | ... + ... : String | semmle.label | ... + ... : String | +| SpringUrlRedirect.java:38:23:38:50 | getHeader(...) : String | semmle.label | getHeader(...) : String | +| SpringUrlRedirect.java:39:30:39:33 | next | semmle.label | next | +| SpringUrlRedirect.java:44:23:44:50 | getHeader(...) : String | semmle.label | getHeader(...) : String | +| SpringUrlRedirect.java:45:47:45:50 | next | semmle.label | next | +| SpringUrlRedirect.java:50:23:50:50 | getHeader(...) : String | semmle.label | getHeader(...) : String | +| SpringUrlRedirect.java:51:33:51:36 | next | semmle.label | next | +| SpringUrlRedirect.java:56:23:56:50 | getHeader(...) : String | semmle.label | getHeader(...) : String | +| SpringUrlRedirect.java:57:16:57:29 | redirect(...) | semmle.label | redirect(...) | +| SpringUrlRedirect.java:57:25:57:28 | next : String | semmle.label | next : String | +| SpringUrlRedirect.java:62:23:62:50 | getParameter(...) : String | semmle.label | getParameter(...) : String | +| SpringUrlRedirect.java:64:21:64:24 | next | semmle.label | next | +| SpringUrlRedirect.java:70:23:70:50 | getParameter(...) : String | semmle.label | getParameter(...) : String | +| SpringUrlRedirect.java:71:33:71:36 | next | semmle.label | next | | UrlRedirect2.java:27:25:27:54 | getParameter(...) | semmle.label | getParameter(...) | | UrlRedirect.java:23:25:23:54 | getParameter(...) | semmle.label | getParameter(...) | | UrlRedirect.java:32:25:32:67 | weakCleanup(...) | semmle.label | weakCleanup(...) | @@ -30,4 +61,5 @@ nodes | mad/Test.java:14:22:14:38 | (...)... | semmle.label | (...)... | | mad/Test.java:14:31:14:38 | source(...) : String | semmle.label | source(...) : String | subpaths +| SpringUrlRedirect.java:57:25:57:28 | next : String | SpringUrlRedirect.java:12:31:12:41 | path : String | SpringUrlRedirect.java:13:16:13:33 | ... + ... : String | SpringUrlRedirect.java:57:16:57:29 | redirect(...) | | UrlRedirect.java:32:37:32:66 | getParameter(...) : String | UrlRedirect.java:45:28:45:39 | input : String | UrlRedirect.java:46:10:46:40 | replaceAll(...) : String | UrlRedirect.java:32:25:32:67 | weakCleanup(...) | diff --git a/java/ql/test/query-tests/security/CWE-601/semmle/tests/options b/java/ql/test/query-tests/security/CWE-601/semmle/tests/options index 637c329a9143..804ee350df4e 100644 --- a/java/ql/test/query-tests/security/CWE-601/semmle/tests/options +++ b/java/ql/test/query-tests/security/CWE-601/semmle/tests/options @@ -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 +//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