Skip to content

Commit d4551e5

Browse files
hvitvedhvitved
authored
Merge pull request #81 from lukecartey/csharp/zipslip-reformat
C#: ZipSlip - Rearrange query, add help and update doc
2 parents 55ceb9b + 86a7df0 commit d4551e5

File tree

9 files changed

+279
-85
lines changed

9 files changed

+279
-85
lines changed

‎change-notes/1.18/analysis-csharp.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414

1515
| **Query** | **Tags** | **Purpose** |
1616
|-----------------------------|-----------|--------------------------------------------------------------------|
17+
| Arbitrary file write during zip extraction ("Zip Slip") (cs/zipslip) | security external/cwe/cwe-022 | Identifies zip extraction routines which allow arbitrary file overwrite vulnerabilities.
1718
| Constant condition (cs/constant-condition) | More results | The query has been generalized to cover both `Null-coalescing left operand is constant (cs/constant-null-coalescing)` and `Switch selector is constant (cs/constant-switch-selector)`. |
1819
| Exposing internal representation (cs/expose-implementation) | Different results | The query has been rewritten, based on the equivalent Java query. |
1920
| Local scope variable shadows member (cs/local-shadows-member) | maintainability, readability | Replaces the existing queries [Local variable shadows class member (cs/local-shadows-class-member)](https://help.semmle.com/wiki/display/CSHARP/Local+variable+shadows+class+member), [Local variable shadows struct member (cs/local-shadows-struct-member)](https://help.semmle.com/wiki/display/CSHARP/Local+variable+shadows+struct+member), [Parameter shadows class member (cs/parameter-shadows-class-member)](https://help.semmle.com/wiki/display/CSHARP/Parameter+shadows+class+member), and [Parameter shadows struct member (cs/parameter-shadows-struct-member)](https://help.semmle.com/wiki/display/CSHARP/Parameter+shadows+struct+member). |

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

Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,78 @@
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 (<code>..</code>) in
9+
archive 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 (<code>..</code>). If these
14+
file paths are used to determine an output file to write the contents of the archive item to, then
15+
the file may be written to an unexpected location. This can result in sensitive information being
16+
revealed or deleted, or an attacker being able to influence behavior by modifying unexpected
17+
files.</p>
18+
19+
<p>For example, if a zip file contains a file entry <code>..\sneaky-file</code>, and the zip file
20+
is extracted to the directory <code>c:\output</code>, then naively combining the paths would result
21+
in an output file path of <code>c:\output\..\sneaky-file</code>, which would cause the file to be
22+
written to <code>c:\sneaky-file</code>.</p>
23+
24+
</overview>
25+
<recommendation>
26+
27+
<p>Ensure that output paths constructed from zip archive entries are validated to prevent writing
28+
files to unexpected locations.</p>
29+
30+
<p>The recommended way of writing an output file from a zip archive entry is to:</p>
31+
32+
<ol>
33+
<li>Use <code>Path.Combine(destinationDirectory, archiveEntry.FullName)</code> to determine the raw
34+
output path.</li>
35+
<li>Use <code>Path.GetFullPath(..)</code> on the raw output path to resolve any directory traversal
36+
elements.</li>
37+
<li>Use <code>Path.GetFullPath(destinationDirectory + Path.DirectorySeparatorChar)</code> to
38+
determine the fully resolved path of the destination directory.</li>
39+
<li>Validate that the resolved output path <code>StartsWith</code> the resolved destination
40+
directory, aborting if this is not true.</li>
41+
</ol>
42+
43+
<p>Another alternative is to validate archive entries against a whitelist of expected files.</p>
44+
45+
</recommendation>
46+
<example>
47+
48+
<p>In this example, a file path taken from a zip archive item entry is combined with a
49+
destination directory. The result is used as the destination file path without verifying that
50+
the result is within the destination directory. If provided with a zip file containing an archive
51+
path like <code>..\sneaky-file</code>, then this file would be written outside the destination
52+
directory.</p>
53+
54+
<sample src="ZipSlipBad.cs" />
55+
56+
<p>To fix this vulnerability, we need to make three changes. Firstly, we need to resolve any
57+
directory traversal or other special characters in the path by using <code>Path.GetFullPath</code>.
58+
Secondly, we need to identify the destination output directory, again using
59+
<code>Path.GetFullPath</code>, this time on the output directory. Finally, we need to ensure that
60+
the resolved output starts with the resolved destination directory, and throw an exception if this
61+
is not the case.</p>
62+
63+
<sample src="ZipSlipGood.cs" />
64+
65+
</example>
66+
<references>
67+
68+
<li>
69+
Snyk:
70+
<a href="https://snyk.io/research/zip-slip-vulnerability">Zip Slip Vulnerability</a>.
71+
</li>
72+
<li>
73+
OWASP:
74+
<a href="https://www.owasp.org/index.php/Path_traversal">Path Traversal</a>.
75+
</li>
76+
77+
</references>
78+
</qhelp>

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

Lines changed: 8 additions & 79 deletions
Original file line numberDiff line numberDiff line change
@@ -1,90 +1,19 @@
11
/**
2-
* @name Potential ZipSlip vulnerability
3-
* @description When extracting files from an archive, don't add archive item's path to the target file system path. Archive path can be relative and can lead to
4-
* file system access outside of the expected file system target path, leading to malicious config changes and remote code execution via lay-and-wait technique
2+
* @name Arbitrary file write during zip extraction ("Zip Slip")
3+
* @description Extracting files from a malicious zip archive without validating that the
4+
* destination file path is within the destination directory can cause files outside
5+
* the destination directory to be overwritten.
56
* @kind problem
67
* @id cs/zipslip
78
* @problem.severity error
9+
* @precision high
810
* @tags security
911
* external/cwe/cwe-022
1012
*/
1113

1214
import csharp
15+
import semmle.code.csharp.security.dataflow.ZipSlip::ZipSlip
1316

14-
// access to full name of the archive item
15-
Expr archiveFullName(PropertyAccess pa) {
16-
pa.getTarget().getDeclaringType().hasQualifiedName("System.IO.Compression.ZipArchiveEntry")
17-
and pa.getTarget().getName() = "FullName"
18-
and result = pa
19-
}
20-
21-
// argument to extract to file extension method
22-
Expr compressionExtractToFileArgument(MethodCall mc) {
23-
mc.getTarget().hasQualifiedName("System.IO.Compression.ZipFileExtensions", "ExtractToFile")
24-
and result = mc.getArgumentForName("destinationFileName")
25-
}
26-
27-
// File Stream created from tainted file name through File.Open/File.Create
28-
Expr fileOpenArgument(MethodCall mc) {
29-
(mc.getTarget().hasQualifiedName("System.IO.File", "Open") or
30-
mc.getTarget().hasQualifiedName("System.IO.File", "OpenWrite") or
31-
mc.getTarget().hasQualifiedName("System.IO.File", "Create"))
32-
and result = mc.getArgumentForName("path")
33-
}
34-
35-
// File Stream created from tainted file name passed directly to the constructor
36-
Expr streamConstructorArgument(ObjectCreation oc) {
37-
oc.getTarget().getDeclaringType().hasQualifiedName("System.IO.FileStream")
38-
and result = oc.getArgumentForName("path")
39-
}
40-
41-
// constructor to FileInfo can take tainted file name and subsequently be used to open file stream
42-
Expr fileInfoConstructorArgument(ObjectCreation oc) {
43-
oc.getTarget().getDeclaringType().hasQualifiedName("System.IO.FileInfo")
44-
and result = oc.getArgumentForName("fileName")
45-
}
46-
// extracting just file name, not the full path
47-
Expr fileNameExtraction(MethodCall mc) {
48-
mc.getTarget().hasQualifiedName("System.IO.Path", "GetFileName")
49-
and result = mc.getAnArgument()
50-
}
51-
52-
// Checks the string for relative path, or checks the destination folder for whitelisted/target path, etc.
53-
Expr stringCheck(MethodCall mc) {
54-
(mc.getTarget().hasQualifiedName("System.String", "StartsWith") or
55-
mc.getTarget().hasQualifiedName("System.String", "Substring"))
56-
and result = mc.getQualifier()
57-
}
58-
59-
// Taint tracking configuration for ZipSlip
60-
class ZipSlipTaintTrackingConfiguration extends TaintTracking::Configuration {
61-
ZipSlipTaintTrackingConfiguration() {
62-
this = "ZipSlipTaintTracking"
63-
}
64-
65-
override predicate isSource(DataFlow::Node source) {
66-
exists(PropertyAccess pa |
67-
source.asExpr() = archiveFullName(pa))
68-
}
69-
70-
override predicate isSink(DataFlow::Node sink) {
71-
exists(MethodCall mc |
72-
sink.asExpr() = compressionExtractToFileArgument(mc) or
73-
sink.asExpr() = fileOpenArgument(mc))
74-
or
75-
exists(ObjectCreation oc |
76-
sink.asExpr() = streamConstructorArgument(oc) or
77-
sink.asExpr() = fileInfoConstructorArgument(oc))
78-
}
79-
80-
override predicate isSanitizer(DataFlow::Node node) {
81-
exists(MethodCall mc |
82-
node.asExpr() = fileNameExtraction(mc) or
83-
node.asExpr() = stringCheck(mc))
84-
}
85-
}
86-
87-
88-
from ZipSlipTaintTrackingConfiguration zipTaintTracking, DataFlow::Node source, DataFlow::Node sink
17+
from TaintTrackingConfiguration zipTaintTracking, DataFlow::Node source, DataFlow::Node sink
8918
where zipTaintTracking.hasFlow(source, sink)
90-
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"
19+
select sink, "Unsanitized zip archive $@, which may contain '..', is used in a file system operation.", source, "item path"

‎csharp/ql/src/Security Features/CWE-022/ZipSlipBad.cs‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
using System.IO;
2+
using System.IO.Compression;
3+
4+
class Bad
5+
{
6+
public static void WriteToDirectory(ZipArchiveEntry entry,
7+
string destDirectory)
8+
{
9+
string destFileName = Path.Combine(destDirectory, entry.FullName);
10+
entry.ExtractToFile(destFileName);
11+
}
12+
}

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

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
using System.IO;
2+
using System.IO.Compression;
3+
4+
class Good
5+
{
6+
public static void WriteToDirectory(ZipArchiveEntry entry,
7+
string destDirectory)
8+
{
9+
string destFileName = Path.GetFullPath(Path.Combine(destDirectory, entry.FullName));
10+
string fullDestDirPath = Path.GetFullPath(destDirectory + Path.DirectorySeparatorChar);
11+
if (!destFileName.StartsWith(fullDestDirPath)) {
12+
throw new System.InvalidOperationException("Entry is outside the target dir: " +
13+
destFileName);
14+
}
15+
entry.ExtractToFile(destFileName);
16+
}
17+
}

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

Lines changed: 127 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,127 @@
1+
/**
2+
* Provides a taint tracking configuration for reasoning about unsafe zip extraction.
3+
*/
4+
import csharp
5+
6+
module ZipSlip {
7+
/**
8+
* A data flow source for unsafe zip extraction.
9+
*/
10+
abstract class Source extends DataFlow::Node { }
11+
12+
/**
13+
* A data flow sink for unsafe zip extraction.
14+
*/
15+
abstract class Sink extends DataFlow::ExprNode { }
16+
17+
/**
18+
* A sanitizer for unsafe zip extraction.
19+
*/
20+
abstract class Sanitizer extends DataFlow::ExprNode { }
21+
22+
/** A taint tracking configuration for Zip Slip */
23+
class TaintTrackingConfiguration extends TaintTracking::Configuration {
24+
TaintTrackingConfiguration() {
25+
this = "ZipSlipTaintTracking"
26+
}
27+
28+
override predicate isSource(DataFlow::Node source) {
29+
source instanceof Source
30+
}
31+
32+
override predicate isSink(DataFlow::Node sink) {
33+
sink instanceof Sink
34+
}
35+
36+
override predicate isSanitizer(DataFlow::Node node) {
37+
node instanceof Sanitizer
38+
}
39+
}
40+
41+
/** An access to the `FullName` property of a `ZipArchiveEntry`. */
42+
class ArchiveFullNameSource extends Source {
43+
ArchiveFullNameSource() {
44+
exists(PropertyAccess pa |
45+
this.asExpr() = pa |
46+
pa.getTarget().getDeclaringType().hasQualifiedName("System.IO.Compression.ZipArchiveEntry") and
47+
pa.getTarget().getName() = "FullName"
48+
)
49+
}
50+
}
51+
52+
/** An argument to the `ExtractToFile` extension method. */
53+
class ExtractToFileArgSink extends Sink {
54+
ExtractToFileArgSink() {
55+
exists(MethodCall mc |
56+
mc.getTarget().hasQualifiedName("System.IO.Compression.ZipFileExtensions", "ExtractToFile") and
57+
this.asExpr() = mc.getArgumentForName("destinationFileName")
58+
)
59+
}
60+
}
61+
62+
/** A path argument to a `File.Open`, `File.OpenWrite`, or `File.Create` method call. */
63+
class FileOpenArgSink extends Sink {
64+
FileOpenArgSink() {
65+
exists(MethodCall mc |
66+
mc.getTarget().hasQualifiedName("System.IO.File", "Open") or
67+
mc.getTarget().hasQualifiedName("System.IO.File", "OpenWrite") or
68+
mc.getTarget().hasQualifiedName("System.IO.File", "Create") |
69+
this.asExpr() = mc.getArgumentForName("path")
70+
)
71+
}
72+
}
73+
74+
/** A path argument to a call to the `FileStream` constructor. */
75+
class FileStreamArgSink extends Sink {
76+
FileStreamArgSink() {
77+
exists(ObjectCreation oc |
78+
oc.getTarget().getDeclaringType().hasQualifiedName("System.IO.FileStream") |
79+
this.asExpr() = oc.getArgumentForName("path")
80+
)
81+
}
82+
}
83+
84+
/**
85+
* A path argument to a call to the `FileStream` constructor.
86+
*
87+
* This constructor can accept a tainted file name and subsequently be used to open a file stream.
88+
*/
89+
class FileInfoArgSink extends Sink {
90+
FileInfoArgSink() {
91+
exists(ObjectCreation oc |
92+
oc.getTarget().getDeclaringType().hasQualifiedName("System.IO.FileInfo") |
93+
this.asExpr() = oc.getArgumentForName("fileName")
94+
)
95+
}
96+
}
97+
98+
/**
99+
* An argument to `GetFileName`.
100+
*
101+
* This is considered a sanitizer because it extracts just the file name, not the full path.
102+
*/
103+
class GetFileNameSanitizer extends Sanitizer {
104+
GetFileNameSanitizer() {
105+
exists(MethodCall mc |
106+
mc.getTarget().hasQualifiedName("System.IO.Path", "GetFileName") |
107+
this.asExpr() = mc.getAnArgument()
108+
)
109+
}
110+
}
111+
112+
/**
113+
* A qualifier in a call to `StartsWith` or `Substring` string method.
114+
*
115+
* A call to a String method such as `StartsWith` or `Substring` can indicate a check for a
116+
* relative path, or a check against the destination folder for whitelisted/target path, etc.
117+
*/
118+
class StringCheckSanitizer extends Sanitizer {
119+
StringCheckSanitizer() {
120+
exists(MethodCall mc |
121+
mc.getTarget().hasQualifiedName("System.String", "StartsWith") or
122+
mc.getTarget().hasQualifiedName("System.String", "Substring") |
123+
this.asExpr() = mc.getQualifier()
124+
)
125+
}
126+
}
127+
}

‎csharp/ql/test/query-tests/Security Features/CWE-022/ZipSlip/ZipSlip.expected‎

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
1-
| ZipSlip.cs:24:41:24:52 | access to local variable destFileName | Make sure to sanitize relative archive item path before creating path for file extraction if the source of $@ is untrusted | ZipSlip.cs:19:31:19:44 | access to property FullName | zip archive |
2-
| ZipSlip.cs:32:41:32:52 | access to local variable destFilePath | Make sure to sanitize relative archive item path before creating path for file extraction if the source of $@ is untrusted | ZipSlip.cs:16:52:16:65 | access to property FullName | zip archive |
3-
| ZipSlip.cs:61:74:61:85 | access to local variable destFilePath | Make sure to sanitize relative archive item path before creating path for file extraction if the source of $@ is untrusted | ZipSlip.cs:54:72:54:85 | access to property FullName | zip archive |
4-
| ZipSlip.cs:68:71:68:82 | access to local variable destFilePath | Make sure to sanitize relative archive item path before creating path for file extraction if the source of $@ is untrusted | ZipSlip.cs:54:72:54:85 | access to property FullName | zip archive |
5-
| ZipSlip.cs:75:57:75:68 | access to local variable destFilePath | Make sure to sanitize relative archive item path before creating path for file extraction if the source of $@ is untrusted | ZipSlip.cs:54:72:54:85 | access to property FullName | zip archive |
6-
| ZipSlip.cs:83:58:83:69 | access to local variable destFilePath | Make sure to sanitize relative archive item path before creating path for file extraction if the source of $@ is untrusted | ZipSlip.cs:54:72:54:85 | access to property FullName | zip archive |
1+
| ZipSlip.cs:24:41:24:52 | access to local variable destFileName | Unsanitized zip archive $@, which may contain '..', is used in a file system operation. | ZipSlip.cs:19:31:19:44 | access to property FullName | item path |
2+
| ZipSlip.cs:32:41:32:52 | access to local variable destFilePath | Unsanitized zip archive $@, which may contain '..', is used in a file system operation. | ZipSlip.cs:16:52:16:65 | access to property FullName | item path |
3+
| ZipSlip.cs:61:74:61:85 | access to local variable destFilePath | Unsanitized zip archive $@, which may contain '..', is used in a file system operation. | ZipSlip.cs:54:72:54:85 | access to property FullName | item path |
4+
| ZipSlip.cs:68:71:68:82 | access to local variable destFilePath | Unsanitized zip archive $@, which may contain '..', is used in a file system operation. | ZipSlip.cs:54:72:54:85 | access to property FullName | item path |
5+
| ZipSlip.cs:75:57:75:68 | access to local variable destFilePath | Unsanitized zip archive $@, which may contain '..', is used in a file system operation. | ZipSlip.cs:54:72:54:85 | access to property FullName | item path |
6+
| ZipSlip.cs:83:58:83:69 | access to local variable destFilePath | Unsanitized zip archive $@, which may contain '..', is used in a file system operation. | ZipSlip.cs:54:72:54:85 | access to property FullName | item path |
7+
| ZipSlipBad.cs:10:29:10:40 | access to local variable destFileName | Unsanitized zip archive $@, which may contain '..', is used in a file system operation. | ZipSlipBad.cs:9:59:9:72 | access to property FullName | item path |

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

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
using System.IO;
2+
using System.IO.Compression;
3+
4+
class Bad
5+
{
6+
public static void WriteToDirectory(ZipArchiveEntry entry,
7+
string destDirectory)
8+
{
9+
string destFileName = Path.Combine(destDirectory, entry.FullName);
10+
entry.ExtractToFile(destFileName);
11+
}
12+
}

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

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
using System.IO;
2+
using System.IO.Compression;
3+
4+
class Good
5+
{
6+
public static void WriteToDirectory(ZipArchiveEntry entry,
7+
string destDirectory)
8+
{
9+
string destFileName = Path.GetFullPath(Path.Combine(destDirectory, entry.FullName));
10+
string fullDestDirPath = Path.GetFullPath(destDirectory + Path.DirectorySeparatorChar);
11+
if (!destFileName.StartsWith(fullDestDirPath)) {
12+
throw new System.InvalidOperationException("Entry is outside the target dir: " +
13+
destFileName);
14+
}
15+
entry.ExtractToFile(destFileName);
16+
}
17+
}

0 commit comments

Comments
 (0)