github · luchua-bc · Jul 11, 2021 · Jul 18, 2021
@@ -0,0 +1,24 @@
+public class UnsafeRequestDispatch extends HttpServlet {
+
+	@Override
+	protected void doGet(HttpServletRequest request, HttpServletResponse response)
+			throws ServletException, IOException {
+		String action = request.getParameter("action");
+		String returnURL = request.getParameter("returnURL");
+
+		ServletConfig cfg = getServletConfig();
+		if (action.equals("Login")) {
+			ServletContext sc = cfg.getServletContext();
+
+			// GOOD: Request dispatcher with a whitelisted URI
+			RequestDispatcher rd = sc.getRequestDispatcher("/Login.jsp");
+			rd.forward(request, response);
+		} else {
+			ServletContext sc = cfg.getServletContext();
+
+			// BAD: Request dispatcher constructed from `ServletContext` with user controlled input 
+			RequestDispatcher rd = sc.getRequestDispatcher(returnURL);
+			rd.forward(request, response);
+		}
+	}
+}
@@ -0,0 +1,37 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+
+<overview>
+<p>Directly incorporating user input into HTTP requests dispatched from the Java EE 
+<code>RequestDispatcher</code> without validating the input can allow any web 
+application resource such as configuration files and source code to be disclosed.</p>
+</overview>
+
+<recommendation>
+<p>Unsanitized user provided data must not be used to construct the path passed to 
+the <code>RequestDispatcher</code>. To guard against sensitive file and directory exposure,
+it is advisable to avoid putting user input directly into Java EE <code>RequestDispatcher</code>.
+Instead, maintain a list of authorized resources on the server; then choose from that list based
+on the user input provided.</p>
+</recommendation>
+
+<example>
+<p>The following example shows an HTTP request parameter being used directly in a request dispatcher
+without validating the input, which allows sensitive file exposure attacks.
+It also shows how to remedy the problem by validating the user input against a known fixed string.
+</p>
+<sample src="UnsafeRequestDispatch.java" />
+</example>
+
+<references>
+<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>
@@ -0,0 +1,62 @@
+/**
+ * @name Java EE directory and file exposure from remote source
+ * @description File and directory exposure based on unvalidated user-input may allow attackers
+ *              to access arbitrary configuration files and source code of Java EE applications.
+ * @kind path-problem
+ * @id java/unsafe-request-dispatch
+ * @tags security
+ *       external/cwe/cwe-552
+ */
+
+import java
+import semmle.code.java.dataflow.FlowSources
+import DataFlow::PathGraph
+
+/** The Java class `javax.servlet.RequestDispatcher` or `jakarta.servlet.RequestDispatcher`. */
+class RequestDispatcher extends RefType {
+  RequestDispatcher() {
+    this.hasQualifiedName("javax.servlet", "RequestDispatcher") or
+    this.hasQualifiedName("jakarta.servlet", "RequestDispatcher")
+  }
+}
+
+/** 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"])
+  }
+}
+
+/** Request dispatcher sink. */
+class RequestDispatcherSink extends DataFlow::Node {
+  RequestDispatcherSink() {
+    exists(MethodAccess ma, MethodAccess ma2 |
+      ma.getMethod() instanceof GetRequestDispatcherMethod and
+      this.asExpr() = ma.getArgument(0) and
+      ma2.getMethod() instanceof RequestDispatchMethod and
+      DataFlow::localExprFlow(ma, ma2.getQualifier())
+    )
+  }
+}
+
+class RequestDispatchConfig extends TaintTracking::Configuration {
+  RequestDispatchConfig() { this = "RequestDispatchConfig" }
+
+  override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
+
+  override predicate isSink(DataFlow::Node sink) { sink instanceof RequestDispatcherSink }
+}
+
+from DataFlow::PathNode source, DataFlow::PathNode sink, RequestDispatchConfig conf
+where conf.hasFlowPath(source, sink)
+select sink.getNode(), source, sink, "Unsafe request dispatcher due to $@.", source.getNode(),
+  "user-provided value"
@@ -0,0 +1,112 @@
+/**
+ * @name File loaded in Java EE applications that can be controlled by an attacker
+ * @description File name or path based on unvalidated user-input may allow attackers to access
+ *              arbitrary configuration file and source code in Java EE applications.
+ * @kind path-problem
+ * @id java/unsafe-resource-load
+ * @tags security
+ *       external/cwe/cwe-552
+ *       external/cwe/cwe-022
+ *       external/cwe/cwe-073
+ */
+
+import java
+import semmle.code.java.dataflow.FlowSources
+import semmle.code.java.dataflow.TaintTracking2
+import DataFlow::PathGraph
+
+/** The class `java.lang.ClassLoader`. */
+class ClassLoader extends RefType {
+  ClassLoader() { this.hasQualifiedName("java.lang", "ClassLoader") }
+}
+
+/** The class `javax.servlet.ServletContext`. */
+class ServletContext extends RefType {
+  ServletContext() { this.hasQualifiedName("javax.servlet", "ServletContext") }
+}
+
+/** The resource loading method. */
+class LoadResourceMethod extends Method {
+  LoadResourceMethod() {
+    (
+      this.getDeclaringType().getASupertype*() instanceof ClassLoader or
+      this.getDeclaringType().getASupertype*() instanceof ServletContext
+    ) and
+    this.getName() = ["getResource", "getResourcePaths", "getResourceAsStream", "getResources"]
+  }
+}
+
+/**
+ * Holds if the resource path in the string format is matched against another path,
+ * probably a whitelisted one, and doesn't contain `..` characters for path navigation.
+ */
+predicate isStringPathMatch(MethodAccess ma) {
+  ma.getMethod().getDeclaringType() instanceof TypeString and
+  ma.getMethod().getName() = ["startsWith", "equals"] and
+  exists(MethodAccess ma2 |
+    ma2.getMethod().getDeclaringType() instanceof TypeString and
+    ma2.getMethod().hasName("contains") and
+    ma2.getAnArgument().(StringLiteral).getValue() = ".." and
+    ma2.getQualifier().(VarAccess).getVariable().getAnAccess() = ma.getQualifier()
+  )
+}
+
+/**
+ * Holds if the resource path of the type `java.nio.file.Path` is matched against another
+ * path, probably a whitelisted one.
+ */
+predicate isFilePathMatch(MethodAccess ma) {
+  ma.getMethod().getDeclaringType() instanceof TypePath and
+  ma.getMethod().getName() = ["startsWith", "equals"]
+}
+
+/** Taint configuration of unsafe resource loading. */
+class LoadResourceConfig extends TaintTracking::Configuration {
+  LoadResourceConfig() { this = "LoadResourceConfig" }
+
+  override predicate isSource(DataFlow::Node source) {
+    source instanceof RemoteFlowSource and
+    not exists(CompareResourcePathConfig cc, DataFlow::Node sink | cc.hasFlow(source, sink))
+  }
+
+  /** Sink of resource loading. */
+  override predicate isSink(DataFlow::Node sink) {
+    exists(MethodAccess ma |
+      ma.getMethod() instanceof LoadResourceMethod and
+      sink.asExpr() = ma.getArgument(0)
+    )
+  }
+}
+
+/** Taint configuration of resource path comparison. */
+class CompareResourcePathConfig extends TaintTracking2::Configuration {
+  CompareResourcePathConfig() { this = "CompareResourcePathConfig" }
+
+  override predicate isSource(DataFlow2::Node source) { source instanceof RemoteFlowSource }
+
+  /** Sink of path matching check. */
+  override predicate isSink(DataFlow2::Node sink) {
+    exists(MethodAccess ma |
+      (
+        isStringPathMatch(ma) or
+        isFilePathMatch(ma)
+      ) and
+      sink.asExpr() = ma.getQualifier()
+    )
+  }
+
+  /** Path normalization. */
+  override predicate isAdditionalTaintStep(DataFlow2::Node node1, DataFlow2::Node node2) {
+    exists(MethodAccess ma |
+      ma.getMethod().getDeclaringType() instanceof TypePath and
+      ma.getMethod().getName() = ["get", "normalize", "resolve"] and
+      (node1.asExpr() = ma.getQualifier() or node1.asExpr() = ma.getArgument(0)) and
+      node2.asExpr() = ma
+    )
+  }
+}
+
+from DataFlow::PathNode source, DataFlow::PathNode sink, LoadResourceConfig conf
+where conf.hasFlowPath(source, sink)
+select sink.getNode(), source, sink, "Unsafe request loading due to $@.", source.getNode(),
+  "user-provided value"
@@ -0,0 +1,15 @@
+edges
+| UnsafeRequestDispatch.java:17:22:17:54 | getParameter(...) : String | UnsafeRequestDispatch.java:26:51:26:59 | returnURL |
+| UnsafeRequestDispatch.java:36:22:36:54 | getParameter(...) : String | UnsafeRequestDispatch.java:42:56:42:64 | returnURL |
+| UnsafeRequestDispatch.java:66:23:66:56 | getParameter(...) : String | UnsafeRequestDispatch.java:67:32:67:41 | includeURL |
+nodes
+| UnsafeRequestDispatch.java:17:22:17:54 | getParameter(...) : String | semmle.label | getParameter(...) : String |
+| UnsafeRequestDispatch.java:26:51:26:59 | returnURL | semmle.label | returnURL |
+| UnsafeRequestDispatch.java:36:22:36:54 | getParameter(...) : String | semmle.label | getParameter(...) : String |
+| UnsafeRequestDispatch.java:42:56:42:64 | returnURL | semmle.label | returnURL |
+| UnsafeRequestDispatch.java:66:23:66:56 | getParameter(...) : String | semmle.label | getParameter(...) : String |
+| UnsafeRequestDispatch.java:67:32:67:41 | includeURL | semmle.label | includeURL |
+#select
+| UnsafeRequestDispatch.java:26:51:26:59 | returnURL | UnsafeRequestDispatch.java:17:22:17:54 | getParameter(...) : String | UnsafeRequestDispatch.java:26:51:26:59 | returnURL | Unsafe request dispatcher due to $@. | UnsafeRequestDispatch.java:17:22:17:54 | getParameter(...) | user-provided value |
+| UnsafeRequestDispatch.java:42:56:42:64 | returnURL | UnsafeRequestDispatch.java:36:22:36:54 | getParameter(...) : String | UnsafeRequestDispatch.java:42:56:42:64 | returnURL | Unsafe request dispatcher due to $@. | UnsafeRequestDispatch.java:36:22:36:54 | getParameter(...) | user-provided value |
+| UnsafeRequestDispatch.java:67:32:67:41 | includeURL | UnsafeRequestDispatch.java:66:23:66:56 | getParameter(...) : String | UnsafeRequestDispatch.java:67:32:67:41 | includeURL | Unsafe request dispatcher due to $@. | UnsafeRequestDispatch.java:66:23:66:56 | getParameter(...) | user-provided value |
@@ -0,0 +1,69 @@
+import java.io.IOException;
+import javax.servlet.http.HttpServlet;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+import javax.servlet.RequestDispatcher;
+import javax.servlet.ServletException;
+import javax.servlet.ServletConfig;
+import javax.servlet.ServletContext;
+
+public class UnsafeRequestDispatch extends HttpServlet {
+
+	@Override
+	// BAD: Request dispatcher constructed from `ServletContext` with user controlled input 
+	protected void doGet(HttpServletRequest request, HttpServletResponse response)
+			throws ServletException, IOException {
+		String action = request.getParameter("action");
+		String returnURL = request.getParameter("returnURL");
+
+		ServletConfig cfg = getServletConfig();
+		if (action.equals("Login")) {
+			ServletContext sc = cfg.getServletContext();
+			RequestDispatcher rd = sc.getRequestDispatcher("/Login.jsp");
+			rd.forward(request, response);
+		} else {
+			ServletContext sc = cfg.getServletContext();
+			RequestDispatcher rd = sc.getRequestDispatcher(returnURL);
+			rd.forward(request, response);
+		}
+	}
+
+	@Override
+	// BAD: Request dispatcher constructed from `HttpServletRequest` with user controlled input 
+	protected void doPost(HttpServletRequest request, HttpServletResponse response)
+			throws ServletException, IOException {
+		String action = request.getParameter("action");
+		String returnURL = request.getParameter("returnURL");
+
+		if (action.equals("Login")) {
+			RequestDispatcher rd = request.getRequestDispatcher("/Login.jsp");
+			rd.forward(request, response);
+		} else {
+			RequestDispatcher rd = request.getRequestDispatcher(returnURL);
+			rd.forward(request, response);
+		}
+	}
+
+	@Override
+	// GOOD: Request dispatcher with a whitelisted URI
+	protected void doPut(HttpServletRequest request, HttpServletResponse response)
+			throws ServletException, IOException {
+		String action = request.getParameter("action");
+
+		if (action.equals("Login")) {
+			RequestDispatcher rd = request.getRequestDispatcher("/Login.jsp");
+			rd.forward(request, response);
+		} else if (action.equals("Register")) {
+			RequestDispatcher rd = request.getRequestDispatcher("/Register.jsp");
+			rd.forward(request, response);
+		}
+	}
+
+	@Override
+	// BAD: Request dispatcher constructed from `HttpServletRequest` with user controlled input 
+	protected void doHead(HttpServletRequest request, HttpServletResponse response)
+			throws ServletException, IOException {
+		String includeURL = request.getParameter("includeURL");
+		request.getRequestDispatcher(includeURL).include(request, response);
+	}
+}
@@ -0,0 +1 @@
+experimental/Security/CWE/CWE-552/UnsafeRequestDispatch.ql
@@ -0,0 +1,15 @@
+edges
+| UnsafeResourceLoad.java:23:17:23:40 | getParameter(...) : String | UnsafeResourceLoad.java:26:49:26:52 | path |
+| UnsafeResourceLoad.java:35:17:35:40 | getParameter(...) : String | UnsafeResourceLoad.java:39:68:39:71 | path |
+| UnsafeResourceLoad.java:87:17:87:40 | getParameter(...) : String | UnsafeResourceLoad.java:90:69:90:72 | path |
+nodes
+| UnsafeResourceLoad.java:23:17:23:40 | getParameter(...) : String | semmle.label | getParameter(...) : String |
+| UnsafeResourceLoad.java:26:49:26:52 | path | semmle.label | path |
+| UnsafeResourceLoad.java:35:17:35:40 | getParameter(...) : String | semmle.label | getParameter(...) : String |
+| UnsafeResourceLoad.java:39:68:39:71 | path | semmle.label | path |
+| UnsafeResourceLoad.java:87:17:87:40 | getParameter(...) : String | semmle.label | getParameter(...) : String |
+| UnsafeResourceLoad.java:90:69:90:72 | path | semmle.label | path |
+#select
+| UnsafeResourceLoad.java:26:49:26:52 | path | UnsafeResourceLoad.java:23:17:23:40 | getParameter(...) : String | UnsafeResourceLoad.java:26:49:26:52 | path | Unsafe request loading due to $@. | UnsafeResourceLoad.java:23:17:23:40 | getParameter(...) | user-provided value |
+| UnsafeResourceLoad.java:39:68:39:71 | path | UnsafeResourceLoad.java:35:17:35:40 | getParameter(...) : String | UnsafeResourceLoad.java:39:68:39:71 | path | Unsafe request loading due to $@. | UnsafeResourceLoad.java:35:17:35:40 | getParameter(...) | user-provided value |
+| UnsafeResourceLoad.java:90:69:90:72 | path | UnsafeResourceLoad.java:87:17:87:40 | getParameter(...) : String | UnsafeResourceLoad.java:90:69:90:72 | path | Unsafe request loading due to $@. | UnsafeResourceLoad.java:87:17:87:40 | getParameter(...) | user-provided value |
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1 @@
		experimental/Security/CWE/CWE-552/UnsafeRequestDispatch.ql