Java: File constructor path sanitizer

java/ql/lib/change-notes/2025-01-16-file-constructor-sanitizer.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* Added a path injection sanitizer for the `child` argument of a `java.io.File` constructor if that argument does not contain path traversal sequences.

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

@@ -6,6 +6,7 @@ private import semmle.code.java.dataflow.FlowSources
 private import semmle.code.java.dataflow.SSA
 private import semmle.code.java.frameworks.kotlin.IO
 private import semmle.code.java.frameworks.kotlin.Text
+private import semmle.code.java.dataflow.Nullness
 
 /** A sanitizer that protects against path injection vulnerabilities. */
 abstract class PathInjectionSanitizer extends DataFlow::Node { }
@@ -137,14 +138,7 @@ private class AllowedPrefixSanitizer extends PathInjectionSanitizer {
  * been checked for a trusted prefix.
  */
 private predicate dotDotCheckGuard(Guard g, Expr e, boolean branch) {
-  // Local taint-flow is used here to handle cases where the validated expression comes from the
-  // expression reaching the sink, but it's not the same one, e.g.:
-  //  Path path = source();
-  //  String strPath = path.toString();
-  //  if (!strPath.contains("..") && strPath.startsWith("/safe/dir"))
-  //    sink(path);
-  branch = g.(PathTraversalGuard).getBranch() and
-  localTaintFlowToPathGuard(e, g) and
+  pathTraversalGuard(g, e, branch) and
   exists(Guard previousGuard |
     previousGuard.(AllowedPrefixGuard).controls(g.getBasicBlock(), true)
     or
@@ -352,3 +346,70 @@ private class FileGetNameSanitizer extends PathInjectionSanitizer {
     )
   }
 }
+
+/** Holds if `expr` may be null. */
+private predicate maybeNull(Expr expr) {
+  exists(DataFlow::Node src, DataFlow::Node sink |
+    src.asExpr() = nullExpr() and
+    sink.asExpr() = expr
+  |
+    DataFlow::localFlow(src, sink)
+  )
+}
+
+/** A taint-tracking configuration for reasoning about tainted arguments. */
+private module TaintedArgConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node src) {
+    src instanceof ActiveThreatModelSource or
+    src instanceof ApiSourceNode or
+    // for InlineFlowTest
+    src.asExpr().(MethodCall).getMethod().getName() = "source"
+  }
+
+  predicate isSink(DataFlow::Node sink) { exists(Call call | sink.asExpr() = call.getAnArgument()) }
+}
+
+/** Tracks taint flow to any argument. */
+private module TaintedArgFlow = TaintTracking::Global<TaintedArgConfig>;
+
+/** Holds if `g` is a guard that checks for `..` components. */
+private predicate pathTraversalGuard(Guard g, Expr e, boolean branch) {
+  // Local taint-flow is used here to handle cases where the validated expression comes from the
+  // expression reaching the sink, but it's not the same one, e.g.:
+  //  Path path = source();
+  //  String strPath = path.toString();
+  //  if (!strPath.contains("..") && strPath.startsWith("/safe/dir"))
+  //    sink(path);
+  branch = g.(PathTraversalGuard).getBranch() and
+  localTaintFlowToPathGuard(e, g)
+}
+
+/**
+ * A sanitizer that considers a `File` constructor safe if its second argument
+ * is checked for `..` components (`PathTraversalGuard`) or if any internal
+ * `..` components are removed from it (`PathNormalizeSanitizer`).
+ *
+ * This also requires a check to ensure that the first argument of the
+ * `File` constructor is not tainted.
+ */
+private class FileConstructorSanitizer extends PathInjectionSanitizer {
+  FileConstructorSanitizer() {
+    exists(ConstructorCall constrCall, Argument arg |
+      constrCall.getConstructedType() instanceof TypeFile and
+      // Exclude cases where the parent argument is null since the
+      // `java.io.File` documentation states that such cases are
+      // treated as if invoking the single-argument `File` constructor.
+      not maybeNull(constrCall.getArgument(0)) and
+      // Since we are sanitizing the constructor call, we need to check
+      // that the parent argument is not tainted.
+      not TaintedArgFlow::flowToExpr(constrCall.getArgument(0)) and
+      arg = constrCall.getArgument(1) and
+      (
+        arg = DataFlow::BarrierGuard<pathTraversalGuard/3>::getABarrierNode().asExpr() or
+        arg = ValidationMethod<pathTraversalGuard/3>::getAValidatedNode().asExpr() or
+        TaintTracking::localExprTaint(any(PathNormalizeSanitizer p), arg)
+      ) and
+      this.asExpr() = constrCall
+    )
+  }
+}

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

@@ -3,6 +3,9 @@
 import java.nio.file.Path;
 import java.nio.file.Paths;
 import android.net.Uri;
+import java.io.BufferedReader;
+import java.io.InputStreamReader;
+import java.net.Socket;
 
 public class Test {
 
@@ -463,4 +466,145 @@ public void blockListGuard() throws Exception {
             }
         }
     }
+
+    private void fileConstructorValidation(String path) throws Exception {
+        // Use `indexOf` instead of `contains` for this test since a `contains`
+        // call in this scenario will already be sanitized due to the inclusion
+        // of `ValidatedVariableAccess` nodes in `defaultTaintSanitizer`.
+        if (path.indexOf("..") != -1)
+            throw new Exception();
+    }
+
+    public void fileConstructorSanitizer() throws Exception {
+        // PathTraversalGuard
+        {
+            String source = (String) source();
+            File f1 = new File("safe/file.txt");
+            if (!source.contains("..")) {
+                File f2 = new File(f1, source);
+                sink(f2); // Safe
+                sink(source); // $ hasTaintFlow
+            } else {
+                File f3 = new File(f1, source);
+                sink(f3); // $ hasTaintFlow
+                sink(source); // $ hasTaintFlow
+            }
+        }
+        {
+            String source = (String) source();
+            File f1Tainted = (File) source();
+            if (!source.contains("..")) {
+                // `f2` is unsafe if `f1` is tainted
+                File f2 = new File(f1Tainted, source);
+                sink(f2); // $ hasTaintFlow
+                sink(source); // $ hasTaintFlow
+            } else {
+                File f3 = new File(f1Tainted, source);
+                sink(f3); // $ hasTaintFlow
+                sink(source); // $ hasTaintFlow
+            }
+        }
+        {
+            String source = (String) source();
+            File f1Null = null;
+            if (!source.contains("..")) {
+                // `f2` is unsafe if `f1` is null
+                File f2 = new File(f1Null, source);
+                sink(f2); // $ hasTaintFlow
+                sink(source); // $ hasTaintFlow
+            } else {
+                File f3 = new File(f1Null, source);
+                sink(f3); // $ hasTaintFlow
+                sink(source); // $ hasTaintFlow
+            }
+        }
+        {
+            String source = (String) source();
+            File f1 = new File("safe/file.txt");
+            if (source.indexOf("..") == -1) {
+                File f2 = new File(f1, source);
+                sink(f2); // Safe
+                sink(source); // $ hasTaintFlow
+            } else {
+                File f3 = new File(f1, source);
+                sink(f3); // $ hasTaintFlow
+                sink(source); // $ hasTaintFlow
+            }
+        }
+        {
+            String source = (String) source();
+            File f1 = new File("safe/file.txt");
+            if (source.indexOf("..") != -1) {
+                File f2 = new File(f1, source);
+                sink(f2); // $ hasTaintFlow
+                sink(source); // $ hasTaintFlow
+            } else {
+                File f3 = new File(f1, source);
+                sink(f3); // Safe
+                sink(source); // $ hasTaintFlow
+            }
+        }
+        {
+            String source = (String) source();
+            File f1 = new File("safe/file.txt");
+            if (source.lastIndexOf("..") == -1) {
+                File f2 = new File(f1, source);
+                sink(f2); // Safe
+                sink(source); // $ hasTaintFlow
+            } else {
+                File f3 = new File(f1, source);
+                sink(f3); // $ hasTaintFlow
+                sink(source); // $ hasTaintFlow
+            }
+        }
+        // validation method
+        {
+            String source = (String) source();
+            File f1 = new File("safe/file.txt");
+            fileConstructorValidation(source);
+            File f2 = new File(f1, source);
+            sink(f2); // Safe
+            sink(source); // $ hasTaintFlow
+        }
+        {
+            String source = (String) source();
+            File f1 = new File("safe/file.txt");
+
+            if (source.contains("..")) {
+                throw new Exception();
+            } else {
+                File f2 = new File(f1, source);
+                sink(f2); // Safe
+                sink(source); // $ hasTaintFlow
+            }
+        }
+        // PathNormalizeSanitizer
+        {
+            File source = (File) source();
+            String normalized = source.getCanonicalPath();
+            File f1 = new File("safe/file.txt");
+            File f2 = new File(f1, normalized);
+            sink(f2); // Safe
+            sink(source); // $ hasTaintFlow
+            sink(normalized); // $ hasTaintFlow
+        }
+        {
+            File source = (File) source();
+            String normalized = source.getCanonicalFile().toString();
+            File f1 = new File("safe/file.txt");
+            File f2 = new File(f1, normalized);
+            sink(f2); // Safe
+            sink(source); // $ hasTaintFlow
+            sink(normalized); // $ hasTaintFlow
+        }
+        {
+            String source = (String) source();
+            String normalized = Paths.get(source).normalize().toString();
+            File f1 = new File("safe/file.txt");
+            File f2 = new File(f1, normalized);
+            sink(f2); // Safe
+            sink(source); // $ hasTaintFlow
+            sink(normalized); // $ hasTaintFlow
+        }
+    }
 }