[Java] CWE-552: Unsafe url forward

java/ql/lib/semmle/code/java/frameworks/Servlets.qll

@@ -74,7 +74,7 @@ library class HttpServletRequestGetQueryStringMethod extends Method {
 /**
  * The method `getPathInfo()` declared in `javax.servlet.http.HttpServletRequest`.
  */
-library class HttpServletRequestGetPathMethod extends Method {
+class HttpServletRequestGetPathMethod extends Method {
   HttpServletRequestGetPathMethod() {
     getDeclaringType() instanceof HttpServletRequest and
     hasName("getPathInfo") and
@@ -120,7 +120,7 @@ library class HttpServletRequestGetHeaderNamesMethod extends Method {
 /**
  * The method `getRequestURL()` declared in `javax.servlet.http.HttpServletRequest`.
  */
-library class HttpServletRequestGetRequestURLMethod extends Method {
+class HttpServletRequestGetRequestURLMethod extends Method {
   HttpServletRequestGetRequestURLMethod() {
     getDeclaringType() instanceof HttpServletRequest and
     hasName("getRequestURL") and
@@ -131,7 +131,7 @@ library class HttpServletRequestGetRequestURLMethod extends Method {
 /**
  * The method `getRequestURI()` declared in `javax.servlet.http.HttpServletRequest`.
  */
-library class HttpServletRequestGetRequestURIMethod extends Method {
+class HttpServletRequestGetRequestURIMethod extends Method {
   HttpServletRequestGetRequestURIMethod() {
     getDeclaringType() instanceof HttpServletRequest and
     hasName("getRequestURI") and
@@ -191,6 +191,16 @@ class HttpServletResponseSendErrorMethod extends Method {
   }
 }
 
+/**
+ * The method `getRequestDispatcher(String)` declared in `javax.servlet.http.HttpServletRequest` or `javax.servlet.ServletRequest`.
+ */
+class ServletRequestGetRequestDispatcherMethod extends Method {
+  ServletRequestGetRequestDispatcherMethod() {
+    getDeclaringType() instanceof ServletRequest and
+    hasName("getRequestDispatcher")
+  }
+}
+
 /**
  * The method `sendRedirect(String)` declared in `javax.servlet.http.HttpServletResponse`.
  */

java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.java

@@ -0,0 +1,38 @@
+import java.io.IOException;
+import javax.servlet.ServletException;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+import org.springframework.stereotype.Controller;
+import org.springframework.web.bind.annotation.GetMapping;
+import org.springframework.web.servlet.ModelAndView;
+
+@Controller
+public class UnsafeUrlForward {
+
+	@GetMapping("/bad1")
+	public ModelAndView bad1(String url) {
+		return new ModelAndView(url);
+	}
+
+	@GetMapping("/bad2")
+	public void bad2(String url, HttpServletRequest request, HttpServletResponse response) {
+		try {
+			request.getRequestDispatcher("/WEB-INF/jsp/" + url + ".jsp").include(request, response);
+		} catch (ServletException e) {
+			e.printStackTrace();
+		} catch (IOException e) {
+			e.printStackTrace();
+		}
+	}
+
+	@GetMapping("/good1")
+	public void good1(String url, HttpServletRequest request, HttpServletResponse response) {
+		try {
+			request.getRequestDispatcher("/index.jsp?token=" + url).forward(request, response);
+		} catch (ServletException e) {
+			e.printStackTrace();
+		} catch (IOException e) {
+			e.printStackTrace();
+		}
+	}
+}

java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qhelp

@@ -0,0 +1,31 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+
+<overview>
+<p>Constructing a server-side redirect path with user input could allow an attacker to download application binaries 
+(including application classes or jar files) or view arbitrary files within protected directories.</p>
+
+</overview>
+<recommendation>
+
+<p>In order to prevent untrusted URL forwarding, it is recommended to avoid concatenating user input directly into the forwarding URL.</p>
+
+</recommendation>
+<example>
+
+<p>The following examples show the bad case and the good case respectively.
+The <code>bad</code> methods show an HTTP request parameter being used directly in a URL forward
+without validating the input, which may cause file leakage. In <code>good1</code> method, 
+ordinary forwarding requests are shown, which will not cause file leakage.
+</p>
+
+<sample src="UnsafeUrlForward.java" />
+
+</example>
+<references>
+<li>File Disclosure: <a href="https://vulncat.fortify.com/en/detail?id=desc.dataflow.java.file_disclosure_spring">Unsafe Url Forward</a>.</li>
+</references>
+</qhelp>

java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.ql

@@ -0,0 +1,58 @@
+/**
+ * @name Unsafe url forward from remote source
+ * @description URL forward based on unvalidated user-input
+ *              may cause file information disclosure.
+ * @kind path-problem
+ * @problem.severity error
+ * @precision high
+ * @id java/unsafe-url-forward
+ * @tags security
+ *       external/cwe-552
+ */
+
+import java
+import UnsafeUrlForward
+import semmle.code.java.dataflow.FlowSources
+import semmle.code.java.frameworks.Servlets
+import DataFlow::PathGraph
+
+private class StartsWithSanitizer extends DataFlow::BarrierGuard {
+  StartsWithSanitizer() {
+    this.(MethodAccess).getMethod().hasName("startsWith") and
+    this.(MethodAccess).getMethod().getDeclaringType() instanceof TypeString and
+    this.(MethodAccess).getMethod().getNumberOfParameters() = 1
+  }
+
+  override predicate checks(Expr e, boolean branch) {
+    e = this.(MethodAccess).getQualifier() and branch = true
+  }
+}
+
+class UnsafeUrlForwardFlowConfig extends TaintTracking::Configuration {
+  UnsafeUrlForwardFlowConfig() { this = "UnsafeUrlForwardFlowConfig" }
+
+  override predicate isSource(DataFlow::Node source) {
+    source instanceof RemoteFlowSource and
+    not exists(MethodAccess ma, Method m | ma.getMethod() = m |
+      (
+        m instanceof HttpServletRequestGetRequestURIMethod or
+        m instanceof HttpServletRequestGetRequestURLMethod or
+        m instanceof HttpServletRequestGetPathMethod
+      ) and
+      ma = source.asExpr()
+    )
+  }
+
+  override predicate isSink(DataFlow::Node sink) { sink instanceof UnsafeUrlForwardSink }
+
+  override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
+    guard instanceof StartsWithSanitizer
+  }
+
+  override predicate isSanitizer(DataFlow::Node node) { node instanceof UnsafeUrlForwardSanitizer }
+}
+
+from DataFlow::PathNode source, DataFlow::PathNode sink, UnsafeUrlForwardFlowConfig conf
+where conf.hasFlowPath(source, sink)
+select sink.getNode(), source, sink, "Potentially untrusted URL forward due to $@.",
+  source.getNode(), "user-provided value"

java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qll

@@ -0,0 +1,72 @@
+import java
+import DataFlow
+import semmle.code.java.dataflow.FlowSources
+import semmle.code.java.frameworks.Servlets
+import semmle.code.java.frameworks.spring.SpringWeb
+import semmle.code.java.security.RequestForgery
+private import semmle.code.java.dataflow.StringPrefixes
+
+/** A sanitizer for unsafe url forward vulnerabilities. */
+abstract class UnsafeUrlForwardSanitizer extends DataFlow::Node { }
+
+private class PrimitiveSanitizer extends UnsafeUrlForwardSanitizer {
+  PrimitiveSanitizer() {
+    this.getType() instanceof PrimitiveType or
+    this.getType() instanceof BoxedType or
+    this.getType() instanceof NumberType
+  }
+}
+
+private class SanitizingPrefix extends InterestingPrefix {
+  SanitizingPrefix() {
+    not this.getStringValue().matches("/WEB-INF/%") and
+    not this.getStringValue() = "forward:"
+  }
+
+  override int getOffset() { result = 0 }
+}
+
+private class FollowsSanitizingPrefix extends UnsafeUrlForwardSanitizer {
+  FollowsSanitizingPrefix() { this.asExpr() = any(SanitizingPrefix fp).getAnAppendedExpression() }
+}
+
+abstract class UnsafeUrlForwardSink extends DataFlow::Node { }
+
+/** An argument to `ServletRequest.getRequestDispatcher`. */
+private class RequestDispatcherSink extends UnsafeUrlForwardSink {
+  RequestDispatcherSink() {
+    exists(MethodAccess ma |
+      ma.getMethod() instanceof ServletRequestGetRequestDispatcherMethod and
+      ma.getArgument(0) = this.asExpr()
+    )
+  }
+}
+
+/** An argument to `new ModelAndView` or `ModelAndView.setViewName`. */
+private class SpringModelAndViewSink extends UnsafeUrlForwardSink {
+  SpringModelAndViewSink() {
+    exists(ClassInstanceExpr cie |
+      cie.getConstructedType() instanceof ModelAndView and
+      cie.getArgument(0) = this.asExpr()
+    )
+    or
+    exists(SpringModelAndViewSetViewNameCall smavsvnc | smavsvnc.getArgument(0) = this.asExpr())
+  }
+}
+
+private class ForwardPrefix extends InterestingPrefix {
+  ForwardPrefix() { this.getStringValue() = "forward:" }
+
+  override int getOffset() { result = 0 }
+}
+
+/**
+ * An expression appended (perhaps indirectly) to `"forward:"`, and which
+ * is reachable from a Spring entry point.
+ */
+private class SpringUrlForwardSink extends UnsafeUrlForwardSink {
+  SpringUrlForwardSink() {
+    any(SpringRequestMappingMethod sqmm).polyCalls*(this.getEnclosingCallable()) and
+    this.asExpr() = any(ForwardPrefix fp).getAnAppendedExpression()
+  }
+}

java/ql/test/experimental/query-tests/security/CWE-552/UnsafeUrlForward.expected

@@ -0,0 +1,31 @@
+edges
+| UnsafeUrlForward.java:13:27:13:36 | url : String | UnsafeUrlForward.java:14:27:14:29 | url |
+| UnsafeUrlForward.java:18:27:18:36 | url : String | UnsafeUrlForward.java:20:28:20:30 | url |
+| UnsafeUrlForward.java:25:21:25:30 | url : String | UnsafeUrlForward.java:26:23:26:25 | url |
+| UnsafeUrlForward.java:30:27:30:36 | url : String | UnsafeUrlForward.java:31:48:31:63 | ... + ... |
+| UnsafeUrlForward.java:30:27:30:36 | url : String | UnsafeUrlForward.java:31:61:31:63 | url |
+| UnsafeUrlForward.java:36:19:36:28 | url : String | UnsafeUrlForward.java:38:33:38:35 | url |
+| UnsafeUrlForward.java:47:19:47:28 | url : String | UnsafeUrlForward.java:49:33:49:62 | ... + ... |
+nodes
+| UnsafeUrlForward.java:13:27:13:36 | url : String | semmle.label | url : String |
+| UnsafeUrlForward.java:14:27:14:29 | url | semmle.label | url |
+| UnsafeUrlForward.java:18:27:18:36 | url : String | semmle.label | url : String |
+| UnsafeUrlForward.java:20:28:20:30 | url | semmle.label | url |
+| UnsafeUrlForward.java:25:21:25:30 | url : String | semmle.label | url : String |
+| UnsafeUrlForward.java:26:23:26:25 | url | semmle.label | url |
+| UnsafeUrlForward.java:30:27:30:36 | url : String | semmle.label | url : String |
+| UnsafeUrlForward.java:31:48:31:63 | ... + ... | semmle.label | ... + ... |
+| UnsafeUrlForward.java:31:61:31:63 | url | semmle.label | url |
+| UnsafeUrlForward.java:36:19:36:28 | url : String | semmle.label | url : String |
+| UnsafeUrlForward.java:38:33:38:35 | url | semmle.label | url |
+| UnsafeUrlForward.java:47:19:47:28 | url : String | semmle.label | url : String |
+| UnsafeUrlForward.java:49:33:49:62 | ... + ... | semmle.label | ... + ... |
+subpaths
+#select
+| UnsafeUrlForward.java:14:27:14:29 | url | UnsafeUrlForward.java:13:27:13:36 | url : String | UnsafeUrlForward.java:14:27:14:29 | url | Potentially untrusted URL forward due to $@. | UnsafeUrlForward.java:13:27:13:36 | url | user-provided value |
+| UnsafeUrlForward.java:20:28:20:30 | url | UnsafeUrlForward.java:18:27:18:36 | url : String | UnsafeUrlForward.java:20:28:20:30 | url | Potentially untrusted URL forward due to $@. | UnsafeUrlForward.java:18:27:18:36 | url | user-provided value |
+| UnsafeUrlForward.java:26:23:26:25 | url | UnsafeUrlForward.java:25:21:25:30 | url : String | UnsafeUrlForward.java:26:23:26:25 | url | Potentially untrusted URL forward due to $@. | UnsafeUrlForward.java:25:21:25:30 | url | user-provided value |
+| UnsafeUrlForward.java:31:48:31:63 | ... + ... | UnsafeUrlForward.java:30:27:30:36 | url : String | UnsafeUrlForward.java:31:48:31:63 | ... + ... | Potentially untrusted URL forward due to $@. | UnsafeUrlForward.java:30:27:30:36 | url | user-provided value |
+| UnsafeUrlForward.java:31:61:31:63 | url | UnsafeUrlForward.java:30:27:30:36 | url : String | UnsafeUrlForward.java:31:61:31:63 | url | Potentially untrusted URL forward due to $@. | UnsafeUrlForward.java:30:27:30:36 | url | user-provided value |
+| UnsafeUrlForward.java:38:33:38:35 | url | UnsafeUrlForward.java:36:19:36:28 | url : String | UnsafeUrlForward.java:38:33:38:35 | url | Potentially untrusted URL forward due to $@. | UnsafeUrlForward.java:36:19:36:28 | url | user-provided value |
+| UnsafeUrlForward.java:49:33:49:62 | ... + ... | UnsafeUrlForward.java:47:19:47:28 | url : String | UnsafeUrlForward.java:49:33:49:62 | ... + ... | Potentially untrusted URL forward due to $@. | UnsafeUrlForward.java:47:19:47:28 | url | user-provided value |

java/ql/test/experimental/query-tests/security/CWE-552/UnsafeUrlForward.java

@@ -0,0 +1,67 @@
+import java.io.IOException;
+import javax.servlet.ServletException;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+import org.springframework.stereotype.Controller;
+import org.springframework.web.bind.annotation.GetMapping;
+import org.springframework.web.servlet.ModelAndView;
+
+@Controller
+public class UnsafeUrlForward {
+
+	@GetMapping("/bad1")
+	public ModelAndView bad1(String url) {
+		return new ModelAndView(url);
+	}
+
+	@GetMapping("/bad2")
+	public ModelAndView bad2(String url) {
+		ModelAndView modelAndView = new ModelAndView();
+		modelAndView.setViewName(url);
+		return modelAndView;
+	}
+
+	@GetMapping("/bad3")
+	public String bad3(String url) {
+		return "forward:" + url + "/swagger-ui/index.html";
+	}
+
+	@GetMapping("/bad4")
+	public ModelAndView bad4(String url) {
+		ModelAndView modelAndView = new ModelAndView("forward:" + url);
+		return modelAndView;
+	}
+
+	@GetMapping("/bad5")
+	public void bad5(String url, HttpServletRequest request, HttpServletResponse response) {
+		try {
+			request.getRequestDispatcher(url).include(request, response);
+		} catch (ServletException e) {
+			e.printStackTrace();
+		} catch (IOException e) {
+			e.printStackTrace();
+		}
+	}
+
+	@GetMapping("/bad6")
+	public void bad6(String url, HttpServletRequest request, HttpServletResponse response) {
+		try {
+			request.getRequestDispatcher("/WEB-INF/jsp/" + url + ".jsp").include(request, response);
+		} catch (ServletException e) {
+			e.printStackTrace();
+		} catch (IOException e) {
+			e.printStackTrace();
+		}
+	}
+
+	@GetMapping("/good1")
+	public void good1(String url, HttpServletRequest request, HttpServletResponse response) {
+		try {
+			request.getRequestDispatcher("/index.jsp?token=" + url).forward(request, response);
+		} catch (ServletException e) {
+			e.printStackTrace();
+		} catch (IOException e) {
+			e.printStackTrace();
+		}
+	}
+}

java/ql/test/experimental/query-tests/security/CWE-552/UnsafeUrlForward.qlref

@@ -0,0 +1 @@
+experimental/Security/CWE/CWE-552/UnsafeUrlForward.ql

java/ql/test/experimental/query-tests/security/CWE-552/options

@@ -0,0 +1 @@
+//semmle-extractor-options: --javac-args -cp ${testdir}/../../../../stubs/servlet-api-2.4:${testdir}/../../../../stubs/springframework-5.3.8/