diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/TaintedPath.qll b/javascript/ql/src/semmle/javascript/security/dataflow/TaintedPath.qll index 8dca676af46..242504a9c59 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/TaintedPath.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/TaintedPath.qll @@ -36,7 +36,7 @@ module TaintedPath { guard instanceof StartsWithDirSanitizer or guard instanceof IsAbsoluteSanitizer or guard instanceof ContainsDotDotSanitizer or - guard instanceof RelativePathStartsWithDotDotSanitizer or + guard instanceof RelativePathStartsWithSanitizer or guard instanceof IsInsideCheckSanitizer } diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/TaintedPathCustomizations.qll b/javascript/ql/src/semmle/javascript/security/dataflow/TaintedPathCustomizations.qll index d1be274314c..dc749d6994c 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/TaintedPathCustomizations.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/TaintedPathCustomizations.qll @@ -368,29 +368,43 @@ module TaintedPath { * // pathname is safe * } * ``` + * + * or + * ``` + * var relative = path.resolve(pathname); // or path.normalize + * if(relative.startsWith(webroot) { + * // pathname is safe + * } else { + * // pathname is unsafe + * } + * ``` */ - class RelativePathStartsWithDotDotSanitizer extends DataFlow::BarrierGuardNode { + class RelativePathStartsWithSanitizer extends DataFlow::BarrierGuardNode { StringOps::StartsWith startsWith; - DataFlow::CallNode relativeCall; + DataFlow::CallNode pathCall; + string member; - RelativePathStartsWithDotDotSanitizer() { + RelativePathStartsWithSanitizer() { + (member = "relative" or member = "resolve" or member = "normalize") and this = startsWith and - relativeCall = NodeJSLib::Path::moduleMember("relative").getACall() and + pathCall = NodeJSLib::Path::moduleMember(member).getACall() and ( - startsWith.getBaseString().getALocalSource() = relativeCall + startsWith.getBaseString().getALocalSource() = pathCall or startsWith .getBaseString() .getALocalSource() .(NormalizingPathCall) .getInput() - .getALocalSource() = relativeCall + .getALocalSource() = pathCall ) and - isDotDotSlashPrefix(startsWith.getSubstring()) + (not member = "relative" or isDotDotSlashPrefix(startsWith.getSubstring())) } override predicate blocks(boolean outcome, Expr e) { - e = relativeCall.getArgument(1).asExpr() and outcome = startsWith.getPolarity().booleanNot() + member = "relative" and e = pathCall.getArgument(1).asExpr() and outcome = startsWith.getPolarity().booleanNot() + or + not member = "relative" and e = pathCall.getArgument(0).asExpr() and outcome = startsWith.getPolarity() } } diff --git a/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/TaintedPath.expected b/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/TaintedPath.expected index 061f3522e34..8b22a2fc186 100644 --- a/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/TaintedPath.expected +++ b/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/TaintedPath.expected @@ -1631,6 +1631,29 @@ nodes | normalizedPaths.js:332:19:332:32 | normalizedPath | | normalizedPaths.js:332:19:332:32 | normalizedPath | | normalizedPaths.js:332:19:332:32 | normalizedPath | +| normalizedPaths.js:339:6:339:46 | path | +| normalizedPaths.js:339:6:339:46 | path | +| normalizedPaths.js:339:6:339:46 | path | +| normalizedPaths.js:339:6:339:46 | path | +| normalizedPaths.js:339:13:339:46 | pathMod ... y.path) | +| normalizedPaths.js:339:13:339:46 | pathMod ... y.path) | +| normalizedPaths.js:339:13:339:46 | pathMod ... y.path) | +| normalizedPaths.js:339:13:339:46 | pathMod ... y.path) | +| normalizedPaths.js:339:32:339:45 | req.query.path | +| normalizedPaths.js:339:32:339:45 | req.query.path | +| normalizedPaths.js:339:32:339:45 | req.query.path | +| normalizedPaths.js:339:32:339:45 | req.query.path | +| normalizedPaths.js:339:32:339:45 | req.query.path | +| normalizedPaths.js:341:18:341:21 | path | +| normalizedPaths.js:341:18:341:21 | path | +| normalizedPaths.js:341:18:341:21 | path | +| normalizedPaths.js:341:18:341:21 | path | +| normalizedPaths.js:341:18:341:21 | path | +| normalizedPaths.js:346:19:346:22 | path | +| normalizedPaths.js:346:19:346:22 | path | +| normalizedPaths.js:346:19:346:22 | path | +| normalizedPaths.js:346:19:346:22 | path | +| normalizedPaths.js:346:19:346:22 | path | | 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") | @@ -4531,6 +4554,34 @@ edges | normalizedPaths.js:320:45:320:48 | path | normalizedPaths.js:320:23:320:49 | pathMod ... , path) | | normalizedPaths.js:320:45:320:48 | path | normalizedPaths.js:320:23:320:49 | pathMod ... , path) | | normalizedPaths.js:320:45:320:48 | path | normalizedPaths.js:320:23:320:49 | pathMod ... , path) | +| normalizedPaths.js:339:6:339:46 | path | normalizedPaths.js:341:18:341:21 | path | +| normalizedPaths.js:339:6:339:46 | path | normalizedPaths.js:341:18:341:21 | path | +| normalizedPaths.js:339:6:339:46 | path | normalizedPaths.js:341:18:341:21 | path | +| normalizedPaths.js:339:6:339:46 | path | normalizedPaths.js:341:18:341:21 | path | +| normalizedPaths.js:339:6:339:46 | path | normalizedPaths.js:341:18:341:21 | path | +| normalizedPaths.js:339:6:339:46 | path | normalizedPaths.js:341:18:341:21 | path | +| normalizedPaths.js:339:6:339:46 | path | normalizedPaths.js:341:18:341:21 | path | +| normalizedPaths.js:339:6:339:46 | path | normalizedPaths.js:341:18:341:21 | path | +| normalizedPaths.js:339:6:339:46 | path | normalizedPaths.js:346:19:346:22 | path | +| normalizedPaths.js:339:6:339:46 | path | normalizedPaths.js:346:19:346:22 | path | +| normalizedPaths.js:339:6:339:46 | path | normalizedPaths.js:346:19:346:22 | path | +| normalizedPaths.js:339:6:339:46 | path | normalizedPaths.js:346:19:346:22 | path | +| normalizedPaths.js:339:6:339:46 | path | normalizedPaths.js:346:19:346:22 | path | +| normalizedPaths.js:339:6:339:46 | path | normalizedPaths.js:346:19:346:22 | path | +| normalizedPaths.js:339:6:339:46 | path | normalizedPaths.js:346:19:346:22 | path | +| normalizedPaths.js:339:6:339:46 | path | normalizedPaths.js:346:19:346:22 | path | +| normalizedPaths.js:339:13:339:46 | pathMod ... y.path) | normalizedPaths.js:339:6:339:46 | path | +| normalizedPaths.js:339:13:339:46 | pathMod ... y.path) | normalizedPaths.js:339:6:339:46 | path | +| normalizedPaths.js:339:13:339:46 | pathMod ... y.path) | normalizedPaths.js:339:6:339:46 | path | +| normalizedPaths.js:339:13:339:46 | pathMod ... y.path) | normalizedPaths.js:339:6:339:46 | path | +| normalizedPaths.js:339:32:339:45 | req.query.path | normalizedPaths.js:339:13:339:46 | pathMod ... y.path) | +| normalizedPaths.js:339:32:339:45 | req.query.path | normalizedPaths.js:339:13:339:46 | pathMod ... y.path) | +| normalizedPaths.js:339:32:339:45 | req.query.path | normalizedPaths.js:339:13:339:46 | pathMod ... y.path) | +| normalizedPaths.js:339:32:339:45 | req.query.path | normalizedPaths.js:339:13:339:46 | pathMod ... y.path) | +| normalizedPaths.js:339:32:339:45 | req.query.path | normalizedPaths.js:339:13:339:46 | pathMod ... y.path) | +| normalizedPaths.js:339:32:339:45 | req.query.path | normalizedPaths.js:339:13:339:46 | pathMod ... y.path) | +| normalizedPaths.js:339:32:339:45 | req.query.path | normalizedPaths.js:339:13:339:46 | pathMod ... y.path) | +| normalizedPaths.js:339:32:339:45 | req.query.path | normalizedPaths.js:339:13:339:46 | pathMod ... y.path) | | tainted-require.js:7:19:7:37 | req.param("module") | tainted-require.js:7:19:7:37 | req.param("module") | | tainted-sendFile.js:8:16:8:33 | req.param("gimme") | tainted-sendFile.js:8:16:8:33 | req.param("gimme") | | 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 | 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 | | 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 | | 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 | +| 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 | +| 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 | | 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 | | 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 | | 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 | diff --git a/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/normalizedPaths.js b/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/normalizedPaths.js index 6a7a143f7bd..cdbdf84b966 100644 --- a/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/normalizedPaths.js +++ b/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/normalizedPaths.js @@ -334,3 +334,17 @@ app.get('/pseudo-normalizations', (req, res) => { } }); + +app.get('/yet-another-prefix', (req, res) => { + let path = pathModule.resolve(req.query.path); + + fs.readFileSync(path); // NOT OK + + var abs = pathModule.resolve(path); + + if (abs.indexOf(root) !== 0) { + fs.readFileSync(path); // NOT OK + return; + } + fs.readFileSync(path); // OK +}); \ No newline at end of file