C#: ZipSlip - Rearrange query, add help and update doc

change-notes/1.18/analysis-csharp.md

@@ -15,6 +15,7 @@
 
 | **Query**                   | **Tags**  | **Purpose**                                                        |
 |-----------------------------|-----------|--------------------------------------------------------------------|
+| Arbitrary file write during zip extraction ("Zip Slip") (cs/zipslip) | security external/cwe/cwe-022  | Identifies zip extraction routines which allow arbitrary file overwrite vulnerabilities.
 | Constant condition (cs/constant-condition) | More results | The query has been generalized to cover both `Null-coalescing left operand is constant (cs/constant-null-coalescing)` and `Switch selector is constant (cs/constant-switch-selector)`. |
 | Exposing internal representation (cs/expose-implementation) | Different results | The query has been rewritten, based on the equivalent Java query. |
 | Local scope variable shadows member (cs/local-shadows-member) | maintainability, readability | Replaces the existing queries [Local variable shadows class member (cs/local-shadows-class-member)](https://help.semmle.com/wiki/display/CSHARP/Local+variable+shadows+class+member), [Local variable shadows struct member (cs/local-shadows-struct-member)](https://help.semmle.com/wiki/display/CSHARP/Local+variable+shadows+struct+member), [Parameter shadows class member (cs/parameter-shadows-class-member)](https://help.semmle.com/wiki/display/CSHARP/Parameter+shadows+class+member), and [Parameter shadows struct member (cs/parameter-shadows-struct-member)](https://help.semmle.com/wiki/display/CSHARP/Parameter+shadows+struct+member). |

csharp/ql/src/Security Features/CWE-022/ZipSlip.qhelp

@@ -0,0 +1,78 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>Extracting files from a malicious zip archive without validating that the destination file path
+is within the destination directory can cause files outside the destination directory to be
+overwritten, due to the possible presence of directory traversal elements (<code>..</code>) in
+archive paths.</p>
+
+<p>Zip archives contain archive entries representing each file in the archive. These entries
+include a file path for the entry, but these file paths are not restricted and may contain
+unexpected special elements such as the directory traversal element (<code>..</code>). If these
+file paths are used to determine an output file to write the contents of the archive item to, then
+the file may be written to an unexpected location. This can result in sensitive information being
+revealed or deleted, or an attacker being able to influence behavior by modifying unexpected
+files.</p>
+
+<p>For example, if a zip file contains a file entry <code>..\sneaky-file</code>, and the zip file
+is extracted to the directory <code>c:\output</code>, then naively combining the paths would result
+in an output file path of <code>c:\output\..\sneaky-file</code>, which would cause the file to be
+written to <code>c:\sneaky-file</code>.</p>
+
+</overview>
+<recommendation>
+
+<p>Ensure that output paths constructed from zip archive entries are validated to prevent writing
+files to unexpected locations.</p>
+
+<p>The recommended way of writing an output file from a zip archive entry is to:</p>
+
+<ol>
+<li>Use <code>Path.Combine(destinationDirectory, archiveEntry.FullName)</code> to determine the raw
+output path.</li>
+<li>Use <code>Path.GetFullPath(..)</code> on the raw output path to resolve any directory traversal
+elements.</li>
+<li>Use <code>Path.GetFullPath(destinationDirectory + Path.DirectorySeparatorChar)</code> to
+determine the fully resolved path of the destination directory.</li>
+<li>Validate that the resolved output path <code>StartsWith</code> the resolved destination
+directory, aborting if this is not true.</li>
+</ol>
+
+<p>Another alternative is to validate archive entries against a whitelist of expected files.</p>
+
+</recommendation>
+<example>
+
+<p>In this example, a file path taken from a zip archive item entry is combined with a
+destination directory. The result is used as the destination file path without verifying that
+the result is within the destination directory. If provided with a zip file containing an archive
+path like <code>..\sneaky-file</code>, then this file would be written outside the destination
+directory.</p>
+
+<sample src="ZipSlipBad.cs" />
+
+<p>To fix this vulnerability, we need to make three changes. Firstly, we need to resolve any
+directory traversal or other special characters in the path by using <code>Path.GetFullPath</code>.
+Secondly, we need to identify the destination output directory, again using
+<code>Path.GetFullPath</code>, this time on the output directory. Finally, we need to ensure that
+the resolved output starts with the resolved destination directory, and throw an exception if this
+is not the case.</p>
+
+<sample src="ZipSlipGood.cs" />
+
+</example>
+<references>
+
+<li>
+Snyk:
+<a href="https://snyk.io/research/zip-slip-vulnerability">Zip Slip Vulnerability</a>.
+</li>
+<li>
+OWASP:
+<a href="https://www.owasp.org/index.php/Path_traversal">Path Traversal</a>.
+</li>
+
+</references>
+</qhelp>

csharp/ql/src/Security Features/CWE-022/ZipSlip.ql

@@ -1,90 +1,19 @@
 /**
- * @name Potential ZipSlip vulnerability 
- * @description When extracting files from an archive, don't add archive item's path to the target file system path. Archive path can be relative and can lead to 
- * file system access outside of the expected file system target path, leading to malicious config changes and remote code execution via lay-and-wait technique 
+ * @name Arbitrary file write during zip extraction ("Zip Slip")
+ * @description Extracting files from a malicious zip archive without validating that the
+ *              destination file path is within the destination directory can cause files outside
+ *              the destination directory to be overwritten.
  * @kind problem
  * @id cs/zipslip
  * @problem.severity error
+ * @precision high
  * @tags security
  *       external/cwe/cwe-022
  */
 
 import csharp
+import semmle.code.csharp.security.dataflow.ZipSlip::ZipSlip
 
-// access to full name of the archive item
-Expr archiveFullName(PropertyAccess pa) {
-  pa.getTarget().getDeclaringType().hasQualifiedName("System.IO.Compression.ZipArchiveEntry") 
-  and pa.getTarget().getName() = "FullName" 
-  and result = pa
-}
-
-// argument to extract to file extension method 
-Expr compressionExtractToFileArgument(MethodCall mc) {
-  mc.getTarget().hasQualifiedName("System.IO.Compression.ZipFileExtensions", "ExtractToFile") 
-  and result = mc.getArgumentForName("destinationFileName") 
-}
-
-// File Stream created from tainted file name through File.Open/File.Create 
-Expr fileOpenArgument(MethodCall mc) {
-  (mc.getTarget().hasQualifiedName("System.IO.File", "Open") or
-   mc.getTarget().hasQualifiedName("System.IO.File", "OpenWrite") or
-   mc.getTarget().hasQualifiedName("System.IO.File", "Create"))
-   and result = mc.getArgumentForName("path")
-}
-
-// File Stream created from tainted file name passed directly to the constructor
-Expr streamConstructorArgument(ObjectCreation oc) {
-  oc.getTarget().getDeclaringType().hasQualifiedName("System.IO.FileStream") 
-  and result = oc.getArgumentForName("path")
-}
-
-// constructor to FileInfo can take tainted file name and subsequently be used to open file stream
-Expr fileInfoConstructorArgument(ObjectCreation oc) {
-  oc.getTarget().getDeclaringType().hasQualifiedName("System.IO.FileInfo") 
-  and result = oc.getArgumentForName("fileName")
-}
-// extracting just file name, not the full path
-Expr fileNameExtraction(MethodCall mc) {
-  mc.getTarget().hasQualifiedName("System.IO.Path", "GetFileName") 
-  and result = mc.getAnArgument()
-}
-
-// Checks the string for relative path, or checks the destination folder for whitelisted/target path, etc.
-Expr stringCheck(MethodCall mc) {
-  (mc.getTarget().hasQualifiedName("System.String", "StartsWith") or
-   mc.getTarget().hasQualifiedName("System.String", "Substring")) 
-  and result = mc.getQualifier()
-}
-
-// Taint tracking configuration for ZipSlip
-class ZipSlipTaintTrackingConfiguration extends TaintTracking::Configuration {
-   ZipSlipTaintTrackingConfiguration() {
-     this = "ZipSlipTaintTracking"
-   }
-
-   override predicate isSource(DataFlow::Node source) {
-     exists(PropertyAccess pa |
-     source.asExpr() = archiveFullName(pa))
-   }
-
-   override predicate isSink(DataFlow::Node sink) {
-     exists(MethodCall mc |
-     sink.asExpr() = compressionExtractToFileArgument(mc) or
-     sink.asExpr() = fileOpenArgument(mc))
-     or
-     exists(ObjectCreation oc |
-     sink.asExpr() = streamConstructorArgument(oc) or
-     sink.asExpr() = fileInfoConstructorArgument(oc))
-   }
-
-   override predicate isSanitizer(DataFlow::Node node) {
-     exists(MethodCall mc |
-     node.asExpr() = fileNameExtraction(mc) or
-     node.asExpr() = stringCheck(mc))
-   }
- }
-
-
-from ZipSlipTaintTrackingConfiguration zipTaintTracking, DataFlow::Node source, DataFlow::Node sink
+from TaintTrackingConfiguration zipTaintTracking, DataFlow::Node source, DataFlow::Node sink
 where zipTaintTracking.hasFlow(source, sink)
-select sink, "Make sure to sanitize relative archive item path before creating path for file extraction if the source of $@ is untrusted", source, "zip archive"
+select sink, "Unsanitized zip archive $@, which may contain '..', is used in a file system operation.", source, "item path"

csharp/ql/src/Security Features/CWE-022/ZipSlipBad.cs

@@ -0,0 +1,12 @@
+using System.IO;
+using System.IO.Compression;
+
+class Bad
+{
+    public static void WriteToDirectory(ZipArchiveEntry entry,
+                                        string destDirectory)
+    {
+        string destFileName = Path.Combine(destDirectory, entry.FullName);
+        entry.ExtractToFile(destFileName);
+    }
+}

csharp/ql/src/Security Features/CWE-022/ZipSlipGood.cs

@@ -0,0 +1,17 @@
+using System.IO;
+using System.IO.Compression;
+
+class Good
+{
+    public static void WriteToDirectory(ZipArchiveEntry entry,
+                                        string destDirectory)
+    {
+        string destFileName = Path.GetFullPath(Path.Combine(destDirectory, entry.FullName));
+        string fullDestDirPath = Path.GetFullPath(destDirectory + Path.DirectorySeparatorChar);
+        if (!destFileName.StartsWith(fullDestDirPath)) {
+            throw new System.InvalidOperationException("Entry is outside the target dir: " +
+                                                                                 destFileName);
+        }
+        entry.ExtractToFile(destFileName);
+    }
+}

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

@@ -0,0 +1,127 @@
+/**
+ * Provides a taint tracking configuration for reasoning about unsafe zip extraction.
+ */
+import csharp
+
+module ZipSlip {
+  /**
+   * A data flow source for unsafe zip extraction.
+   */
+  abstract class Source extends DataFlow::Node { }
+
+  /**
+   * A data flow sink for unsafe zip extraction.
+   */
+  abstract class Sink extends DataFlow::ExprNode { }
+
+  /**
+   * A sanitizer for unsafe zip extraction.
+   */
+  abstract class Sanitizer extends DataFlow::ExprNode { }
+
+  /** A taint tracking configuration for Zip Slip */
+  class TaintTrackingConfiguration extends TaintTracking::Configuration {
+    TaintTrackingConfiguration() {
+      this = "ZipSlipTaintTracking"
+    }
+
+    override predicate isSource(DataFlow::Node source) {
+      source instanceof Source
+    }
+
+    override predicate isSink(DataFlow::Node sink) {
+      sink instanceof Sink
+    }
+
+    override predicate isSanitizer(DataFlow::Node node) {
+      node instanceof Sanitizer
+    }
+  }
+
+  /** An access to the `FullName` property of a `ZipArchiveEntry`. */
+  class ArchiveFullNameSource extends Source {
+    ArchiveFullNameSource() {
+      exists(PropertyAccess pa |
+        this.asExpr() = pa |
+        pa.getTarget().getDeclaringType().hasQualifiedName("System.IO.Compression.ZipArchiveEntry") and
+        pa.getTarget().getName() = "FullName" 
+      )
+    }
+  }
+
+  /** An argument to the `ExtractToFile` extension method. */
+  class ExtractToFileArgSink extends Sink {
+    ExtractToFileArgSink() {
+      exists(MethodCall mc |
+        mc.getTarget().hasQualifiedName("System.IO.Compression.ZipFileExtensions", "ExtractToFile") and
+        this.asExpr() = mc.getArgumentForName("destinationFileName")
+      )
+    }
+  }
+
+  /** A path argument to a `File.Open`, `File.OpenWrite`, or `File.Create` method call. */
+  class FileOpenArgSink extends Sink {
+    FileOpenArgSink() {
+      exists(MethodCall mc |
+        mc.getTarget().hasQualifiedName("System.IO.File", "Open") or
+        mc.getTarget().hasQualifiedName("System.IO.File", "OpenWrite") or
+        mc.getTarget().hasQualifiedName("System.IO.File", "Create") |
+        this.asExpr() = mc.getArgumentForName("path")
+      )
+    }
+  }
+
+  /** A path argument to a call to the `FileStream` constructor. */
+  class FileStreamArgSink extends Sink {
+    FileStreamArgSink() {
+      exists(ObjectCreation oc |
+        oc.getTarget().getDeclaringType().hasQualifiedName("System.IO.FileStream") |
+        this.asExpr() = oc.getArgumentForName("path")
+      )
+    }
+  }
+
+  /**
+   * A path argument to a call to the `FileStream` constructor.
+   *
+   * This constructor can accept a tainted file name and subsequently be used to open a file stream.
+   */
+  class FileInfoArgSink extends Sink {
+    FileInfoArgSink() {
+      exists(ObjectCreation oc |
+        oc.getTarget().getDeclaringType().hasQualifiedName("System.IO.FileInfo") |
+        this.asExpr() = oc.getArgumentForName("fileName")
+      )
+    }
+  }
+
+  /**
+   * An argument to `GetFileName`.
+   *
+   * This is considered a sanitizer because it extracts just the file name, not the full path.
+   */
+  class GetFileNameSanitizer extends Sanitizer {
+    GetFileNameSanitizer() {
+      exists(MethodCall mc |
+        mc.getTarget().hasQualifiedName("System.IO.Path", "GetFileName") |
+        this.asExpr() = mc.getAnArgument()
+      )
+    }
+  }
+
+  /**
+   * A qualifier in a call to `StartsWith` or `Substring` string method.
+   *
+   * A call to a String method such as `StartsWith` or `Substring` 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") or
+        mc.getTarget().hasQualifiedName("System.String", "Substring") |
+        this.asExpr() = mc.getQualifier()
+      )
+    }
+  }
+}

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

@@ -1,6 +1,7 @@
-| ZipSlip.cs:24:41:24:52 | access to local variable destFileName | Make sure to sanitize relative archive item path before creating path for file extraction if the source of $@ is untrusted | ZipSlip.cs:19:31:19:44 | access to property FullName | zip archive |
-| ZipSlip.cs:32:41:32:52 | access to local variable destFilePath | Make sure to sanitize relative archive item path before creating path for file extraction if the source of $@ is untrusted | ZipSlip.cs:16:52:16:65 | access to property FullName | zip archive |
-| ZipSlip.cs:61:74:61:85 | access to local variable destFilePath | Make sure to sanitize relative archive item path before creating path for file extraction if the source of $@ is untrusted | ZipSlip.cs:54:72:54:85 | access to property FullName | zip archive |
-| ZipSlip.cs:68:71:68:82 | access to local variable destFilePath | Make sure to sanitize relative archive item path before creating path for file extraction if the source of $@ is untrusted | ZipSlip.cs:54:72:54:85 | access to property FullName | zip archive |
-| ZipSlip.cs:75:57:75:68 | access to local variable destFilePath | Make sure to sanitize relative archive item path before creating path for file extraction if the source of $@ is untrusted | ZipSlip.cs:54:72:54:85 | access to property FullName | zip archive |
-| ZipSlip.cs:83:58:83:69 | access to local variable destFilePath | Make sure to sanitize relative archive item path before creating path for file extraction if the source of $@ is untrusted | ZipSlip.cs:54:72:54:85 | access to property FullName | zip archive |
+| 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 |
+| 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 |

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

@@ -0,0 +1,12 @@
+using System.IO;
+using System.IO.Compression;
+
+class Bad
+{
+    public static void WriteToDirectory(ZipArchiveEntry entry,
+                                        string destDirectory)
+    {
+        string destFileName = Path.Combine(destDirectory, entry.FullName);
+        entry.ExtractToFile(destFileName);
+    }
+}

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

@@ -0,0 +1,17 @@
+using System.IO;
+using System.IO.Compression;
+
+class Good
+{
+    public static void WriteToDirectory(ZipArchiveEntry entry,
+                                        string destDirectory)
+    {
+        string destFileName = Path.GetFullPath(Path.Combine(destDirectory, entry.FullName));
+        string fullDestDirPath = Path.GetFullPath(destDirectory + Path.DirectorySeparatorChar);
+        if (!destFileName.StartsWith(fullDestDirPath)) {
+            throw new System.InvalidOperationException("Entry is outside the target dir: " +
+                                                                                 destFileName);
+        }
+        entry.ExtractToFile(destFileName);
+    }
+}