Optimize the query

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

@@ -20,7 +20,7 @@ untrusted URL forwarding, it is recommended to avoid concatenating user input di
 
 <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>
 

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

@@ -18,16 +18,25 @@ import semmle.code.java.controlflow.Guards
 import DataFlow::PathGraph
 
 /**
- * Holds if `ma` is a method call of matching with a path string, probably a whitelisted one.
+ * Holds if `ma` is a call to a method that checks exact match of string, probably a whitelisted one.
  */
-predicate isStringPathMatch(MethodAccess ma) {
+predicate isExactStringPathMatch(MethodAccess ma) {
   ma.getMethod().getDeclaringType() instanceof TypeString and
-  ma.getMethod().getName() = ["startsWith", "matches", "regionMatches"]
+  ma.getMethod().getName() = ["equals", "equalsIgnoreCase"]
 }
 
 /**
- * Holds if `ma` is a method call of `java.nio.file.Path` which matches with another
+ * Holds if `ma` is a call to a method that checks a path string, probably a whitelisted one.
- * path, probably a whitelisted one.
+ */
+predicate isStringPathMatch(MethodAccess ma) {
+  ma.getMethod().getDeclaringType() instanceof TypeString and
+  ma.getMethod().getName() =
+    ["contains", "startsWith", "matches", "regionMatches", "indexOf", "lastIndexOf"]
+}
+
+/**
+ * Holds if `ma` is a call to a method of `java.nio.file.Path` that checks a path, probably
+ * a whitelisted one.
  */
 predicate isFilePathMatch(MethodAccess ma) {
   ma.getMethod().getDeclaringType() instanceof TypePath and
@@ -35,7 +44,7 @@ predicate isFilePathMatch(MethodAccess ma) {
 }
 
 /**
- * Holds if `ma` is a method call that checks an input doesn't match using the `!`
+ * Holds if `ma` is a call to a method that checks an input doesn't match using the `!`
  * logical negation expression.
  */
 predicate checkNoPathMatch(MethodAccess ma) {
@@ -46,7 +55,7 @@ predicate checkNoPathMatch(MethodAccess ma) {
 }
 
 /**
- * Holds if `ma` is a method call to check special characters `..` used in path traversal.
+ * Holds if `ma` is a call to a method that checks special characters `..` used in path traversal.
  */
 predicate isPathTraversalCheck(MethodAccess ma) {
   ma.getMethod().getDeclaringType() instanceof TypeString and
@@ -55,7 +64,7 @@ predicate isPathTraversalCheck(MethodAccess ma) {
 }
 
 /**
- * Holds if `ma` is a method call to decode a url string or check url encoding.
+ * Holds if `ma` is a call to a method that decodes a URL string or check URL encoding.
  */
 predicate isPathDecoding(MethodAccess ma) {
   // Search the special character `%` used in url encoding
@@ -68,44 +77,65 @@ predicate isPathDecoding(MethodAccess ma) {
   ma.getMethod().hasName("decode")
 }
 
-private class PathMatchSanitizer extends DataFlow::Node {
+/** The Java method `normalize` of `java.nio.file.Path`. */
+class PathNormalizeMethod extends Method {
+  PathNormalizeMethod() {
+    this.getDeclaringType().hasQualifiedName("java.nio.file", "Path") and
+    this.hasName("normalize")
+  }
+}
+
+/**
+ * Sanitizer to check the following scenarios in a web application:
+ *  1. Exact string match
+ *  2. String startsWith or match check with path traversal validation
+ *  3. String not startsWith or not match check with decoding processing
+ *  4. java.nio.file.Path startsWith check having path normalization
+ */
+private class PathMatchSanitizer extends DataFlow::BarrierGuard {
   PathMatchSanitizer() {
-    exists(MethodAccess ma |
+    isExactStringPathMatch(this)
-      (
+    or
-        isStringPathMatch(ma) and
+    isStringPathMatch(this) and
-        exists(MethodAccess ma2 |
+    not checkNoPathMatch(this) and
-          isPathTraversalCheck(ma2) and
+    exists(MethodAccess tma |
-          ma.getQualifier().(VarAccess).getVariable().getAnAccess() = ma2.getQualifier()
+      isPathTraversalCheck(tma) and
-        )
+      DataFlow::localExprFlow(this.(MethodAccess).getQualifier(), tma.getQualifier())
-        or
+    )
-        isFilePathMatch(ma)
+    or
-      ) and
+    checkNoPathMatch(this) and
-      (
+    exists(MethodAccess dma |
-        not checkNoPathMatch(ma)
+      isPathDecoding(dma) and
-        or
+      DataFlow::localExprFlow(dma, this.(MethodAccess).getQualifier())
-        // 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
+    or
-        exists(MethodAccess ma2 |
+    isFilePathMatch(this) and
-          isPathDecoding(ma2) and
+    exists(MethodAccess pma |
-          ma.getQualifier().(VarAccess).getVariable().getAnAccess() = ma2.getQualifier()
+      pma.getMethod() instanceof PathNormalizeMethod and
-        )
+      DataFlow::localExprFlow(pma, this.(MethodAccess).getQualifier())
-      ) and
+    )
-      this.asExpr() = ma.getQualifier()
+  }
+
+  override predicate checks(Expr e, boolean branch) {
+    e = this.(MethodAccess).getQualifier() and
+    (
+      branch = true and not checkNoPathMatch(this)
+      or
+      branch = false and checkNoPathMatch(this)
     )
   }
 }
 
 /**
- * Holds if `ma` is a method call to check string content, which means an input string is not
+ * Holds if `ma` is a call to a method that checks 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",
+          "charAt", "getBytes", "getChars", "length", "replace", "replaceAll", "replaceFirst",
-          "lastIndexOf", "length", "matches", "regionMatches", "replace", "replaceAll",
+          "substring"
-          "replaceFirst", "substring"
         ]) and
   expr = ma.getQualifier()
   or
@@ -145,23 +175,6 @@ private class NullOrEmptyCheckSanitizer extends DataFlow::Node {
   NullOrEmptyCheckSanitizer() { isNullOrEmptyCheck(this.asExpr()) }
 }
 
-/** Holds if `ma` is a virtual method call of Map::get or Object::toString. */
-predicate isVirtualMethod(MethodAccess ma, Expr expr) {
-  ma.getMethod().getDeclaringType() instanceof TypeObject and
-  ma.getMethod().hasName("toString") and
-  (expr = ma or expr = ma.getQualifier())
-  or
-  (
-    ma.getMethod().getDeclaringType().getASupertype*().hasQualifiedName("java.util", "Map") and
-    ma.getMethod().hasName(["get", "getOrDefault"])
-  ) and
-  (expr = ma or expr = ma.getAnArgument())
-}
-
-private class VirtualMethodSanitizer extends DataFlow::Node {
-  VirtualMethodSanitizer() { exists(MethodAccess ma | isVirtualMethod(ma, this.asExpr())) }
-}
-
 class UnsafeUrlForwardFlowConfig extends TaintTracking::Configuration {
   UnsafeUrlForwardFlowConfig() { this = "UnsafeUrlForwardFlowConfig" }
 
@@ -181,10 +194,16 @@ class UnsafeUrlForwardFlowConfig extends TaintTracking::Configuration {
 
   override predicate isSanitizer(DataFlow::Node node) {
     node instanceof UnsafeUrlForwardSanitizer or
-    node instanceof PathMatchSanitizer or
     node instanceof StringOperationSanitizer or
-    node instanceof NullOrEmptyCheckSanitizer or
+    node instanceof NullOrEmptyCheckSanitizer
-    node instanceof VirtualMethodSanitizer
+  }
+
+  override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
+    guard instanceof PathMatchSanitizer
+  }
+
+  override DataFlow::FlowFeature getAFeature() {
+    result instanceof DataFlow::FeatureHasSourceCallContext
   }
 }
 

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

@@ -87,10 +87,9 @@ private class FilePathFlowStep extends SummaryModelCsv {
   override predicate row(string row) {
     row =
       [
-        "java.nio.file;Paths;true;get;;;Argument[-1];ReturnValue;taint",
+        "java.nio.file;Paths;true;get;;;Argument[0..1];ReturnValue;taint",
-        "java.nio.file;Path;true;resolve;;;Argument[0];ReturnValue;taint",
+        "java.nio.file;Path;true;resolve;;;Argument[-1..0];ReturnValue;taint",
         "java.nio.file;Path;true;normalize;;;Argument[-1];ReturnValue;taint",
-        "java.nio.file;Path;true;startsWith;;;Argument[-1];ReturnValue;taint",
         "java.nio.file;Path;true;toString;;;Argument[-1];ReturnValue;taint"
       ]
   }

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

@@ -100,7 +100,7 @@ public class UnsafeServletRequestDispatch extends HttpServlet {
 		}
 	}
 
-	// BAD: Request dispatcher with improper negation check and without url decoding 
+	// GOOD: Request dispatcher with negation check and path normalization
 	protected void doHead5(HttpServletRequest request, HttpServletResponse response)
 			throws ServletException, IOException {
 		String path = request.getParameter("path");

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

@@ -3,11 +3,6 @@ edges
 | UnsafeServletRequestDispatch.java:23:22:23:54 | getParameter(...) : String | UnsafeServletRequestDispatch.java:32:51:32:59 | returnURL |
 | UnsafeServletRequestDispatch.java:42:22:42:54 | getParameter(...) : String | UnsafeServletRequestDispatch.java:48:56:48:64 | returnURL |
 | UnsafeServletRequestDispatch.java:71:17:71:44 | getParameter(...) : String | UnsafeServletRequestDispatch.java:76:53:76:56 | path |
-| UnsafeServletRequestDispatch.java:106:17:106:44 | getParameter(...) : String | UnsafeServletRequestDispatch.java:107:53:107:56 | path : String |
-| UnsafeServletRequestDispatch.java:107:24:107:57 | resolve(...) : Path | UnsafeServletRequestDispatch.java:107:24:107:69 | normalize(...) : Path |
-| UnsafeServletRequestDispatch.java:107:24:107:69 | normalize(...) : Path | UnsafeServletRequestDispatch.java:110:53:110:65 | requestedPath : Path |
-| UnsafeServletRequestDispatch.java:107:53:107:56 | path : String | UnsafeServletRequestDispatch.java:107:24:107:57 | resolve(...) : Path |
-| UnsafeServletRequestDispatch.java:110:53:110:65 | requestedPath : Path | UnsafeServletRequestDispatch.java:110:53:110:76 | toString(...) |
 | 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 |
@@ -25,12 +20,6 @@ nodes
 | UnsafeServletRequestDispatch.java:48:56:48:64 | returnURL | semmle.label | returnURL |
 | UnsafeServletRequestDispatch.java:71:17:71:44 | getParameter(...) : String | semmle.label | getParameter(...) : String |
 | UnsafeServletRequestDispatch.java:76:53:76:56 | path | semmle.label | path |
-| UnsafeServletRequestDispatch.java:106:17:106:44 | getParameter(...) : String | semmle.label | getParameter(...) : String |
-| UnsafeServletRequestDispatch.java:107:24:107:57 | resolve(...) : Path | semmle.label | resolve(...) : Path |
-| UnsafeServletRequestDispatch.java:107:24:107:69 | normalize(...) : Path | semmle.label | normalize(...) : Path |
-| UnsafeServletRequestDispatch.java:107:53:107:56 | path : String | semmle.label | path : String |
-| UnsafeServletRequestDispatch.java:110:53:110:65 | requestedPath : Path | semmle.label | requestedPath : Path |
-| UnsafeServletRequestDispatch.java:110:53:110:76 | toString(...) | semmle.label | toString(...) |
 | 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 |
@@ -52,7 +41,6 @@ subpaths
 | UnsafeServletRequestDispatch.java:32:51:32:59 | returnURL | UnsafeServletRequestDispatch.java:23:22:23:54 | getParameter(...) : String | UnsafeServletRequestDispatch.java:32:51:32:59 | returnURL | Potentially untrusted URL forward due to $@. | UnsafeServletRequestDispatch.java:23:22:23:54 | getParameter(...) | user-provided value |
 | UnsafeServletRequestDispatch.java:48:56:48:64 | returnURL | UnsafeServletRequestDispatch.java:42:22:42:54 | getParameter(...) : String | UnsafeServletRequestDispatch.java:48:56:48:64 | returnURL | Potentially untrusted URL forward due to $@. | UnsafeServletRequestDispatch.java:42:22:42:54 | getParameter(...) | user-provided value |
 | UnsafeServletRequestDispatch.java:76:53:76:56 | path | UnsafeServletRequestDispatch.java:71:17:71:44 | getParameter(...) : String | UnsafeServletRequestDispatch.java:76:53:76:56 | path | Potentially untrusted URL forward due to $@. | UnsafeServletRequestDispatch.java:71:17:71:44 | getParameter(...) | user-provided value |
-| UnsafeServletRequestDispatch.java:110:53:110:76 | toString(...) | UnsafeServletRequestDispatch.java:106:17:106:44 | getParameter(...) : String | UnsafeServletRequestDispatch.java:110:53:110:76 | toString(...) | Potentially untrusted URL forward due to $@. | UnsafeServletRequestDispatch.java:106:17:106:44 | getParameter(...) | user-provided value |
 | 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 |