Merge pull request #18044 from asgerf/js/shared-dataflow-bump

JS: Merge 'main' and implement 'speculativeTaintStep'
github · Nov 27, 2024 · 66d6bda · 66d6bda
2 parents 82d61e4 + 805fd0b
commit 66d6bda
Show file tree

Hide file tree

Showing 12 changed files with 423 additions and 143 deletions.
diff --git a/javascript/ql/lib/semmle/javascript/dataflow/internal/TaintTrackingPrivate.qll b/javascript/ql/lib/semmle/javascript/dataflow/internal/TaintTrackingPrivate.qll
@@ -104,3 +104,22 @@ predicate defaultImplicitTaintRead(DataFlow::Node node, ContentSet c) {
   // Optional steps are added through isAdditionalFlowStep but we don't want the implicit reads
   not optionalStep(node, _, _)
 }
+
+private predicate isArgumentToResolvedCall(DataFlow::Node arg) {
+  exists(DataFlowCall c |
+    exists(viableCallable(c)) and
+    isArgumentNode(arg, c, _)
+  )
+}
+
+predicate speculativeTaintStep(DataFlow::Node node1, DataFlow::Node node2) {
+  exists(DataFlow::CallNode call |
+    node1 = call.getAnArgument() and
+    node2 = call and
+    // A given node can appear as argument in more than one call. For example `x` in `fn.call(x)` is
+    // is argument 0 of the `fn.call` call, but also the receiver of a reflective call to `fn`.
+    // It is thus not enough to check if `call` has a known target; we nede to ensure that none of the calls
+    // involving `node1` have a known target.
+    not isArgumentToResolvedCall(node1)
+  )
+}
diff --git a/javascript/ql/lib/semmle/javascript/security/dataflow/IndirectCommandInjectionQuery.qll b/javascript/ql/lib/semmle/javascript/security/dataflow/IndirectCommandInjectionQuery.qll
@@ -26,10 +26,6 @@ module IndirectCommandInjectionConfig implements DataFlow::ConfigSig {
   predicate isSink(DataFlow::Node sink) { isSinkWithHighlight(sink, _) }
 
   predicate isBarrier(DataFlow::Node node) { node instanceof Sanitizer }
-
-  predicate isAdditionalFlowStep(DataFlow::Node pred, DataFlow::Node succ) {
-    argsParseStep(pred, succ)
-  }
 }
 
 /**

diff --git a/javascript/ql/test/experimental/Security/CWE-918/SSRF.expected b/javascript/ql/test/experimental/Security/CWE-918/SSRF.expected
@@ -12,6 +12,7 @@ edges
 | check-regex.js:31:29:31:45 | req.query.tainted | check-regex.js:31:15:31:45 | "test.c ... tainted | provenance |  |
 | check-regex.js:34:25:34:42 | req.params.tainted | check-regex.js:34:15:34:42 | baseURL ... tainted | provenance |  |
 | check-regex.js:41:27:41:43 | req.query.tainted | check-regex.js:41:13:41:43 | "test.c ... tainted | provenance |  |
+| check-regex.js:61:25:61:42 | req.params.tainted | check-regex.js:61:15:61:42 | baseURL ... tainted | provenance |  |
 | check-validator.js:15:29:15:45 | req.query.tainted | check-validator.js:15:15:15:45 | "test.c ... tainted | provenance |  |
 | check-validator.js:27:29:27:45 | req.query.tainted | check-validator.js:27:15:27:45 | "test.c ... tainted | provenance |  |
 | check-validator.js:50:29:50:45 | req.query.tainted | check-validator.js:50:15:50:45 | "test.c ... tainted | provenance |  |
@@ -47,6 +48,8 @@ nodes
 | check-regex.js:34:25:34:42 | req.params.tainted | semmle.label | req.params.tainted |
 | check-regex.js:41:13:41:43 | "test.c ... tainted | semmle.label | "test.c ... tainted |
 | check-regex.js:41:27:41:43 | req.query.tainted | semmle.label | req.query.tainted |
+| check-regex.js:61:15:61:42 | baseURL ... tainted | semmle.label | baseURL ... tainted |
+| check-regex.js:61:25:61:42 | req.params.tainted | semmle.label | req.params.tainted |
 | check-validator.js:15:15:15:45 | "test.c ... tainted | semmle.label | "test.c ... tainted |
 | check-validator.js:15:29:15:45 | req.query.tainted | semmle.label | req.query.tainted |
 | check-validator.js:27:15:27:45 | "test.c ... tainted | semmle.label | "test.c ... tainted |
@@ -76,6 +79,7 @@ subpaths
 | check-regex.js:31:15:31:45 | "test.c ... tainted | check-regex.js:31:29:31:45 | req.query.tainted | check-regex.js:31:15:31:45 | "test.c ... tainted | The URL of this request depends on a user-provided value. |
 | check-regex.js:34:15:34:42 | baseURL ... tainted | check-regex.js:34:25:34:42 | req.params.tainted | check-regex.js:34:15:34:42 | baseURL ... tainted | The URL of this request depends on a user-provided value. |
 | check-regex.js:41:13:41:43 | "test.c ... tainted | check-regex.js:41:27:41:43 | req.query.tainted | check-regex.js:41:13:41:43 | "test.c ... tainted | The URL of this request depends on a user-provided value. |
+| check-regex.js:61:15:61:42 | baseURL ... tainted | check-regex.js:61:25:61:42 | req.params.tainted | check-regex.js:61:15:61:42 | baseURL ... tainted | The URL of this request depends on a user-provided value. |
 | check-validator.js:15:15:15:45 | "test.c ... tainted | check-validator.js:15:29:15:45 | req.query.tainted | check-validator.js:15:15:15:45 | "test.c ... tainted | The URL of this request depends on a user-provided value. |
 | check-validator.js:27:15:27:45 | "test.c ... tainted | check-validator.js:27:29:27:45 | req.query.tainted | check-validator.js:27:15:27:45 | "test.c ... tainted | The URL of this request depends on a user-provided value. |
 | check-validator.js:50:15:50:45 | "test.c ... tainted | check-validator.js:50:29:50:45 | req.query.tainted | check-validator.js:50:15:50:45 | "test.c ... tainted | The URL of this request depends on a user-provided value. |

diff --git a/javascript/ql/test/library-tests/Arrays/DataFlow.expected b/javascript/ql/test/library-tests/Arrays/DataFlow.expected
@@ -17,6 +17,7 @@ flow
 | arrays.js:2:16:2:23 | "source" | arrays.js:86:8:86:35 | arrayFi ... llback) |
 | arrays.js:2:16:2:23 | "source" | arrays.js:90:10:90:10 | x |
 | arrays.js:2:16:2:23 | "source" | arrays.js:93:8:93:17 | arr.at(-1) |
+| arrays.js:2:16:2:23 | "source" | arrays.js:110:8:110:24 | arr8_spread.pop() |
 | arrays.js:18:22:18:29 | "source" | arrays.js:18:50:18:50 | e |
 | arrays.js:22:15:22:22 | "source" | arrays.js:23:8:23:17 | arr2.pop() |
 | arrays.js:25:15:25:22 | "source" | arrays.js:26:8:26:17 | arr3.pop() |
@@ -28,3 +29,5 @@ flow
 | arrays.js:53:4:53:11 | "source" | arrays.js:54:10:54:18 | ary.pop() |
 | arrays.js:96:9:96:16 | "source" | arrays.js:96:8:96:40 | ["sourc ... ).pop() |
 | arrays.js:97:9:97:16 | "source" | arrays.js:97:8:97:42 | ["sourc ... ).pop() |
+| arrays.js:100:31:100:38 | "source" | arrays.js:101:8:101:17 | arr8.pop() |
+| arrays.js:104:55:104:62 | "source" | arrays.js:106:8:106:25 | arr8_variant.pop() |
diff --git a/javascript/ql/test/library-tests/Arrays/TaintFlow.expected b/javascript/ql/test/library-tests/Arrays/TaintFlow.expected
@@ -16,6 +16,7 @@ flow
 | arrays.js:2:16:2:23 | "source" | arrays.js:86:8:86:35 | arrayFi ... llback) |
 | arrays.js:2:16:2:23 | "source" | arrays.js:90:10:90:10 | x |
 | arrays.js:2:16:2:23 | "source" | arrays.js:93:8:93:17 | arr.at(-1) |
+| arrays.js:2:16:2:23 | "source" | arrays.js:110:8:110:24 | arr8_spread.pop() |
 | arrays.js:18:22:18:29 | "source" | arrays.js:18:50:18:50 | e |
 | arrays.js:22:15:22:22 | "source" | arrays.js:23:8:23:17 | arr2.pop() |
 | arrays.js:25:15:25:22 | "source" | arrays.js:26:8:26:17 | arr3.pop() |
@@ -29,3 +30,5 @@ flow
 | arrays.js:95:9:95:16 | "source" | arrays.js:95:8:95:17 | ["source"] |
 | arrays.js:96:9:96:16 | "source" | arrays.js:96:8:96:40 | ["sourc ... ).pop() |
 | arrays.js:97:9:97:16 | "source" | arrays.js:97:8:97:42 | ["sourc ... ).pop() |
+| arrays.js:100:31:100:38 | "source" | arrays.js:101:8:101:17 | arr8.pop() |
+| arrays.js:104:55:104:62 | "source" | arrays.js:106:8:106:25 | arr8_variant.pop() |