Skip to content

Commit 285f8b0

Browse files
semmle-qlcisemmle-qlci
authored
Merge pull request #1118 from jcreedcmu/jcreed/tarslip
Approved by xiemaisi
2 parents 5a3cf2c + 4475dd4 commit 285f8b0

File tree

5 files changed

+61
-16
lines changed

5 files changed

+61
-16
lines changed

‎change-notes/1.21/analysis-javascript.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,5 +19,6 @@
1919
|--------------------------------|------------------------------|---------------------------------------------------------------------------|
2020
| Expression has no effect | Fewer false-positive results | This rule now treats uses of `Object.defineProperty` more conservatively. |
2121
| Useless assignment to property | Fewer false-positive results | This rule now ignore reads of additional getters. |
22+
| Arbitrary file write during zip extraction ("Zip Slip") | More results | This rule now considers more libraries, including tar as well as zip. |
2223

2324
## Changes to QL libraries

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

Lines changed: 25 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,31 +1,32 @@
11
/**
2-
* Provides a taint tracking configuration for reasoning about unsafe zip extraction.
2+
* Provides a taint tracking configuration for reasoning about unsafe
3+
* zip and tar archive extraction.
34
*/
45

56
import javascript
67

78
module ZipSlip {
89
/**
9-
* A data flow source for unsafe zip extraction.
10+
* A data flow source for unsafe archive extraction.
1011
*/
1112
abstract class Source extends DataFlow::Node { }
1213

1314
/**
14-
* A data flow sink for unsafe zip extraction.
15+
* A data flow sink for unsafe archive extraction.
1516
*/
1617
abstract class Sink extends DataFlow::Node { }
1718

1819
/**
19-
* A sanitizer for unsafe zip extraction.
20+
* A sanitizer for unsafe archive extraction.
2021
*/
2122
abstract class Sanitizer extends DataFlow::Node { }
2223

2324
/**
24-
* A sanitizer guard for unsafe zip extraction.
25+
* A sanitizer guard for unsafe archive extraction.
2526
*/
2627
abstract class SanitizerGuard extends TaintTracking::SanitizerGuardNode, DataFlow::ValueNode { }
2728

28-
/** A taint tracking configuration for unsafe zip extraction. */
29+
/** A taint tracking configuration for unsafe archive extraction. */
2930
class Configuration extends TaintTracking::Configuration {
3031
Configuration() { this = "ZipSlip" }
3132

@@ -41,11 +42,15 @@ module ZipSlip {
4142
}
4243

4344
/**
44-
* Gets a node that can be a parsed zip archive.
45+
* Gets a node that can be a parsed archive.
4546
*/
4647
private DataFlow::SourceNode parsedArchive() {
48+
result = DataFlow::moduleImport("unzipper").getAMemberCall("Parse")
49+
or
4750
result = DataFlow::moduleImport("unzip").getAMemberCall("Parse")
4851
or
52+
result = DataFlow::moduleImport("tar-stream").getAMemberCall("extract")
53+
or
4954
// `streamProducer.pipe(unzip.Parse())` is a typical (but not
5055
// universal) pattern when using nodejs streams, whose return
5156
// value is the parsed stream.
@@ -56,7 +61,14 @@ module ZipSlip {
5661
)
5762
}
5863

59-
/** A zip archive entry path access, as a source for unsafe zip extraction. */
64+
/** Gets a property that is used to get the filename part of an archive entry. */
65+
private string getAFilenameProperty() {
66+
result = "path" // Used by library 'unzip'.
67+
or
68+
result = "name" // Used by library 'tar-stream'.
69+
}
70+
71+
/** An archive entry path access, as a source for unsafe archive extraction. */
6072
class UnzipEntrySource extends Source {
6173
// For example, in
6274
// ```javascript
@@ -74,13 +86,12 @@ module ZipSlip {
7486
exists(DataFlow::CallNode cn |
7587
cn = parsedArchive().getAMemberCall("on") and
7688
cn.getArgument(0).mayHaveStringValue("entry") and
77-
this = cn.getCallback(1)
78-
.getParameter(0)
79-
.getAPropertyRead("path"))
89+
this = cn.getCallback(1).getParameter(0).getAPropertyRead(getAFilenameProperty())
90+
)
8091
}
8192
}
8293

83-
/** A call to `fs.createWriteStream`, as a sink for unsafe zip extraction. */
94+
/** A call to `fs.createWriteStream`, as a sink for unsafe archive extraction. */
8495
class CreateWriteStreamSink extends Sink {
8596
CreateWriteStreamSink() {
8697
// This is not covered by `FileSystemWriteSink`, because it is
@@ -92,16 +103,14 @@ module ZipSlip {
92103
}
93104
}
94105

95-
/** A file path of a file write, as a sink for unsafe zip extraction. */
106+
/** A file path of a file write, as a sink for unsafe archive extraction. */
96107
class FileSystemWriteSink extends Sink {
97108
FileSystemWriteSink() { exists(FileSystemWriteAccess fsw | fsw.getAPathArgument() = this) }
98109
}
99110

100111
/** An expression that sanitizes by calling path.basename */
101112
class BasenameSanitizer extends Sanitizer {
102-
BasenameSanitizer() {
103-
this = DataFlow::moduleImport("path").getAMemberCall("basename")
104-
}
113+
BasenameSanitizer() { this = DataFlow::moduleImport("path").getAMemberCall("basename") }
105114
}
106115

107116
/**

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

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
const fs = require('fs');
2+
const tar = require('tar-stream');
3+
const extract = tar.extract();
4+
5+
extract.on('entry', (header, stream, next) => {
6+
const out = fs.createWriteStream(header.name);
7+
stream.pipe(out);
8+
stream.on('end', () => {
9+
next();
10+
})
11+
stream.resume();
12+
})
13+
14+
extract.on('finish', () => {
15+
console.log('finished');
16+
});
17+
18+
fs.createReadStream('./bad.tar').pipe(extract);

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

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,17 +1,25 @@
11
nodes
2+
| TarSlipBad.js:6:36:6:46 | header.name |
23
| ZipSlipBad2.js:5:9:5:46 | fileName |
34
| ZipSlipBad2.js:5:20:5:46 | 'output ... ry.path |
45
| ZipSlipBad2.js:5:37:5:46 | entry.path |
56
| ZipSlipBad2.js:6:22:6:29 | fileName |
67
| ZipSlipBad.js:7:11:7:31 | fileName |
78
| ZipSlipBad.js:7:22:7:31 | entry.path |
89
| ZipSlipBad.js:8:37:8:44 | fileName |
10+
| ZipSlipBadUnzipper.js:7:9:7:29 | fileName |
11+
| ZipSlipBadUnzipper.js:7:20:7:29 | entry.path |
12+
| ZipSlipBadUnzipper.js:8:37:8:44 | fileName |
913
edges
1014
| ZipSlipBad2.js:5:9:5:46 | fileName | ZipSlipBad2.js:6:22:6:29 | fileName |
1115
| ZipSlipBad2.js:5:20:5:46 | 'output ... ry.path | ZipSlipBad2.js:5:9:5:46 | fileName |
1216
| ZipSlipBad2.js:5:37:5:46 | entry.path | ZipSlipBad2.js:5:20:5:46 | 'output ... ry.path |
1317
| ZipSlipBad.js:7:11:7:31 | fileName | ZipSlipBad.js:8:37:8:44 | fileName |
1418
| ZipSlipBad.js:7:22:7:31 | entry.path | ZipSlipBad.js:7:11:7:31 | fileName |
19+
| ZipSlipBadUnzipper.js:7:9:7:29 | fileName | ZipSlipBadUnzipper.js:8:37:8:44 | fileName |
20+
| ZipSlipBadUnzipper.js:7:20:7:29 | entry.path | ZipSlipBadUnzipper.js:7:9:7:29 | fileName |
1521
#select
22+
| TarSlipBad.js:6:36:6:46 | header.name | TarSlipBad.js:6:36:6:46 | header.name | TarSlipBad.js:6:36:6:46 | header.name | Unsanitized zip archive $@, which may contain '..', is used in a file system operation. | TarSlipBad.js:6:36:6:46 | header.name | item path |
1623
| ZipSlipBad2.js:6:22:6:29 | fileName | ZipSlipBad2.js:5:37:5:46 | entry.path | ZipSlipBad2.js:6:22:6:29 | fileName | Unsanitized zip archive $@, which may contain '..', is used in a file system operation. | ZipSlipBad2.js:5:37:5:46 | entry.path | item path |
1724
| ZipSlipBad.js:8:37:8:44 | fileName | ZipSlipBad.js:7:22:7:31 | entry.path | ZipSlipBad.js:8:37:8:44 | fileName | Unsanitized zip archive $@, which may contain '..', is used in a file system operation. | ZipSlipBad.js:7:22:7:31 | entry.path | item path |
25+
| ZipSlipBadUnzipper.js:8:37:8:44 | fileName | ZipSlipBadUnzipper.js:7:20:7:29 | entry.path | ZipSlipBadUnzipper.js:8:37:8:44 | fileName | Unsanitized zip archive $@, which may contain '..', is used in a file system operation. | ZipSlipBadUnzipper.js:7:20:7:29 | entry.path | item path |

‎javascript/ql/test/query-tests/Security/CWE-022/ZipSlip/ZipSlipBadUnzipper.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 unzipper = require('unzipper');
3+
4+
fs.createReadStream('path/to/archive.zip')
5+
.pipe(unzipper.Parse())
6+
.on('entry', function (entry) {
7+
var fileName = entry.path;
8+
entry.pipe(fs.createWriteStream(fileName));
9+
});

0 commit comments

Comments
 (0)