diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/TaintedPath.qll b/javascript/ql/src/semmle/javascript/security/dataflow/TaintedPath.qll index 242504a9c59..06b0a6eb9e6 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/TaintedPath.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/TaintedPath.qll @@ -197,6 +197,15 @@ module TaintedPath { srclabel = dstlabel ) or + // foo.replace(/\./, "") and similar + exists(DotRemovingReplaceCall call | + src = call.getInput() and + dst = call.getOutput() and + srclabel.isAbsolute() and + dstlabel.isAbsolute() and + dstlabel.isNormalized() + ) + or // path.join() exists(DataFlow::CallNode join, int n | join = NodeJSLib::Path::moduleMember("join").getACall() diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/TaintedPathCustomizations.qll b/javascript/ql/src/semmle/javascript/security/dataflow/TaintedPathCustomizations.qll index 6a8d1ddd825..c55fa8bad33 100644 --- a/javascript/ql/src/semmle/javascript/security/dataflow/TaintedPathCustomizations.qll +++ b/javascript/ql/src/semmle/javascript/security/dataflow/TaintedPathCustomizations.qll @@ -239,6 +239,39 @@ module TaintedPath { DataFlow::Node getOutput() { result = output } } + /** + * A call that removes all "." or ".." from a path, without also removing all forward slashes. + */ + class DotRemovingReplaceCall extends DataFlow::CallNode { + DataFlow::Node input; + DataFlow::Node output; + + DotRemovingReplaceCall() { + this.getCalleeName() = "replace" and + input = getReceiver() and + output = this and + exists(RegExpLiteral literal, RegExpTerm term | + getArgument(0).getALocalSource().asExpr() = literal and + literal.isGlobal() and + literal.getRoot() = term and + not term.getAMatchedString() = "/" + | + term.getAMatchedString() = "." or + term.getAMatchedString() = ".." + ) + } + + /** + * Gets the input path to be normalized. + */ + DataFlow::Node getInput() { result = input } + + /** + * Gets the normalized path. + */ + DataFlow::Node getOutput() { result = output } + } + /** * Holds if `node` is a prefix of the string `../`. */ 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 de3de113811..8dd69861f7e 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 @@ -1225,6 +1225,58 @@ nodes | TaintedPath.js:177:29:177:55 | path.re ... /g, '') | | TaintedPath.js:177:29:177:55 | path.re ... /g, '') | | TaintedPath.js:177:29:177:55 | path.re ... /g, '') | +| TaintedPath.js:183:29:183:32 | path | +| TaintedPath.js:183:29:183:32 | path | +| TaintedPath.js:183:29:183:32 | path | +| TaintedPath.js:183:29:183:32 | path | +| TaintedPath.js:183:29:183:32 | path | +| TaintedPath.js:183:29:183:32 | path | +| TaintedPath.js:183:29:183:32 | path | +| TaintedPath.js:183:29:183:32 | path | +| TaintedPath.js:183:29:183:52 | path.re ... /g, '') | +| TaintedPath.js:183:29:183:52 | path.re ... /g, '') | +| TaintedPath.js:183:29:183:52 | path.re ... /g, '') | +| TaintedPath.js:183:29:183:52 | path.re ... /g, '') | +| TaintedPath.js:183:29:183:52 | path.re ... /g, '') | +| TaintedPath.js:184:29:184:32 | path | +| TaintedPath.js:184:29:184:32 | path | +| TaintedPath.js:184:29:184:32 | path | +| TaintedPath.js:184:29:184:32 | path | +| TaintedPath.js:184:29:184:32 | path | +| TaintedPath.js:184:29:184:32 | path | +| TaintedPath.js:184:29:184:32 | path | +| TaintedPath.js:184:29:184:32 | path | +| TaintedPath.js:184:29:184:53 | path.re ... /g, '') | +| TaintedPath.js:184:29:184:53 | path.re ... /g, '') | +| TaintedPath.js:184:29:184:53 | path.re ... /g, '') | +| TaintedPath.js:184:29:184:53 | path.re ... /g, '') | +| TaintedPath.js:184:29:184:53 | path.re ... /g, '') | +| TaintedPath.js:185:29:185:32 | path | +| TaintedPath.js:185:29:185:32 | path | +| TaintedPath.js:185:29:185:32 | path | +| TaintedPath.js:185:29:185:32 | path | +| TaintedPath.js:185:29:185:32 | path | +| TaintedPath.js:185:29:185:32 | path | +| TaintedPath.js:185:29:185:32 | path | +| TaintedPath.js:185:29:185:32 | path | +| TaintedPath.js:185:29:185:51 | path.re ... /g, '') | +| TaintedPath.js:185:29:185:51 | path.re ... /g, '') | +| TaintedPath.js:185:29:185:51 | path.re ... /g, '') | +| TaintedPath.js:185:29:185:51 | path.re ... /g, '') | +| TaintedPath.js:185:29:185:51 | path.re ... /g, '') | +| TaintedPath.js:186:29:186:32 | path | +| TaintedPath.js:186:29:186:32 | path | +| TaintedPath.js:186:29:186:32 | path | +| TaintedPath.js:186:29:186:32 | path | +| TaintedPath.js:186:29:186:32 | path | +| TaintedPath.js:186:29:186:32 | path | +| TaintedPath.js:186:29:186:32 | path | +| TaintedPath.js:186:29:186:32 | path | +| TaintedPath.js:186:29:186:57 | path.re ... /g, '') | +| TaintedPath.js:186:29:186:57 | path.re ... /g, '') | +| TaintedPath.js:186:29:186:57 | path.re ... /g, '') | +| TaintedPath.js:186:29:186:57 | path.re ... /g, '') | +| TaintedPath.js:186:29:186:57 | path.re ... /g, '') | | normalizedPaths.js:11:7:11:27 | path | | normalizedPaths.js:11:7:11:27 | path | | normalizedPaths.js:11:7:11:27 | path | @@ -4069,6 +4121,38 @@ edges | TaintedPath.js:173:7:173:48 | path | TaintedPath.js:177:29:177:32 | path | | TaintedPath.js:173:7:173:48 | path | TaintedPath.js:177:29:177:32 | path | | TaintedPath.js:173:7:173:48 | path | TaintedPath.js:177:29:177:32 | path | +| TaintedPath.js:173:7:173:48 | path | TaintedPath.js:183:29:183:32 | path | +| TaintedPath.js:173:7:173:48 | path | TaintedPath.js:183:29:183:32 | path | +| TaintedPath.js:173:7:173:48 | path | TaintedPath.js:183:29:183:32 | path | +| TaintedPath.js:173:7:173:48 | path | TaintedPath.js:183:29:183:32 | path | +| TaintedPath.js:173:7:173:48 | path | TaintedPath.js:183:29:183:32 | path | +| TaintedPath.js:173:7:173:48 | path | TaintedPath.js:183:29:183:32 | path | +| TaintedPath.js:173:7:173:48 | path | TaintedPath.js:183:29:183:32 | path | +| TaintedPath.js:173:7:173:48 | path | TaintedPath.js:183:29:183:32 | path | +| TaintedPath.js:173:7:173:48 | path | TaintedPath.js:184:29:184:32 | path | +| TaintedPath.js:173:7:173:48 | path | TaintedPath.js:184:29:184:32 | path | +| TaintedPath.js:173:7:173:48 | path | TaintedPath.js:184:29:184:32 | path | +| TaintedPath.js:173:7:173:48 | path | TaintedPath.js:184:29:184:32 | path | +| TaintedPath.js:173:7:173:48 | path | TaintedPath.js:184:29:184:32 | path | +| TaintedPath.js:173:7:173:48 | path | TaintedPath.js:184:29:184:32 | path | +| TaintedPath.js:173:7:173:48 | path | TaintedPath.js:184:29:184:32 | path | +| TaintedPath.js:173:7:173:48 | path | TaintedPath.js:184:29:184:32 | path | +| TaintedPath.js:173:7:173:48 | path | TaintedPath.js:185:29:185:32 | path | +| TaintedPath.js:173:7:173:48 | path | TaintedPath.js:185:29:185:32 | path | +| TaintedPath.js:173:7:173:48 | path | TaintedPath.js:185:29:185:32 | path | +| TaintedPath.js:173:7:173:48 | path | TaintedPath.js:185:29:185:32 | path | +| TaintedPath.js:173:7:173:48 | path | TaintedPath.js:185:29:185:32 | path | +| TaintedPath.js:173:7:173:48 | path | TaintedPath.js:185:29:185:32 | path | +| TaintedPath.js:173:7:173:48 | path | TaintedPath.js:185:29:185:32 | path | +| TaintedPath.js:173:7:173:48 | path | TaintedPath.js:185:29:185:32 | path | +| TaintedPath.js:173:7:173:48 | path | TaintedPath.js:186:29:186:32 | path | +| TaintedPath.js:173:7:173:48 | path | TaintedPath.js:186:29:186:32 | path | +| TaintedPath.js:173:7:173:48 | path | TaintedPath.js:186:29:186:32 | path | +| TaintedPath.js:173:7:173:48 | path | TaintedPath.js:186:29:186:32 | path | +| TaintedPath.js:173:7:173:48 | path | TaintedPath.js:186:29:186:32 | path | +| TaintedPath.js:173:7:173:48 | path | TaintedPath.js:186:29:186:32 | path | +| TaintedPath.js:173:7:173:48 | path | TaintedPath.js:186:29:186:32 | path | +| TaintedPath.js:173:7:173:48 | path | TaintedPath.js:186:29:186:32 | path | | TaintedPath.js:173:14:173:37 | url.par ... , true) | TaintedPath.js:173:14:173:43 | url.par ... ).query | | TaintedPath.js:173:14:173:37 | url.par ... , true) | TaintedPath.js:173:14:173:43 | url.par ... ).query | | TaintedPath.js:173:14:173:37 | url.par ... , true) | TaintedPath.js:173:14:173:43 | url.par ... ).query | @@ -4181,6 +4265,70 @@ edges | TaintedPath.js:177:29:177:32 | path | TaintedPath.js:177:29:177:55 | path.re ... /g, '') | | TaintedPath.js:177:29:177:32 | path | TaintedPath.js:177:29:177:55 | path.re ... /g, '') | | TaintedPath.js:177:29:177:32 | path | TaintedPath.js:177:29:177:55 | path.re ... /g, '') | +| TaintedPath.js:183:29:183:32 | path | TaintedPath.js:183:29:183:52 | path.re ... /g, '') | +| TaintedPath.js:183:29:183:32 | path | TaintedPath.js:183:29:183:52 | path.re ... /g, '') | +| TaintedPath.js:183:29:183:32 | path | TaintedPath.js:183:29:183:52 | path.re ... /g, '') | +| TaintedPath.js:183:29:183:32 | path | TaintedPath.js:183:29:183:52 | path.re ... /g, '') | +| TaintedPath.js:183:29:183:32 | path | TaintedPath.js:183:29:183:52 | path.re ... /g, '') | +| TaintedPath.js:183:29:183:32 | path | TaintedPath.js:183:29:183:52 | path.re ... /g, '') | +| TaintedPath.js:183:29:183:32 | path | TaintedPath.js:183:29:183:52 | path.re ... /g, '') | +| TaintedPath.js:183:29:183:32 | path | TaintedPath.js:183:29:183:52 | path.re ... /g, '') | +| TaintedPath.js:183:29:183:32 | path | TaintedPath.js:183:29:183:52 | path.re ... /g, '') | +| TaintedPath.js:183:29:183:32 | path | TaintedPath.js:183:29:183:52 | path.re ... /g, '') | +| TaintedPath.js:183:29:183:32 | path | TaintedPath.js:183:29:183:52 | path.re ... /g, '') | +| TaintedPath.js:183:29:183:32 | path | TaintedPath.js:183:29:183:52 | path.re ... /g, '') | +| TaintedPath.js:183:29:183:32 | path | TaintedPath.js:183:29:183:52 | path.re ... /g, '') | +| TaintedPath.js:183:29:183:32 | path | TaintedPath.js:183:29:183:52 | path.re ... /g, '') | +| TaintedPath.js:183:29:183:32 | path | TaintedPath.js:183:29:183:52 | path.re ... /g, '') | +| TaintedPath.js:183:29:183:32 | path | TaintedPath.js:183:29:183:52 | path.re ... /g, '') | +| TaintedPath.js:184:29:184:32 | path | TaintedPath.js:184:29:184:53 | path.re ... /g, '') | +| TaintedPath.js:184:29:184:32 | path | TaintedPath.js:184:29:184:53 | path.re ... /g, '') | +| TaintedPath.js:184:29:184:32 | path | TaintedPath.js:184:29:184:53 | path.re ... /g, '') | +| TaintedPath.js:184:29:184:32 | path | TaintedPath.js:184:29:184:53 | path.re ... /g, '') | +| TaintedPath.js:184:29:184:32 | path | TaintedPath.js:184:29:184:53 | path.re ... /g, '') | +| TaintedPath.js:184:29:184:32 | path | TaintedPath.js:184:29:184:53 | path.re ... /g, '') | +| TaintedPath.js:184:29:184:32 | path | TaintedPath.js:184:29:184:53 | path.re ... /g, '') | +| TaintedPath.js:184:29:184:32 | path | TaintedPath.js:184:29:184:53 | path.re ... /g, '') | +| TaintedPath.js:184:29:184:32 | path | TaintedPath.js:184:29:184:53 | path.re ... /g, '') | +| TaintedPath.js:184:29:184:32 | path | TaintedPath.js:184:29:184:53 | path.re ... /g, '') | +| TaintedPath.js:184:29:184:32 | path | TaintedPath.js:184:29:184:53 | path.re ... /g, '') | +| TaintedPath.js:184:29:184:32 | path | TaintedPath.js:184:29:184:53 | path.re ... /g, '') | +| TaintedPath.js:184:29:184:32 | path | TaintedPath.js:184:29:184:53 | path.re ... /g, '') | +| TaintedPath.js:184:29:184:32 | path | TaintedPath.js:184:29:184:53 | path.re ... /g, '') | +| TaintedPath.js:184:29:184:32 | path | TaintedPath.js:184:29:184:53 | path.re ... /g, '') | +| TaintedPath.js:184:29:184:32 | path | TaintedPath.js:184:29:184:53 | path.re ... /g, '') | +| TaintedPath.js:185:29:185:32 | path | TaintedPath.js:185:29:185:51 | path.re ... /g, '') | +| TaintedPath.js:185:29:185:32 | path | TaintedPath.js:185:29:185:51 | path.re ... /g, '') | +| TaintedPath.js:185:29:185:32 | path | TaintedPath.js:185:29:185:51 | path.re ... /g, '') | +| TaintedPath.js:185:29:185:32 | path | TaintedPath.js:185:29:185:51 | path.re ... /g, '') | +| TaintedPath.js:185:29:185:32 | path | TaintedPath.js:185:29:185:51 | path.re ... /g, '') | +| TaintedPath.js:185:29:185:32 | path | TaintedPath.js:185:29:185:51 | path.re ... /g, '') | +| TaintedPath.js:185:29:185:32 | path | TaintedPath.js:185:29:185:51 | path.re ... /g, '') | +| TaintedPath.js:185:29:185:32 | path | TaintedPath.js:185:29:185:51 | path.re ... /g, '') | +| TaintedPath.js:185:29:185:32 | path | TaintedPath.js:185:29:185:51 | path.re ... /g, '') | +| TaintedPath.js:185:29:185:32 | path | TaintedPath.js:185:29:185:51 | path.re ... /g, '') | +| TaintedPath.js:185:29:185:32 | path | TaintedPath.js:185:29:185:51 | path.re ... /g, '') | +| TaintedPath.js:185:29:185:32 | path | TaintedPath.js:185:29:185:51 | path.re ... /g, '') | +| TaintedPath.js:185:29:185:32 | path | TaintedPath.js:185:29:185:51 | path.re ... /g, '') | +| TaintedPath.js:185:29:185:32 | path | TaintedPath.js:185:29:185:51 | path.re ... /g, '') | +| TaintedPath.js:185:29:185:32 | path | TaintedPath.js:185:29:185:51 | path.re ... /g, '') | +| TaintedPath.js:185:29:185:32 | path | TaintedPath.js:185:29:185:51 | path.re ... /g, '') | +| TaintedPath.js:186:29:186:32 | path | TaintedPath.js:186:29:186:57 | path.re ... /g, '') | +| TaintedPath.js:186:29:186:32 | path | TaintedPath.js:186:29:186:57 | path.re ... /g, '') | +| TaintedPath.js:186:29:186:32 | path | TaintedPath.js:186:29:186:57 | path.re ... /g, '') | +| TaintedPath.js:186:29:186:32 | path | TaintedPath.js:186:29:186:57 | path.re ... /g, '') | +| TaintedPath.js:186:29:186:32 | path | TaintedPath.js:186:29:186:57 | path.re ... /g, '') | +| TaintedPath.js:186:29:186:32 | path | TaintedPath.js:186:29:186:57 | path.re ... /g, '') | +| TaintedPath.js:186:29:186:32 | path | TaintedPath.js:186:29:186:57 | path.re ... /g, '') | +| TaintedPath.js:186:29:186:32 | path | TaintedPath.js:186:29:186:57 | path.re ... /g, '') | +| TaintedPath.js:186:29:186:32 | path | TaintedPath.js:186:29:186:57 | path.re ... /g, '') | +| TaintedPath.js:186:29:186:32 | path | TaintedPath.js:186:29:186:57 | path.re ... /g, '') | +| TaintedPath.js:186:29:186:32 | path | TaintedPath.js:186:29:186:57 | path.re ... /g, '') | +| TaintedPath.js:186:29:186:32 | path | TaintedPath.js:186:29:186:57 | path.re ... /g, '') | +| TaintedPath.js:186:29:186:32 | path | TaintedPath.js:186:29:186:57 | path.re ... /g, '') | +| TaintedPath.js:186:29:186:32 | path | TaintedPath.js:186:29:186:57 | path.re ... /g, '') | +| TaintedPath.js:186:29:186:32 | path | TaintedPath.js:186:29:186:57 | path.re ... /g, '') | +| TaintedPath.js:186:29:186:32 | path | TaintedPath.js:186:29:186:57 | path.re ... /g, '') | | normalizedPaths.js:11:7:11:27 | path | normalizedPaths.js:13:19:13:22 | path | | normalizedPaths.js:11:7:11:27 | path | normalizedPaths.js:13:19:13:22 | path | | normalizedPaths.js:11:7:11:27 | path | normalizedPaths.js:13:19:13:22 | path | @@ -5640,6 +5788,10 @@ edges | TaintedPath.js:166:19:166:38 | concatted2.join("/") | TaintedPath.js:149:24:149:30 | req.url | TaintedPath.js:166:19:166:38 | concatted2.join("/") | This path depends on $@. | TaintedPath.js:149:24:149:30 | req.url | a user-provided value | | TaintedPath.js:168:19:168:29 | split.pop() | TaintedPath.js:149:24:149:30 | req.url | TaintedPath.js:168:19:168:29 | split.pop() | This path depends on $@. | TaintedPath.js:149:24:149:30 | req.url | a user-provided value | | TaintedPath.js:177:29:177:55 | path.re ... /g, '') | TaintedPath.js:173:24:173:30 | req.url | TaintedPath.js:177:29:177:55 | path.re ... /g, '') | This path depends on $@. | TaintedPath.js:173:24:173:30 | req.url | a user-provided value | +| TaintedPath.js:183:29:183:52 | path.re ... /g, '') | TaintedPath.js:173:24:173:30 | req.url | TaintedPath.js:183:29:183:52 | path.re ... /g, '') | This path depends on $@. | TaintedPath.js:173:24:173:30 | req.url | a user-provided value | +| TaintedPath.js:184:29:184:53 | path.re ... /g, '') | TaintedPath.js:173:24:173:30 | req.url | TaintedPath.js:184:29:184:53 | path.re ... /g, '') | This path depends on $@. | TaintedPath.js:173:24:173:30 | req.url | a user-provided value | +| TaintedPath.js:185:29:185:51 | path.re ... /g, '') | TaintedPath.js:173:24:173:30 | req.url | TaintedPath.js:185:29:185:51 | path.re ... /g, '') | This path depends on $@. | TaintedPath.js:173:24:173:30 | req.url | a user-provided value | +| TaintedPath.js:186:29:186:57 | path.re ... /g, '') | TaintedPath.js:173:24:173:30 | req.url | TaintedPath.js:186:29:186:57 | path.re ... /g, '') | This path depends on $@. | TaintedPath.js:173:24:173:30 | req.url | a user-provided value | | normalizedPaths.js:13:19:13:22 | path | normalizedPaths.js:11:14:11:27 | req.query.path | normalizedPaths.js:13:19:13:22 | path | This path depends on $@. | normalizedPaths.js:11:14:11:27 | req.query.path | a user-provided value | | normalizedPaths.js:14:19:14:29 | './' + path | normalizedPaths.js:11:14:11:27 | req.query.path | normalizedPaths.js:14:19:14:29 | './' + path | This path depends on $@. | normalizedPaths.js:11:14:11:27 | req.query.path | a user-provided value | | normalizedPaths.js:15:19:15:38 | path + '/index.html' | normalizedPaths.js:11:14:11:27 | req.query.path | normalizedPaths.js:15:19:15:38 | path + '/index.html' | This path depends on $@. | normalizedPaths.js:11:14:11:27 | req.query.path | a user-provided value | diff --git a/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/TaintedPath.js b/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/TaintedPath.js index d12709d6344..0ecf680367f 100644 --- a/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/TaintedPath.js +++ b/javascript/ql/test/query-tests/Security/CWE-022/TaintedPath/TaintedPath.js @@ -175,12 +175,20 @@ var server = http.createServer(function(req, res) { // Removal of forward-slash or dots. res.write(fs.readFileSync(path.replace(/[\]\[*,;'"`<>\\?\/]/g, ''))); // OK. res.write(fs.readFileSync(path.replace(/[abcd]/g, ''))); // NOT OK - res.write(fs.readFileSync(path.replace(/[.]/g, ''))); // OK (can still be absolute) - res.write(fs.readFileSync(path.replace(/[..]/g, ''))); // OK res.write(fs.readFileSync(path.replace(/[./]/g, ''))); // OK res.write(fs.readFileSync(path.replace(/[foobar/foobar]/g, ''))); // OK res.write(fs.readFileSync(path.replace(/\//g, ''))); // OK - res.write(fs.readFileSync(path.replace(/\./g, ''))); // OK res.write(fs.readFileSync(path.replace(/\.|\//g, ''))); // OK - res.write(fs.readFileSync(path.replace(/\.\.|BLA/g, ''))); // OK + + res.write(fs.readFileSync(path.replace(/[.]/g, ''))); // NOT OK (can be absolute) + res.write(fs.readFileSync(path.replace(/[..]/g, ''))); // NOT OK (can be absolute) + res.write(fs.readFileSync(path.replace(/\./g, ''))); // NOT OK (can be absolute) + res.write(fs.readFileSync(path.replace(/\.\.|BLA/g, ''))); // NOT OK (can be absolute) + + if (!pathModule.isAbsolute(path)) { + res.write(fs.readFileSync(path.replace(/[.]/g, ''))); // OK + res.write(fs.readFileSync(path.replace(/[..]/g, ''))); // OK + res.write(fs.readFileSync(path.replace(/\./g, ''))); // OK + res.write(fs.readFileSync(path.replace(/\.\.|BLA/g, ''))); // OK + } }); \ No newline at end of file