Skip to content

Commit bb2c853

Browse files
jcreedcmujcreedcmu
committed
JavaScript: Add new query (CWE-022).
1 parent be3191a commit bb2c853

8 files changed

Lines changed: 264 additions & 0 deletions

File tree

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

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
6+
<overview>
7+
<p>Extracting files from a malicious zip archive 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+
</overview>
20+
21+
<recommendation>
22+
23+
<p>Ensure that output paths constructed from zip archive entries are validated
24+
to prevent writing files to unexpected locations.</p>
25+
26+
</recommendation>
27+
28+
<example>
29+
<p>
30+
Here is an example of extracting an archive without validating
31+
filenames. If <code>archive.zip</code> contained relative paths (for
32+
instance, if it were created by something like <code>zip archive.zip
33+
../file.txt</code>) then executing this code would write to those paths.
34+
</p>
35+
36+
<sample src="ZipSlipBad.js" />
37+
</example>
38+
</qhelp>

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

Lines changed: 104 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,104 @@
1+
/**
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.
6+
* @kind path-problem
7+
* @id cs/zipslip
8+
* @problem.severity error
9+
* @precision high
10+
* @tags security
11+
* external/cwe/cwe-022
12+
*/
13+
14+
import javascript
15+
import semmle.javascript.security.dataflow.ZipSlip::ZipSlip
16+
import DataFlow::PathGraph
17+
18+
from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink
19+
where cfg.hasFlowPath(source, sink)
20+
select sink.getNode(), source, sink,
21+
"Unsanitized zip archive $@, which may contain '..', is used in a file system operation.",
22+
source.getNode(), "item path"
23+
24+
25+
26+
// class Configuration extends TaintTracking::Configuration {
27+
// Configuration() { this = "TarSlip" }
28+
29+
// override predicate isSource(DataFlow::Node nd) {
30+
// isEntrySource(nd)
31+
// }
32+
33+
// override predicate isSink(DataFlow::Node sink) {
34+
// isFuncSink(sink.asExpr())
35+
// }
36+
37+
// override predicate isSanitizerGuard(TaintTracking::SanitizerGuardNode nd) {
38+
// nd instanceof AbsentStringSanitizer
39+
// }
40+
// }
41+
42+
// /**
43+
// * A guard that suffices to sanitize a value by establishing that it
44+
// * does *not* contain a certain bad substring. For example,
45+
// *
46+
// * if (s.indexOf("..") == -1) { ... }
47+
// *
48+
// * is considered to sanitize s.
49+
// */
50+
// class AbsentStringSanitizer extends TaintTracking::SanitizerGuardNode, DataFlow::ValueNode {
51+
// MethodCallExpr indexOf;
52+
// override EqualityTest astNode;
53+
54+
// AbsentStringSanitizer() {
55+
// indexOf.getMethodName() = "indexOf" and
56+
// astNode.getAnOperand().getIntValue() = -1 and
57+
// astNode.getAnOperand() = indexOf
58+
// }
59+
60+
// override predicate sanitizes(boolean outcome, Expr e) {
61+
// outcome = true and
62+
// e = indexOf.getReceiver()
63+
// }
64+
// }
65+
66+
// /**
67+
// * Holds if `nd` is the argument of a tar-archive file-entry event
68+
// * callback that contains the main bundle of metadata about the file
69+
// * entry, which includes its file name.
70+
// */
71+
// predicate isEntrySource(DataFlow::Node nd) {
72+
// exists(MethodCallExpr mce |
73+
// mce.getMethodName() = "on"
74+
// and mce.getArgument(0).(StringLiteral).getStringValue() = "entry"
75+
// and DataFlow::parameterNode(mce.getArgument(1).(Function).getParameter(0)) = nd
76+
// )
77+
// }
78+
79+
// /**
80+
// * Holds if `s` is the name of a method whose first argument is
81+
// * a filename that may be written to.
82+
// */
83+
// predicate isFileWritingMethod(string s) {
84+
// /* FIXME: Perhaps this should be unified with the related (private)
85+
// predicates in semmle.javascript.frameworks.NodeJSLib */
86+
// s = "createWriteStream" or
87+
// s = "writeFile" or
88+
// s = "writeFileSync"
89+
// }
90+
91+
// /**
92+
// * Holds if `e` is an expression that is at risk of
93+
// * being used as a filename which is written to.
94+
// */
95+
// predicate isFuncSink(Expr e) {
96+
// exists(MethodCallExpr mce |
97+
// e = mce.getArgument(0) and
98+
// isFileWritingMethod(mce.getMethodName())
99+
// )
100+
// }
101+
102+
// from DataFlow::Node src, DataFlow::Node tgt, Configuration cfg
103+
// where cfg.hasFlow(src, tgt)
104+
// select src, tgt

‎javascript/ql/src/Security/CWE-022/ZipSlipBad.js‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
const fs = require('fs');
2+
const unzip = require('unzip');
3+
4+
fs.createReadStream('archive.zip')
5+
.pipe(unzip.Parse())
6+
.on('entry', entry => {
7+
const fileName = entry.path;
8+
entry.pipe(fs.createWriteStream(entry.path));
9+
});

‎javascript/ql/src/semmle/javascript/security/dataflow/ZipSlip.qll‎

Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,84 @@
1+
/**
2+
* Provides a taint tracking configuration for reasoning about unsafe zip extraction.
3+
*/
4+
5+
import javascript
6+
7+
module ZipSlip {
8+
/**
9+
* A data flow source for unsafe zip extraction.
10+
*/
11+
abstract class Source extends DataFlow::Node { }
12+
13+
/**
14+
* A data flow sink for unsafe zip extraction.
15+
*/
16+
abstract class Sink extends DataFlow::Node { }
17+
18+
/**
19+
* A sanitizer guard for unsafe zip extraction.
20+
*/
21+
abstract class SanitizerGuard extends
22+
TaintTracking::SanitizerGuardNode,
23+
DataFlow::ValueNode { }
24+
25+
/** A taint tracking configuration for Zip Slip */
26+
class Configuration extends TaintTracking::Configuration {
27+
Configuration() { this = "ZipSlip" }
28+
29+
override predicate isSource(DataFlow::Node source) { source instanceof Source }
30+
31+
override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
32+
33+
override predicate isSanitizerGuard(TaintTracking::SanitizerGuardNode nd) {
34+
nd instanceof SanitizerGuard
35+
}
36+
}
37+
38+
/**
39+
* An access to the filepath of an entry of a zipfile being extracted by
40+
* npm module `unzip`.
41+
*/
42+
class UnzipEntrySource extends Source {
43+
UnzipEntrySource() {
44+
exists(DataFlow::MethodCallNode pipe, DataFlow::MethodCallNode on |
45+
pipe.getMethodName() = "pipe"
46+
and pipe.getArgument(0) = DataFlow::moduleImport("unzip").getAMemberCall("Parse")
47+
and on = pipe.getAMemberCall("on")
48+
and this = on.getCallback(1).getParameter(0).getAPropertyRead("path"))
49+
}
50+
}
51+
52+
/**
53+
* A sink that is the path that a createWriteStream gets created at.
54+
* This is not covered by FileSystemWriteSink, because it is
55+
* required that a write actually takes place to the stream.
56+
* However, we want to consider even the bare createWriteStream to
57+
* be a zipslip vulnerability since it may truncate an existing file.
58+
*/
59+
class CreateWriteStreamSink extends Sink {
60+
CreateWriteStreamSink() {
61+
this = DataFlow::moduleImport("fs").getAMemberCall("createWriteStream").getArgument(0)
62+
}
63+
}
64+
65+
/** A sink that is a file path that gets written to. */
66+
class FileSystemWriteSink extends Sink {
67+
FileSystemWriteSink() {
68+
exists(FileSystemWriteAccess fsw | fsw.getAPathArgument() = this)
69+
}
70+
}
71+
72+
/** A check that a path string does not include '..' */
73+
class NoParentDirSanitizerGuard extends SanitizerGuard {
74+
StringOps::Includes incl;
75+
76+
NoParentDirSanitizerGuard() { this = incl }
77+
78+
override predicate sanitizes(boolean outcome, Expr e) {
79+
incl.getPolarity().booleanNot() = outcome
80+
and incl.getBaseString().asExpr() = e
81+
and incl.getSubstring().mayHaveStringValue("..")
82+
}
83+
}
84+
}

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

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
nodes
2+
| ZipSlipBad.js:8:37:8:46 | entry.path |
3+
edges
4+
#select
5+
| ZipSlipBad.js:8:37:8:46 | entry.path | ZipSlipBad.js:8:37:8:46 | entry.path | ZipSlipBad.js:8:37:8:46 | entry.path | Unsanitized zip archive $@, which may contain '..', is used in a file system operation. | ZipSlipBad.js:8:37:8:46 | entry.path | item path |

‎javascript/ql/test/query-tests/Security/CWE-022/ZipSlip/ZipSlip.qlref‎

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

‎javascript/ql/test/query-tests/Security/CWE-022/ZipSlip/ZipSlipBad.js‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
const fs = require('fs');
2+
const unzip = require('unzip');
3+
4+
fs.createReadStream('archive.zip')
5+
.pipe(unzip.Parse())
6+
.on('entry', entry => {
7+
const fileName = entry.path;
8+
entry.pipe(fs.createWriteStream(entry.path));
9+
});

‎javascript/ql/test/query-tests/Security/CWE-022/ZipSlip/ZipSlipGood.js‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
const fs = require('fs');
2+
const unzip = require('unzip');
3+
4+
fs.createReadStream('archive.zip')
5+
.pipe(unzip.Parse())
6+
.on('entry', entry => {
7+
const fileName = entry.path;
8+
if (entry.path.indexOf('..') == -1) {
9+
entry.pipe(fs.createWriteStream(entry.path));
10+
}
11+
else {
12+
console.log('skipping bad path', entry.path);
13+
}
14+
});

0 commit comments

Comments
 (0)