C#: ZipSlip - Update help, compile and test samples.

Author: lcartey

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

@@ -5,15 +5,16 @@
 <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 ("..") in archive
-paths.</p>
+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 (".."). If these file paths are
-use 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>
+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
@@ -50,7 +51,7 @@ the result is within the destination directory. If provided with a zip file cont
 path like <code>..\sneaky-file</code>, then this file would be written outside of the destination
 directory.</p>
 
-<sample src="ZipSlipBroken.cs" />
+<sample src="ZipSlipBad.cs" />
 
 <p>In order 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>.
@@ -59,7 +60,7 @@ Secondly, we need to identify the destination output directory, again using
 the resolved output starts with the resolved destination directory, and throw and exception if this
 is not the case.</p>
 
-<sample src="ZipSlipFixed.cs" />
+<sample src="ZipSlipGood.cs" />
 
 </example>
 <references>

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/ZipSlipBroken.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 of the target dir: " +
+                                                                                 destFileName);
+        }
+        entry.ExtractToFile(destFileName);
+    }
+}

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

@@ -4,3 +4,4 @@
 | 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 |

csharp/ql/src/Security Features/CWE-022/ZipSlipGood.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/ZipSlip.expected

@@ -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 of the target dir: " +
+                                                                                 destFileName);
+        }
+        entry.ExtractToFile(destFileName);
+    }
+}