Add check for Spring redirect

2025-12-24 04:36:35 +01:00 · 2022-07-29 01:59:47 +00:00
parent 1ce31ec32c
commit b69eba9238
5 changed files with 261 additions and 3 deletions
--- a/java/ql/src/experimental/Security/CWE/CWE-625/PermissiveDotRegex.ql
+++ b/java/ql/src/experimental/Security/CWE/CWE-625/PermissiveDotRegex.ql
@@ -18,6 +18,7 @@ import semmle.code.java.dataflow.FlowSources
 import semmle.code.java.security.UrlRedirect
 import DataFlow::PathGraph
 import Regex
+import SpringUrlRedirect

 /** Source model of remote flow source with servlets. */
 private class GetServletUriSource extends SourceModelCsv {
@@ -51,6 +52,30 @@ private class UrlFilterSink extends SinkModelCsv {
  }
 }

+/** A Spring framework annotation indicating remote uri user input. */
+class SpringUriInputAnnotation extends Annotation {
+  SpringUriInputAnnotation() {
+    exists(AnnotationType a |
+      a = this.getType() and
+      a.getPackage().getName() = "org.springframework.web.bind.annotation"
+    |
+      (
+        a.hasName("PathVariable") or
+        a.hasName("RequestParam")
+      )
+    )
+  }
+}
+
+class SpringUriInputParameterSource extends DataFlow::Node {
+  SpringUriInputParameterSource() {
+    this.asParameter() =
+      any(SpringRequestMappingParameter srmp |
+        srmp.getAnAnnotation() instanceof SpringUriInputAnnotation
+      )
+  }
+}
+
 /**
 * `.` without a `\` prefix, which is likely not a character literal in regex
 */
@@ -121,7 +146,10 @@ class PermissiveDotRegexConfig extends DataFlow::Configuration {
 class MatchRegexConfiguration extends TaintTracking::Configuration {
  MatchRegexConfiguration() { this = "PermissiveDotRegex::MatchRegexConfiguration" }

-  override predicate isSource(DataFlow::Node source) { sourceNode(source, "uri-path") }
+  override predicate isSource(DataFlow::Node source) {
+    sourceNode(source, "uri-path") or // Servlet uri source
+    source instanceof SpringUriInputParameterSource // Spring uri source
+  }

  override predicate isSink(DataFlow::Node sink) {
    sink instanceof MatchRegexSink and
@@ -140,7 +168,8 @@ class MatchRegexConfiguration extends TaintTracking::Configuration {
      (
        DataFlow::exprNode(se) instanceof UrlRedirectSink or
        sinkNode(DataFlow::exprNode(se), "url-dispatch") or
-        sinkNode(DataFlow::exprNode(se), "url-filter")
+        sinkNode(DataFlow::exprNode(se), "url-filter") or
+        DataFlow::exprNode(se) instanceof SpringUrlRedirectSink
      ) and
      guard.controls(se.getBasicBlock(), true)
    )
--- a/java/ql/src/experimental/Security/CWE/CWE-625/SpringUrlRedirect.qll
+++ b/java/ql/src/experimental/Security/CWE/CWE-625/SpringUrlRedirect.qll
@@ -0,0 +1,109 @@
+/** Provides methods related to Spring URL redirect from /src/experimental/Security/CWE/CWE-601/SpringUrlRedirect.qll. */
+
+private import java
+private import semmle.code.java.dataflow.FlowSources
+
+/**
+ * A concatenate expression using the string `redirect:` or `ajaxredirect:` or `forward:` on the left.
+ *
+ * E.g: `"redirect:" + redirectUrl`
+ */
+class RedirectBuilderExpr extends AddExpr {
+  RedirectBuilderExpr() {
+    this.getLeftOperand().(CompileTimeConstantExpr).getStringValue() in [
+        "redirect:", "ajaxredirect:", "forward:"
+      ]
+  }
+}
+
+/**
+ * A call to `StringBuilder.append` or `StringBuffer.append` method, and the parameter value is
+ * `"redirect:"` or `"ajaxredirect:"` or `"forward:"`.
+ *
+ * E.g: `StringBuilder.append("redirect:")`
+ */
+class RedirectAppendCall extends MethodAccess {
+  RedirectAppendCall() {
+    this.getMethod().hasName("append") and
+    this.getMethod().getDeclaringType() instanceof StringBuildingType and
+    this.getArgument(0).(CompileTimeConstantExpr).getStringValue() in [
+        "redirect:", "ajaxredirect:", "forward:"
+      ]
+  }
+}
+
+/** A URL redirection sink from spring controller method. */
+abstract class SpringUrlRedirectSink extends DataFlow::Node { }
+
+/**
+ * A sink for URL Redirection via the Spring View classes.
+ */
+private class SpringViewUrlRedirectSink extends SpringUrlRedirectSink {
+  SpringViewUrlRedirectSink() {
+    // Hardcoded redirect such as "redirect:login"
+    this.asExpr()
+        .(CompileTimeConstantExpr)
+        .getStringValue()
+        .indexOf(["redirect:", "ajaxredirect:", "forward:"]) = 0 and
+    any(SpringRequestMappingMethod sqmm).polyCalls*(this.getEnclosingCallable())
+    or
+    exists(RedirectBuilderExpr rbe |
+      rbe.getRightOperand() = this.asExpr() and
+      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
+      any(SpringRequestMappingMethod sqmm).polyCalls*(this.getEnclosingCallable())
+    )
+    or
+    exists(MethodAccess ma |
+      ma.getMethod().hasName("setUrl") and
+      ma.getMethod()
+          .getDeclaringType()
+          .hasQualifiedName("org.springframework.web.servlet.view", "AbstractUrlBasedView") and
+      ma.getArgument(0) = this.asExpr()
+    )
+    or
+    exists(ClassInstanceExpr cie |
+      cie.getConstructedType()
+          .hasQualifiedName("org.springframework.web.servlet.view", "RedirectView") and
+      cie.getArgument(0) = this.asExpr()
+    )
+    or
+    exists(ClassInstanceExpr cie |
+      cie.getConstructedType().hasQualifiedName("org.springframework.web.servlet", "ModelAndView") and
+      exists(RedirectBuilderExpr rbe |
+        rbe = cie.getArgument(0) and rbe.getRightOperand() = this.asExpr()
+      )
+    )
+  }
+}
+
+/**
+ * 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)
+    )
+  }
+}
--- a/java/ql/test/experimental/query-tests/security/CWE-625/DotRegexSpring.java
+++ b/java/ql/test/experimental/query-tests/security/CWE-625/DotRegexSpring.java
@@ -0,0 +1,75 @@
+import org.springframework.stereotype.Controller;
+import org.springframework.ui.Model;
+import org.springframework.web.bind.annotation.GetMapping;
+import org.springframework.web.bind.annotation.PathVariable;
+import org.springframework.web.bind.annotation.RequestParam;
+import org.springframework.web.servlet.view.RedirectView;
+
+import java.io.UnsupportedEncodingException;
+import java.net.URLDecoder;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
+@Controller
+public class DotRegexSpring {
+	private static final String PROTECTED_PATTERN = "/protected/.*";
+	private static final String CONSTRAINT_PATTERN = "/protected/xyz\\.xml";
+	
+	@GetMapping("param")
+	// BAD: A string with line return e.g. `/protected/%0dxyz` can bypass the path check
+	public String withParam(@RequestParam String path, Model model) throws UnsupportedEncodingException {
+		Pattern p = Pattern.compile(PROTECTED_PATTERN);
+		path = decodePath(path);
+		Matcher m = p.matcher(path);
+
+		if (m.matches()) {
+			// Protected page - check access token and redirect to login page
+			if (model.getAttribute("secAttr") == null || !model.getAttribute("secAttr").equals("secValue")) {
+				return "redirect:login";
+			}
+		}
+		// Not protected page - render content
+		return path;
+	}
+
+	@GetMapping("{path}")
+	// BAD: A string with line return e.g. `%252Fprotected%252F%250dxyz` can bypass the path check
+	public RedirectView withPathVariable1(@PathVariable String path, Model model) throws UnsupportedEncodingException {
+		Pattern p = Pattern.compile(PROTECTED_PATTERN);
+		path = decodePath(path);
+		Matcher m = p.matcher(path);
+
+		if (m.matches()) {
+			// Protected page - check access token and redirect to login page
+			if (model.getAttribute("secAttr") == null || !model.getAttribute("secAttr").equals("secValue")) {
+				RedirectView redirectView = new RedirectView("login", true);
+				return redirectView;
+			}
+		}
+		return null;
+	}
+	
+	@GetMapping("/sp/{path}")
+	// GOOD: A string with line return e.g. `%252Fprotected%252F%250dxyz` cannot bypass the path check
+	public String withPathVariable2(@PathVariable String path, Model model) throws UnsupportedEncodingException {
+		Pattern p = Pattern.compile(CONSTRAINT_PATTERN);
+		path = decodePath(path);
+		Matcher m = p.matcher(path);
+
+		if (m.matches()) {
+			// Protected page - check access token and redirect to login page
+			if (model.getAttribute("secAttr") == null || !model.getAttribute("secAttr").equals("secValue")) {
+				return "redirect:login";
+			}	
+		}
+		// Not protected page - render content
+		return path;
+	}
+
+	private String decodePath(String path) throws UnsupportedEncodingException {
+		while (path.indexOf("%") > -1) {
+			path = URLDecoder.decode(path, "UTF-8");
+		}
+		return path;
+	}
+}
--- a/java/ql/test/experimental/query-tests/security/CWE-625/PermissiveDotRegex.expected
+++ b/java/ql/test/experimental/query-tests/security/CWE-625/PermissiveDotRegex.expected
@@ -15,6 +15,26 @@ edges
 | DotRegexServlet.java:93:19:93:39 | getPathInfo(...) : String | DotRegexServlet.java:96:25:96:30 | source |
 | DotRegexServlet.java:112:19:112:39 | getPathInfo(...) : String | DotRegexServlet.java:115:25:115:30 | source |
 | DotRegexServlet.java:133:19:133:39 | getPathInfo(...) : String | DotRegexServlet.java:136:25:136:30 | source |
+| DotRegexSpring.java:15:30:15:46 | PROTECTED_PATTERN : String | DotRegexSpring.java:21:31:21:47 | PROTECTED_PATTERN |
+| DotRegexSpring.java:15:30:15:46 | PROTECTED_PATTERN : String | DotRegexSpring.java:38:31:38:47 | PROTECTED_PATTERN |
+| DotRegexSpring.java:15:50:15:64 | "/protected/.*" : String | DotRegexSpring.java:15:30:15:46 | PROTECTED_PATTERN : String |
+| DotRegexSpring.java:20:26:20:50 | path : String | DotRegexSpring.java:22:21:22:24 | path : String |
+| DotRegexSpring.java:22:10:22:25 | decodePath(...) : String | DotRegexSpring.java:23:25:23:28 | path |
+| DotRegexSpring.java:22:21:22:24 | path : String | DotRegexSpring.java:22:10:22:25 | decodePath(...) : String |
+| DotRegexSpring.java:22:21:22:24 | path : String | DotRegexSpring.java:69:28:69:38 | path : String |
+| DotRegexSpring.java:37:40:37:64 | path : String | DotRegexSpring.java:39:21:39:24 | path : String |
+| DotRegexSpring.java:39:10:39:25 | decodePath(...) : String | DotRegexSpring.java:40:25:40:28 | path |
+| DotRegexSpring.java:39:21:39:24 | path : String | DotRegexSpring.java:39:10:39:25 | decodePath(...) : String |
+| DotRegexSpring.java:39:21:39:24 | path : String | DotRegexSpring.java:69:28:69:38 | path : String |
+| DotRegexSpring.java:54:34:54:58 | path : String | DotRegexSpring.java:56:21:56:24 | path : String |
+| DotRegexSpring.java:56:10:56:25 | decodePath(...) : String | DotRegexSpring.java:57:25:57:28 | path |
+| DotRegexSpring.java:56:21:56:24 | path : String | DotRegexSpring.java:56:10:56:25 | decodePath(...) : String |
+| DotRegexSpring.java:56:21:56:24 | path : String | DotRegexSpring.java:69:28:69:38 | path : String |
+| DotRegexSpring.java:69:28:69:38 | path : String | DotRegexSpring.java:71:29:71:32 | path : String |
+| DotRegexSpring.java:69:28:69:38 | path : String | DotRegexSpring.java:73:10:73:13 | path : String |
+| DotRegexSpring.java:71:11:71:42 | decode(...) : String | DotRegexSpring.java:71:29:71:32 | path : String |
+| DotRegexSpring.java:71:11:71:42 | decode(...) : String | DotRegexSpring.java:73:10:73:13 | path : String |
+| DotRegexSpring.java:71:29:71:32 | path : String | DotRegexSpring.java:71:11:71:42 | decode(...) : String |
 nodes
 | DotRegexFilter.java:16:30:16:46 | PROTECTED_PATTERN : String | semmle.label | PROTECTED_PATTERN : String |
 | DotRegexFilter.java:16:50:16:64 | "/protected/.*" : String | semmle.label | "/protected/.*" : String |
@@ -43,10 +63,35 @@ nodes
 | DotRegexServlet.java:115:25:115:30 | source | semmle.label | source |
 | DotRegexServlet.java:133:19:133:39 | getPathInfo(...) : String | semmle.label | getPathInfo(...) : String |
 | DotRegexServlet.java:136:25:136:30 | source | semmle.label | source |
+| DotRegexSpring.java:15:30:15:46 | PROTECTED_PATTERN : String | semmle.label | PROTECTED_PATTERN : String |
+| DotRegexSpring.java:15:50:15:64 | "/protected/.*" : String | semmle.label | "/protected/.*" : String |
+| DotRegexSpring.java:20:26:20:50 | path : String | semmle.label | path : String |
+| DotRegexSpring.java:21:31:21:47 | PROTECTED_PATTERN | semmle.label | PROTECTED_PATTERN |
+| DotRegexSpring.java:22:10:22:25 | decodePath(...) : String | semmle.label | decodePath(...) : String |
+| DotRegexSpring.java:22:21:22:24 | path : String | semmle.label | path : String |
+| DotRegexSpring.java:23:25:23:28 | path | semmle.label | path |
+| DotRegexSpring.java:37:40:37:64 | path : String | semmle.label | path : String |
+| DotRegexSpring.java:38:31:38:47 | PROTECTED_PATTERN | semmle.label | PROTECTED_PATTERN |
+| DotRegexSpring.java:39:10:39:25 | decodePath(...) : String | semmle.label | decodePath(...) : String |
+| DotRegexSpring.java:39:21:39:24 | path : String | semmle.label | path : String |
+| DotRegexSpring.java:40:25:40:28 | path | semmle.label | path |
+| DotRegexSpring.java:54:34:54:58 | path : String | semmle.label | path : String |
+| DotRegexSpring.java:56:10:56:25 | decodePath(...) : String | semmle.label | decodePath(...) : String |
+| DotRegexSpring.java:56:21:56:24 | path : String | semmle.label | path : String |
+| DotRegexSpring.java:57:25:57:28 | path | semmle.label | path |
+| DotRegexSpring.java:69:28:69:38 | path : String | semmle.label | path : String |
+| DotRegexSpring.java:71:11:71:42 | decode(...) : String | semmle.label | decode(...) : String |
+| DotRegexSpring.java:71:29:71:32 | path : String | semmle.label | path : String |
+| DotRegexSpring.java:73:10:73:13 | path : String | semmle.label | path : String |
 subpaths
+| DotRegexSpring.java:22:21:22:24 | path : String | DotRegexSpring.java:69:28:69:38 | path : String | DotRegexSpring.java:73:10:73:13 | path : String | DotRegexSpring.java:22:10:22:25 | decodePath(...) : String |
+| DotRegexSpring.java:39:21:39:24 | path : String | DotRegexSpring.java:69:28:69:38 | path : String | DotRegexSpring.java:73:10:73:13 | path : String | DotRegexSpring.java:39:10:39:25 | decodePath(...) : String |
+| DotRegexSpring.java:56:21:56:24 | path : String | DotRegexSpring.java:69:28:69:38 | path : String | DotRegexSpring.java:73:10:73:13 | path : String | DotRegexSpring.java:56:10:56:25 | decodePath(...) : String |
 #select
 | DotRegexFilter.java:32:25:32:30 | source | DotRegexFilter.java:29:19:29:43 | getPathInfo(...) : String | DotRegexFilter.java:32:25:32:30 | source | Potentially authentication bypass due to $@. | DotRegexFilter.java:29:19:29:43 | getPathInfo(...) | user-provided value |
 | DotRegexServlet.java:22:25:22:30 | source | DotRegexServlet.java:19:19:19:39 | getPathInfo(...) : String | DotRegexServlet.java:22:25:22:30 | source | Potentially authentication bypass due to $@. | DotRegexServlet.java:19:19:19:39 | getPathInfo(...) | user-provided value |
 | DotRegexServlet.java:59:21:59:26 | source | DotRegexServlet.java:57:19:57:41 | getRequestURI(...) : String | DotRegexServlet.java:59:21:59:26 | source | Potentially authentication bypass due to $@. | DotRegexServlet.java:57:19:57:41 | getRequestURI(...) | user-provided value |
 | DotRegexServlet.java:77:56:77:61 | source | DotRegexServlet.java:75:19:75:39 | getPathInfo(...) : String | DotRegexServlet.java:77:56:77:61 | source | Potentially authentication bypass due to $@. | DotRegexServlet.java:75:19:75:39 | getPathInfo(...) | user-provided value |
 | DotRegexServlet.java:115:25:115:30 | source | DotRegexServlet.java:112:19:112:39 | getPathInfo(...) : String | DotRegexServlet.java:115:25:115:30 | source | Potentially authentication bypass due to $@. | DotRegexServlet.java:112:19:112:39 | getPathInfo(...) | user-provided value |
+| DotRegexSpring.java:23:25:23:28 | path | DotRegexSpring.java:20:26:20:50 | path : String | DotRegexSpring.java:23:25:23:28 | path | Potentially authentication bypass due to $@. | DotRegexSpring.java:20:26:20:50 | path | user-provided value |
+| DotRegexSpring.java:40:25:40:28 | path | DotRegexSpring.java:37:40:37:64 | path : String | DotRegexSpring.java:40:25:40:28 | path | Potentially authentication bypass due to $@. | DotRegexSpring.java:37:40:37:64 | path | user-provided value |
--- a/java/ql/test/experimental/query-tests/security/CWE-625/options
+++ b/java/ql/test/experimental/query-tests/security/CWE-625/options
@@ -1 +1 @@
-//semmle-extractor-options: --javac-args -cp ${testdir}/../../../../stubs/servlet-api-2.4
+//semmle-extractor-options: --javac-args -cp ${testdir}/../../../../stubs/servlet-api-2.4:${testdir}/../../../../stubs/springframework-5.3.8