diff --git a/javascript/ql/src/Security/CWE-094/ImproperCodeSanitization.ql b/javascript/ql/src/Security/CWE-094/ImproperCodeSanitization.ql index 83695656aa5..87861567d9d 100644 --- a/javascript/ql/src/Security/CWE-094/ImproperCodeSanitization.ql +++ b/javascript/ql/src/Security/CWE-094/ImproperCodeSanitization.ql @@ -20,17 +20,22 @@ private import semmle.javascript.security.dataflow.CodeInjectionCustomizations /** * Gets a type-tracked instance of `RemoteFlowSource` using type-tracker `t`. */ -private DataFlow::SourceNode remoteFlow(DataFlow::TypeTracker t) { +private DataFlow::Node remoteFlow(DataFlow::TypeTracker t) { t.start() and result instanceof RemoteFlowSource or - exists(DataFlow::TypeTracker t2 | result = remoteFlow(t2).track(t2, t)) + exists(DataFlow::TypeTracker t2, DataFlow::Node prev | prev = remoteFlow(t2) | + t2 = t.smallstep(prev, result) + or + any(TaintTracking::AdditionalTaintStep dts).step(prev, result) and + t = t2 + ) } /** * Gets a type-tracked reference to a `RemoteFlowSource`. */ -private DataFlow::SourceNode remoteFlow() { result = remoteFlow(DataFlow::TypeTracker::end()) } +private DataFlow::Node remoteFlow() { result = remoteFlow(DataFlow::TypeTracker::end()) } /** * Gets a type-back-tracked instance of a code-injection sink using type-tracker `t`. @@ -60,7 +65,7 @@ where // Basic detection of duplicate results with `js/code-injection`. not ( sink.getNode().(StringOps::ConcatenationLeaf).getRoot() = endsInCodeInjectionSink() and - remoteFlow().flowsTo(source.getNode().(DataFlow::InvokeNode).getAnArgument()) + remoteFlow() = source.getNode().(DataFlow::InvokeNode).getAnArgument() ) select sink.getNode(), source, sink, "$@ flows to here and is used to construct code.", source.getNode(), "Improperly sanitized value" diff --git a/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/ImproperCodeSanitization.expected b/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/ImproperCodeSanitization.expected index 79c97be21a7..8914f18057b 100644 --- a/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/ImproperCodeSanitization.expected +++ b/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/ImproperCodeSanitization.expected @@ -31,6 +31,9 @@ nodes | bad-code-sanitization.js:54:29:54:63 | JSON.st ... bble")) | | bad-code-sanitization.js:54:29:54:63 | JSON.st ... bble")) | | bad-code-sanitization.js:54:29:54:63 | JSON.st ... bble")) | +| bad-code-sanitization.js:58:29:58:49 | JSON.st ... (taint) | +| bad-code-sanitization.js:58:29:58:49 | JSON.st ... (taint) | +| bad-code-sanitization.js:58:29:58:49 | JSON.st ... (taint) | edges | bad-code-sanitization.js:2:12:2:90 | /^[_$a- ... key)}]` | bad-code-sanitization.js:7:31:7:43 | safeProp(key) | | bad-code-sanitization.js:2:65:2:90 | `[${JSO ... key)}]` | bad-code-sanitization.js:2:12:2:90 | /^[_$a- ... key)}]` | @@ -49,6 +52,7 @@ edges | bad-code-sanitization.js:44:22:44:42 | JSON.st ... (input) | bad-code-sanitization.js:44:22:44:42 | JSON.st ... (input) | | bad-code-sanitization.js:52:28:52:62 | JSON.st ... bble")) | bad-code-sanitization.js:52:28:52:62 | JSON.st ... bble")) | | bad-code-sanitization.js:54:29:54:63 | JSON.st ... bble")) | bad-code-sanitization.js:54:29:54:63 | JSON.st ... bble")) | +| bad-code-sanitization.js:58:29:58:49 | JSON.st ... (taint) | bad-code-sanitization.js:58:29:58:49 | JSON.st ... (taint) | #select | bad-code-sanitization.js:8:27:8:46 | statements.join(';') | bad-code-sanitization.js:2:69:2:87 | JSON.stringify(key) | bad-code-sanitization.js:8:27:8:46 | statements.join(';') | $@ flows to here and is used to construct code. | bad-code-sanitization.js:2:69:2:87 | JSON.stringify(key) | Improperly sanitized value | | bad-code-sanitization.js:15:44:15:63 | htmlescape(pathname) | bad-code-sanitization.js:15:44:15:63 | htmlescape(pathname) | bad-code-sanitization.js:15:44:15:63 | htmlescape(pathname) | $@ flows to here and is used to construct code. | bad-code-sanitization.js:15:44:15:63 | htmlescape(pathname) | Improperly sanitized value | diff --git a/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/bad-code-sanitization.js b/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/bad-code-sanitization.js index ab5a5ff892d..cb2a1269b2c 100644 --- a/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/bad-code-sanitization.js +++ b/javascript/ql/test/query-tests/Security/CWE-094/CodeInjection/bad-code-sanitization.js @@ -52,4 +52,8 @@ app.get('/some/path', function(req, res) { var foo = `(function(){${JSON.stringify(req.param("wobble"))}))` // NOT - the source is remote-flow, but we know of no sink. setTimeout(`(function(){${JSON.stringify(req.param("wobble"))}))`); // OK - the source is remote-flow, and the sink is code-injection. + + var taint = [req.body.name, "foo"].join("\n"); + + setTimeout(`(function(){${JSON.stringify(taint)}))`); // OK - the source is remote-flow, and the sink is code-injection. });