Skip to content

Commit 4bbb7ad

Browse files
erik-krogherik-krogh
authored
Merge pull request #7876 from erik-krogh/zipRelative
JS: recognize more startswith sanitizers for path-injection queries
2 parents ade7921 + 28ba78c commit 4bbb7ad

File tree

3 files changed

+66
-31
lines changed

3 files changed

+66
-31
lines changed

‎javascript/ql/lib/semmle/javascript/security/dataflow/TaintedPathCustomizations.qll

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -513,13 +513,29 @@ module TaintedPath {
513513

514514
override predicate blocks(boolean outcome, Expr e) {
515515
member = "relative" and
516-
e = pathCall.getArgument(1).asExpr() and
516+
e = this.maybeGetPathSuffix(pathCall.getArgument(1)).asExpr() and
517517
outcome = startsWith.getPolarity().booleanNot()
518518
or
519519
not member = "relative" and
520-
e = pathCall.getArgument(0).asExpr() and
520+
e = this.maybeGetPathSuffix(pathCall.getArgument(0)).asExpr() and
521521
outcome = startsWith.getPolarity()
522522
}
523+
524+
/**
525+
* Gets the last argument to the given `path.join()` call,
526+
* or the node itself if it is not a join call.
527+
* Is used to get the suffix of the path.
528+
*/
529+
bindingset[e]
530+
private DataFlow::Node maybeGetPathSuffix(DataFlow::Node e) {
531+
exists(DataFlow::CallNode call |
532+
call = NodeJSLib::Path::moduleMember("join").getACall() and e = call
533+
|
534+
result = call.getLastArgument()
535+
)
536+
or
537+
result = e
538+
}
523539
}
524540

525541
/**

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

Lines changed: 28 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -45,18 +45,18 @@ nodes
4545
| ZipSlipBad.js:23:28:23:35 | fileName |
4646
| ZipSlipBad.js:23:28:23:35 | fileName |
4747
| ZipSlipBad.js:23:28:23:35 | fileName |
48-
| ZipSlipBad.js:29:14:29:17 | name |
49-
| ZipSlipBad.js:29:14:29:17 | name |
50-
| ZipSlipBad.js:29:14:29:17 | name |
51-
| ZipSlipBad.js:30:26:30:29 | name |
52-
| ZipSlipBad.js:30:26:30:29 | name |
53-
| ZipSlipBad.js:30:26:30:29 | name |
54-
| ZipSlipBad.js:33:16:33:19 | name |
55-
| ZipSlipBad.js:33:16:33:19 | name |
56-
| ZipSlipBad.js:33:16:33:19 | name |
57-
| ZipSlipBad.js:34:26:34:29 | name |
58-
| ZipSlipBad.js:34:26:34:29 | name |
59-
| ZipSlipBad.js:34:26:34:29 | name |
48+
| ZipSlipBad.js:30:14:30:17 | name |
49+
| ZipSlipBad.js:30:14:30:17 | name |
50+
| ZipSlipBad.js:30:14:30:17 | name |
51+
| ZipSlipBad.js:31:26:31:29 | name |
52+
| ZipSlipBad.js:31:26:31:29 | name |
53+
| ZipSlipBad.js:31:26:31:29 | name |
54+
| ZipSlipBad.js:34:16:34:19 | name |
55+
| ZipSlipBad.js:34:16:34:19 | name |
56+
| ZipSlipBad.js:34:16:34:19 | name |
57+
| ZipSlipBad.js:35:26:35:29 | name |
58+
| ZipSlipBad.js:35:26:35:29 | name |
59+
| ZipSlipBad.js:35:26:35:29 | name |
6060
| ZipSlipBadUnzipper.js:7:9:7:29 | fileName |
6161
| ZipSlipBadUnzipper.js:7:9:7:29 | fileName |
6262
| ZipSlipBadUnzipper.js:7:20:7:29 | entry.path |
@@ -103,20 +103,20 @@ edges
103103
| ZipSlipBad.js:22:22:22:31 | entry.path | ZipSlipBad.js:22:11:22:31 | fileName |
104104
| ZipSlipBad.js:22:22:22:31 | entry.path | ZipSlipBad.js:22:11:22:31 | fileName |
105105
| ZipSlipBad.js:22:22:22:31 | entry.path | ZipSlipBad.js:22:11:22:31 | fileName |
106-
| ZipSlipBad.js:29:14:29:17 | name | ZipSlipBad.js:30:26:30:29 | name |
107-
| ZipSlipBad.js:29:14:29:17 | name | ZipSlipBad.js:30:26:30:29 | name |
108-
| ZipSlipBad.js:29:14:29:17 | name | ZipSlipBad.js:30:26:30:29 | name |
109-
| ZipSlipBad.js:29:14:29:17 | name | ZipSlipBad.js:30:26:30:29 | name |
110-
| ZipSlipBad.js:29:14:29:17 | name | ZipSlipBad.js:30:26:30:29 | name |
111-
| ZipSlipBad.js:29:14:29:17 | name | ZipSlipBad.js:30:26:30:29 | name |
112-
| ZipSlipBad.js:29:14:29:17 | name | ZipSlipBad.js:30:26:30:29 | name |
113-
| ZipSlipBad.js:33:16:33:19 | name | ZipSlipBad.js:34:26:34:29 | name |
114-
| ZipSlipBad.js:33:16:33:19 | name | ZipSlipBad.js:34:26:34:29 | name |
115-
| ZipSlipBad.js:33:16:33:19 | name | ZipSlipBad.js:34:26:34:29 | name |
116-
| ZipSlipBad.js:33:16:33:19 | name | ZipSlipBad.js:34:26:34:29 | name |
117-
| ZipSlipBad.js:33:16:33:19 | name | ZipSlipBad.js:34:26:34:29 | name |
118-
| ZipSlipBad.js:33:16:33:19 | name | ZipSlipBad.js:34:26:34:29 | name |
119-
| ZipSlipBad.js:33:16:33:19 | name | ZipSlipBad.js:34:26:34:29 | name |
106+
| ZipSlipBad.js:30:14:30:17 | name | ZipSlipBad.js:31:26:31:29 | name |
107+
| ZipSlipBad.js:30:14:30:17 | name | ZipSlipBad.js:31:26:31:29 | name |
108+
| ZipSlipBad.js:30:14:30:17 | name | ZipSlipBad.js:31:26:31:29 | name |
109+
| ZipSlipBad.js:30:14:30:17 | name | ZipSlipBad.js:31:26:31:29 | name |
110+
| ZipSlipBad.js:30:14:30:17 | name | ZipSlipBad.js:31:26:31:29 | name |
111+
| ZipSlipBad.js:30:14:30:17 | name | ZipSlipBad.js:31:26:31:29 | name |
112+
| ZipSlipBad.js:30:14:30:17 | name | ZipSlipBad.js:31:26:31:29 | name |
113+
| ZipSlipBad.js:34:16:34:19 | name | ZipSlipBad.js:35:26:35:29 | name |
114+
| ZipSlipBad.js:34:16:34:19 | name | ZipSlipBad.js:35:26:35:29 | name |
115+
| ZipSlipBad.js:34:16:34:19 | name | ZipSlipBad.js:35:26:35:29 | name |
116+
| ZipSlipBad.js:34:16:34:19 | name | ZipSlipBad.js:35:26:35:29 | name |
117+
| ZipSlipBad.js:34:16:34:19 | name | ZipSlipBad.js:35:26:35:29 | name |
118+
| ZipSlipBad.js:34:16:34:19 | name | ZipSlipBad.js:35:26:35:29 | name |
119+
| ZipSlipBad.js:34:16:34:19 | name | ZipSlipBad.js:35:26:35:29 | name |
120120
| ZipSlipBadUnzipper.js:7:9:7:29 | fileName | ZipSlipBadUnzipper.js:8:37:8:44 | fileName |
121121
| ZipSlipBadUnzipper.js:7:9:7:29 | fileName | ZipSlipBadUnzipper.js:8:37:8:44 | fileName |
122122
| ZipSlipBadUnzipper.js:7:9:7:29 | fileName | ZipSlipBadUnzipper.js:8:37:8:44 | fileName |
@@ -133,6 +133,6 @@ edges
133133
| 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 |
134134
| ZipSlipBad.js:16:30:16:37 | fileName | ZipSlipBad.js:15:22:15:31 | entry.path | ZipSlipBad.js:16:30:16:37 | fileName | Unsanitized zip archive $@, which may contain '..', is used in a file system operation. | ZipSlipBad.js:15:22:15:31 | entry.path | item path |
135135
| ZipSlipBad.js:23:28:23:35 | fileName | ZipSlipBad.js:22:22:22:31 | entry.path | ZipSlipBad.js:23:28:23:35 | fileName | Unsanitized zip archive $@, which may contain '..', is used in a file system operation. | ZipSlipBad.js:22:22:22:31 | entry.path | item path |
136-
| ZipSlipBad.js:30:26:30:29 | name | ZipSlipBad.js:29:14:29:17 | name | ZipSlipBad.js:30:26:30:29 | name | Unsanitized zip archive $@, which may contain '..', is used in a file system operation. | ZipSlipBad.js:29:14:29:17 | name | item path |
137-
| ZipSlipBad.js:34:26:34:29 | name | ZipSlipBad.js:33:16:33:19 | name | ZipSlipBad.js:34:26:34:29 | name | Unsanitized zip archive $@, which may contain '..', is used in a file system operation. | ZipSlipBad.js:33:16:33:19 | name | item path |
136+
| ZipSlipBad.js:31:26:31:29 | name | ZipSlipBad.js:30:14:30:17 | name | ZipSlipBad.js:31:26:31:29 | name | Unsanitized zip archive $@, which may contain '..', is used in a file system operation. | ZipSlipBad.js:30:14:30:17 | name | item path |
137+
| ZipSlipBad.js:35:26:35:29 | name | ZipSlipBad.js:34:16:34:19 | name | ZipSlipBad.js:35:26:35:29 | name | Unsanitized zip archive $@, which may contain '..', is used in a file system operation. | ZipSlipBad.js:34:16:34:19 | name | item path |
138138
| 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/ZipSlipBad.js

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ fs.createReadStream('archive.zip')
2525

2626
const JSZip = require('jszip');
2727
const zip = new JSZip();
28+
const path = require('path');
2829
function doZipSlip() {
2930
for (const name in zip.files) {
3031
fs.createWriteStream(name);
@@ -33,4 +34,22 @@ function doZipSlip() {
3334
zip.forEach((name, file) => {
3435
fs.createWriteStream(name);
3536
});
36-
}
37+
38+
const extractTo = path.resolve("/some/path/to/extract/to");
39+
var files = [];
40+
41+
for (var name in zip.files) {
42+
var entry = zip.files[name];
43+
44+
var targetPath = path.resolve(
45+
path.join(extractTo, name)
46+
);
47+
if (!targetPath.startsWith(extractTo)) {
48+
throw new Error("Entry is outside the extraction path");
49+
}
50+
files.push(name);
51+
}
52+
for (const file of files) {
53+
fs.createWriteStream(path.join(extractTo, file)); // OK
54+
}
55+
}

0 commit comments

Comments
 (0)