diff --git a/java/ql/src/experimental/Security/CWE/CWE-601/SpringUrlRedirect.ql b/java/ql/src/experimental/Security/CWE/CWE-601/SpringUrlRedirect.ql index 138bce57ac9..5c8a8ea78d8 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-601/SpringUrlRedirect.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-601/SpringUrlRedirect.ql @@ -33,6 +33,16 @@ class SpringUrlRedirectFlowConfig extends TaintTracking::Configuration { ae.getRightOperand() = node.asExpr() and not ae instanceof RedirectBuilderExpr ) + or + exists(MethodAccess ma, int index | + ma.getMethod().hasName("format") and + ma.getMethod().getDeclaringType() instanceof TypeString and + ma.getArgument(index) = node.asExpr() and + ( + index != 0 and + not ma.getArgument(0).(CompileTimeConstantExpr).getStringValue().regexpMatch("^%s.*") + ) + ) } } diff --git a/java/ql/src/experimental/Security/CWE/CWE-601/SpringUrlRedirect.qll b/java/ql/src/experimental/Security/CWE/CWE-601/SpringUrlRedirect.qll index 866eaae1c34..84bb5d9adf8 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-601/SpringUrlRedirect.qll +++ b/java/ql/src/experimental/Security/CWE/CWE-601/SpringUrlRedirect.qll @@ -51,14 +51,14 @@ class SpringUrlRedirectSink extends DataFlow::Node { SpringUrlRedirectSink() { exists(RedirectBuilderExpr rbe | rbe.getRightOperand() = this.asExpr() and - exists(RedirectBuilderFlowConfig rbfc | rbfc.hasFlow(exprNode(rbe), _)) + any(SpringRequestMappingMethod sqmm).polyCalls*(this.getEnclosingCallable()) ) or exists(MethodAccess ma, RedirectAppendCall rac | DataFlow2::localExprFlow(rac.getQualifier(), ma.getQualifier()) and ma.getMethod().hasName("append") and ma.getArgument(0) = this.asExpr() and - exists(RedirectBuilderFlowConfig rbfc | rbfc.hasFlow(exprNode(ma.getQualifier()), _)) + any(SpringRequestMappingMethod sqmm).polyCalls*(this.getEnclosingCallable()) ) or exists(MethodAccess ma | @@ -66,8 +66,7 @@ class SpringUrlRedirectSink extends DataFlow::Node { ma.getMethod() .getDeclaringType() .hasQualifiedName("org.springframework.web.servlet.view", "AbstractUrlBasedView") and - ma.getArgument(0) = this.asExpr() and - exists(RedirectViewFlowConfig rvfc | rvfc.hasFlowToExpr(ma.getQualifier())) + ma.getArgument(0) = this.asExpr() ) or exists(ClassInstanceExpr cie | @@ -84,57 +83,3 @@ class SpringUrlRedirectSink extends DataFlow::Node { ) } } - -/** A data flow configuration tracing flow from redirect builder expression to spring controller method return expression. */ -private class RedirectBuilderFlowConfig extends DataFlow2::Configuration { - RedirectBuilderFlowConfig() { this = "RedirectBuilderFlowConfig" } - - override predicate isSource(DataFlow::Node src) { - exists(RedirectBuilderExpr rbe | rbe = src.asExpr()) - or - exists(MethodAccess ma, RedirectAppendCall rac | - DataFlow2::localExprFlow(rac.getQualifier(), ma.getQualifier()) and - ma.getMethod().hasName("append") and - ma.getQualifier() = src.asExpr() - ) - } - - override predicate isSink(DataFlow::Node sink) { - exists(ReturnStmt rs, SpringRequestMappingMethod sqmm | - rs.getResult() = sink.asExpr() and - sqmm.getBody().getAStmt() = rs - ) - } - - override predicate isAdditionalFlowStep(Node prod, Node succ) { - exists(MethodAccess ma | - ma.getMethod().hasName("toString") and - ma.getMethod().getDeclaringType() instanceof StringBuildingType and - ma.getQualifier() = prod.asExpr() and - ma = succ.asExpr() - ) - } -} - -/** A data flow configuration tracing flow from RedirectView object to calling setUrl method. */ -private class RedirectViewFlowConfig extends DataFlow2::Configuration { - RedirectViewFlowConfig() { this = "RedirectViewFlowConfig" } - - override predicate isSource(DataFlow::Node src) { - exists(ClassInstanceExpr cie | - cie.getConstructedType() - .hasQualifiedName("org.springframework.web.servlet.view", "RedirectView") and - cie = src.asExpr() - ) - } - - override predicate isSink(DataFlow::Node sink) { - exists(MethodAccess ma | - ma.getMethod().hasName("setUrl") and - ma.getMethod() - .getDeclaringType() - .hasQualifiedName("org.springframework.web.servlet.view", "AbstractUrlBasedView") and - ma.getQualifier() = sink.asExpr() - ) - } -} diff --git a/java/ql/test/experimental/query-tests/security/CWE-601/SpringUrlRedirect.expected b/java/ql/test/experimental/query-tests/security/CWE-601/SpringUrlRedirect.expected index 26b8acd7770..bcf5e892e1b 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-601/SpringUrlRedirect.expected +++ b/java/ql/test/experimental/query-tests/security/CWE-601/SpringUrlRedirect.expected @@ -5,6 +5,8 @@ edges | SpringUrlRedirect.java:32:30:32:47 | redirectUrl : String | SpringUrlRedirect.java:33:47:33:57 | redirectUrl | | SpringUrlRedirect.java:37:24:37:41 | redirectUrl : String | SpringUrlRedirect.java:40:29:40:39 | redirectUrl | | SpringUrlRedirect.java:45:24:45:41 | redirectUrl : String | SpringUrlRedirect.java:48:30:48:40 | redirectUrl | +| SpringUrlRedirect.java:53:24:53:41 | redirectUrl : String | SpringUrlRedirect.java:54:30:54:66 | format(...) | +| SpringUrlRedirect.java:58:24:58:41 | redirectUrl : String | SpringUrlRedirect.java:59:30:59:76 | format(...) | nodes | SpringUrlRedirect.java:13:30:13:47 | redirectUrl : String | semmle.label | redirectUrl : String | | SpringUrlRedirect.java:15:19:15:29 | redirectUrl | semmle.label | redirectUrl | @@ -18,6 +20,10 @@ nodes | SpringUrlRedirect.java:40:29:40:39 | redirectUrl | semmle.label | redirectUrl | | SpringUrlRedirect.java:45:24:45:41 | redirectUrl : String | semmle.label | redirectUrl : String | | SpringUrlRedirect.java:48:30:48:40 | redirectUrl | semmle.label | redirectUrl | +| SpringUrlRedirect.java:53:24:53:41 | redirectUrl : String | semmle.label | redirectUrl : String | +| SpringUrlRedirect.java:54:30:54:66 | format(...) | semmle.label | format(...) | +| SpringUrlRedirect.java:58:24:58:41 | redirectUrl : String | semmle.label | redirectUrl : String | +| SpringUrlRedirect.java:59:30:59:76 | format(...) | semmle.label | format(...) | #select | 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 | | 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 | @@ -25,3 +31,5 @@ nodes | 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 | | 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 | | 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 | +| 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 | +| 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 | diff --git a/java/ql/test/experimental/query-tests/security/CWE-601/SpringUrlRedirect.java b/java/ql/test/experimental/query-tests/security/CWE-601/SpringUrlRedirect.java index 5124d8cd8c6..f3958cba102 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-601/SpringUrlRedirect.java +++ b/java/ql/test/experimental/query-tests/security/CWE-601/SpringUrlRedirect.java @@ -50,6 +50,16 @@ public class SpringUrlRedirect { } @GetMapping("url7") + public String bad7(String redirectUrl) { + return "redirect:" + String.format("%s/?aaa", redirectUrl); + } + + @GetMapping("url8") + public String bad8(String redirectUrl, String token) { + return "redirect:" + String.format(redirectUrl + "?token=%s", token); + } + + @GetMapping("url9") public RedirectView good1(String redirectUrl) { RedirectView rv = new RedirectView(); if (redirectUrl.startsWith(VALID_REDIRECT)){ @@ -60,9 +70,14 @@ public class SpringUrlRedirect { return rv; } - @GetMapping("url8") + @GetMapping("url10") public ModelAndView good2(String token) { String url = "/edit?token=" + token; return new ModelAndView("redirect:" + url); } + + @GetMapping("url11") + public String good3(String status) { + return "redirect:" + String.format("/stories/search/criteria?status=%s", status); + } }