Merge ZipSlip sanitization logic into PathSanitizer.qll

Apply code review suggestions regarding weak sanitizers

java/ql/lib/semmle/code/java/security/PathSanitizer.qll

@@ -2,51 +2,80 @@
 
 import java
 private import semmle.code.java.controlflow.Guards
-private import semmle.code.java.dataflow.FlowSources
 private import semmle.code.java.dataflow.ExternalFlow
+private import semmle.code.java.dataflow.FlowSources
+private import semmle.code.java.dataflow.SSA
 
 /** A sanitizer that protects against path injection vulnerabilities. */
 abstract class PathInjectionSanitizer extends DataFlow::Node { }
 
 /**
- * Holds if `g` is guard that compares a string to a trusted value.
+ * Provides a set of nodes validated by a method that uses a validation guard.
  */
-private predicate exactStringPathMatchGuard(Guard g, Expr e, boolean branch) {
-  exists(MethodAccess ma |
+private module ValidationMethod<DataFlow::guardChecksSig/3 validationGuard> {
+  /** Gets a node that is safely guarded by a method that uses the given guard check. */
+  DataFlow::Node getAValidatedNode() {
+    exists(MethodAccess ma, int pos, RValue rv |
+      validationMethod(ma.getMethod(), pos) and
+      ma.getArgument(pos) = rv and
+      adjacentUseUseSameVar(rv, result.asExpr()) and
+      ma.getBasicBlock().bbDominates(result.asExpr().getBasicBlock())
+    )
+  }
+
+  /**
+   * Holds if `m` validates its `arg`th parameter by using `validationGuard`.
+   */
+  private predicate validationMethod(Method m, int arg) {
+    exists(
+      Guard g, SsaImplicitInit var, ControlFlowNode exit, ControlFlowNode normexit, boolean branch
+    |
+      validationGuard(g, var.getAUse(), branch) and
+      var.isParameterDefinition(m.getParameter(arg)) and
+      exit = m and
+      normexit.getANormalSuccessor() = exit and
+      1 = strictcount(ControlFlowNode n | n.getANormalSuccessor() = exit)
+    |
+      g.(ConditionNode).getABranchSuccessor(branch) = exit or
+      g.controls(normexit.getBasicBlock(), branch)
+    )
+  }
+}
+
+/**
+ * Holds if `g` is guard that compares a path to a trusted value.
+ */
+private predicate exactPathMatchGuard(Guard g, Expr e, boolean branch) {
+  exists(MethodAccess ma, RefType t |
+    t instanceof TypeString or
+    t instanceof TypeUri or
+    t instanceof TypePath or
+    t instanceof TypeFile or
+    t.hasQualifiedName("android.net", "Uri")
+  |
+    ma.getMethod().getDeclaringType() = t and
     ma = g and
-    ma.getMethod().getDeclaringType() instanceof TypeString and
     ma.getMethod().getName() = ["equals", "equalsIgnoreCase"] and
     e = ma.getQualifier() and
     branch = true
   )
 }
 
-private class ExactStringPathMatchSanitizer extends PathInjectionSanitizer {
-  ExactStringPathMatchSanitizer() {
-    this = DataFlow::BarrierGuard<exactStringPathMatchGuard/3>::getABarrierNode()
+private class ExactPathMatchSanitizer extends PathInjectionSanitizer {
+  ExactPathMatchSanitizer() {
+    this = DataFlow::BarrierGuard<exactPathMatchGuard/3>::getABarrierNode()
+    or
+    this = ValidationMethod<exactPathMatchGuard/3>::getAValidatedNode()
   }
 }
 
-/**
- * Given input `e` = `v.method1(...).method2(...)...`, returns `v` where `v` is a `VarAccess`.
- *
- * This is used to look through field accessors such as `uri.getPath()`.
- */
-private Expr getUnderlyingVarAccess(Expr e) {
-  result = getUnderlyingVarAccess(e.(MethodAccess).getQualifier().getUnderlyingExpr())
-  or
-  result = e.(VarAccess)
-}
-
 private class AllowListGuard extends Guard instanceof MethodAccess {
   AllowListGuard() {
-    (isStringPartialMatch(this) or isPathPartialMatch(this)) and
-    not isDisallowedWord(super.getAnArgument())
+    (isStringPrefixMatch(this) or isPathPrefixMatch(this)) and
+    not isDisallowedPrefix(super.getAnArgument())
   }
 
-  Expr getCheckedExpr() {
-    result = getUnderlyingVarAccess(super.getQualifier().getUnderlyingExpr())
-  }
+  Expr getCheckedExpr() { result = super.getQualifier() }
 }
 
 /**
@@ -55,22 +84,21 @@ private class AllowListGuard extends Guard instanceof MethodAccess {
  * or a sanitizer (`PathNormalizeSanitizer`), to ensure any internal `..` components are removed from the path.
  */
 private predicate allowListGuard(Guard g, Expr e, boolean branch) {
-  e = g.(AllowListGuard).getCheckedExpr() and
   branch = true and
-  (
-    // Either a path normalization sanitizer comes before the guard,
-    exists(PathNormalizeSanitizer sanitizer | DataFlow::localExprFlow(sanitizer, e))
+  TaintTracking::localExprTaint(e, g.(AllowListGuard).getCheckedExpr()) and
+  exists(MethodAccess previousGuard |
+    TaintTracking::localExprTaint(previousGuard.(PathNormalizeSanitizer),
+      g.(AllowListGuard).getCheckedExpr())
     or
-    // or a check like `!path.contains("..")` comes before the guard
-    exists(PathTraversalGuard previousGuard |
-      DataFlow::localExprFlow(previousGuard.getCheckedExpr(), e) and
-      previousGuard.controls(g.getBasicBlock().(ConditionBlock), false)
-    )
+    previousGuard.(PathTraversalGuard).controls(g.getBasicBlock().(ConditionBlock), false)
   )
 }
 
 private class AllowListSanitizer extends PathInjectionSanitizer {
-  AllowListSanitizer() { this = DataFlow::BarrierGuard<allowListGuard/3>::getABarrierNode() }
+  AllowListSanitizer() {
+    this = DataFlow::BarrierGuard<allowListGuard/3>::getABarrierNode() or
+    this = ValidationMethod<allowListGuard/3>::getAValidatedNode()
+  }
 }
 
 /**
@@ -78,93 +106,97 @@ private class AllowListSanitizer extends PathInjectionSanitizer {
  * been checked for a trusted prefix.
  */
 private predicate dotDotCheckGuard(Guard g, Expr e, boolean branch) {
-  e = g.(PathTraversalGuard).getCheckedExpr() and
   branch = false and
-  // The same value has previously been checked against a list of allowed prefixes:
-  exists(AllowListGuard previousGuard |
-    DataFlow::localExprFlow(previousGuard.getCheckedExpr(), e) and
-    previousGuard.controls(g.getBasicBlock().(ConditionBlock), true)
+  TaintTracking::localExprTaint(e, g.(PathTraversalGuard).getCheckedExpr()) and
+  exists(MethodAccess previousGuard |
+    previousGuard.(AllowListGuard).controls(g.getBasicBlock().(ConditionBlock), true)
+    or
+    previousGuard.(BlockListGuard).controls(g.getBasicBlock().(ConditionBlock), false)
   )
 }
 
 private class DotDotCheckSanitizer extends PathInjectionSanitizer {
-  DotDotCheckSanitizer() { this = DataFlow::BarrierGuard<dotDotCheckGuard/3>::getABarrierNode() }
+  DotDotCheckSanitizer() {
+    this = DataFlow::BarrierGuard<dotDotCheckGuard/3>::getABarrierNode() or
+    this = ValidationMethod<dotDotCheckGuard/3>::getAValidatedNode()
+  }
 }
 
 private class BlockListGuard extends Guard instanceof MethodAccess {
   BlockListGuard() {
-    (isStringPartialMatch(this) or isPathPartialMatch(this)) and
+    (isStringPrefixMatch(this) or isPathPrefixMatch(this)) and
+    isDisallowedPrefix(super.getAnArgument())
+    or
+    isStringPartialMatch(this) and
     isDisallowedWord(super.getAnArgument())
   }
 
-  Expr getCheckedExpr() {
-    result = getUnderlyingVarAccess(super.getQualifier().getUnderlyingExpr())
-  }
+  Expr getCheckedExpr() { result = super.getQualifier() }
 }
 
 /**
  * Holds if `g` is a guard that considers a string safe because it is checked against a blocklist of known dangerous values.
- * This requires a prior check for URL encoding concealing a forbidden value, either a guard (`UrlEncodingGuard`)
- * or a sanitizer (`UrlDecodeSanitizer`).
+ * This requires additional protection against path traversal, either another guard (`PathTraversalGuard`)
+ * or a sanitizer (`PathNormalizeSanitizer`), to ensure any internal `..` components are removed from the path.
  */
 private predicate blockListGuard(Guard g, Expr e, boolean branch) {
-  e = g.(BlockListGuard).getCheckedExpr() and
   branch = false and
-  (
-    // Either `e` has been URL decoded:
-    exists(UrlDecodeSanitizer sanitizer | DataFlow::localExprFlow(sanitizer, e))
+  TaintTracking::localExprTaint(e, g.(BlockListGuard).getCheckedExpr()) and
+  exists(MethodAccess previousGuard |
+    TaintTracking::localExprTaint(previousGuard.(PathNormalizeSanitizer),
+      g.(BlockListGuard).getCheckedExpr())
     or
-    // or `e` has previously been checked for URL encoding sequences:
-    exists(UrlEncodingGuard previousGuard |
-      DataFlow::localExprFlow(previousGuard.getCheckedExpr(), e) and
-      previousGuard.controls(g.getBasicBlock(), false)
-    )
+    previousGuard.(PathTraversalGuard).controls(g.getBasicBlock().(ConditionBlock), false)
   )
 }
 
 private class BlockListSanitizer extends PathInjectionSanitizer {
-  BlockListSanitizer() { this = DataFlow::BarrierGuard<blockListGuard/3>::getABarrierNode() }
+  BlockListSanitizer() {
+    this = DataFlow::BarrierGuard<blockListGuard/3>::getABarrierNode() or
+    this = ValidationMethod<blockListGuard/3>::getAValidatedNode()
+  }
 }
 
-/**
- * Holds if `g` is a guard that considers a string safe because it is checked for URL encoding sequences,
- * having previously been checked against a block-list of forbidden values.
- */
-private predicate urlEncodingGuard(Guard g, Expr e, boolean branch) {
-  e = g.(UrlEncodingGuard).getCheckedExpr() and
-  branch = false and
-  exists(BlockListGuard previousGuard |
-    DataFlow::localExprFlow(previousGuard.getCheckedExpr(), e) and
-    previousGuard.controls(g.getBasicBlock(), false)
+private predicate isStringPrefixMatch(MethodAccess ma) {
+  exists(Method m | m = ma.getMethod() and m.getDeclaringType() instanceof TypeString |
+    m.hasName("startsWith")
+    or
+    m.hasName("regionMatches") and
+    ma.getArgument(0).(CompileTimeConstantExpr).getIntValue() = 0
+    or
+    m.hasName("matches") and
+    not ma.getArgument(0).(CompileTimeConstantExpr).getStringValue().matches(".*%")
   )
 }
 
-private class UrlEncodingSanitizer extends PathInjectionSanitizer {
-  UrlEncodingSanitizer() { this = DataFlow::BarrierGuard<urlEncodingGuard/3>::getABarrierNode() }
-}
-
 /**
  * Holds if `ma` is a call to a method that checks a partial string match.
  */
 private predicate isStringPartialMatch(MethodAccess ma) {
   ma.getMethod().getDeclaringType() instanceof TypeString and
-  ma.getMethod()
-      .hasName(["contains", "startsWith", "matches", "regionMatches", "indexOf", "lastIndexOf"])
+  ma.getMethod().hasName(["contains", "matches", "regionMatches", "indexOf", "lastIndexOf"])
 }
 
 /**
- * Holds if `ma` is a call to a method that checks a partial path match.
+ * Holds if `ma` is a call to a method that checks whether a path starts with a prefix.
  */
-private predicate isPathPartialMatch(MethodAccess ma) {
-  ma.getMethod().getDeclaringType() instanceof TypePath and
-  ma.getMethod().hasName("startsWith")
-  or
-  ma.getMethod().getDeclaringType().hasQualifiedName("kotlin.io", "FilesKt") and
-  ma.getMethod().hasName("startsWith")
+private predicate isPathPrefixMatch(MethodAccess ma) {
+  exists(RefType t |
+    t instanceof TypePath
+    or
+    t.hasQualifiedName("kotlin.io", "FilesKt")
+  |
+    t = ma.getMethod().getDeclaringType() and
+    ma.getMethod().hasName("startsWith")
+  )
+}
+
+private predicate isDisallowedPrefix(CompileTimeConstantExpr prefix) {
+  prefix.getStringValue().matches(["%WEB-INF%", "/data%"])
 }
 
 private predicate isDisallowedWord(CompileTimeConstantExpr word) {
-  word.getStringValue().matches(["%WEB-INF%", "%META-INF%", "%..%"])
+  word.getStringValue().matches(["/", "\\"])
 }
 
 /** A complementary guard that protects against path traversal, by looking for the literal `..`. */
@@ -175,9 +207,7 @@ private class PathTraversalGuard extends Guard instanceof MethodAccess {
     super.getAnArgument().(CompileTimeConstantExpr).getStringValue() = ".."
   }
 
-  Expr getCheckedExpr() {
-    result = getUnderlyingVarAccess(super.getQualifier().getUnderlyingExpr())
-  }
+  Expr getCheckedExpr() { result = super.getQualifier() }
 }
 
 /** A complementary sanitizer that protects against path traversal using path normalization. */
@@ -196,30 +226,6 @@ private class PathNormalizeSanitizer extends MethodAccess {
   }
 }
 
-/** A complementary guard that protects against double URL encoding, by looking for the literal `%`. */
-private class UrlEncodingGuard extends Guard instanceof MethodAccess {
-  UrlEncodingGuard() {
-    super.getMethod().getDeclaringType() instanceof TypeString and
-    super.getMethod().hasName(["contains", "indexOf"]) and
-    super.getAnArgument().(CompileTimeConstantExpr).getStringValue() = "%"
-  }
-
-  Expr getCheckedExpr() { result = super.getQualifier() }
-}
-
-/** A complementary sanitizer that protects against double URL encoding using URL decoding. */
-private class UrlDecodeSanitizer extends MethodAccess {
-  UrlDecodeSanitizer() {
-    exists(RefType t |
-      this.getMethod().getDeclaringType() = t and
-      this.getMethod().hasName("decode")
-    |
-      t.hasQualifiedName("java.net", "URLDecoder") or
-      t.hasQualifiedName("android.net", "Uri")
-    )
-  }
-}
-
 /** A node with path normalization. */
 class NormalizedPathNode extends DataFlow::Node {
   NormalizedPathNode() {

java/ql/src/Security/CWE/CWE-022/ZipSlip.ql

@@ -71,54 +71,6 @@ predicate fileTaintStep(ExprNode n1, ExprNode n2) {
   )
 }
 
-predicate localFileValueStep(Node n1, Node n2) {
-  localFlowStep(n1, n2) or
-  filePathStep(n1, n2)
-}
-
-predicate localFileValueStepPlus(Node n1, Node n2) = fastTC(localFileValueStep/2)(n1, n2)
-
-/**
- * Holds if `check` is a guard that checks whether `var` is a file path with a
- * specific prefix when put in canonical form, thus guarding against ZipSlip.
- */
-predicate validateFilePath(SsaVariable var, Guard check) {
-  // `var.getCanonicalFile().toPath().startsWith(...)`,
-  // `var.getCanonicalPath().startsWith(...)`, or
-  // `var.toPath().normalize().startsWith(...)`
-  exists(MethodAccess normalize, MethodAccess startsWith, Node n1, Node n2, Node n3, Node n4 |
-    n1.asExpr() = var.getAUse() and
-    n2.asExpr() = normalize.getQualifier() and
-    (n1 = n2 or localFileValueStepPlus(n1, n2)) and
-    n3.asExpr() = normalize and
-    n4.asExpr() = startsWith.getQualifier() and
-    (n3 = n4 or localFileValueStepPlus(n3, n4)) and
-    check = startsWith and
-    startsWith.getMethod().hasName("startsWith") and
-    (
-      normalize.getMethod().hasName("getCanonicalFile") or
-      normalize.getMethod().hasName("getCanonicalPath") or
-      normalize.getMethod().hasName("normalize")
-    )
-  )
-}
-
-/**
- * Holds if `m` validates its `arg`th parameter.
- */
-predicate validationMethod(Method m, int arg) {
-  exists(Guard check, SsaImplicitInit var, ControlFlowNode exit, ControlFlowNode normexit |
-    validateFilePath(var, check) and
-    var.isParameterDefinition(m.getParameter(arg)) and
-    exit = m and
-    normexit.getANormalSuccessor() = exit and
-    1 = strictcount(ControlFlowNode n | n.getANormalSuccessor() = exit)
-  |
-    check.(ConditionNode).getATrueSuccessor() = exit or
-    check.controls(normexit.getBasicBlock(), true)
-  )
-}
-
 class ZipSlipConfiguration extends TaintTracking::Configuration {
   ZipSlipConfiguration() { this = "ZipSlip" }
 
@@ -132,23 +84,7 @@ class ZipSlipConfiguration extends TaintTracking::Configuration {
     filePathStep(n1, n2) or fileTaintStep(n1, n2)
   }
 
-  override predicate isSanitizer(Node node) {
-    // TODO: Merge this sanitizers into PathInjectionSanitizer
-    exists(Guard g, SsaVariable var, RValue varuse | validateFilePath(var, g) |
-      varuse = node.asExpr() and
-      varuse = var.getAUse() and
-      g.controls(varuse.getBasicBlock(), true)
-    )
-    or
-    exists(MethodAccess ma, int pos, RValue rv |
-      validationMethod(ma.getMethod(), pos) and
-      ma.getArgument(pos) = rv and
-      adjacentUseUseSameVar(rv, node.asExpr()) and
-      ma.getBasicBlock().bbDominates(node.asExpr().getBasicBlock())
-    )
-    or
-    node instanceof PathInjectionSanitizer
-  }
+  override predicate isSanitizer(Node node) { node instanceof PathInjectionSanitizer }
 }
 
 /**

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

@@ -100,7 +100,10 @@ public class UnsafeServletRequestDispatch extends HttpServlet {
 		}
 	}
 
-	// BAD: Request dispatcher with negation check and path normalization, but without URL decoding
+	// FN: Request dispatcher with negation check and path normalization, but without URL decoding
+	// When promoting this query, consider using FlowStates to make `getRequestDispatcher` a sink
+	// only if a URL-decoding step has NOT been crossed (i.e. make URLDecoder.decode change the
+	// state to a different value than the one required at the sink).
 	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

@@ -20,11 +20,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 |
@@ -69,12 +64,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 |
@@ -106,7 +95,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 |