Skip to content

Commit 6589813

Browse files
committed
JavaScript: Add tar-stream extraction to ZipSlip query.
1 parent 5a3cf2c commit 6589813

3 files changed

Lines changed: 28 additions & 6 deletions

File tree

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

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,8 @@ module ZipSlip {
4646
private DataFlow::SourceNode parsedArchive() {
4747
result = DataFlow::moduleImport("unzip").getAMemberCall("Parse")
4848
or
49+
result = DataFlow::moduleImport("tar-stream").getAMemberCall("extract")
50+
or
4951
// `streamProducer.pipe(unzip.Parse())` is a typical (but not
5052
// universal) pattern when using nodejs streams, whose return
5153
// value is the parsed stream.
@@ -56,6 +58,9 @@ module ZipSlip {
5658
)
5759
}
5860

61+
/** Gets a property that is used to get the filename part of an archive entry. */
62+
private string getAFilenameProperty() { result = "path" or result = "name" }
63+
5964
/** A zip archive entry path access, as a source for unsafe zip extraction. */
6065
class UnzipEntrySource extends Source {
6166
// For example, in
@@ -74,9 +79,8 @@ module ZipSlip {
7479
exists(DataFlow::CallNode cn |
7580
cn = parsedArchive().getAMemberCall("on") and
7681
cn.getArgument(0).mayHaveStringValue("entry") and
77-
this = cn.getCallback(1)
78-
.getParameter(0)
79-
.getAPropertyRead("path"))
82+
this = cn.getCallback(1).getParameter(0).getAPropertyRead(getAFilenameProperty())
83+
)
8084
}
8185
}
8286

@@ -99,9 +103,7 @@ module ZipSlip {
99103

100104
/** An expression that sanitizes by calling path.basename */
101105
class BasenameSanitizer extends Sanitizer {
102-
BasenameSanitizer() {
103-
this = DataFlow::moduleImport("path").getAMemberCall("basename")
104-
}
106+
BasenameSanitizer() { this = DataFlow::moduleImport("path").getAMemberCall("basename") }
105107
}
106108

107109
/**
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: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
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 |
@@ -13,5 +14,6 @@ edges
1314
| ZipSlipBad.js:7:11:7:31 | fileName | ZipSlipBad.js:8:37:8:44 | fileName |
1415
| ZipSlipBad.js:7:22:7:31 | entry.path | ZipSlipBad.js:7:11:7:31 | fileName |
1516
#select
17+
| 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 |
1618
| 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 |
1719
| 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 |

0 commit comments

Comments
 (0)