Add tests and fix bugs highlighted by them

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

@@ -82,6 +82,7 @@ private class FileSummaryModels extends SummaryModelCsv {
         "java.io;File;true;getCanonicalFile;;;Argument[-1];ReturnValue;taint;manual",
         "java.io;File;true;getCanonicalPath;;;Argument[-1];ReturnValue;taint;manual",
         "java.io;File;true;toPath;;;Argument[-1];ReturnValue;taint;manual",
+        "java.io;File;true;toString;;;Argument[-1];ReturnValue;taint;manual",
         "java.io;File;true;toURI;;;Argument[-1];ReturnValue;taint;manual",
         "java.nio.file;Path;true;normalize;;;Argument[-1];ReturnValue;taint;manual",
         "java.nio.file;Path;true;resolve;;;Argument[-1..0];ReturnValue;taint;manual",

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

@@ -72,7 +72,8 @@ private class ExactPathMatchSanitizer extends PathInjectionSanitizer {
 private class AllowListGuard extends Guard instanceof MethodAccess {
   AllowListGuard() {
     (isStringPrefixMatch(this) or isPathPrefixMatch(this)) and
-    not isDisallowedPrefix(super.getAnArgument())
+    not isDisallowedPrefix(super.getAnArgument()) and
+    not isDisallowedWord(super.getAnArgument())
   }
 
   Expr getCheckedExpr() { result = super.getQualifier() }
@@ -86,11 +87,13 @@ private class AllowListGuard extends Guard instanceof MethodAccess {
 private predicate allowListGuard(Guard g, Expr e, boolean branch) {
   branch = true and
   TaintTracking::localExprTaint(e, g.(AllowListGuard).getCheckedExpr()) and
-  exists(MethodAccess previousGuard |
+  exists(Expr previousGuard |
     TaintTracking::localExprTaint(previousGuard.(PathNormalizeSanitizer),
       g.(AllowListGuard).getCheckedExpr())
     or
-    previousGuard.(PathTraversalGuard).controls(g.getBasicBlock().(ConditionBlock), false)
+    previousGuard
+        .(PathTraversalGuard)
+        .controls(g.getBasicBlock().(ConditionBlock), previousGuard.(PathTraversalGuard).getBranch())
   )
 }
 
@@ -106,9 +109,9 @@ private class AllowListSanitizer extends PathInjectionSanitizer {
  * been checked for a trusted prefix.
  */
 private predicate dotDotCheckGuard(Guard g, Expr e, boolean branch) {
-  branch = false and
+  branch = g.(PathTraversalGuard).getBranch() and
   TaintTracking::localExprTaint(e, g.(PathTraversalGuard).getCheckedExpr()) and
-  exists(MethodAccess previousGuard |
+  exists(Guard previousGuard |
     previousGuard.(AllowListGuard).controls(g.getBasicBlock().(ConditionBlock), true)
     or
     previousGuard.(BlockListGuard).controls(g.getBasicBlock().(ConditionBlock), false)
@@ -142,11 +145,13 @@ private class BlockListGuard extends Guard instanceof MethodAccess {
 private predicate blockListGuard(Guard g, Expr e, boolean branch) {
   branch = false and
   TaintTracking::localExprTaint(e, g.(BlockListGuard).getCheckedExpr()) and
-  exists(MethodAccess previousGuard |
+  exists(Expr previousGuard |
     TaintTracking::localExprTaint(previousGuard.(PathNormalizeSanitizer),
       g.(BlockListGuard).getCheckedExpr())
     or
-    previousGuard.(PathTraversalGuard).controls(g.getBasicBlock().(ConditionBlock), false)
+    previousGuard
+        .(PathTraversalGuard)
+        .controls(g.getBasicBlock().(ConditionBlock), previousGuard.(PathTraversalGuard).getBranch())
   )
 }
 
@@ -200,14 +205,35 @@ private predicate isDisallowedWord(CompileTimeConstantExpr word) {
 }
 
 /** A complementary guard that protects against path traversal, by looking for the literal `..`. */
-private class PathTraversalGuard extends Guard instanceof MethodAccess {
+private class PathTraversalGuard extends Guard {
   PathTraversalGuard() {
-    super.getMethod().getDeclaringType() instanceof TypeString and
-    super.getMethod().hasName(["contains", "indexOf"]) and
-    super.getAnArgument().(CompileTimeConstantExpr).getStringValue() = ".."
+    exists(MethodAccess ma |
+      ma.getMethod().getDeclaringType() instanceof TypeString and
+      ma.getAnArgument().(CompileTimeConstantExpr).getStringValue() = ".."
+    |
+      this = ma and
+      ma.getMethod().hasName("contains")
+      or
+      exists(EqualityTest eq |
+        this = eq and
+        ma.getMethod().hasName(["indexOf", "lastIndexOf"]) and
+        eq.getAnOperand() = ma and
+        eq.getAnOperand().(CompileTimeConstantExpr).getIntValue() = -1
+      )
+    )
   }
 
-  Expr getCheckedExpr() { result = super.getQualifier() }
+  Expr getCheckedExpr() {
+    exists(MethodAccess ma | ma = this.(BinaryExpr).getAnOperand() or ma = this |
+      result = ma.getQualifier()
+    )
+  }
+
+  boolean getBranch() {
+    this instanceof MethodAccess and result = false
+    or
+    result = this.(EqualityTest).polarity()
+  }
 }
 
 /** A complementary sanitizer that protects against path traversal using path normalization. */

java/ql/test/library-tests/pathsanitizer/Test.java

@@ -0,0 +1,459 @@
+import java.io.File;
+import java.net.URI;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import android.net.Uri;
+
+public class Test {
+
+    Object source() {
+        return null;
+    }
+
+    void sink(Object o) {}
+
+    private void exactPathMatchGuardValidation(String path) throws Exception {
+        if (!path.equals("/safe/path"))
+            throw new Exception();
+    }
+
+    public void exactPathMatchGuard() throws Exception {
+        {
+            String source = (String) source();
+            if (source.equals("/safe/path"))
+                sink(source); // Safe
+            else
+                sink(source); // $ hasTaintFlow
+        }
+        {
+            URI source = (URI) source();
+            if (source.equals(new URI("http://safe/uri")))
+                sink(source); // Safe
+            else
+                sink(source); // $ hasTaintFlow
+        }
+        {
+            File source = (File) source();
+            if (source.equals(new File("/safe/file")))
+                sink(source); // Safe
+            else
+                sink(source); // $ hasTaintFlow
+        }
+        {
+            Uri source = (Uri) source();
+            if (source.equals(Uri.parse("http://safe/uri")))
+                sink(source); // Safe
+            else
+                sink(source); // $ hasTaintFlow
+        }
+        {
+            String source = (String) source();
+            exactPathMatchGuardValidation(source);
+            sink(source); // Safe
+        }
+    }
+
+    private void allowListGuardValidation(String path) throws Exception {
+        if (path.contains("..") || !path.startsWith("/safe"))
+            throw new Exception();
+    }
+
+    public void allowListGuard() throws Exception {
+        // Prefix check by itself is not enough
+        {
+            String source = (String) source();
+            if (source.startsWith("/safe")) {
+                sink(source); // $ hasTaintFlow
+            } else
+                sink(source); // $ hasTaintFlow
+        }
+        // PathTraversalGuard + allowListGuard
+        {
+            String source = (String) source();
+            if (!source.contains("..") && source.startsWith("/safe"))
+                sink(source); // Safe
+            else
+                sink(source); // $ hasTaintFlow
+        }
+        {
+            String source = (String) source();
+            if (source.indexOf("..") == -1 && source.startsWith("/safe"))
+                sink(source); // Safe
+            else
+                sink(source); // $ hasTaintFlow
+        }
+        {
+            String source = (String) source();
+            if (source.lastIndexOf("..") == -1 && source.startsWith("/safe"))
+                sink(source); // Safe
+            else
+                sink(source); // $ hasTaintFlow
+        }
+        // PathTraversalSanitizer + allowListGuard
+        {
+            File source = (File) source();
+            String normalized = source.getCanonicalPath();
+            if (normalized.startsWith("/safe")) {
+                sink(source); // Safe
+                sink(normalized); // Safe
+            } else {
+                sink(source); // $ hasTaintFlow
+                sink(normalized); // $ hasTaintFlow
+            }
+        }
+        {
+            File source = (File) source();
+            String normalized = source.getCanonicalFile().toString();
+            if (normalized.startsWith("/safe")) {
+                sink(source); // Safe
+                sink(normalized); // Safe
+            } else {
+                sink(source); // $ hasTaintFlow
+                sink(normalized); // $ hasTaintFlow
+            }
+        }
+        {
+            String source = (String) source();
+            Path normalized = Paths.get(source).normalize();
+            if (normalized.startsWith("/safe")) {
+                sink(source); // Safe
+                sink(normalized); // Safe
+            } else {
+                sink(source); // $ hasTaintFlow
+                sink(normalized); // $ hasTaintFlow
+            }
+        }
+        {
+            String source = (String) source();
+            String normalized = Paths.get(source).normalize().toString();
+            if (normalized.startsWith("/safe")) {
+                sink(source); // Safe
+                sink(normalized); // Safe
+            } else {
+                sink(source); // $ hasTaintFlow
+                sink(normalized); // $ hasTaintFlow
+            }
+        }
+        {
+            String source = (String) source();
+            String normalized = Paths.get(source).normalize().toString();
+            if (normalized.regionMatches(0, "/safe", 0, 5)) {
+                sink(source); // Safe
+                sink(normalized); // Safe
+            } else {
+                sink(source); // $ hasTaintFlow
+                sink(normalized); // $ hasTaintFlow
+            }
+        }
+        {
+            String source = (String) source();
+            String normalized = Paths.get(source).normalize().toString();
+            if (normalized.matches("/safe/.*")) {
+                sink(source); // Safe
+                sink(normalized); // Safe
+            } else {
+                sink(source); // $ hasTaintFlow
+                sink(normalized); // $ hasTaintFlow
+            }
+        }
+        // validation method
+        {
+            String source = (String) source();
+            allowListGuardValidation(source);
+            sink(source); // Safe
+        }
+        // PathInjectionSanitizer + partial string match is considered unsafe
+        {
+            String source = (String) source();
+            String normalized = Paths.get(source).normalize().toString();
+            if (normalized.contains("/safe")) {
+                sink(source); // $ hasTaintFlow
+                sink(normalized); // $ hasTaintFlow
+            } else {
+                sink(source); // $ hasTaintFlow
+                sink(normalized); // $ hasTaintFlow
+            }
+        }
+        {
+            String source = (String) source();
+            String normalized = Paths.get(source).normalize().toString();
+            if (normalized.regionMatches(1, "/safe", 0, 5)) {
+                sink(source); // $ hasTaintFlow
+                sink(normalized); // $ hasTaintFlow
+            } else {
+                sink(source); // $ hasTaintFlow
+                sink(normalized); // $ hasTaintFlow
+            }
+        }
+        {
+            String source = (String) source();
+            String normalized = Paths.get(source).normalize().toString();
+            if (normalized.matches(".*/safe/.*")) {
+                sink(source); // $ hasTaintFlow
+                sink(normalized); // $ hasTaintFlow
+            } else {
+                sink(source); // $ hasTaintFlow
+                sink(normalized); // $ hasTaintFlow
+            }
+        }
+    }
+
+    private void dotDotCheckGuardValidation(String path) throws Exception {
+        if (!path.startsWith("/safe") || path.contains(".."))
+            throw new Exception();
+    }
+
+    public void dotDotCheckGuard() throws Exception {
+        // dot dot check by itself is not enough
+        {
+            String source = (String) source();
+            if (!source.contains("..")) {
+                sink(source); // $ hasTaintFlow
+            } else
+                sink(source); // $ hasTaintFlow
+        }
+        // allowListGuard + dotDotCheckGuard
+        {
+            String source = (String) source();
+            if (source.startsWith("/safe") && !source.contains(".."))
+                sink(source); // Safe
+            else
+                sink(source); // $ hasTaintFlow
+        }
+        {
+            String source = (String) source();
+            if (source.startsWith("/safe") && source.indexOf("..") == -1)
+                sink(source); // Safe
+            else
+                sink(source); // $ hasTaintFlow
+        }
+        {
+            String source = (String) source();
+            if (!source.startsWith("/safe") || source.indexOf("..") != -1)
+                sink(source); // $ hasTaintFlow
+            else
+                sink(source); // Safe
+        }
+        {
+            String source = (String) source();
+            if (source.startsWith("/safe") && source.lastIndexOf("..") == -1)
+                sink(source); // Safe
+            else
+                sink(source); // $ hasTaintFlow
+        }
+        // blockListGuard + dotDotCheckGuard
+        {
+            String source = (String) source();
+            if (!source.startsWith("/data") && !source.contains(".."))
+                sink(source); // Safe
+            else
+                sink(source); // $ hasTaintFlow
+        }
+        {
+            String source = (String) source();
+            if (!source.startsWith("/data") && source.indexOf("..") == -1)
+                sink(source); // Safe
+            else
+                sink(source); // $ hasTaintFlow
+        }
+        {
+            String source = (String) source();
+            if (!source.startsWith("/data") && source.indexOf("..") == -1)
+                sink(source); // Safe
+            else
+                sink(source); // $ hasTaintFlow
+        }
+        {
+            String source = (String) source();
+            if (!source.startsWith("/data") && source.lastIndexOf("..") == -1)
+                sink(source); // Safe
+            else
+                sink(source); // $ hasTaintFlow
+        }
+        // validation method
+        {
+            String source = (String) source();
+            dotDotCheckGuardValidation(source);
+            sink(source); // Safe
+        }
+    }
+
+    private void blockListGuardValidation(String path) throws Exception {
+        if (path.contains("..") || !path.startsWith("/data"))
+            throw new Exception();
+    }
+
+    public void blockListGuard() throws Exception {
+        // Prefix check by itself is not enough
+        {
+            String source = (String) source();
+            if (!source.startsWith("/data")) {
+                sink(source); // $ hasTaintFlow
+            } else
+                sink(source); // $ hasTaintFlow
+        }
+        // PathTraversalGuard + blockListGuard
+        {
+            String source = (String) source();
+            if (!source.contains("..") && !source.startsWith("/data"))
+                sink(source); // Safe
+            else
+                sink(source); // $ hasTaintFlow
+        }
+        {
+            String source = (String) source();
+            if (source.indexOf("..") == -1 && !source.startsWith("/data"))
+                sink(source); // Safe
+            else
+                sink(source); // $ hasTaintFlow
+        }
+        {
+            String source = (String) source();
+            if (source.lastIndexOf("..") == -1 && !source.startsWith("/data"))
+                sink(source); // Safe
+            else
+                sink(source); // $ hasTaintFlow
+        }
+        // PathTraversalSanitizer + blockListGuard
+        {
+            File source = (File) source();
+            String normalized = source.getCanonicalPath();
+            if (!normalized.startsWith("/data")) {
+                sink(source); // Safe
+                sink(normalized); // Safe
+            } else {
+                sink(source); // $ hasTaintFlow
+                sink(normalized); // $ hasTaintFlow
+            }
+        }
+        {
+            File source = (File) source();
+            String normalized = source.getCanonicalFile().toString();
+            if (!normalized.startsWith("/data")) {
+                sink(source); // Safe
+                sink(normalized); // Safe
+            } else {
+                sink(source); // $ hasTaintFlow
+                sink(normalized); // $ hasTaintFlow
+            }
+        }
+        {
+            String source = (String) source();
+            Path normalized = Paths.get(source).normalize();
+            if (!normalized.startsWith("/data")) {
+                sink(source); // Safe
+                sink(normalized); // Safe
+            } else {
+                sink(source); // $ hasTaintFlow
+                sink(normalized); // $ hasTaintFlow
+            }
+        }
+        {
+            String source = (String) source();
+            String normalized = Paths.get(source).normalize().toString();
+            if (!normalized.startsWith("/data")) {
+                sink(source); // Safe
+                sink(normalized); // Safe
+            } else {
+                sink(source); // $ hasTaintFlow
+                sink(normalized); // $ hasTaintFlow
+            }
+        }
+        {
+            String source = (String) source();
+            String normalized = Paths.get(source).normalize().toString();
+            if (!normalized.regionMatches(0, "/data", 0, 5)) {
+                sink(source); // Safe
+                sink(normalized); // Safe
+            } else {
+                sink(source); // $ hasTaintFlow
+                sink(normalized); // $ hasTaintFlow
+            }
+        }
+        {
+            String source = (String) source();
+            String normalized = Paths.get(source).normalize().toString();
+            if (!normalized.matches("/data/.*")) {
+                sink(source); // Safe
+                sink(normalized); // Safe
+            } else {
+                sink(source); // $ hasTaintFlow
+                sink(normalized); // $ hasTaintFlow
+            }
+        }
+        // validation method
+        {
+            String source = (String) source();
+            blockListGuardValidation(source);
+            sink(source); // Safe
+        }
+        // PathInjectionSanitizer + partial string match with disallowed words
+        {
+            String source = (String) source();
+            String normalized = Paths.get(source).normalize().toString();
+            if (!normalized.contains("/")) {
+                sink(source); // Safe
+                sink(normalized); // Safe
+            } else {
+                sink(source); // $ hasTaintFlow
+                sink(normalized); // $ hasTaintFlow
+            }
+        }
+        {
+            String source = (String) source();
+            String normalized = Paths.get(source).normalize().toString();
+            if (!normalized.regionMatches(1, "/", 0, 5)) {
+                sink(source); // Safe
+                sink(normalized); // Safe
+            } else {
+                sink(source); // $ hasTaintFlow
+                sink(normalized); // $ hasTaintFlow
+            }
+        }
+        {
+            String source = (String) source();
+            String normalized = Paths.get(source).normalize().toString();
+            if (!normalized.matches("/")) {
+                sink(source); // Safe
+                sink(normalized); // Safe
+            } else {
+                sink(source); // $ hasTaintFlow
+                sink(normalized); // $ hasTaintFlow
+            }
+        }
+        // PathInjectionSanitizer + partial string match with prefixes is considered unsafe
+        {
+            String source = (String) source();
+            String normalized = Paths.get(source).normalize().toString();
+            if (normalized.contains("/data")) {
+                sink(source); // $ hasTaintFlow
+                sink(normalized); // $ hasTaintFlow
+            } else {
+                sink(source); // $ hasTaintFlow
+                sink(normalized); // $ hasTaintFlow
+            }
+        }
+        {
+            String source = (String) source();
+            String normalized = Paths.get(source).normalize().toString();
+            if (normalized.regionMatches(1, "/data", 0, 5)) {
+                sink(source); // $ hasTaintFlow
+                sink(normalized); // $ hasTaintFlow
+            } else {
+                sink(source); // $ hasTaintFlow
+                sink(normalized); // $ hasTaintFlow
+            }
+        }
+        {
+            String source = (String) source();
+            String normalized = Paths.get(source).normalize().toString();
+            if (normalized.matches(".*/data/.*")) {
+                sink(source); // $ hasTaintFlow
+                sink(normalized); // $ hasTaintFlow
+            } else {
+                sink(source); // $ hasTaintFlow
+                sink(normalized); // $ hasTaintFlow
+            }
+        }
+    }
+}

java/ql/test/library-tests/pathsanitizer/options

@@ -0,0 +1 @@
+//semmle-extractor-options: --javac-args -cp ${testdir}/../../stubs/google-android-9.0.0

java/ql/test/library-tests/pathsanitizer/test.ql

@@ -0,0 +1,15 @@
+import java
+import semmle.code.java.security.PathSanitizer
+import TestUtilities.InlineFlowTest
+
+class PathSanitizerConf extends DefaultTaintFlowConf {
+  override predicate isSanitizer(DataFlow::Node sanitizer) {
+    sanitizer instanceof PathInjectionSanitizer
+  }
+}
+
+class Test extends InlineFlowTest {
+  override DataFlow::Configuration getValueFlowConfig() { none() }
+
+  override DataFlow::Configuration getTaintFlowConfig() { result = any(PathSanitizerConf config) }
+}