Skip to content

Commit fa78d04

Browse files
committed
C#: ZipSlip - Add qhelp file.
This adds a help file which describes the problem, provides recommendations on how to fix it and an example.
1 parent 99d1cf7 commit fa78d04

File tree

3 files changed

+95
-0
lines changed

3 files changed

+95
-0
lines changed
Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,77 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
<overview>
6+
<p>Extracting files from a malicious zip archive without validating that the destination file path
7+
is within the destination directory can cause files outside the destination directory to be
8+
overwritten, due to the possible presence of directory traversal elements ("..") in archive
9+
paths.</p>
10+
11+
<p>Zip archives contain archive entries representing each file in the archive. These entries
12+
include a file path for the entry, but these file paths are not restricted and may contain
13+
unexpected special elements such as the directory traversal element (".."). If these file paths are
14+
use to determine an output file to write the contents of the archive item to, then the file may be
15+
written to an unexpected location. This can result in sensitive information being revealed or
16+
deleted, or an attacker being able to influence behavior by modifying unexpected files.</p>
17+
18+
<p>For example, if a zip file contains a file entry <code>..\sneaky-file</code>, and the zip file
19+
is extracted to the directory <code>c:\output</code>, then naively combining the paths would result
20+
in an output file path of <code>c:\output\..\sneaky-file</code>, which would cause the file to be
21+
written to <code>c:\sneaky-file</code>.</p>
22+
23+
</overview>
24+
<recommendation>
25+
26+
<p>Ensure that output paths constructed from zip archive entries are validated to prevent writing
27+
files to unexpected locations.</p>
28+
29+
<p>The recommended way of writing an output file from a zip archive entry is to:</p>
30+
31+
<ol>
32+
<li>Use <code>Path.Combine(destinationDirectory, archiveEntry.FullName)</code> to determine the raw
33+
output path.</li>
34+
<li>Use <code>Path.GetFullPath(..)</code> on the raw output path to resolve any directory traversal
35+
elements.</li>
36+
<li>Use <code>Path.GetFullPath(destinationDirectory + Path.DirectorySeparatorChar)</code> to
37+
determine the fully resolved path of the destination directory.</li>
38+
<li>Validate that the resolved output path <code>StartsWith</code> the resolved destination
39+
directory, aborting if this is not true.</li>
40+
</ol>
41+
42+
<p>Another alternative is to validate archive entries against a whitelist of expected files.</p>
43+
44+
</recommendation>
45+
<example>
46+
47+
<p>In this example, a file path taken from a zip archive item entry is combined with a
48+
destination directory, and the result is used as the destination file path without verifying that
49+
the result is within the destination directory. If provided with a zip file containing an archive
50+
path like <code>..\sneaky-file</code>, then this file would be written outside of the destination
51+
directory.</p>
52+
53+
<sample src="ZipSlipBroken.cs" />
54+
55+
<p>In order to fix this vulnerability, we need to make three changes. Firstly, we need to resolve any
56+
directory traversal or other special characters in the path by using <code>Path.GetFullPath</code>.
57+
Secondly, we need to identify the destination output directory, again using
58+
<code>Path.GetFullPath</code>, this time on the output directory. Finally, we need to ensure that
59+
the resolved output starts with the resolved destination directory, and throw and exception if this
60+
is not the case.</p>
61+
62+
<sample src="ZipSlipFixed.cs" />
63+
64+
</example>
65+
<references>
66+
67+
<li>
68+
Snyk:
69+
<a href="https://snyk.io/research/zip-slip-vulnerability">Zip Slip Vulnerability</a>.
70+
</li>
71+
<li>
72+
OWASP:
73+
<a href="https://www.owasp.org/index.php/Path_traversal">Path Traversal</a>.
74+
</li>
75+
76+
</references>
77+
</qhelp>
Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
public static void WriteToDirectory(IArchiveEntry entry,
2+
string destDirectory,
3+
ExtractionOptions options){
4+
string file = Path.GetFileName(entry.Key);
5+
string destFileName = Path.Combine(destDirectory, file);
6+
entry.WriteToFile(destFileName, options);
7+
}
Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
public static void WriteToDirectory(IArchiveEntry entry,
2+
string destDirectory,
3+
ExtractionOptions options){
4+
string file = Path.GetFileName(entry.Key);
5+
string destFileName = Path.GetFullPath(Path.Combine(destDirecory, entry.Key));
6+
string fullDestDirPath = Path.GetFullPath(destDirectory + Path.DirectorySeparatorChar);
7+
if (!destFileName.StartsWith(fullDestDirPath)) {
8+
throw new ExtractionException("Entry is outside of the target dir: " + destFileName);
9+
}
10+
entry.WriteToFile(destFileName, options);
11+
}

0 commit comments

Comments
 (0)