Merge pull request #7286 from luchua-bc/java/unsafe-url-forward-dispatch

Java: CWE-552 Query to detect unsafe request dispatcher usage

Author: smowton

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

@@ -347,3 +347,27 @@ predicate isRequestGetParamMethod(MethodAccess ma) {
   ma.getMethod() instanceof ServletRequestGetParameterMapMethod or
   ma.getMethod() instanceof HttpServletRequestGetQueryStringMethod
 }
+
+/** The Java EE RequestDispatcher. */
+class RequestDispatcher extends RefType {
+  RequestDispatcher() {
+    this.hasQualifiedName(["javax.servlet", "jakarta.servlet"], "RequestDispatcher") or
+    this.hasQualifiedName("javax.portlet", "PortletRequestDispatcher")
+  }
+}
+
+/** The `getRequestDispatcher` method. */
+class GetRequestDispatcherMethod extends Method {
+  GetRequestDispatcherMethod() {
+    this.getReturnType() instanceof RequestDispatcher and
+    this.getName() = "getRequestDispatcher"
+  }
+}
+
+/** The request dispatch method. */
+class RequestDispatchMethod extends Method {
+  RequestDispatchMethod() {
+    this.getDeclaringType() instanceof RequestDispatcher and
+    this.hasName(["forward", "include"])
+  }
+}

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

@@ -0,0 +1,11 @@
+// BAD: no URI validation
+String returnURL = request.getParameter("returnURL");
+RequestDispatcher rd = sc.getRequestDispatcher(returnURL);
+rd.forward(request, response);
+
+// GOOD: check for a trusted prefix, ensuring path traversal is not used to erase that prefix:
+// (alternatively use `Path.normalize` instead of checking for `..`)
+if (!returnURL.contains("..") && returnURL.hasPrefix("/pages")) { ... }
+// Also GOOD: check for a forbidden prefix, ensuring URL-encoding is not used to evade the check:
+// (alternatively use `URLDecoder.decode` before `hasPrefix`)
+if (returnURL.hasPrefix("/internal") && !returnURL.contains("%")) { ... }

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

@@ -5,27 +5,47 @@
 
 
 <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.
+Instead, user input should be checked against allowed (e.g., must come within <code>user_content/</code>) or disallowed
+(e.g. must not come within <code>/internal</code>) paths, ensuring that neither path traversal using <code>../</code>
+or URL encoding are used to evade these checks.
+</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 the <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" />
+
 </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>

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

@@ -1,33 +1,20 @@
 /**
- * @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
  */
 
 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" }
 
@@ -45,11 +32,15 @@ class UnsafeUrlForwardFlowConfig extends TaintTracking::Configuration {
 
   override predicate isSink(DataFlow::Node sink) { sink instanceof UnsafeUrlForwardSink }
 
+  override predicate isSanitizer(DataFlow::Node node) { node instanceof UnsafeUrlForwardSanitizer }
+
   override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
-    guard instanceof StartsWithSanitizer
+    guard instanceof UnsafeUrlForwardBarrierGuard
   }
 
-  override predicate isSanitizer(DataFlow::Node node) { node instanceof UnsafeUrlForwardSanitizer }
+  override DataFlow::FlowFeature getAFeature() {
+    result instanceof DataFlow::FeatureHasSourceCallContext
+  }
 }
 
 from DataFlow::PathNode source, DataFlow::PathNode sink, UnsafeUrlForwardFlowConfig conf