Merge pull request github#6247 from p0wn4j/spring-responseentity-redirect-sink

[Java] CWE-601: Add Spring URL Redirect ResponseEntity sink

Author: smowton

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

@@ -34,6 +34,29 @@ public ModelAndView bad4(String redirectUrl) {
     }
 
     @GetMapping("url5")
+    public ResponseEntity<Void> bad5(String redirectUrl) {
+        return ResponseEntity.status(HttpStatus.FOUND)
+                .location(URI.create(redirectUrl))
+                .build();
+    }
+
+    @GetMapping("url6")
+    public ResponseEntity<Void> bad6(String redirectUrl) {
+        HttpHeaders httpHeaders = new HttpHeaders();
+        httpHeaders.setLocation(URI.create(redirectUrl));
+
+        return new ResponseEntity<>(httpHeaders, HttpStatus.SEE_OTHER);
+    }
+
+    @GetMapping("url7")
+    public ResponseEntity<Void> bad7(String redirectUrl) {
+        HttpHeaders httpHeaders = new HttpHeaders();
+        httpHeaders.add("Location", redirectUrl);
+
+        return ResponseEntity.status(HttpStatus.SEE_OTHER).headers(httpHeaders).build();
+    }
+
+    @GetMapping("url8")
     public RedirectView good1(String redirectUrl) {
         RedirectView rv = new RedirectView();
         if (redirectUrl.startsWith(VALID_REDIRECT)){

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

@@ -21,10 +21,10 @@ redirects on the server; then choose from that list based on the user input prov
 <example>
 
 <p>The following examples show the bad case and the good case respectively.
-In <code>bad1</code> method and <code>bad2</code> method and <code>bad3</code> method and 
-<code>bad4</code> method, shows an HTTP request parameter being used directly in a URL redirect 
-without validating the input, which facilitates phishing attacks. In <code>good1</code> method, 
-shows how to solve this problem by verifying whether the user input is a known fixed string beginning.
+The <code>bad</code> methods show an HTTP request parameter being used directly 
+in a URL redirect without validating the input, which facilitates phishing attacks. 
+In the <code>good1</code> method, it is shown how to solve this problem by verifying whether 
+the user input is a known fixed string beginning.
 </p>
 
 <sample src="SpringUrlRedirect.java" />
@@ -33,5 +33,6 @@ shows how to solve this problem by verifying whether the user input is a known f
 <references>
 <li>A Guide To Spring Redirects: <a href="https://www.baeldung.com/spring-redirect-and-forward">Spring Redirects</a>.</li>
 <li>Url redirection - attack and defense: <a href="https://www.virtuesecurity.com/kb/url-redirection-attack-and-defense/">Url Redirection</a>.</li>
+<li>How to redirect to an external URL from Spring Boot REST Controller (Post/Redirect/Get pattern)?: <a href="https://fullstackdeveloper.guru/2021/03/12/how-to-redirect-to-an-external-url-from-spring-boot-rest-controller/">ResponseEntity Redirection</a>.</li>
 </references>
 </qhelp>

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

@@ -34,6 +34,10 @@ class SpringUrlRedirectFlowConfig extends TaintTracking::Configuration {
 
   override predicate isSink(DataFlow::Node sink) { sink instanceof SpringUrlRedirectSink }
 
+  override predicate isAdditionalTaintStep(DataFlow::Node fromNode, DataFlow::Node toNode) {
+    springUrlRedirectTaintStep(fromNode, toNode)
+  }
+
   override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
     guard instanceof StartsWithSanitizer
   }
@@ -57,6 +61,8 @@ class SpringUrlRedirectFlowConfig extends TaintTracking::Configuration {
         not ma.getArgument(0).(CompileTimeConstantExpr).getStringValue().regexpMatch("^%s.*")
       )
     )
+    or
+    nonLocationHeaderSanitizer(node)
   }
 }
 

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

@@ -35,8 +35,13 @@ class RedirectAppendCall extends MethodAccess {
 }
 
 /** A URL redirection sink from spring controller method. */
-class SpringUrlRedirectSink extends DataFlow::Node {
-  SpringUrlRedirectSink() {
+abstract class SpringUrlRedirectSink extends DataFlow::Node { }
+
+/**
+ * A sink for URL Redirection via the Spring View classes.
+ */
+private class SpringViewUrlRedirectSink extends SpringUrlRedirectSink {
+  SpringViewUrlRedirectSink() {
     exists(RedirectBuilderExpr rbe |
       rbe.getRightOperand() = this.asExpr() and
       any(SpringRequestMappingMethod sqmm).polyCalls*(this.getEnclosingCallable())
@@ -71,3 +76,64 @@ class SpringUrlRedirectSink extends DataFlow::Node {
     )
   }
 }
+
+/**
+ * A sink for URL Redirection via the `ResponseEntity` class.
+ */
+private class SpringResponseEntityUrlRedirectSink extends SpringUrlRedirectSink {
+  SpringResponseEntityUrlRedirectSink() {
+    // Find `new ResponseEntity(httpHeaders, ...)` or
+    // `new ResponseEntity(..., httpHeaders, ...)` sinks
+    exists(ClassInstanceExpr cie, Argument argument |
+      cie.getConstructedType() instanceof SpringResponseEntity and
+      argument.getType() instanceof SpringHttpHeaders and
+      argument = cie.getArgument([0, 1]) and
+      this.asExpr() = argument
+    )
+    or
+    // Find `ResponseEntity.status(...).headers(taintHeaders).build()` or
+    // `ResponseEntity.status(...).location(URI.create(taintURL)).build()` sinks
+    exists(MethodAccess ma |
+      ma.getMethod()
+          .getDeclaringType()
+          .hasQualifiedName("org.springframework.http", "ResponseEntity$HeadersBuilder<BodyBuilder>") and
+      ma.getMethod().getName() in ["headers", "location"] and
+      this.asExpr() = ma.getArgument(0)
+    )
+  }
+}
+
+private class HttpHeadersMethodAccess extends MethodAccess {
+  HttpHeadersMethodAccess() { this.getMethod().getDeclaringType() instanceof SpringHttpHeaders }
+}
+
+private class HttpHeadersAddSetMethodAccess extends HttpHeadersMethodAccess {
+  HttpHeadersAddSetMethodAccess() { this.getMethod().getName() in ["add", "set"] }
+}
+
+private class HttpHeadersSetLocationMethodAccess extends HttpHeadersMethodAccess {
+  HttpHeadersSetLocationMethodAccess() { this.getMethod().hasName("setLocation") }
+}
+
+/**
+ * Holds if `fromNode` to `toNode` is a dataflow step from a tainted argument to
+ * a `HttpHeaders` instance qualifier, i.e. `httpHeaders.setLocation(tainted)`.
+ */
+predicate springUrlRedirectTaintStep(DataFlow::Node fromNode, DataFlow::Node toNode) {
+  exists(HttpHeadersSetLocationMethodAccess ma |
+    fromNode.asExpr() = ma.getArgument(0) and
+    toNode.asExpr() = ma.getQualifier()
+  )
+}
+
+/**
+ * A sanitizer to exclude the cases where the `HttpHeaders.add` or `HttpHeaders.set`
+ * methods are called with a HTTP header other than "Location".
+ * E.g: `httpHeaders.add("X-Some-Header", taintedUrlString)`
+ */
+predicate nonLocationHeaderSanitizer(DataFlow::Node node) {
+  exists(HttpHeadersAddSetMethodAccess ma, Argument firstArg | node.asExpr() = ma.getArgument(1) |
+    firstArg = ma.getArgument(0) and
+    not firstArg.(CompileTimeConstantExpr).getStringValue().matches("Location")
+  )
+}