-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Conversation
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.
<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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
".."
-> <code>..</code>
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use -> used
There was a problem hiding this comment.
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" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename Broken
-> Bad
.
There was a problem hiding this comment.
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" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename Fixed
-> Good
.
There was a problem hiding this comment.
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); | ||
} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove linebreak.
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add backticks
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the just -> just
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this 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" |
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo "zipe"
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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"
There was a problem hiding this comment.
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. */ |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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 ...")
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: "throw an exception"
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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: " + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove "of"
There was a problem hiding this comment.
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: " + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove "of"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Thanks @shati-semmle, changes made and pushed. |
Kotlin: Speed up the consistency queries
Add some queries for qldoc style
Add some queries for qldoc style
I have made the following changes to the ZipSlip query:
ZipSlip
module (80e4815) and reformatted it according to our style guide (3bc035f).Source
,Sink
andSanitizer
classes (09b2387) and migrating the existing predicates (b6c9f84).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