Use PathInjectionSanitizer in relevant queries

Author: atorralba

java/ql/src/Security/CWE/CWE-022/TaintedPath.ql

@@ -17,36 +17,26 @@ import java
 import semmle.code.java.dataflow.FlowSources
 private import semmle.code.java.dataflow.ExternalFlow
 import semmle.code.java.security.PathCreation
+import semmle.code.java.security.PathSanitizer
 import DataFlow::PathGraph
 import TaintedPathCommon
 
-predicate containsDotDotSanitizer(Guard g, Expr e, boolean branch) {
-  exists(MethodAccess contains | g = contains |
-    contains.getMethod().hasName("contains") and
-    contains.getAnArgument().(StringLiteral).getValue() = ".." and
-    e = contains.getQualifier() and
-    branch = false
-  )
-}
-
 class TaintedPathConfig extends TaintTracking::Configuration {
   TaintedPathConfig() { this = "TaintedPathConfig" }
 
   override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
 
   override predicate isSink(DataFlow::Node sink) {
-    (
-      sink.asExpr() = any(PathCreation p).getAnInput()
-      or
-      sinkNode(sink, "create-file")
-    ) and
-    not guarded(sink.asExpr())
+    sink.asExpr() = any(PathCreation p).getAnInput()
+    or
+    sinkNode(sink, "create-file")
   }
 
-  override predicate isSanitizer(DataFlow::Node node) {
-    exists(Type t | t = node.getType() | t instanceof BoxedType or t instanceof PrimitiveType)
-    or
-    node = DataFlow::BarrierGuard<containsDotDotSanitizer/3>::getABarrierNode()
+  override predicate isSanitizer(DataFlow::Node sanitizer) {
+    sanitizer.getType() instanceof BoxedType or
+    sanitizer.getType() instanceof PrimitiveType or
+    sanitizer.getType() instanceof NumberType or
+    sanitizer instanceof PathInjectionSanitizer
   }
 
   override predicate isAdditionalTaintStep(DataFlow::Node n1, DataFlow::Node n2) {

java/ql/src/Security/CWE/CWE-022/TaintedPathCommon.qll

@@ -3,8 +3,6 @@
  */
 
 import java
-import semmle.code.java.controlflow.Guards
-import semmle.code.java.security.PathCreation
 import semmle.code.java.frameworks.Networking
 import semmle.code.java.dataflow.DataFlow
 
@@ -48,29 +46,3 @@ private class TaintPreservingUriCtorParam extends Parameter {
     )
   }
 }
-
-private predicate inWeakCheck(Expr e) {
-  // None of these are sufficient to guarantee that a string is safe.
-  exists(MethodAccess m, Method def | m.getQualifier() = e and m.getMethod() = def |
-    def.getName() = "startsWith" or
-    def.getName() = "endsWith" or
-    def.getName() = "isEmpty" or
-    def.getName() = "equals"
-  )
-  or
-  // Checking against `null` has no bearing on path traversal.
-  exists(EqualityTest b | b.getAnOperand() = e | b.getAnOperand() instanceof NullLiteral)
-}
-
-// Ignore cases where the variable has been checked somehow,
-// but allow some particularly obviously bad cases.
-predicate guarded(VarAccess e) {
-  exists(PathCreation p | e = p.getAnInput()) and
-  exists(ConditionBlock cb, Expr c |
-    cb.getCondition().getAChildExpr*() = c and
-    c = e.getVariable().getAnAccess() and
-    cb.controls(e.getBasicBlock(), true) and
-    // Disallow a few obviously bad checks.
-    not inWeakCheck(c)
-  )
-}

java/ql/src/Security/CWE/CWE-022/TaintedPathLocal.ql

@@ -17,6 +17,7 @@ import java
 import semmle.code.java.dataflow.FlowSources
 private import semmle.code.java.dataflow.ExternalFlow
 import semmle.code.java.security.PathCreation
+import semmle.code.java.security.PathSanitizer
 import DataFlow::PathGraph
 import TaintedPathCommon
 
@@ -26,12 +27,16 @@ class TaintedPathLocalConfig extends TaintTracking::Configuration {
   override predicate isSource(DataFlow::Node source) { source instanceof LocalUserInput }
 
   override predicate isSink(DataFlow::Node sink) {
-    (
-      sink.asExpr() = any(PathCreation p).getAnInput()
-      or
-      sinkNode(sink, "create-file")
-    ) and
-    not guarded(sink.asExpr())
+    sink.asExpr() = any(PathCreation p).getAnInput()
+    or
+    sinkNode(sink, "create-file")
+  }
+
+  override predicate isSanitizer(DataFlow::Node sanitizer) {
+    sanitizer.getType() instanceof BoxedType or
+    sanitizer.getType() instanceof PrimitiveType or
+    sanitizer.getType() instanceof NumberType or
+    sanitizer instanceof PathInjectionSanitizer
   }
 
   override predicate isAdditionalTaintStep(DataFlow::Node n1, DataFlow::Node n2) {

java/ql/src/Security/CWE/CWE-022/ZipSlip.ql

@@ -16,6 +16,7 @@ import java
 import semmle.code.java.controlflow.Guards
 import semmle.code.java.dataflow.SSA
 import semmle.code.java.dataflow.TaintTracking
+import semmle.code.java.security.PathSanitizer
 import DataFlow
 import PathGraph
 private import semmle.code.java.dataflow.ExternalFlow
@@ -132,6 +133,7 @@ class ZipSlipConfiguration extends TaintTracking::Configuration {
   }
 
   override predicate isSanitizer(Node node) {
+    // TODO: Merge this sanitizers into PathInjectionSanitizer
     exists(Guard g, SsaVariable var, RValue varuse | validateFilePath(var, g) |
       varuse = node.asExpr() and
       varuse = var.getAUse() and
@@ -144,6 +146,8 @@ class ZipSlipConfiguration extends TaintTracking::Configuration {
       adjacentUseUseSameVar(rv, node.asExpr()) and
       ma.getBasicBlock().bbDominates(node.asExpr().getBasicBlock())
     )
+    or
+    node instanceof PathInjectionSanitizer
   }
 }