Add filtering of String.format

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.*")
+      )
+    )
   }
 }
 

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()
-    )
-  }
-}

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 |

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);
+    }
 }