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

lcartey · 2018-08-20T16:02:07Z

I have made the following changes to the ZipSlip query:

Extracted a ZipSlip module (80e4815) and reformatted it according to our style guide (3bc035f).
Rearranged the ZipSlip module to match the other security modules, by introducing Source, Sink and Sanitizer classes (09b2387) and migrating the existing predicates (b6c9f84).
Added a help file (ebe8b5b), a change note (eb6a1cd) and a precision tag (6554672), and updated the name, description and alert message (d92c6f7) to match our existing style.

I have a follow up, once this is merged, which makes some improvements to the handling of sanitizers for this query.

@denislevin fyi

@Semmle/doc

Reformat the ZipSlip module to adhere to the "QL Style Guide".

This commit refactors the existing predicates to be classes extending Source, Sink or Sanitizer, as appropriate.

This commit updates the name, description and message to better match the house style for the security queries.

This adds a help file which describes the problem, provides recommendations on how to fix it and an example.

hvitved · 2018-08-21T07:45:38Z

csharp/ql/src/Security Features/CWE-022/ZipSlip.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 ("..") in archive


".." -> <code>..</code>

And further down as well

hvitved · 2018-08-21T07:46:27Z

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

+<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


use -> used

hvitved · 2018-08-21T07:56:17Z

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

+path like <code>..\sneaky-file</code>, then this file would be written outside of the destination
+directory.</p>
+
+<sample src="ZipSlipBroken.cs" />


Rename Broken -> Bad.

hvitved · 2018-08-21T07:58:06Z

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

+the resolved output starts with the resolved destination directory, and throw and exception if this
+is not the case.</p>
+
+<sample src="ZipSlipFixed.cs" />


Rename Fixed -> Good.

hvitved · 2018-08-21T08:06:20Z

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

+  string file = Path.GetFileName(entry.Key);
+  string destFileName = Path.Combine(destDirectory, file);
+  entry.WriteToFile(destFileName, options);
+}


We have started formatting all C# code using 4 spaces indentation, and { on it's own line (following the standard that Microsoft uses). Moreover, I would like for all help examples to be compilable, so we can have a copy of the examples in the test directory as well.

The "Bad" example should always have a "class Bad" top-level (e.g. https://github.com/Semmle/ql/blob/master/csharp/ql/src/Bad%20Practices/Declarations/LocalScopeVariableShadowsMemberBad.cs), and the "Good" example should have a "class Good" top-level (e.g. https://github.com/Semmle/ql/blob/master/csharp/ql/src/Bad%20Practices/Declarations/LocalScopeVariableShadowsMemberGood.cs). The examples are then duplicated inside https://github.com/Semmle/ql/tree/master/csharp/ql/test/query-tests/Bad%20Practices/Declarations/LocalScopeVariableShadowsMember, and in case extractor options are required, I put them in a separate file https://github.com/Semmle/ql/blob/master/csharp/ql/test/query-tests/Bad%20Practices/Declarations/LocalScopeVariableShadowsMember/ExtractorOptions.cs.

Good idea - done.

hvitved · 2018-08-21T08:07:11Z

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

+import csharp
+
+module ZipSlip {
+


Remove linebreak.

hvitved · 2018-08-21T08:09:20Z

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

+  }
+
+  /**
+   * An argument to GetFileName.


Add backticks

hvitved · 2018-08-21T08:09:38Z

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

+  /**
+   * An argument to GetFileName.
+   *
+   * This is considered a sanitizer because it extracts the just the file name, not the full path.


the just -> just

shati-patel

Looks good, though I've made a couple of suggestions inline.

shati-patel · 2018-08-23T12:25:44Z

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

 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 '..' used in a file system operation.", source, "item path"


Suggestion: Add commas and "is".
"Unsanitized zip archive $@, which may contain '..', is used in a file system operation."

(This will require updating the .expected file too.)

shati-patel · 2018-08-23T12:26:33Z

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

+  abstract class Sink extends DataFlow::ExprNode { }
+
+  /**
+   * A sanitizer for unsafe zipe extraction.


Typo "zipe"

shati-patel · 2018-08-23T12:28:46Z

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.


Remove hyphen from "taint-tracking"

shati-patel · 2018-08-23T12:30:11Z

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

+    }
+  }
+
+  /** A path argument to a `File.Open`, `File.OpenWrite` or `File.Create` method call. */


Minor point: could you add another comma after File.OpenWrite?

shati-patel · 2018-08-23T12:31:51Z

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

+  }
+
+  /**
+   * A qualifier in a call to `StartsWith` or `Substring` string method.


Perhaps "a call to a ..."?
(or "a call to the ...")

I'll leave this for now - I have a follow up PR that splits this class and modifies the documentation.

shati-patel · 2018-08-23T13:34:14Z

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

+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


typo: "throw an exception"

shati-patel · 2018-08-23T13:44:04Z

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

+ * @name Arbitrary file write during zip extraction ("ZipSlip")
+ * @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


Maybe truncate the description after "overwritten"? The rest of the information is duplicated in the query help file anyway.

shati-patel · 2018-08-23T13:46:21Z

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

+<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
+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


You've used "outside the directory" and "outside of the directory" a few times each. I'd recommend removing the "of" in all cases.

shati-patel · 2018-08-23T13:47:02Z

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

+        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: " +


Remove "of"

shati-patel · 2018-08-23T13:47:16Z

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

+        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: " +


Remove "of"

lcartey · 2018-08-23T14:58:06Z

Thanks @shati-semmle, changes made and pushed.

Update ruby.dbscheme.stats

Kotlin: Speed up the consistency queries

Add some queries for qldoc style

lcartey added 9 commits August 20, 2018 12:03

C#: Extract ZipSlip library

80e4815

C#: Reformat ZipSlip module.

3bc035f

Reformat the ZipSlip module to adhere to the "QL Style Guide".

C#: ZipSlip, introduce source, sink, sanitizer classes.

09b2387

C#: ZipSlip - refactor to use Source, Sink, Sanitizer

b6c9f84

This commit refactors the existing predicates to be classes extending Source, Sink or Sanitizer, as appropriate.

C#: ZipSlip - remove ZipSlip prefix from TaintTracking class name.

112d104

C#: ZipSlip - Update name, description and message.

99d1cf7

This commit updates the name, description and message to better match the house style for the security queries.

C#: ZipSlip - Add qhelp file.

fa78d04

This adds a help file which describes the problem, provides recommendations on how to fix it and an example.

C#: ZipSlip - Add change note.

0477bd7

C#: ZipSlip - Add precision tag.

d6c58d6

lcartey requested review from a team August 20, 2018 16:02

denislevin previously approved these changes Aug 20, 2018

View reviewed changes

hvitved requested changes Aug 21, 2018

View reviewed changes

lcartey added 3 commits August 21, 2018 12:17

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

6959d80

C#: ZipSlip - Address review comments in module.

6453153

C#: ZipSlip - Add spaces into bad example.

4f57456

hvitved previously approved these changes Aug 21, 2018

View reviewed changes

shati-patel reviewed Aug 23, 2018

View reviewed changes

C#: ZipSlip - Address doc team comments.

86a7df0

lcartey dismissed stale reviews from hvitved and denislevin via 86a7df0 August 23, 2018 14:57

shati-patel approved these changes Aug 23, 2018

View reviewed changes

hvitved merged commit d4551e5 into github:master Aug 24, 2018

lcartey deleted the csharp/zipslip-reformat branch August 24, 2018 10:57

aibaars added a commit that referenced this pull request Oct 14, 2021

Merge pull request #81 from github/aibaars/revert-dup-code

f0ddeaa

Update ruby.dbscheme.stats

smowton pushed a commit to smowton/codeql that referenced this pull request Dec 6, 2021

Merge pull request github#81 from github/igfoo/consis

8d510be

Kotlin: Speed up the consistency queries

erik-krogh pushed a commit to erik-krogh/ql that referenced this pull request Dec 15, 2021

Merge pull request github#81 from github/doc-style

78caab4

Add some queries for qldoc style

erik-krogh pushed a commit to erik-krogh/ql that referenced this pull request Dec 15, 2021

QL: Merge pull request github#81 from github/doc-style

1762394

Add some queries for qldoc style

		@@ -0,0 +1,127 @@
		/**
		* Provides a taint-tracking configuration for reasoning about unsafe zip extraction.

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

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

Uh oh!

Conversation

lcartey commented Aug 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shati-patel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment