Merge pull request #8055 from luchua-bc/java/unsafe-url-forward-with-shared-lib

CWE-552: Switch to the shared PathSanitizer library

Author: atorralba

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

@@ -12,7 +12,7 @@
 
 import java
 import UnsafeUrlForward
-import semmle.code.java.dataflow.FlowSources
+import experimental.semmle.code.java.PathSanitizer
 import DataFlow::PathGraph
 
 class UnsafeUrlForwardFlowConfig extends TaintTracking::Configuration {
@@ -35,7 +35,7 @@ class UnsafeUrlForwardFlowConfig extends TaintTracking::Configuration {
   override predicate isSanitizer(DataFlow::Node node) { node instanceof UnsafeUrlForwardSanitizer }
 
   override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
-    guard instanceof UnsafeUrlForwardBarrierGuard
+    guard instanceof PathTraversalBarrierGuard
   }
 
   override DataFlow::FlowFeature getAFeature() {

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

@@ -1,10 +1,5 @@
 import java
-import DataFlow
-import semmle.code.java.controlflow.Guards
 import semmle.code.java.dataflow.FlowSources
-import semmle.code.java.frameworks.Servlets
-import semmle.code.java.frameworks.spring.SpringWeb
-import semmle.code.java.security.RequestForgery
 private import semmle.code.java.dataflow.StringPrefixes
 
 /** A sink for unsafe URL forward vulnerabilities. */
@@ -13,9 +8,6 @@ abstract class UnsafeUrlForwardSink extends DataFlow::Node { }
 /** A sanitizer for unsafe URL forward vulnerabilities. */
 abstract class UnsafeUrlForwardSanitizer extends DataFlow::Node { }
 
-/** A barrier guard that protects against URL forward vulnerabilities. */
-abstract class UnsafeUrlForwardBarrierGuard extends DataFlow::BarrierGuard { }
-
 /** An argument to `getRequestDispatcher`. */
 private class RequestDispatcherSink extends UnsafeUrlForwardSink {
   RequestDispatcherSink() {
@@ -59,175 +51,6 @@ private class FollowsSanitizingPrefix extends UnsafeUrlForwardSanitizer {
   FollowsSanitizingPrefix() { this.asExpr() = any(SanitizingPrefix fp).getAnAppendedExpression() }
 }
 
-/**
- * A guard that considers safe a string being exactly compared to a trusted value.
- */
-private class ExactStringPathMatchGuard extends UnsafeUrlForwardBarrierGuard instanceof MethodAccess {
-  ExactStringPathMatchGuard() {
-    super.getMethod().getDeclaringType() instanceof TypeString and
-    super.getMethod().getName() = ["equals", "equalsIgnoreCase"]
-  }
-
-  override predicate checks(Expr e, boolean branch) {
-    e = super.getQualifier() and
-    branch = true
-  }
-}
-
-private class AllowListGuard extends Guard instanceof MethodAccess {
-  AllowListGuard() {
-    (isStringPartialMatch(this) or isPathPartialMatch(this)) and
-    not isDisallowedWord(super.getAnArgument())
-  }
-
-  Expr getCheckedExpr() { result = super.getQualifier() }
-}
-
-/**
- * A guard that considers a path safe because it is checked against an allowlist of partial trusted values.
- * 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 class AllowListBarrierGuard extends UnsafeUrlForwardBarrierGuard instanceof AllowListGuard {
-  override predicate checks(Expr e, boolean branch) {
-    e = super.getCheckedExpr() and
-    branch = true and
-    (
-      // Either a path normalization sanitizer comes before the guard,
-      exists(PathNormalizeSanitizer sanitizer | DataFlow::localExprFlow(sanitizer, e))
-      or
-      // or a check like `!path.contains("..")` comes before the guard
-      exists(PathTraversalGuard previousGuard |
-        DataFlow::localExprFlow(previousGuard.getCheckedExpr(), e) and
-        previousGuard.controls(this.getBasicBlock().(ConditionBlock), false)
-      )
-    )
-  }
-}
-
-/**
- * A guard that considers a path safe because it is checked for `..` components, having previously
- * been checked for a trusted prefix.
- */
-private class DotDotCheckBarrierGuard extends UnsafeUrlForwardBarrierGuard instanceof PathTraversalGuard {
-  override predicate checks(Expr e, boolean branch) {
-    e = super.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(this.getBasicBlock().(ConditionBlock), true)
-    )
-  }
-}
-
-private class BlockListGuard extends Guard instanceof MethodAccess {
-  BlockListGuard() {
-    (isStringPartialMatch(this) or isPathPartialMatch(this)) and
-    isDisallowedWord(super.getAnArgument())
-  }
-
-  Expr getCheckedExpr() { result = super.getQualifier() }
-}
-
-/**
- * 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`).
- */
-private class BlockListBarrierGuard extends UnsafeUrlForwardBarrierGuard instanceof BlockListGuard {
-  override predicate checks(Expr e, boolean branch) {
-    e = super.getCheckedExpr() and
-    branch = false and
-    (
-      // Either `e` has been URL decoded:
-      exists(UrlDecodeSanitizer sanitizer | DataFlow::localExprFlow(sanitizer, e))
-      or
-      // or `e` has previously been checked for URL encoding sequences:
-      exists(UrlEncodingGuard previousGuard |
-        DataFlow::localExprFlow(previousGuard.getCheckedExpr(), e) and
-        previousGuard.controls(this.getBasicBlock(), false)
-      )
-    )
-  }
-}
-
-/**
- * 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 class URLEncodingBarrierGuard extends UnsafeUrlForwardBarrierGuard instanceof UrlEncodingGuard {
-  override predicate checks(Expr e, boolean branch) {
-    e = super.getCheckedExpr() and
-    branch = false and
-    exists(BlockListGuard previousGuard |
-      DataFlow::localExprFlow(previousGuard.getCheckedExpr(), e) and
-      previousGuard.controls(this.getBasicBlock(), false)
-    )
-  }
-}
-
-/**
- * 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().getName() =
-    ["contains", "startsWith", "matches", "regionMatches", "indexOf", "lastIndexOf"]
-}
-
-/**
- * Holds if `ma` is a call to a method of `java.nio.file.Path` that checks a partial path match.
- */
-private predicate isPathPartialMatch(MethodAccess ma) {
-  ma.getMethod().getDeclaringType() instanceof TypePath and
-  ma.getMethod().getName() = "startsWith"
-}
-
-private predicate isDisallowedWord(CompileTimeConstantExpr word) {
-  word.getStringValue().matches(["%WEB-INF%", "%META-INF%", "%..%"])
-}
-
-/** A complementary guard that protects against path traversal, by looking for the literal `..`. */
-private class PathTraversalGuard extends Guard instanceof MethodAccess {
-  Expr checked;
-
-  PathTraversalGuard() {
-    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 path traversal using path normalization. */
-private class PathNormalizeSanitizer extends MethodAccess {
-  PathNormalizeSanitizer() {
-    this.getMethod().getDeclaringType().hasQualifiedName("java.nio.file", "Path") and
-    this.getMethod().hasName("normalize")
-  }
-}
-
-/** 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() {
-    this.getMethod().getDeclaringType().hasQualifiedName("java.net", "URLDecoder") and
-    this.getMethod().hasName("decode")
-  }
-}
-
 private class ForwardPrefix extends InterestingPrefix {
   ForwardPrefix() { this.getStringValue() = "forward:" }