C#: ZipSlip - Refine StartsWith sanitizer.

ZipSlip can be avoided by checking that the combined and resolved
path `StartsWith` the appropriate destination directory. Refine the
`StartsWith` sanitizer to:

 * Consider expressions guarded by an appropriate StartsWith check to be
sanitized.
 * Consider a StartsWith check to be inappropriate if it is checking the
result of `Path.Combine`, as that has not been appropriately resolved.

Tests have been updated to reflect this refinement.

Author: lcartey

csharp/ql/src/semmle/code/csharp/security/dataflow/ZipSlip.qll

@@ -4,6 +4,8 @@
 import csharp
 
 module ZipSlip {
+  import semmle.code.csharp.controlflow.Guards
+
   /**
    * A data flow source for unsafe zip extraction.
    */
@@ -125,16 +127,26 @@ module ZipSlip {
   }
 
   /**
-   * A qualifier in a call to `StartsWith` string method.
+   * An expression which is guarded by a call to `StartsWith`.
    *
    * A call to a String method such as `StartsWith` can indicate a check for a
    * relative path, or a check against the destination folder for whitelisted/target path, etc.
    */
   class StringCheckSanitizer extends Sanitizer {
     StringCheckSanitizer() {
-      exists(MethodCall mc |
-        mc.getTarget().hasQualifiedName("System.String", "StartsWith") |
-        this.asExpr() = mc.getQualifier()
+      exists(GuardedExpr ge, MethodCall mc, Expr startsWithQualifier |
+        ge = this.asExpr() and
+        ge.isGuardedBy(mc, startsWithQualifier, true) |
+        mc.getTarget().hasQualifiedName("System.String", "StartsWith") and
+        mc.getQualifier() = startsWithQualifier and
+        /*
+         * A StartsWith check against Path.Combine is not sufficient, because the ".." elements have
+         * not yet been resolved.
+         */
+        not exists(MethodCall combineCall |
+          combineCall.getTarget().hasQualifiedName("System.IO.Path", "Combine") and
+          DataFlow::localFlow(DataFlow::exprNode(combineCall), DataFlow::exprNode(startsWithQualifier))
+        )
       )
     }
   }

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

@@ -31,7 +31,15 @@ public static void UnzipFileByFile(ZipArchive archive,
                     string destFilePath = Path.Combine(destDirectory, fullPath);
                     entry.ExtractToFile(destFilePath, true);
 
-                    // GOOD: some check on destination.
+                    // BAD: destFilePath isn't fully resolved, so may still contain ..
+                    if (destFilePath.StartsWith(destDirectory))
+                        entry.ExtractToFile(destFilePath, true);
+
+                    // BAD
+                    destFilePath = Path.GetFullPath(Path.Combine(destDirectory, fullPath));
+                    entry.ExtractToFile(destFilePath, true);
+
+                    // GOOD: a check for StartsWith against a fully resolved path
                     if (destFilePath.StartsWith(destDirectory))
                         entry.ExtractToFile(destFilePath, true);
                 }
@@ -115,7 +123,7 @@ static void Main(string[] args)
                     // GOOD: the path is checked in this extension method
                     archive.ExtractToDirectory(targetPath);
 
-		    UnzipToStream(file, targetPath);
+                    UnzipToStream(file, targetPath);
                 }
             }
         }

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

@@ -1,7 +1,9 @@
 | ZipSlip.cs:24:41:24:52 | access to local variable destFileName | Unsanitized zip archive $@, which may contain '..', is used in a file system operation. | ZipSlip.cs:19:31:19:44 | access to property FullName | item path |
 | ZipSlip.cs:32:41:32:52 | access to local variable destFilePath | Unsanitized zip archive $@, which may contain '..', is used in a file system operation. | ZipSlip.cs:16:52:16:65 | access to property FullName | item path |
-| ZipSlip.cs:61:74:61:85 | access to local variable destFilePath | Unsanitized zip archive $@, which may contain '..', is used in a file system operation. | ZipSlip.cs:54:72:54:85 | access to property FullName | item path |
-| ZipSlip.cs:68:71:68:82 | access to local variable destFilePath | Unsanitized zip archive $@, which may contain '..', is used in a file system operation. | ZipSlip.cs:54:72:54:85 | access to property FullName | item path |
-| ZipSlip.cs:75:57:75:68 | access to local variable destFilePath | Unsanitized zip archive $@, which may contain '..', is used in a file system operation. | ZipSlip.cs:54:72:54:85 | access to property FullName | item path |
-| ZipSlip.cs:83:58:83:69 | access to local variable destFilePath | Unsanitized zip archive $@, which may contain '..', is used in a file system operation. | ZipSlip.cs:54:72:54:85 | access to property FullName | item path |
+| ZipSlip.cs:36:45:36:56 | access to local variable destFilePath | Unsanitized zip archive $@, which may contain '..', is used in a file system operation. | ZipSlip.cs:16:52:16:65 | access to property FullName | item path |
+| ZipSlip.cs:40:41:40:52 | access to local variable destFilePath | Unsanitized zip archive $@, which may contain '..', is used in a file system operation. | ZipSlip.cs:16:52:16:65 | access to property FullName | item path |
+| ZipSlip.cs:69:74:69:85 | access to local variable destFilePath | Unsanitized zip archive $@, which may contain '..', is used in a file system operation. | ZipSlip.cs:62:72:62:85 | access to property FullName | item path |
+| ZipSlip.cs:76:71:76:82 | access to local variable destFilePath | Unsanitized zip archive $@, which may contain '..', is used in a file system operation. | ZipSlip.cs:62:72:62:85 | access to property FullName | item path |
+| ZipSlip.cs:83:57:83:68 | access to local variable destFilePath | Unsanitized zip archive $@, which may contain '..', is used in a file system operation. | ZipSlip.cs:62:72:62:85 | access to property FullName | item path |
+| ZipSlip.cs:91:58:91:69 | access to local variable destFilePath | Unsanitized zip archive $@, which may contain '..', is used in a file system operation. | ZipSlip.cs:62:72:62:85 | access to property FullName | item path |
 | ZipSlipBad.cs:10:29:10:40 | access to local variable destFileName | Unsanitized zip archive $@, which may contain '..', is used in a file system operation. | ZipSlipBad.cs:9:59:9:72 | access to property FullName | item path |