Skip to content

Commit fa81084

Browse files
semmle-qlcisemmle-qlci
authored
Merge pull request #330 from aschackmull/java/zipslip
Approved by yh-semmle
2 parents f00863f + c3f71c2 commit fa81084

File tree

9 files changed

+319
-2
lines changed

9 files changed

+319
-2
lines changed

‎change-notes/1.19/analysis-java.md‎

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

77
| **Query** | **Tags** | **Purpose** |
88
|-----------------------------|-----------|--------------------------------------------------------------------|
9+
| Arbitrary file write during archive extraction ("Zip Slip") (`java/zipslip`) | security, external/cwe/cwe-022 | Identifies extraction routines that allow arbitrary file overwrite vulnerabilities. |
910
| Missing catch of NumberFormatException (`java/uncaught-number-format-exception`) | reliability, external/cwe/cwe-248 | Finds calls to `Integer.parseInt` and similar string-to-number conversions that might raise a `NumberFormatException` without a corresponding `catch`-clause. |
1011

1112
## Changes to existing queries

‎java/ql/src/Security/CWE/CWE-022/ZipSlip.qhelp‎

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,70 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
<overview>
6+
<p>Extracting files from a malicious zip archive (or another archive format)
7+
without validating that the destination file path
8+
is within the destination directory can cause files outside the destination directory to be
9+
overwritten, due to the possible presence of directory traversal elements (<code>..</code>) in
10+
archive paths.</p>
11+
12+
<p>Zip archives contain archive entries representing each file in the archive. These entries
13+
include a file path for the entry, but these file paths are not restricted and may contain
14+
unexpected special elements such as the directory traversal element (<code>..</code>). If these
15+
file paths are used to determine an output file to write the contents of the archive item to, then
16+
the file may be written to an unexpected location. This can result in sensitive information being
17+
revealed or deleted, or an attacker being able to influence behavior by modifying unexpected
18+
files.</p>
19+
20+
<p>For example, if a zip file contains a file entry <code>..\sneaky-file</code>, and the zip file
21+
is extracted to the directory <code>c:\output</code>, then naively combining the paths would result
22+
in an output file path of <code>c:\output\..\sneaky-file</code>, which would cause the file to be
23+
written to <code>c:\sneaky-file</code>.</p>
24+
25+
</overview>
26+
<recommendation>
27+
28+
<p>Ensure that output paths constructed from zip archive entries are validated to prevent writing
29+
files to unexpected locations.</p>
30+
31+
<p>The recommended way of writing an output file from a zip archive entry is to
32+
verify that the normalized full path of the output file starts with a prefix that matches the
33+
destination directory. Path normalization can be done with either
34+
<code>java.io.File.getCanonicalFile()</code> or <code>java.nio.file.Path.normalize()</code>.
35+
Prefix checking can be done with <code>String.startsWith(..)</code>, but it is better to use
36+
<code>java.nio.file.Path.startsWith(..)</code>, as the latter works on complete path segments.
37+
</p>
38+
39+
<p>Another alternative is to validate archive entries against a whitelist of expected files.</p>
40+
41+
</recommendation>
42+
<example>
43+
44+
<p>In this example, a file path taken from a zip archive item entry is combined with a
45+
destination directory. The result is used as the destination file path without verifying that
46+
the result is within the destination directory. If provided with a zip file containing an archive
47+
path like <code>..\sneaky-file</code>, then this file would be written outside the destination
48+
directory.</p>
49+
50+
<sample src="ZipSlipBad.java" />
51+
52+
<p>To fix this vulnerability, we need to verify that the normalized <code>file</code> still has
53+
<code>destinationDir</code> as its prefix, and throw an exception if this is not the case.</p>
54+
55+
<sample src="ZipSlipGood.java" />
56+
57+
</example>
58+
<references>
59+
60+
<li>
61+
Snyk:
62+
<a href="https://snyk.io/research/zip-slip-vulnerability">Zip Slip Vulnerability</a>.
63+
</li>
64+
<li>
65+
OWASP:
66+
<a href="https://www.owasp.org/index.php/Path_traversal">Path Traversal</a>.
67+
</li>
68+
69+
</references>
70+
</qhelp>

‎java/ql/src/Security/CWE/CWE-022/ZipSlip.ql‎

Lines changed: 176 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,176 @@
1+
/**
2+
* @name Arbitrary file write during archive extraction ("Zip Slip")
3+
* @description Extracting files from a malicious 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.
6+
* @kind problem
7+
* @id java/zipslip
8+
* @problem.severity error
9+
* @precision high
10+
* @tags security
11+
* external/cwe/cwe-022
12+
*/
13+
14+
import java
15+
import semmle.code.java.controlflow.Guards
16+
import semmle.code.java.dataflow.SSA
17+
import semmle.code.java.dataflow.TaintTracking
18+
import DataFlow
19+
20+
/**
21+
* A method that returns the name of an archive entry.
22+
*/
23+
class ArchiveEntryNameMethod extends Method {
24+
ArchiveEntryNameMethod() {
25+
exists(RefType archiveEntry |
26+
archiveEntry.hasQualifiedName("java.util.zip", "ZipEntry") or
27+
archiveEntry.hasQualifiedName("org.apache.commons.compress.archivers", "ArchiveEntry")
28+
|
29+
this.getDeclaringType().getASupertype*() = archiveEntry and
30+
this.hasName("getName")
31+
)
32+
}
33+
}
34+
35+
/**
36+
* An expression that will be treated as the destination of a write.
37+
*/
38+
class WrittenFileName extends Expr {
39+
WrittenFileName() {
40+
// Constructors that write to their first argument.
41+
exists(ConstructorCall ctr | this = ctr.getArgument(0) |
42+
exists(Class c | ctr.getConstructor() = c.getAConstructor() |
43+
c.hasQualifiedName("java.io", "FileOutputStream") or
44+
c.hasQualifiedName("java.io", "RandomAccessFile") or
45+
c.hasQualifiedName("java.io", "FileWriter")
46+
)
47+
)
48+
or
49+
// Methods that write to their n'th argument
50+
exists(MethodAccess call, int n | this = call.getArgument(n) |
51+
call.getMethod().getDeclaringType().hasQualifiedName("java.nio.file", "Files") and
52+
(
53+
call.getMethod().getName().regexpMatch("new.*Reader|newOutputStream|create.*") and n = 0
54+
or
55+
call.getMethod().hasName("copy") and n = 1
56+
or
57+
call.getMethod().hasName("move") and n = 1
58+
)
59+
)
60+
}
61+
}
62+
63+
/**
64+
* Holds if `n1` to `n2` is a dataflow step that converts between `String`,
65+
* `File`, and `Path`.
66+
*/
67+
predicate filePathStep(ExprNode n1, ExprNode n2) {
68+
exists(ConstructorCall cc | cc.getConstructedType() instanceof TypeFile |
69+
n1.asExpr() = cc.getAnArgument() and
70+
n2.asExpr() = cc
71+
)
72+
or
73+
exists(MethodAccess ma, Method m |
74+
ma.getMethod() = m and
75+
n1.asExpr() = ma.getQualifier() and
76+
n2.asExpr() = ma
77+
|
78+
m.getDeclaringType() instanceof TypeFile and m.hasName("toPath")
79+
or
80+
m.getDeclaringType() instanceof TypePath and m.hasName("toAbsolutePath")
81+
)
82+
}
83+
84+
predicate fileTaintStep(ExprNode n1, ExprNode n2) {
85+
exists(MethodAccess ma, Method m |
86+
n1.asExpr() = ma.getQualifier() or
87+
n1.asExpr() = ma.getAnArgument()
88+
|
89+
n2.asExpr() = ma and
90+
ma.getMethod() = m and
91+
m.getDeclaringType() instanceof TypePath and
92+
m.hasName("resolve")
93+
)
94+
}
95+
96+
predicate localFileValueStep(Node n1, Node n2) {
97+
localFlowStep(n1, n2) or
98+
filePathStep(n1, n2)
99+
}
100+
101+
predicate localFileValueStepPlus(Node n1, Node n2) = fastTC(localFileValueStep/2)(n1, n2)
102+
103+
/**
104+
* Holds if `check` is a guard that checks whether `var` is a file path with a
105+
* specific prefix when put in canonical form, thus guarding against ZipSlip.
106+
*/
107+
predicate validateFilePath(SsaVariable var, Guard check) {
108+
// `var.getCanonicalFile().toPath().startsWith(...)`,
109+
// `var.getCanonicalPath().startsWith(...)`, or
110+
// `var.toPath().normalize().startsWith(...)`
111+
exists(MethodAccess normalize, MethodAccess startsWith, Node n1, Node n2, Node n3, Node n4 |
112+
n1.asExpr() = var.getAUse() and
113+
n2.asExpr() = normalize.getQualifier() and
114+
(n1 = n2 or localFileValueStepPlus(n1, n2)) and
115+
n3.asExpr() = normalize and
116+
n4.asExpr() = startsWith.getQualifier() and
117+
(n3 = n4 or localFileValueStepPlus(n3, n4)) and
118+
check = startsWith and
119+
startsWith.getMethod().hasName("startsWith") and
120+
(
121+
normalize.getMethod().hasName("getCanonicalFile") or
122+
normalize.getMethod().hasName("getCanonicalPath") or
123+
normalize.getMethod().hasName("normalize")
124+
)
125+
)
126+
}
127+
128+
/**
129+
* Holds if `m` validates its `arg`th parameter.
130+
*/
131+
predicate validationMethod(Method m, int arg) {
132+
exists(Guard check, SsaImplicitInit var, ControlFlowNode exit, ControlFlowNode normexit |
133+
validateFilePath(var, check) and
134+
var.isParameterDefinition(m.getParameter(arg)) and
135+
exit = m and
136+
normexit.getANormalSuccessor() = exit and
137+
1 = strictcount(ControlFlowNode n | n.getANormalSuccessor() = exit)
138+
|
139+
check.(ConditionNode).getATrueSuccessor() = exit or
140+
check.controls(normexit.getBasicBlock(), true)
141+
)
142+
}
143+
144+
class ZipSlipConfiguration extends TaintTracking::Configuration {
145+
ZipSlipConfiguration() { this = "ZipSlip" }
146+
147+
override predicate isSource(Node source) {
148+
source.asExpr().(MethodAccess).getMethod() instanceof ArchiveEntryNameMethod
149+
}
150+
151+
override predicate isSink(Node sink) { sink.asExpr() instanceof WrittenFileName }
152+
153+
override predicate isAdditionalTaintStep(Node n1, Node n2) {
154+
filePathStep(n1, n2) or fileTaintStep(n1, n2)
155+
}
156+
157+
override predicate isSanitizer(Node node) {
158+
exists(Guard g, SsaVariable var, RValue varuse | validateFilePath(var, g) |
159+
varuse = node.asExpr() and
160+
varuse = var.getAUse() and
161+
g.controls(varuse.getBasicBlock(), true)
162+
)
163+
or
164+
exists(MethodAccess ma, int pos, RValue rv |
165+
validationMethod(ma.getMethod(), pos) and
166+
ma.getArgument(pos) = rv and
167+
adjacentUseUseSameVar(rv, node.asExpr()) and
168+
ma.getBasicBlock().bbDominates(node.asExpr().getBasicBlock())
169+
)
170+
}
171+
}
172+
173+
from Node source, Node sink
174+
where any(ZipSlipConfiguration c).hasFlow(source, sink)
175+
select source, "Unsanitized archive entry, which may contain '..', is used in a $@.", sink,
176+
"file system operation"

‎java/ql/src/Security/CWE/CWE-022/ZipSlipBad.java‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
void writeZipEntry(ZipEntry entry, File destinationDir) {
2+
File file = new File(destinationDir, entry.getName());
3+
FileOutputStream fos = new FileOutputStream(file); // BAD
4+
// ... write entry to fos ...
5+
}

‎java/ql/src/Security/CWE/CWE-022/ZipSlipGood.java‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
void writeZipEntry(ZipEntry entry, File destinationDir) {
2+
File file = new File(destinationDir, entry.getName());
3+
if (!file.toPath().normalize().startsWith(destinationDir.toPath()))
4+
throw new Exception("Bad zip entry");
5+
FileOutputStream fos = new FileOutputStream(file); // OK
6+
// ... write entry to fos ...
7+
}

‎java/ql/src/semmle/code/java/JDK.qll‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -133,8 +133,8 @@ class TypeObjectOutputStream extends RefType {
133133
/** The class `java.nio.file.Paths`. */
134134
class TypePaths extends Class { TypePaths() { this.hasQualifiedName("java.nio.file", "Paths") } }
135135

136-
/** The class `java.nio.file.Path`. */
137-
class TypePath extends Class { TypePath() { this.hasQualifiedName("java.nio.file", "Path") } }
136+
/** The type `java.nio.file.Path`. */
137+
class TypePath extends RefType { TypePath() { this.hasQualifiedName("java.nio.file", "Path") } }
138138

139139
/** The class `java.nio.file.FileSystem`. */
140140
class TypeFileSystem extends Class {

‎java/ql/test/query-tests/security/CWE-022/semmle/tests/ZipSlip.expected‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
| ZipTest.java:7:19:7:33 | getName(...) | Unsanitized archive entry, which may contain '..', is used in a $@. | ZipTest.java:9:48:9:51 | file | file system operation |
2+
| ZipTest.java:7:19:7:33 | getName(...) | Unsanitized archive entry, which may contain '..', is used in a $@. | ZipTest.java:10:49:10:52 | file | file system operation |
3+
| ZipTest.java:7:19:7:33 | getName(...) | Unsanitized archive entry, which may contain '..', is used in a $@. | ZipTest.java:11:36:11:39 | file | file system operation |

‎java/ql/test/query-tests/security/CWE-022/semmle/tests/ZipSlip.qlref‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Security/CWE/CWE-022/ZipSlip.ql

‎java/ql/test/query-tests/security/CWE-022/semmle/tests/ZipTest.java‎

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
import java.io.*;
2+
import java.nio.file.*;
3+
import java.util.zip.*;
4+
5+
public class ZipTest {
6+
public void m1(ZipEntry entry, File dir) {
7+
String name = entry.getName();
8+
File file = new File(dir, name);
9+
FileOutputStream os = new FileOutputStream(file); // ZipSlip
10+
RandomAccessFile raf = new RandomAccessFile(file, "rw"); // ZipSlip
11+
FileWriter fw = new FileWriter(file); // ZipSlip
12+
}
13+
14+
public void m2(ZipEntry entry, File dir) {
15+
String name = entry.getName();
16+
File file = new File(dir, name);
17+
File canFile = file.getCanonicalFile();
18+
String canDir = dir.getCanonicalPath();
19+
if (!canFile.toPath().startsWith(canDir))
20+
throw new Exception();
21+
FileOutputStream os = new FileOutputStream(file); // OK
22+
}
23+
24+
public void m3(ZipEntry entry, File dir) {
25+
String name = entry.getName();
26+
File file = new File(dir, name);
27+
if (!file.toPath().normalize().startsWith(dir.toPath()))
28+
throw new Exception();
29+
FileOutputStream os = new FileOutputStream(file); // OK
30+
}
31+
32+
private void validate(File tgtdir, File file) {
33+
File canFile = file.getCanonicalFile();
34+
if (!canFile.toPath().startsWith(tgtdir.toPath()))
35+
throw new Exception();
36+
}
37+
38+
public void m4(ZipEntry entry, File dir) {
39+
String name = entry.getName();
40+
File file = new File(dir, name);
41+
validate(dir, file);
42+
FileOutputStream os = new FileOutputStream(file); // OK
43+
}
44+
45+
public void m5(ZipEntry entry, File dir) {
46+
String name = entry.getName();
47+
File file = new File(dir, name);
48+
Path absfile = file.toPath().toAbsolutePath().normalize();
49+
Path absdir = dir.toPath().toAbsolutePath().normalize();
50+
if (!absfile.startsWith(absdir))
51+
throw new Exception();
52+
FileOutputStream os = new FileOutputStream(file); // OK
53+
}
54+
}

0 commit comments

Comments
 (0)