Java: test perf with arg

Author: Jami Cogswell

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

@@ -354,42 +354,35 @@ private class FileGetNameSanitizer extends PathInjectionSanitizer {
 }
 
 /** Holds if `expr` may be null. */
-private predicate maybeNullArg(Expr expr) {
-  exists(DataFlow::Node src, DataFlow::Node sink, ConstructorCall constrCall |
-    constrCall.getConstructedType() instanceof TypeFile and
-    constrCall.getNumArgument() = 2 and
-    expr = constrCall.getArgument(0) and
+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 nodes. */
-private module TaintedConfig implements DataFlow::ConfigSig {
-  predicate isSource(DataFlow::Node source) {
-    source instanceof ActiveThreatModelSource
-    or
-    // ZipSlip uses ApiSourceNode instead of ActiveThreatModelSource
-    source instanceof ApiSourceNode
-  }
-
-  predicate isSink(DataFlow::Node sink) {
-    exists(ConstructorCall constrCall |
-      constrCall.getConstructedType() instanceof TypeFile and
-      constrCall.getNumArgument() = 2 and
-      sink.asExpr() = constrCall.getArgument(0)
-    )
-  }
-}
-
-/** Tracks flow from any `ActiveThreatModelSource` to any node. */
-private module TaintedFlow = TaintTracking::Global<TaintedConfig>;
-
-/** Holds if `expr is tainted by an `ActiveThreatModelSource`. */
-private predicate isTaintedArg(Expr expr) { TaintedFlow::flowToExpr(expr) }
-
+// /** A taint-tracking configuration for reasoning about tainted nodes. */
+// private module TaintedConfig implements DataFlow::ConfigSig {
+//   predicate isSource(DataFlow::Node source) {
+//     source instanceof ActiveThreatModelSource
+//     or
+//     // ZipSlip uses ApiSourceNode instead of ActiveThreatModelSource
+//     source instanceof ApiSourceNode
+//   }
+//   predicate isSink(DataFlow::Node sink) {
+//     exists(ConstructorCall constrCall |
+//       constrCall.getConstructedType() instanceof TypeFile and
+//       constrCall.getNumArgument() = 2 and
+//       sink.asExpr() = constrCall.getArgument(0)
+//     )
+//   }
+// }
+// /** Tracks flow from any `ActiveThreatModelSource` to any node. */
+// private module TaintedFlow = TaintTracking::Global<TaintedConfig>;
+// /** Holds if `expr is tainted by an `ActiveThreatModelSource`. */
+// private predicate isTaintedArg(Expr expr) { TaintedFlow::flowToExpr(expr) }
 /** Holds if `g` is a guard that checks for `..` components. */
 private predicate pathTraversalGuard(Guard g, Expr e, boolean branch) {
   branch = g.(PathTraversalGuard).getBranch() and
@@ -408,15 +401,15 @@ private class FileConstructorSanitizer extends PathInjectionSanitizer {
       // 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 maybeNullArg(constrCall.getArgument(0)) and
-      not isTaintedArg(constrCall.getArgument(0)) and
+      not maybeNull(constrCall.getArgument(0)) and
+      // not isTaintedArg(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
+      this.asExpr() = arg
     )
   }
 }

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

@@ -483,7 +483,7 @@ public void fileConstructorSanitizer() throws Exception {
             if (!source.contains("..")) {
                 File f2 = new File(f1, source);
                 sink(f2); // Safe
-                sink(source); // $ hasTaintFlow
+                sink(source); // $ MISSING: hasTaintFlow
             } else {
                 File f3 = new File(f1, source);
                 sink(f3); // $ hasTaintFlow
@@ -502,9 +502,9 @@ public void fileConstructorSanitizer() throws Exception {
 
             if (!source.contains("..")) {
                 // `f2` is unsafe if `f1` is tainted
-                File f2 = new File(f1Tainted, source);
+                File f2 = new File((File) source(), source);
                 sink(f2); // $ hasTaintFlow
-                sink(source); // $ hasTaintFlow
+                sink(source); // $ MISSING: hasTaintFlow
             } else {
                 File f3 = new File(f1Tainted, source);
                 sink(f3); // $ hasTaintFlow
@@ -531,7 +531,7 @@ public void fileConstructorSanitizer() throws Exception {
             if (source.indexOf("..") == -1) {
                 File f2 = new File(f1, source);
                 sink(f2); // Safe
-                sink(source); // $ hasTaintFlow
+                sink(source); // $ MISSING: hasTaintFlow
             } else {
                 File f3 = new File(f1, source);
                 sink(f3); // $ hasTaintFlow
@@ -548,7 +548,7 @@ public void fileConstructorSanitizer() throws Exception {
             } else {
                 File f3 = new File(f1, source);
                 sink(f3); // Safe
-                sink(source); // $ hasTaintFlow
+                sink(source); // $ MISSING: hasTaintFlow
             }
         }
         {
@@ -557,7 +557,7 @@ public void fileConstructorSanitizer() throws Exception {
             if (source.lastIndexOf("..") == -1) {
                 File f2 = new File(f1, source);
                 sink(f2); // Safe
-                sink(source); // $ hasTaintFlow
+                sink(source); // $ MISSING: hasTaintFlow
             } else {
                 File f3 = new File(f1, source);
                 sink(f3); // $ hasTaintFlow
@@ -571,7 +571,7 @@ public void fileConstructorSanitizer() throws Exception {
             fileConstructorValidation(source);
             File f2 = new File(f1, source);
             sink(f2); // Safe
-            sink(source); // $ hasTaintFlow
+            sink(source); // $ MISSING: hasTaintFlow
         }
         {
             String source = (String) source();
@@ -582,7 +582,7 @@ public void fileConstructorSanitizer() throws Exception {
             } else {
                 File f2 = new File(f1, source);
                 sink(f2); // Safe
-                sink(source); // $ hasTaintFlow
+                sink(source); // $ MISSING: hasTaintFlow
             }
         }
         // PathNormalizeSanitizer
@@ -593,7 +593,7 @@ public void fileConstructorSanitizer() throws Exception {
             File f2 = new File(f1, normalized);
             sink(f2); // Safe
             sink(source); // $ hasTaintFlow
-            sink(normalized); // $ hasTaintFlow
+            sink(normalized); // $ MISSING: hasTaintFlow
         }
         {
             File source = (File) source();
@@ -602,7 +602,7 @@ public void fileConstructorSanitizer() throws Exception {
             File f2 = new File(f1, normalized);
             sink(f2); // Safe
             sink(source); // $ hasTaintFlow
-            sink(normalized); // $ hasTaintFlow
+            sink(normalized); // $ MISSING: hasTaintFlow
         }
         {
             String source = (String) source();
@@ -611,7 +611,7 @@ public void fileConstructorSanitizer() throws Exception {
             File f2 = new File(f1, normalized);
             sink(f2); // Safe
             sink(source); // $ hasTaintFlow
-            sink(normalized); // $ hasTaintFlow
+            sink(normalized); // $ MISSING: hasTaintFlow
         }
     }
 }