Skip to content

Commit 6f80387

Browse files
codeql-cicodeql-ci
authored
Merge pull request #6993 from asgerf/js/tainted-path-regexp-contains-check
Approved by erik-krogh
2 parents 618d135 + 971f032 commit 6f80387

File tree

3 files changed

+64
-8
lines changed

3 files changed

+64
-8
lines changed

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

Lines changed: 15 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -445,17 +445,25 @@ module TaintedPath {
445445
/**
446446
* An expression of form `x.includes("..")` or similar.
447447
*/
448-
class ContainsDotDotSanitizer extends BarrierGuardNode {
449-
StringOps::Includes contains;
448+
class ContainsDotDotSanitizer extends BarrierGuardNode instanceof StringOps::Includes {
449+
ContainsDotDotSanitizer() { isDotDotSlashPrefix(super.getSubstring()) }
450450

451-
ContainsDotDotSanitizer() {
452-
this = contains and
453-
isDotDotSlashPrefix(contains.getSubstring())
451+
override predicate blocks(boolean outcome, Expr e, DataFlow::FlowLabel label) {
452+
e = super.getBaseString().asExpr() and
453+
outcome = super.getPolarity().booleanNot() and
454+
label.(Label::PosixPath).canContainDotDotSlash() // can still be bypassed by normalized absolute path
454455
}
456+
}
457+
458+
/**
459+
* An expression of form `x.matches(/\.\./)` or similar.
460+
*/
461+
class ContainsDotDotRegExpSanitizer extends BarrierGuardNode instanceof StringOps::RegExpTest {
462+
ContainsDotDotRegExpSanitizer() { super.getRegExp().getConstantValue() = [".", "..", "../"] }
455463

456464
override predicate blocks(boolean outcome, Expr e, DataFlow::FlowLabel label) {
457-
e = contains.getBaseString().asExpr() and
458-
outcome = contains.getPolarity().booleanNot() and
465+
e = super.getStringOperand().asExpr() and
466+
outcome = super.getPolarity().booleanNot() and
459467
label.(Label::PosixPath).canContainDotDotSlash() // can still be bypassed by normalized absolute path
460468
}
461469
}

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

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2116,6 +2116,19 @@ nodes
21162116
| normalizedPaths.js:381:25:381:28 | path |
21172117
| normalizedPaths.js:381:25:381:28 | path |
21182118
| normalizedPaths.js:381:25:381:28 | path |
2119+
| normalizedPaths.js:385:7:385:46 | path |
2120+
| normalizedPaths.js:385:7:385:46 | path |
2121+
| normalizedPaths.js:385:14:385:46 | pathMod ... uery.x) |
2122+
| normalizedPaths.js:385:14:385:46 | pathMod ... uery.x) |
2123+
| normalizedPaths.js:385:35:385:45 | req.query.x |
2124+
| normalizedPaths.js:385:35:385:45 | req.query.x |
2125+
| normalizedPaths.js:385:35:385:45 | req.query.x |
2126+
| normalizedPaths.js:388:19:388:22 | path |
2127+
| normalizedPaths.js:388:19:388:22 | path |
2128+
| normalizedPaths.js:388:19:388:22 | path |
2129+
| normalizedPaths.js:399:21:399:24 | path |
2130+
| normalizedPaths.js:399:21:399:24 | path |
2131+
| normalizedPaths.js:399:21:399:24 | path |
21192132
| other-fs-libraries.js:9:7:9:48 | path |
21202133
| other-fs-libraries.js:9:7:9:48 | path |
21212134
| other-fs-libraries.js:9:7:9:48 | path |
@@ -6998,6 +7011,20 @@ edges
69987011
| normalizedPaths.js:381:25:381:28 | path | normalizedPaths.js:381:19:381:29 | slash(path) |
69997012
| normalizedPaths.js:381:25:381:28 | path | normalizedPaths.js:381:19:381:29 | slash(path) |
70007013
| normalizedPaths.js:381:25:381:28 | path | normalizedPaths.js:381:19:381:29 | slash(path) |
7014+
| normalizedPaths.js:385:7:385:46 | path | normalizedPaths.js:388:19:388:22 | path |
7015+
| normalizedPaths.js:385:7:385:46 | path | normalizedPaths.js:388:19:388:22 | path |
7016+
| normalizedPaths.js:385:7:385:46 | path | normalizedPaths.js:388:19:388:22 | path |
7017+
| normalizedPaths.js:385:7:385:46 | path | normalizedPaths.js:388:19:388:22 | path |
7018+
| normalizedPaths.js:385:7:385:46 | path | normalizedPaths.js:399:21:399:24 | path |
7019+
| normalizedPaths.js:385:7:385:46 | path | normalizedPaths.js:399:21:399:24 | path |
7020+
| normalizedPaths.js:385:7:385:46 | path | normalizedPaths.js:399:21:399:24 | path |
7021+
| normalizedPaths.js:385:7:385:46 | path | normalizedPaths.js:399:21:399:24 | path |
7022+
| normalizedPaths.js:385:14:385:46 | pathMod ... uery.x) | normalizedPaths.js:385:7:385:46 | path |
7023+
| normalizedPaths.js:385:14:385:46 | pathMod ... uery.x) | normalizedPaths.js:385:7:385:46 | path |
7024+
| normalizedPaths.js:385:35:385:45 | req.query.x | normalizedPaths.js:385:14:385:46 | pathMod ... uery.x) |
7025+
| normalizedPaths.js:385:35:385:45 | req.query.x | normalizedPaths.js:385:14:385:46 | pathMod ... uery.x) |
7026+
| normalizedPaths.js:385:35:385:45 | req.query.x | normalizedPaths.js:385:14:385:46 | pathMod ... uery.x) |
7027+
| normalizedPaths.js:385:35:385:45 | req.query.x | normalizedPaths.js:385:14:385:46 | pathMod ... uery.x) |
70017028
| other-fs-libraries.js:9:7:9:48 | path | other-fs-libraries.js:11:19:11:22 | path |
70027029
| other-fs-libraries.js:9:7:9:48 | path | other-fs-libraries.js:11:19:11:22 | path |
70037030
| other-fs-libraries.js:9:7:9:48 | path | other-fs-libraries.js:11:19:11:22 | path |
@@ -9670,6 +9697,8 @@ edges
96709697
| normalizedPaths.js:363:21:363:31 | requestPath | normalizedPaths.js:354:14:354:27 | req.query.path | normalizedPaths.js:363:21:363:31 | requestPath | This path depends on $@. | normalizedPaths.js:354:14:354:27 | req.query.path | a user-provided value |
96719698
| normalizedPaths.js:379:19:379:22 | path | normalizedPaths.js:377:14:377:27 | req.query.path | normalizedPaths.js:379:19:379:22 | path | This path depends on $@. | normalizedPaths.js:377:14:377:27 | req.query.path | a user-provided value |
96729699
| normalizedPaths.js:381:19:381:29 | slash(path) | normalizedPaths.js:377:14:377:27 | req.query.path | normalizedPaths.js:381:19:381:29 | slash(path) | This path depends on $@. | normalizedPaths.js:377:14:377:27 | req.query.path | a user-provided value |
9700+
| normalizedPaths.js:388:19:388:22 | path | normalizedPaths.js:385:35:385:45 | req.query.x | normalizedPaths.js:388:19:388:22 | path | This path depends on $@. | normalizedPaths.js:385:35:385:45 | req.query.x | a user-provided value |
9701+
| normalizedPaths.js:399:21:399:24 | path | normalizedPaths.js:385:35:385:45 | req.query.x | normalizedPaths.js:399:21:399:24 | path | This path depends on $@. | normalizedPaths.js:385:35:385:45 | req.query.x | a user-provided value |
96739702
| other-fs-libraries.js:11:19:11:22 | path | other-fs-libraries.js:9:24:9:30 | req.url | other-fs-libraries.js:11:19:11:22 | path | This path depends on $@. | other-fs-libraries.js:9:24:9:30 | req.url | a user-provided value |
96749703
| other-fs-libraries.js:12:27:12:30 | path | other-fs-libraries.js:9:24:9:30 | req.url | other-fs-libraries.js:12:27:12:30 | path | This path depends on $@. | other-fs-libraries.js:9:24:9:30 | req.url | a user-provided value |
96759704
| other-fs-libraries.js:13:24:13:27 | path | other-fs-libraries.js:9:24:9:30 | req.url | other-fs-libraries.js:13:24:13:27 | path | This path depends on $@. | other-fs-libraries.js:9:24:9:30 | req.url | a user-provided value |

‎javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/normalizedPaths.js

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -379,4 +379,23 @@ app.get('/slash-stuff', (req, res) => {
379379
fs.readFileSync(path); // NOT OK
380380

381381
fs.readFileSync(slash(path)); // NOT OK
382-
});
382+
});
383+
384+
app.get('/dotdot-regexp', (req, res) => {
385+
let path = pathModule.normalize(req.query.x);
386+
if (pathModule.isAbsolute(path))
387+
return;
388+
fs.readFileSync(path); // NOT OK
389+
if (!path.match(/\./)) {
390+
fs.readFileSync(path); // OK
391+
}
392+
if (!path.match(/\.\./)) {
393+
fs.readFileSync(path); // OK
394+
}
395+
if (!path.match(/\.\.\//)) {
396+
fs.readFileSync(path); // OK
397+
}
398+
if (!path.match(/\.\.\/foo/)) {
399+
fs.readFileSync(path); // NOT OK
400+
}
401+
});

0 commit comments

Comments
 (0)