Query to detect unsafe request dispatcher usage

smowton · Jan 18, 2022 · Dec 2, 2021 · Dec 15, 2021 · Jan 12, 2022 · Jan 13, 2022
commit 8bcffc2886f3c17d6083287d4177f053683e36db
@@ -347,3 +347,27 @@ predicate isRequestGetParamMethod(MethodAccess ma) {
   ma.getMethod() instanceof ServletRequestGetParameterMapMethod or
   ma.getMethod() instanceof HttpServletRequestGetQueryStringMethod
 }
+
+/** The Java EE RequestDispatcher. */
+library class RequestDispatcher extends RefType {
+  RequestDispatcher() {
+    this.hasQualifiedName(["javax.servlet", "jakarta.servlet"], "RequestDispatcher") or
+    this.hasQualifiedName("javax.portlet", "PortletRequestDispatcher")
+  }
+}
+
+/** The `getRequestDispatcher` method. */
+library class GetRequestDispatcherMethod extends Method {
+  GetRequestDispatcherMethod() {
+    this.getReturnType() instanceof RequestDispatcher and
+    this.getName() = "getRequestDispatcher"
+  }
+}
+
+/** The request dispatch method. */
+library class RequestDispatchMethod extends Method {
+  RequestDispatchMethod() {
+    this.getDeclaringType() instanceof RequestDispatcher and
+    this.hasName(["forward", "include"])
+  }
+}
diff --git a/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeRequestPath.java b/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeRequestPath.java
@@ -0,0 +1,30 @@
+public class UnsafeRequestPath implements Filter {
+	private static final String BASE_PATH = "/pages";
+
+	@Override
+	public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain)
+		throws IOException, ServletException {
+
+		{
+			// BAD: Request dispatcher from servlet path without check 
+			String path = ((HttpServletRequest) request).getServletPath();
+			// A sample payload "/%57EB-INF/web.xml" can bypass this `startsWith` check
+			if (path != null && !path.startsWith("/WEB-INF")) {
+				request.getRequestDispatcher(path).forward(request, response);
+			} else {
+				chain.doFilter(request, response);
+			}
+		}
+
+		{
+			// GOOD: Request dispatcher from servlet path with path traversal check 
+			String path = ((HttpServletRequest) request).getServletPath();
+
+			if (path.startsWith(BASE_PATH) && !path.contains("..")) {
+				request.getRequestDispatcher(path).forward(request, response);
+			} else {
+				chain.doFilter(request, response);
+			}
+		}
+	}
+}
@@ -0,0 +1,82 @@
+public class UnsafeServletRequestDispatch extends HttpServlet {
+	private static final String BASE_PATH = "/pages";
+
+	@Override
+	protected void doGet(HttpServletRequest request, HttpServletResponse response)
+			throws ServletException, IOException {
+		{
+			// GOOD: whitelisted URI
+			if (action.equals("Login")) {
+				ServletContext sc = cfg.getServletContext();
+				RequestDispatcher rd = sc.getRequestDispatcher("/Login.jsp");
+				rd.forward(request, response);
+			} 
+		}
+
+		{
+			// BAD: Request dispatcher constructed from `ServletContext` without input validation
+			String returnURL = request.getParameter("returnURL");
+			ServletConfig cfg = getServletConfig();
+
+			ServletContext sc = cfg.getServletContext();
+			RequestDispatcher rd = sc.getRequestDispatcher(returnURL);
+			rd.forward(request, response);
+		}
+
+		{
+			// BAD: Request dispatcher without path traversal check 
+			String path = request.getParameter("path");
+
+			// A sample payload "/pages/welcome.jsp/../WEB-INF/web.xml" can bypass the `startsWith` check 
+			// The payload "/pages/welcome.jsp/../../%57EB-INF/web.xml" can bypass the check as well since RequestDispatcher will decode `%57` as `W`  
+			if (path.startsWith(BASE_PATH)) {
+				request.getServletContext().getRequestDispatcher(path).include(request, response);
+			}
+		}
+	}
+
+	{
+		// GOOD: Request dispatcher with path traversal check 
+		String path = request.getParameter("path");
+
+		if (path.startsWith(BASE_PATH) && !path.contains("..")) {
+			request.getServletContext().getRequestDispatcher(path).include(request, response);
+		}
+	}
+
+	{
+		// GOOD: Request dispatcher with path normalization 
+		String path = request.getParameter("path");
+		Path requestedPath = Paths.get(BASE_PATH).resolve(path).normalize();
+
+		// /pages/welcome.jsp/../../WEB-INF/web.xml becomes /WEB-INF/web.xml
+		// /pages/welcome.jsp/../../%57EB-INF/web.xml becomes /%57EB-INF/web.xml
+		if (requestedPath.startsWith(BASE_PATH)) {
+			request.getServletContext().getRequestDispatcher(requestedPath.toString()).forward(request, response);
+		}
+	}
+
+	{
+		// BAD: Request dispatcher with improper negation check and without url decoding 
+		String path = request.getParameter("path");
+		Path requestedPath = Paths.get(BASE_PATH).resolve(path).normalize();
+
+		if (!requestedPath.startsWith("/WEB-INF") && !requestedPath.startsWith("/META-INF")) {
+			request.getServletContext().getRequestDispatcher(requestedPath.toString()).forward(request, response);
+		}
+	}
+
+	{
+		// GOOD: Request dispatcher with path traversal check and url decoding
+		String path = request.getParameter("path");
+		boolean hasEncoding = path.contains("%");
+		while (hasEncoding) {
+			path = URLDecoder.decode(path, "UTF-8");
+			hasEncoding = path.contains("%");
+		}
+
+		if (!path.startsWith("/WEB-INF/") && !path.contains("..")) {
+			request.getServletContext().getRequestDispatcher(path).include(request, response);
+		}
+	}
+}
@@ -5,27 +5,45 @@
 
 
 <overview>
-<p>Constructing a server-side redirect path with user input could allow an attacker to download application binaries 
+<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>
+<p>Unsanitized user provided data must not be used to construct the path for URL forwarding. 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, 
+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" />
 
+<p>The following examples show an HTTP request parameter or request path being used directly in a
+request dispatcher of Java EE without validating the input, which allows sensitive file exposure
+attacks. It also shows how to remedy the problem by validating the user input.
+</p>
+
+<sample src="UnsafeServletRequestDispatch.java" />
+<sample src="UnsafeRequestPath.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>
+<li>File Disclosure:
+  <a href="https://vulncat.fortify.com/en/detail?id=desc.dataflow.java.file_disclosure_spring">Unsafe Url Forward</a>.
+</li>
+<li>Jakarta Javadoc:
+  <a href="https://jakarta.ee/specifications/webprofile/9/apidocs/jakarta/servlet/servletrequest#getRequestDispatcher-java.lang.String-">Security vulnerability with unsafe usage of RequestDispatcher</a>.
+</li>
+<li>Micro Focus:
+  <a href="https://vulncat.fortify.com/en/detail?id=desc.dataflow.java.file_disclosure_j2ee">File Disclosure: J2EE</a>
+</li>
 </references>
 </qhelp>
@@ -1,11 +1,11 @@
 /**
- * @name Unsafe url forward from remote source
- * @description URL forward based on unvalidated user-input
+ * @name Unsafe url forward or dispatch from remote source
+ * @description URL forward or dispatch based on unvalidated user-input
  *              may cause file information disclosure.
  * @kind path-problem
  * @problem.severity error
  * @precision high
- * @id java/unsafe-url-forward
+ * @id java/unsafe-url-forward-dispatch
  * @tags security
  *       external/cwe-552
  */
@@ -14,20 +14,137 @@ import java
 import UnsafeUrlForward
 import semmle.code.java.dataflow.FlowSources
 import semmle.code.java.frameworks.Servlets
+import semmle.code.java.controlflow.Guards
 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
-  }
+/**
+ * Holds if `ma` is a method call of matching with a path string, probably a whitelisted one.
+ */
+predicate isStringPathMatch(MethodAccess ma) {
+  ma.getMethod().getDeclaringType() instanceof TypeString and
+  ma.getMethod().getName() = ["startsWith", "matches", "regionMatches"]
+}
+
+/**
+ * Holds if `ma` is a method call of `java.nio.file.Path` which matches with another
+ * path, probably a whitelisted one.
+ */
+predicate isFilePathMatch(MethodAccess ma) {
+  ma.getMethod().getDeclaringType() instanceof TypePath and
+  ma.getMethod().getName() = "startsWith"
+}
+
+/**
+ * Holds if `ma` is a method call that checks an input doesn't match using the `!`
+ * logical negation expression.
+ */
+predicate checkNoPathMatch(MethodAccess ma) {
+  exists(LogNotExpr lne |
+    (isStringPathMatch(ma) or isFilePathMatch(ma)) and
+    lne.getExpr() = ma
+  )
+}
+
+/**
+ * Holds if `ma` is a method call to check special characters `..` used in path traversal.
+ */
+predicate isPathTraversalCheck(MethodAccess ma) {
+  ma.getMethod().getDeclaringType() instanceof TypeString and
+  ma.getMethod().hasName(["contains", "indexOf"]) and
+  ma.getAnArgument().(CompileTimeConstantExpr).getStringValue() = ".."
+}
+
+/**
+ * Holds if `ma` is a method call to decode a url string or check url encoding.
+ */
+predicate isPathDecoding(MethodAccess ma) {
+  // Search the special character `%` used in url encoding
+  ma.getMethod().getDeclaringType() instanceof TypeString and
+  ma.getMethod().hasName(["contains", "indexOf"]) and
+  ma.getAnArgument().(CompileTimeConstantExpr).getStringValue() = "%"
+  or
+  // Call to `URLDecoder` assuming the implementation handles double encoding correctly
+  ma.getMethod().getDeclaringType().hasQualifiedName("java.net", "URLDecoder") and
+  ma.getMethod().hasName("decode")
+}
 
-  override predicate checks(Expr e, boolean branch) {
-    e = this.(MethodAccess).getQualifier() and branch = true
+private class PathMatchSanitizer extends DataFlow::Node {
+  PathMatchSanitizer() {
+    exists(MethodAccess ma |
+      (
+        isStringPathMatch(ma) and
+        exists(MethodAccess ma2 |
+          isPathTraversalCheck(ma2) and
+          ma.getQualifier().(VarAccess).getVariable().getAnAccess() = ma2.getQualifier()
+        )
+        or
+        isFilePathMatch(ma)
+      ) and
+      (
+        not checkNoPathMatch(ma)
+        or
+        // non-match check needs decoding e.g. !path.startsWith("/WEB-INF/") won't detect /%57EB-INF/web.xml, which will be decoded and served by RequestDispatcher
+        checkNoPathMatch(ma) and
+        exists(MethodAccess ma2 |
+          isPathDecoding(ma2) and
+          ma.getQualifier().(VarAccess).getVariable().getAnAccess() = ma2.getQualifier()
+        )
+      ) and
+      this.asExpr() = ma.getQualifier()
+    )
   }
 }
 
+/**
+ * Holds if `ma` is a method call to check string content, which means an input string is not
+ * blindly trusted and helps to reduce FPs.
+ */
+predicate checkStringContent(MethodAccess ma, Expr expr) {
+  ma.getMethod().getDeclaringType() instanceof TypeString and
+  ma.getMethod()
+      .hasName([
+          "charAt", "contains", "equals", "equalsIgnoreCase", "getBytes", "getChars", "indexOf",
+          "lastIndexOf", "length", "matches", "regionMatches", "replace", "replaceAll",
+          "replaceFirst", "substring"
+        ]) and
+  expr = ma.getQualifier()
+  or
+  (
+    ma.getMethod().getDeclaringType() instanceof TypeStringBuffer or
+    ma.getMethod().getDeclaringType() instanceof TypeStringBuilder
+  ) and
+  expr = ma.getAnArgument()
+}
+
+private class StringOperationSanitizer extends DataFlow::Node {
+  StringOperationSanitizer() { exists(MethodAccess ma | checkStringContent(ma, this.asExpr())) }
+}
+
+/**
+ * Holds if `expr` is an expression returned from null or empty string check.
+ */
+predicate isNullOrEmptyCheck(Expr expr) {
+  exists(ConditionBlock cb, ReturnStmt rt |
+    cb.controls(rt.getBasicBlock(), true) and
+    (
+      cb.getCondition().(EQExpr).getAnOperand() instanceof NullLiteral // if (path == null)
+      or
+      // if (path.equals(""))
+      exists(MethodAccess ma |
+        cb.getCondition() = ma and
+        ma.getMethod().getDeclaringType() instanceof TypeString and
+        ma.getMethod().hasName("equals") and
+        ma.getArgument(0).(CompileTimeConstantExpr).getStringValue() = ""
+      )
+    ) and
+    expr.getParent+() = rt
+  )
+}
+
+private class NullOrEmptyCheckSanitizer extends DataFlow::Node {
+  NullOrEmptyCheckSanitizer() { isNullOrEmptyCheck(this.asExpr()) }
+}
+
 class UnsafeUrlForwardFlowConfig extends TaintTracking::Configuration {
   UnsafeUrlForwardFlowConfig() { this = "UnsafeUrlForwardFlowConfig" }
 
@@ -45,11 +162,12 @@ class UnsafeUrlForwardFlowConfig extends TaintTracking::Configuration {
 
   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 or
+    node instanceof PathMatchSanitizer or
+    node instanceof StringOperationSanitizer or
+    node instanceof NullOrEmptyCheckSanitizer
   }
-
-  override predicate isSanitizer(DataFlow::Node node) { node instanceof UnsafeUrlForwardSanitizer }
 }
 
 from DataFlow::PathNode source, DataFlow::PathNode sink, UnsafeUrlForwardFlowConfig conf