Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 13 commits into from
Aug 24, 2018

Conversation

lcartey
Copy link
Contributor

@lcartey lcartey commented Aug 20, 2018

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.
@lcartey lcartey requested review from a team August 20, 2018 16:02
denislevin
denislevin previously approved these changes Aug 20, 2018
<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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And further down as well

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed.

<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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use -> used

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

path like <code>..\sneaky-file</code>, then this file would be written outside of the destination
directory.</p>

<sample src="ZipSlipBroken.cs" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename Broken -> Bad.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed.

the resolved output starts with the resolved destination directory, and throw and exception if this
is not the case.</p>

<sample src="ZipSlipFixed.cs" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename Fixed -> Good.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea - done.

import csharp

module ZipSlip {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove linebreak.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

}

/**
* An argument to GetFileName.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add backticks

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the just -> just

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

hvitved
hvitved previously approved these changes Aug 21, 2018
Copy link
Contributor

@shati-patel shati-patel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

abstract class Sink extends DataFlow::ExprNode { }

/**
* A sanitizer for unsafe zipe extraction.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo "zipe"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove hyphen from "taint-tracking"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

}
}

/** A path argument to a `File.Open`, `File.OpenWrite` or `File.Create` method call. */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

}

/**
* A qualifier in a call to `StartsWith` or `Substring` string method.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: "throw an exception"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

* @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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

<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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

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: " +
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove "of"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

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: " +
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove "of"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@lcartey lcartey dismissed stale reviews from hvitved and denislevin via 86a7df0 August 23, 2018 14:57
@lcartey
Copy link
Contributor Author

lcartey commented Aug 23, 2018

Thanks @shati-semmle, changes made and pushed.

@hvitved hvitved merged commit d4551e5 into github:master Aug 24, 2018
@lcartey lcartey deleted the csharp/zipslip-reformat branch August 24, 2018 10:57
aibaars added a commit that referenced this pull request Oct 14, 2021
smowton pushed a commit to smowton/codeql that referenced this pull request Dec 6, 2021
Kotlin: Speed up the consistency queries
erik-krogh pushed a commit to erik-krogh/ql that referenced this pull request Dec 15, 2021
Add some queries for qldoc style
erik-krogh pushed a commit to erik-krogh/ql that referenced this pull request Dec 15, 2021
Add some queries for qldoc style
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants