False Positive - cs/zipslip

csharp/ql/lib/semmle/code/csharp/security/dataflow/ZipSlipQuery.qll

@@ -51,13 +51,26 @@ private module GetFullPathToQualifierTaintTrackingConfiguration implements DataF
   }
 }
 
+class ZipArchiveEntryClass extends Class{
+  ZipArchiveEntryClass(){
+    this.hasFullyQualifiedName("System.IO.Compression", "ZipArchiveEntry")
+  }
+}
+
+/**
+ * The `FullName` property of `System.IO.Compression.ZipArchiveEntry`.
+ */
+class ZipArchiveEntryFullNameAccess extends Property{
+  ZipArchiveEntryFullNameAccess(){
+      this.getDeclaringType() instanceof ZipArchiveEntryClass and
+      this.getName() = "FullName"
+  }
+}
+
 /** An access to the `FullName` property of a `ZipArchiveEntry`. */
 class ArchiveFullNameSource extends Source {
   ArchiveFullNameSource() {
-    exists(PropertyAccess pa | this.asExpr() = pa |
-      pa.getTarget().getDeclaringType().hasFullyQualifiedName("System.IO.Compression", "ZipArchiveEntry") and
-      pa.getTarget().getName() = "FullName"
-    )
+    exists(ZipArchiveEntryFullNameAccess pa | pa.getAnAccess() = this.asExpr())
   }
 }
 
@@ -125,10 +138,9 @@ private predicate safeCombineGetFullPathSequence(MethodCallGetFullPath mcGetFull
 class RootSanitizerMethodCall extends SanitizerMethodCall {
   RootSanitizerMethodCall() {
     exists(MethodSystemStringStartsWith sm | this.getTarget() = sm) and
-    exists(Expr q, AbstractValue v |
+    exists(Expr q, MethodCallGetFullPath mcGetFullPath |
       this.getQualifier() = q and
-      v.(AbstractValues::BooleanValue).getValue() = true and
-      exists(MethodCallGetFullPath mcGetFullPath | safeCombineGetFullPathSequence(mcGetFullPath, q))
+      safeCombineGetFullPathSequence(mcGetFullPath, q)
     )
   }
 
@@ -179,13 +191,42 @@ private module SanitizedGuardTaintTrackingConfiguration implements DataFlow::Con
   }
 
   predicate isSink(DataFlow::Node sink) {
-    exists(RootSanitizerMethodCall smc |
-      smc.getAnArgument() = sink.asExpr() or
-      smc.getQualifier() = sink.asExpr()
+    exists(RootSanitizerMethodCall smc, Expr e |
+      e = sink.asExpr() and
+      e = [
+        smc.getAnArgument(),
+        smc.getQualifier()
+      ]
     )
   }
 }
 
+/**
+ * A Callable that successfully validates a path will resolve under a given directory,
+ * and if it does not, throws an exception.
+ */
+private class ValidatingCallableThrowing extends Callable{
+  Parameter paramFilename;
+  ValidatingCallableThrowing(){
+    paramFilename = this.getAParameter() and
+    // It passes the guard, contraining the function argument to the Guard argument.
+    exists(ZipSlipGuard g, DataFlow::ParameterNode source, DataFlow::Node sink |
+      g.getEnclosingCallable() = this and
+      source = DataFlow::parameterNode(paramFilename) and
+      sink = DataFlow::exprNode(g.getFilePathArgument()) and
+      SanitizedGuardTT::flow(source, sink) and
+      exists(AbstractValues::BooleanValue bv, ThrowStmt throw |
+        throw.getEnclosingCallable() = this and
+        forall(TryStmt try | try.getEnclosingCallable() = this | not throw.getParent+() = try) and
+        // If there exists a control block that guards against misuse
+        bv.getValue() = false and
+        g.controlsNode(throw.getAControlFlowNode(), bv)
+      )
+    )
+  }
+  Parameter paramFilePath() { result = paramFilename }
+}
+
 /**
  * An AbstractWrapperSanitizerMethod is a Method that
  *  is a suitable sanitizer for a ZipSlip path that may not have been canonicalized prior.
@@ -199,19 +240,12 @@ abstract private class AbstractWrapperSanitizerMethod extends AbstractSanitizerM
 
   AbstractWrapperSanitizerMethod() {
     this.getReturnType() instanceof BoolType and
-    this.getAParameter() = paramFilename
+    paramFilename = this.getAParameter()
   }
 
   Parameter paramFilePath() { result = paramFilename }
 }
 
-/* predicate aaaa(ZipSlipGuard g, DataFlow::ParameterNode source){
-      exists(DataFlow::Node sink |
-        sink = DataFlow::exprNode(g.getFilePathArgument()) and
-        SanitizedGuardTT::flow(source, sink) and
-      )
-} */
-
 /**
  * A DirectWrapperSantizierMethod is a Method where
  *      The function can /only/ returns true when passes through the RootSanitizerGuard
@@ -350,6 +384,17 @@ class WrapperCheckSanitizer extends Sanitizer {
   WrapperCheckSanitizer() { this = DataFlow::BarrierGuard<wrapperCheckGuard/3>::getABarrierNode() }
 }
 
+/**
+ * A Call to `ValidatingCallableThrowing` which acts as a barrier in a DataFlow
+ */
+class ValidatingCallableThrowingSanitizer extends Sanitizer {
+  ValidatingCallableThrowingSanitizer(){
+    exists(ValidatingCallableThrowing validator, Call validatorCall | validatorCall = validator.getACall() |
+      this = DataFlow::exprNode(validatorCall.getAnArgument())
+    )
+  }
+}
+
 /**
  * A data flow source for unsafe zip extraction.
  */

csharp/ql/test/query-tests/Security Features/CWE-022/ZipSlip/ZipSlip.cs

@@ -196,6 +196,22 @@ public static bool ContainsPath(string? fullPath, string? path, bool excludeSame
             }
         }
 
+        /* Test that the given `fullPath` exists within the given `path` directory.
+         * If it does not, throw an exception to terminate the request.
+        */
+        public static void ContainsPathValidationThrowing(string? fullPath, string? path)
+        {
+            fullPath = Path.GetFullPath(fullPath);
+            path = Path.GetFullPath(path);
+
+            fullPath = AddBackslashIfNotPresent(fullPath);
+            path = AddBackslashIfNotPresent(path);
+
+            if (!fullPath!.StartsWith(path, StringComparison.OrdinalIgnoreCase)) {
+                throw new Exception("Attempting path traversal");
+            }
+        }
+
         static void Main(string[] args)
         {
             string zipFileName;
@@ -231,5 +247,66 @@ static void fp_throw(ZipArchive archive, string root){
                 entry.ExtractToFile(destinationOnDisk, true);
             }
         }
+
+        /**
+        * Negative - dangerous path terminates early due to exception thrown by guarded condition in descendent call.
+        */
+        static void fp_throw_nested_exception(ZipArchive archive, string root){
+            foreach (var entry in archive.Entries){
+                string destinationOnDisk = Path.GetFullPath(Path.Combine(root, entry.FullName));
+                string fullRoot = Path.GetFullPath(root + Path.DirectorySeparatorChar);
+                ContainsPathValidationThrowing(destinationOnDisk, fullRoot);
+                // NEGATIVE, above exception short circuits by exception on invalid input by path traversal.
+                entry.ExtractToFile(destinationOnDisk, true);
+            }
+        }
+
+        /**
+        * Negative - no extraction, only sanitization
+        */
+        static void fp_throw_sanitizer_valid(string file, string root){
+            string destinationOnDisk = Path.GetFullPath(file);
+            string fullRoot = Path.GetFullPath(root + Path.DirectorySeparatorChar);
+            if (!destinationOnDisk.StartsWith(fullRoot)){
+                throw new Exception("Entry is outside of target directory. There may have been some directory traversal sequences in filename.");
+            }
+        }
+
+        /**
+        * Negative - dangerous path terminates early due to throw in fp_throw_sanitizer_valid
+        */
+        static void fp_throw_nested_exception_uncaught(ZipArchive archive, string root){
+            foreach (var entry in archive.Entries){
+                string destinationOnDisk = Path.GetFullPath(Path.Combine(root, entry.FullName));
+                string fullRoot = Path.GetFullPath(root + Path.DirectorySeparatorChar);
+                fp_throw_sanitizer_valid(destinationOnDisk, fullRoot);
+                entry.ExtractToFile(destinationOnDisk, true);
+            }
+        }
+
+        /**
+        * Negative - no extraction, only sanitization
+        */
+        static void fp_throw_sanitizer_invalid(string file, string root){
+            try{
+                string destinationOnDisk = Path.GetFullPath(file);
+                string fullRoot = Path.GetFullPath(root);
+                if (!destinationOnDisk.StartsWith(fullRoot)){
+                    throw new Exception("Entry is outside of target directory. There may have been some directory traversal sequences in filename.");
+                }
+            }catch(Exception e){}
+        }
+
+        /**
+        * Positive - dangerous path does not terminate early due to try block in fp_throw_sanitizer_invalid
+        */
+        static void tp_throw_nested_exception_caught(ZipArchive archive, string root){
+            foreach (var entry in archive.Entries){
+                string destinationOnDisk = Path.GetFullPath(Path.Combine(root, entry.FullName));
+                string fullRoot = Path.GetFullPath(root + Path.DirectorySeparatorChar);
+                fp_throw_sanitizer_invalid(destinationOnDisk, fullRoot);
+                entry.ExtractToFile(destinationOnDisk, true);
+            }
+        }
     }
 }

csharp/ql/test/query-tests/Security Features/CWE-022/ZipSlip/ZipSlip.expected

@@ -8,6 +8,7 @@
 | ZipSlip.cs:105:72:105:85 | access to property FullName | ZipSlip.cs:105:72:105:85 | access to property FullName : String | ZipSlip.cs:119:71:119:82 | access to local variable destFilePath | Unsanitized archive entry, which may contain '..', is used in a $@. | ZipSlip.cs:119:71:119:82 | access to local variable destFilePath | file system operation |
 | ZipSlip.cs:105:72:105:85 | access to property FullName | ZipSlip.cs:105:72:105:85 | access to property FullName : String | ZipSlip.cs:126:57:126:68 | access to local variable destFilePath | Unsanitized archive entry, which may contain '..', is used in a $@. | ZipSlip.cs:126:57:126:68 | access to local variable destFilePath | file system operation |
 | ZipSlip.cs:105:72:105:85 | access to property FullName | ZipSlip.cs:105:72:105:85 | access to property FullName : String | ZipSlip.cs:134:58:134:69 | access to local variable destFilePath | Unsanitized archive entry, which may contain '..', is used in a $@. | ZipSlip.cs:134:58:134:69 | access to local variable destFilePath | file system operation |
+| ZipSlip.cs:305:80:305:93 | access to property FullName | ZipSlip.cs:305:80:305:93 | access to property FullName : String | ZipSlip.cs:308:37:308:53 | access to local variable destinationOnDisk | Unsanitized archive entry, which may contain '..', is used in a $@. | ZipSlip.cs:308:37:308:53 | access to local variable destinationOnDisk | file system operation |
 | ZipSlipBad.cs:9:59:9:72 | access to property FullName | ZipSlipBad.cs:9:59:9:72 | access to property FullName : String | ZipSlipBad.cs:10:29:10:40 | access to local variable destFileName | Unsanitized archive entry, which may contain '..', is used in a $@. | ZipSlipBad.cs:10:29:10:40 | access to local variable destFileName | file system operation |
 edges
 | ZipSlip.cs:15:24:15:40 | access to local variable fullPath_relative : String | ZipSlip.cs:30:71:30:87 | access to local variable fullPath_relative : String | provenance |  |
@@ -47,6 +48,13 @@ edges
 | ZipSlip.cs:121:71:121:82 | access to local variable destFilePath : String | ZipSlip.cs:126:57:126:68 | access to local variable destFilePath | provenance |  |
 | ZipSlip.cs:121:71:121:82 | access to local variable destFilePath : String | ZipSlip.cs:129:71:129:82 | access to local variable destFilePath : String | provenance |  |
 | ZipSlip.cs:129:71:129:82 | access to local variable destFilePath : String | ZipSlip.cs:134:58:134:69 | access to local variable destFilePath | provenance |  |
+| ZipSlip.cs:305:24:305:40 | access to local variable destinationOnDisk : String | ZipSlip.cs:307:44:307:60 | access to local variable destinationOnDisk : String | provenance |  |
+| ZipSlip.cs:305:44:305:95 | call to method GetFullPath : String | ZipSlip.cs:305:24:305:40 | access to local variable destinationOnDisk : String | provenance |  |
+| ZipSlip.cs:305:61:305:94 | call to method Combine : String | ZipSlip.cs:305:44:305:95 | call to method GetFullPath : String | provenance | Config |
+| ZipSlip.cs:305:61:305:94 | call to method Combine : String | ZipSlip.cs:305:44:305:95 | call to method GetFullPath : String | provenance | MaD:2 |
+| ZipSlip.cs:305:80:305:93 | access to property FullName : String | ZipSlip.cs:305:61:305:94 | call to method Combine : String | provenance | Config |
+| ZipSlip.cs:305:80:305:93 | access to property FullName : String | ZipSlip.cs:305:61:305:94 | call to method Combine : String | provenance | MaD:1 |
+| ZipSlip.cs:307:44:307:60 | access to local variable destinationOnDisk : String | ZipSlip.cs:308:37:308:53 | access to local variable destinationOnDisk | provenance |  |
 | ZipSlipBad.cs:9:16:9:27 | access to local variable destFileName : String | ZipSlipBad.cs:10:29:10:40 | access to local variable destFileName | provenance |  |
 | ZipSlipBad.cs:9:31:9:73 | call to method Combine : String | ZipSlipBad.cs:9:16:9:27 | access to local variable destFileName : String | provenance |  |
 | ZipSlipBad.cs:9:59:9:72 | access to property FullName : String | ZipSlipBad.cs:9:31:9:73 | call to method Combine : String | provenance | Config |
@@ -91,6 +99,12 @@ nodes
 | ZipSlip.cs:126:57:126:68 | access to local variable destFilePath | semmle.label | access to local variable destFilePath |
 | ZipSlip.cs:129:71:129:82 | access to local variable destFilePath : String | semmle.label | access to local variable destFilePath : String |
 | ZipSlip.cs:134:58:134:69 | access to local variable destFilePath | semmle.label | access to local variable destFilePath |
+| ZipSlip.cs:305:24:305:40 | access to local variable destinationOnDisk : String | semmle.label | access to local variable destinationOnDisk : String |
+| ZipSlip.cs:305:44:305:95 | call to method GetFullPath : String | semmle.label | call to method GetFullPath : String |
+| ZipSlip.cs:305:61:305:94 | call to method Combine : String | semmle.label | call to method Combine : String |
+| ZipSlip.cs:305:80:305:93 | access to property FullName : String | semmle.label | access to property FullName : String |
+| ZipSlip.cs:307:44:307:60 | access to local variable destinationOnDisk : String | semmle.label | access to local variable destinationOnDisk : String |
+| ZipSlip.cs:308:37:308:53 | access to local variable destinationOnDisk | semmle.label | access to local variable destinationOnDisk |
 | ZipSlipBad.cs:9:16:9:27 | access to local variable destFileName : String | semmle.label | access to local variable destFileName : String |
 | ZipSlipBad.cs:9:31:9:73 | call to method Combine : String | semmle.label | call to method Combine : String |
 | ZipSlipBad.cs:9:59:9:72 | access to property FullName : String | semmle.label | access to property FullName : String |