From 90020b6aabbd277df8e84b50b2bebe1e9d4dfd18 Mon Sep 17 00:00:00 2001
From: Tony Torralba <atorralba@users.noreply.github.com>
Date: Thu, 1 Sep 2022 11:32:52 +0200
Subject: [PATCH] Make block lists work with substring matching too

A block list approach doesn't need to restrict itself to prefix matching
---
 .../code/java/security/PathSanitizer.qll      | 14 ++++---------
 .../library-tests/pathsanitizer/Test.java     | 20 +++++++++----------
 2 files changed, 14 insertions(+), 20 deletions(-)

diff --git a/java/ql/lib/semmle/code/java/security/PathSanitizer.qll b/java/ql/lib/semmle/code/java/security/PathSanitizer.qll
index 42bf13d8c98..f1f623da161 100644
--- a/java/ql/lib/semmle/code/java/security/PathSanitizer.qll
+++ b/java/ql/lib/semmle/code/java/security/PathSanitizer.qll
@@ -72,7 +72,6 @@ private class ExactPathMatchSanitizer extends PathInjectionSanitizer {
 private class AllowListGuard extends Guard instanceof MethodAccess {
   AllowListGuard() {
     (isStringPrefixMatch(this) or isPathPrefixMatch(this)) and
-    not isDisallowedPrefix(super.getAnArgument()) and
     not isDisallowedWord(super.getAnArgument())
   }
 
@@ -127,10 +126,7 @@ private class DotDotCheckSanitizer extends PathInjectionSanitizer {
 
 private class BlockListGuard extends Guard instanceof MethodAccess {
   BlockListGuard() {
-    (isStringPrefixMatch(this) or isPathPrefixMatch(this)) and
-    isDisallowedPrefix(super.getAnArgument())
-    or
-    isStringPartialMatch(this) and
+    (isStringPartialMatch(this) or isPathPrefixMatch(this)) and
     isDisallowedWord(super.getAnArgument())
   }
 
@@ -178,6 +174,8 @@ private predicate isStringPrefixMatch(MethodAccess ma) {
  * Holds if `ma` is a call to a method that checks a partial string match.
  */
 private predicate isStringPartialMatch(MethodAccess ma) {
+  isStringPrefixMatch(ma)
+  or
   ma.getMethod().getDeclaringType() instanceof TypeString and
   ma.getMethod().hasName(["contains", "matches", "regionMatches", "indexOf", "lastIndexOf"])
 }
@@ -196,12 +194,8 @@ private predicate isPathPrefixMatch(MethodAccess ma) {
   )
 }
 
-private predicate isDisallowedPrefix(CompileTimeConstantExpr prefix) {
-  prefix.getStringValue().matches(["%WEB-INF%", "/data%"])
-}
-
 private predicate isDisallowedWord(CompileTimeConstantExpr word) {
-  word.getStringValue().matches(["/", "\\"])
+  word.getStringValue().matches(["/", "\\", "%WEB-INF%", "%/data%"])
 }
 
 /** A complementary guard that protects against path traversal, by looking for the literal `..`. */
diff --git a/java/ql/test/library-tests/pathsanitizer/Test.java b/java/ql/test/library-tests/pathsanitizer/Test.java
index 69334125a46..095a6b8f160 100644
--- a/java/ql/test/library-tests/pathsanitizer/Test.java
+++ b/java/ql/test/library-tests/pathsanitizer/Test.java
@@ -421,13 +421,13 @@ public class Test {
                 sink(normalized); // $ hasTaintFlow
             }
         }
-        // PathInjectionSanitizer + partial string match with prefixes is considered unsafe
+        // PathInjectionSanitizer + partial string match with disallowed prefixes
         {
             String source = (String) source();
             String normalized = Paths.get(source).normalize().toString();
-            if (normalized.contains("/data")) {
-                sink(source); // $ hasTaintFlow
-                sink(normalized); // $ hasTaintFlow
+            if (!normalized.contains("/data")) {
+                sink(source); // Safe
+                sink(normalized); // Safe
             } else {
                 sink(source); // $ hasTaintFlow
                 sink(normalized); // $ hasTaintFlow
@@ -436,9 +436,9 @@ public class Test {
         {
             String source = (String) source();
             String normalized = Paths.get(source).normalize().toString();
-            if (normalized.regionMatches(1, "/data", 0, 5)) {
-                sink(source); // $ hasTaintFlow
-                sink(normalized); // $ hasTaintFlow
+            if (!normalized.regionMatches(1, "/data", 0, 5)) {
+                sink(source); // Safe
+                sink(normalized); // Safe
             } else {
                 sink(source); // $ hasTaintFlow
                 sink(normalized); // $ hasTaintFlow
@@ -447,9 +447,9 @@ public class Test {
         {
             String source = (String) source();
             String normalized = Paths.get(source).normalize().toString();
-            if (normalized.matches(".*/data/.*")) {
-                sink(source); // $ hasTaintFlow
-                sink(normalized); // $ hasTaintFlow
+            if (!normalized.matches(".*/data/.*")) {
+                sink(source); // Safe
+                sink(normalized); // Safe
             } else {
                 sink(source); // $ hasTaintFlow
                 sink(normalized); // $ hasTaintFlow