Skip to content

Commit 228bd73

Browse files
semmle-qlcisemmle-qlci
authored
Merge pull request github#2944 from erik-krogh/YetAnotherPrefix
Approved by esbena
2 parents 3a3aa75 + 71ff32e commit 228bd73

File tree

4 files changed

+90
-9
lines changed

4 files changed

+90
-9
lines changed

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ module TaintedPath {
3636
guard instanceof StartsWithDirSanitizer or
3737
guard instanceof IsAbsoluteSanitizer or
3838
guard instanceof ContainsDotDotSanitizer or
39-
guard instanceof RelativePathStartsWithDotDotSanitizer or
39+
guard instanceof RelativePathStartsWithSanitizer or
4040
guard instanceof IsInsideCheckSanitizer
4141
}
4242

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

Lines changed: 22 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -368,29 +368,43 @@ module TaintedPath {
368368
* // pathname is safe
369369
* }
370370
* ```
371+
*
372+
* or
373+
* ```
374+
* var relative = path.resolve(pathname); // or path.normalize
375+
* if(relative.startsWith(webroot) {
376+
* // pathname is safe
377+
* } else {
378+
* // pathname is unsafe
379+
* }
380+
* ```
371381
*/
372-
class RelativePathStartsWithDotDotSanitizer extends DataFlow::BarrierGuardNode {
382+
class RelativePathStartsWithSanitizer extends DataFlow::BarrierGuardNode {
373383
StringOps::StartsWith startsWith;
374-
DataFlow::CallNode relativeCall;
384+
DataFlow::CallNode pathCall;
385+
string member;
375386

376-
RelativePathStartsWithDotDotSanitizer() {
387+
RelativePathStartsWithSanitizer() {
388+
(member = "relative" or member = "resolve" or member = "normalize") and
377389
this = startsWith and
378-
relativeCall = NodeJSLib::Path::moduleMember("relative").getACall() and
390+
pathCall = NodeJSLib::Path::moduleMember(member).getACall() and
379391
(
380-
startsWith.getBaseString().getALocalSource() = relativeCall
392+
startsWith.getBaseString().getALocalSource() = pathCall
381393
or
382394
startsWith
383395
.getBaseString()
384396
.getALocalSource()
385397
.(NormalizingPathCall)
386398
.getInput()
387-
.getALocalSource() = relativeCall
399+
.getALocalSource() = pathCall
388400
) and
389-
isDotDotSlashPrefix(startsWith.getSubstring())
401+
(not member = "relative" or isDotDotSlashPrefix(startsWith.getSubstring()))
390402
}
391403

392404
override predicate blocks(boolean outcome, Expr e) {
393-
e = relativeCall.getArgument(1).asExpr() and outcome = startsWith.getPolarity().booleanNot()
405+
member = "relative" and e = pathCall.getArgument(1).asExpr() and outcome = startsWith.getPolarity().booleanNot()
406+
or
407+
not member = "relative" and e = pathCall.getArgument(0).asExpr() and outcome = startsWith.getPolarity()
394408
}
395409
}
396410

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

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1631,6 +1631,29 @@ nodes
16311631
| normalizedPaths.js:332:19:332:32 | normalizedPath |
16321632
| normalizedPaths.js:332:19:332:32 | normalizedPath |
16331633
| normalizedPaths.js:332:19:332:32 | normalizedPath |
1634+
| normalizedPaths.js:339:6:339:46 | path |
1635+
| normalizedPaths.js:339:6:339:46 | path |
1636+
| normalizedPaths.js:339:6:339:46 | path |
1637+
| normalizedPaths.js:339:6:339:46 | path |
1638+
| normalizedPaths.js:339:13:339:46 | pathMod ... y.path) |
1639+
| normalizedPaths.js:339:13:339:46 | pathMod ... y.path) |
1640+
| normalizedPaths.js:339:13:339:46 | pathMod ... y.path) |
1641+
| normalizedPaths.js:339:13:339:46 | pathMod ... y.path) |
1642+
| normalizedPaths.js:339:32:339:45 | req.query.path |
1643+
| normalizedPaths.js:339:32:339:45 | req.query.path |
1644+
| normalizedPaths.js:339:32:339:45 | req.query.path |
1645+
| normalizedPaths.js:339:32:339:45 | req.query.path |
1646+
| normalizedPaths.js:339:32:339:45 | req.query.path |
1647+
| normalizedPaths.js:341:18:341:21 | path |
1648+
| normalizedPaths.js:341:18:341:21 | path |
1649+
| normalizedPaths.js:341:18:341:21 | path |
1650+
| normalizedPaths.js:341:18:341:21 | path |
1651+
| normalizedPaths.js:341:18:341:21 | path |
1652+
| normalizedPaths.js:346:19:346:22 | path |
1653+
| normalizedPaths.js:346:19:346:22 | path |
1654+
| normalizedPaths.js:346:19:346:22 | path |
1655+
| normalizedPaths.js:346:19:346:22 | path |
1656+
| normalizedPaths.js:346:19:346:22 | path |
16341657
| tainted-require.js:7:19:7:37 | req.param("module") |
16351658
| tainted-require.js:7:19:7:37 | req.param("module") |
16361659
| tainted-require.js:7:19:7:37 | req.param("module") |
@@ -4531,6 +4554,34 @@ edges
45314554
| normalizedPaths.js:320:45:320:48 | path | normalizedPaths.js:320:23:320:49 | pathMod ... , path) |
45324555
| normalizedPaths.js:320:45:320:48 | path | normalizedPaths.js:320:23:320:49 | pathMod ... , path) |
45334556
| normalizedPaths.js:320:45:320:48 | path | normalizedPaths.js:320:23:320:49 | pathMod ... , path) |
4557+
| normalizedPaths.js:339:6:339:46 | path | normalizedPaths.js:341:18:341:21 | path |
4558+
| normalizedPaths.js:339:6:339:46 | path | normalizedPaths.js:341:18:341:21 | path |
4559+
| normalizedPaths.js:339:6:339:46 | path | normalizedPaths.js:341:18:341:21 | path |
4560+
| normalizedPaths.js:339:6:339:46 | path | normalizedPaths.js:341:18:341:21 | path |
4561+
| normalizedPaths.js:339:6:339:46 | path | normalizedPaths.js:341:18:341:21 | path |
4562+
| normalizedPaths.js:339:6:339:46 | path | normalizedPaths.js:341:18:341:21 | path |
4563+
| normalizedPaths.js:339:6:339:46 | path | normalizedPaths.js:341:18:341:21 | path |
4564+
| normalizedPaths.js:339:6:339:46 | path | normalizedPaths.js:341:18:341:21 | path |
4565+
| normalizedPaths.js:339:6:339:46 | path | normalizedPaths.js:346:19:346:22 | path |
4566+
| normalizedPaths.js:339:6:339:46 | path | normalizedPaths.js:346:19:346:22 | path |
4567+
| normalizedPaths.js:339:6:339:46 | path | normalizedPaths.js:346:19:346:22 | path |
4568+
| normalizedPaths.js:339:6:339:46 | path | normalizedPaths.js:346:19:346:22 | path |
4569+
| normalizedPaths.js:339:6:339:46 | path | normalizedPaths.js:346:19:346:22 | path |
4570+
| normalizedPaths.js:339:6:339:46 | path | normalizedPaths.js:346:19:346:22 | path |
4571+
| normalizedPaths.js:339:6:339:46 | path | normalizedPaths.js:346:19:346:22 | path |
4572+
| normalizedPaths.js:339:6:339:46 | path | normalizedPaths.js:346:19:346:22 | path |
4573+
| normalizedPaths.js:339:13:339:46 | pathMod ... y.path) | normalizedPaths.js:339:6:339:46 | path |
4574+
| normalizedPaths.js:339:13:339:46 | pathMod ... y.path) | normalizedPaths.js:339:6:339:46 | path |
4575+
| normalizedPaths.js:339:13:339:46 | pathMod ... y.path) | normalizedPaths.js:339:6:339:46 | path |
4576+
| normalizedPaths.js:339:13:339:46 | pathMod ... y.path) | normalizedPaths.js:339:6:339:46 | path |
4577+
| normalizedPaths.js:339:32:339:45 | req.query.path | normalizedPaths.js:339:13:339:46 | pathMod ... y.path) |
4578+
| normalizedPaths.js:339:32:339:45 | req.query.path | normalizedPaths.js:339:13:339:46 | pathMod ... y.path) |
4579+
| normalizedPaths.js:339:32:339:45 | req.query.path | normalizedPaths.js:339:13:339:46 | pathMod ... y.path) |
4580+
| normalizedPaths.js:339:32:339:45 | req.query.path | normalizedPaths.js:339:13:339:46 | pathMod ... y.path) |
4581+
| normalizedPaths.js:339:32:339:45 | req.query.path | normalizedPaths.js:339:13:339:46 | pathMod ... y.path) |
4582+
| normalizedPaths.js:339:32:339:45 | req.query.path | normalizedPaths.js:339:13:339:46 | pathMod ... y.path) |
4583+
| normalizedPaths.js:339:32:339:45 | req.query.path | normalizedPaths.js:339:13:339:46 | pathMod ... y.path) |
4584+
| normalizedPaths.js:339:32:339:45 | req.query.path | normalizedPaths.js:339:13:339:46 | pathMod ... y.path) |
45344585
| tainted-require.js:7:19:7:37 | req.param("module") | tainted-require.js:7:19:7:37 | req.param("module") |
45354586
| tainted-sendFile.js:8:16:8:33 | req.param("gimme") | tainted-sendFile.js:8:16:8:33 | req.param("gimme") |
45364587
| tainted-sendFile.js:10:16:10:33 | req.param("gimme") | tainted-sendFile.js:10:16:10:33 | req.param("gimme") |
@@ -5411,6 +5462,8 @@ edges
54115462
| normalizedPaths.js:316:19:316:22 | path | normalizedPaths.js:303:13:303:26 | req.query.path | normalizedPaths.js:316:19:316:22 | path | This path depends on $@. | normalizedPaths.js:303:13:303:26 | req.query.path | a user-provided value |
54125463
| normalizedPaths.js:325:19:325:32 | normalizedPath | normalizedPaths.js:303:13:303:26 | req.query.path | normalizedPaths.js:325:19:325:32 | normalizedPath | This path depends on $@. | normalizedPaths.js:303:13:303:26 | req.query.path | a user-provided value |
54135464
| normalizedPaths.js:332:19:332:32 | normalizedPath | normalizedPaths.js:303:13:303:26 | req.query.path | normalizedPaths.js:332:19:332:32 | normalizedPath | This path depends on $@. | normalizedPaths.js:303:13:303:26 | req.query.path | a user-provided value |
5465+
| normalizedPaths.js:341:18:341:21 | path | normalizedPaths.js:339:32:339:45 | req.query.path | normalizedPaths.js:341:18:341:21 | path | This path depends on $@. | normalizedPaths.js:339:32:339:45 | req.query.path | a user-provided value |
5466+
| normalizedPaths.js:346:19:346:22 | path | normalizedPaths.js:339:32:339:45 | req.query.path | normalizedPaths.js:346:19:346:22 | path | This path depends on $@. | normalizedPaths.js:339:32:339:45 | req.query.path | a user-provided value |
54145467
| tainted-require.js:7:19:7:37 | req.param("module") | tainted-require.js:7:19:7:37 | req.param("module") | tainted-require.js:7:19:7:37 | req.param("module") | This path depends on $@. | tainted-require.js:7:19:7:37 | req.param("module") | a user-provided value |
54155468
| tainted-sendFile.js:8:16:8:33 | req.param("gimme") | tainted-sendFile.js:8:16:8:33 | req.param("gimme") | tainted-sendFile.js:8:16:8:33 | req.param("gimme") | This path depends on $@. | tainted-sendFile.js:8:16:8:33 | req.param("gimme") | a user-provided value |
54165469
| tainted-sendFile.js:10:16:10:33 | req.param("gimme") | tainted-sendFile.js:10:16:10:33 | req.param("gimme") | tainted-sendFile.js:10:16:10:33 | req.param("gimme") | This path depends on $@. | tainted-sendFile.js:10:16:10:33 | req.param("gimme") | a user-provided value |

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

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -334,3 +334,17 @@ app.get('/pseudo-normalizations', (req, res) => {
334334
}
335335

336336
});
337+
338+
app.get('/yet-another-prefix', (req, res) => {
339+
let path = pathModule.resolve(req.query.path);
340+
341+
fs.readFileSync(path); // NOT OK
342+
343+
var abs = pathModule.resolve(path);
344+
345+
if (abs.indexOf(root) !== 0) {
346+
fs.readFileSync(path); // NOT OK
347+
return;
348+
}
349+
fs.readFileSync(path); // OK
350+
});

0 commit comments

Comments
 (0)