C#: ZipSlip - Address doc team comments.

Author: lcartey

change-notes/1.18/analysis-csharp.md

@@ -15,7 +15,7 @@
 
 | **Query**                   | **Tags**  | **Purpose**                                                        |
 |-----------------------------|-----------|--------------------------------------------------------------------|
-| Arbitrary file write during zip extraction ("ZipSlip") | security external/cwe/cwe-022  | Identifies zip extraction routines which allow tainted path and arbitrary file overwrite vulnerabilities.
+| 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

@@ -46,18 +46,18 @@ directory, aborting if this is not true.</li>
 <example>
 
 <p>In this example, a file path taken from a zip archive item entry is combined with a
-destination directory, and the result is used as the destination file path without verifying that
+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 of the destination
+path like <code>..\sneaky-file</code>, then this file would be written outside the destination
 directory.</p>
 
 <sample src="ZipSlipBad.cs" />
 
-<p>In order to fix this vulnerability, we need to make three changes. Firstly, we need to resolve any
+<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 and exception if this
+the resolved output starts with the resolved destination directory, and throw an exception if this
 is not the case.</p>
 
 <sample src="ZipSlipGood.cs" />

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

@@ -1,9 +1,8 @@
 /**
- * @name Arbitrary file write during zip extraction ("ZipSlip")
+ * @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, due to the possible presence of
- *              directory traversal elements ("..") in archive paths.
+ *              the destination directory to be overwritten.
  * @kind problem
  * @id cs/zipslip
  * @problem.severity error
@@ -17,4 +16,4 @@ import semmle.code.csharp.security.dataflow.ZipSlip::ZipSlip
 
 from TaintTrackingConfiguration zipTaintTracking, DataFlow::Node source, DataFlow::Node sink
 where zipTaintTracking.hasFlow(source, sink)
-select sink, "Unsanitized zip archive $@ which may contain '..' used in a file system operation.", source, "item path"
+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/ZipSlipGood.cs

@@ -9,7 +9,7 @@ public static void WriteToDirectory(ZipArchiveEntry entry,
         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 of the target dir: " +
+            throw new System.InvalidOperationException("Entry is outside the target dir: " +
                                                                                  destFileName);
         }
         entry.ExtractToFile(destFileName);

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

@@ -1,5 +1,5 @@
 /**
- * Provides a taint-tracking configuration for reasoning about unsafe zip extraction.
+ * Provides a taint tracking configuration for reasoning about unsafe zip extraction.
  */
 import csharp
 
@@ -15,11 +15,11 @@ module ZipSlip {
   abstract class Sink extends DataFlow::ExprNode { }
 
   /**
-   * A sanitizer for unsafe zipe extraction.
+   * A sanitizer for unsafe zip extraction.
    */
   abstract class Sanitizer extends DataFlow::ExprNode { }
 
-  /** A taint tracking configuration for ZipSlip */
+  /** A taint tracking configuration for Zip Slip */
   class TaintTrackingConfiguration extends TaintTracking::Configuration {
     TaintTrackingConfiguration() {
       this = "ZipSlipTaintTracking"
@@ -59,7 +59,7 @@ module ZipSlip {
     }
   }
 
-  /** A path argument to a `File.Open`, `File.OpenWrite` or `File.Create` method call. */
+  /** A path argument to a `File.Open`, `File.OpenWrite`, or `File.Create` method call. */
   class FileOpenArgSink extends Sink {
     FileOpenArgSink() {
       exists(MethodCall mc |

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

@@ -1,7 +1,7 @@
-| ZipSlip.cs:24:41:24:52 | access to local variable destFileName | Unsanitized zip archive $@ which may contain '..' 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 '..' 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 '..' 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 '..' 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 '..' 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 '..' 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 '..' used in a file system operation. | ZipSlipBad.cs:9:59:9:72 | access to property FullName | item path |
+| 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/ZipSlipGood.cs

@@ -9,7 +9,7 @@ public static void WriteToDirectory(ZipArchiveEntry entry,
         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 of the target dir: " +
+            throw new System.InvalidOperationException("Entry is outside the target dir: " +
                                                                                  destFileName);
         }
         entry.ExtractToFile(destFileName);