Merge pull request #6253 from asgerf/js/more-precise-capture-steps

Approved by erik-krogh

Author: codeql-ci

javascript/change-notes/2021-07-12-more-precise-capture-steps.md

@@ -0,0 +1,4 @@
+lgtm,codescanning
+* Fixed a bug that could occur when data was tracked through a function whose parameter
+  flows through a captured variable before reaching the return.
+  This can lead to fewer false-positive results and more true-positive results.

javascript/ql/src/semmle/javascript/dataflow/Configuration.qll

@@ -1183,6 +1183,13 @@ private predicate flowThroughCall(
     not cfg.isLabeledBarrier(output, summary.getEndLabel())
   )
   or
+  exists(Function f, LocalVariable variable |
+    reachableFromInput(f, _, input, output, cfg, summary) and
+    output = DataFlow::capturedVariableNode(variable) and
+    getCapturedVariableDepth(variable) < getContainerDepth(f) and // Only step outwards
+    not cfg.isLabeledBarrier(output, summary.getEndLabel())
+  )
+  or
   exists(Function f, DataFlow::Node invk, DataFlow::Node ret |
     DataFlow::exceptionalFunctionReturnNode(ret, f) and
     DataFlow::exceptionalInvocationReturnNode(output, invk.asExpr()) and

javascript/ql/src/semmle/javascript/dataflow/internal/FlowSteps.qll

@@ -109,13 +109,30 @@ DataFlow::Node getThrowTarget(DataFlow::Node thrower) {
  */
 cached
 private module CachedSteps {
+  /** Gets the nesting depth of the given container, starting with the top-level at 0. */
+  cached
+  int getContainerDepth(StmtContainer container) {
+    not exists(container.getEnclosingContainer()) and
+    result = 0
+    or
+    result = 1 + getContainerDepth(container.getEnclosingContainer())
+  }
+
+  /** Gets the nesting depth of the container declaring the given captured variable. */
+  cached
+  int getCapturedVariableDepth(LocalVariable v) {
+    v.isCaptured() and
+    result = getContainerDepth(v.getDeclaringContainer())
+  }
+
   /**
    * Holds if `f` captures the given `variable` in `cap`.
    */
   cached
   predicate captures(Function f, LocalVariable variable, SsaVariableCapture cap) {
     variable = cap.getSourceVariable() and
-    f = cap.getContainer()
+    f = cap.getContainer() and
+    not f = variable.getDeclaringContainer()
   }
 
   /**

javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected

@@ -34,6 +34,8 @@ typeInferenceMismatch
 | callbacks.js:51:18:51:25 | source() | callbacks.js:30:29:30:29 | y |
 | callbacks.js:53:23:53:30 | source() | callbacks.js:58:10:58:10 | x |
 | capture-flow.js:9:11:9:18 | source() | capture-flow.js:14:10:14:16 | outer() |
+| capture-flow.js:9:11:9:18 | source() | capture-flow.js:19:6:19:16 | outerMost() |
+| capture-flow.js:31:14:31:21 | source() | capture-flow.js:31:6:31:22 | confuse(source()) |
 | captured-sanitizer.js:25:3:25:10 | source() | captured-sanitizer.js:15:10:15:10 | x |
 | case.js:2:16:2:23 | source() | case.js:5:8:5:35 | changeC ... source) |
 | case.js:2:16:2:23 | source() | case.js:8:8:8:24 | camelCase(source) |

javascript/ql/test/library-tests/TaintTracking/DataFlowTracking.expected

@@ -24,6 +24,8 @@
 | callbacks.js:51:18:51:25 | source() | callbacks.js:30:29:30:29 | y |
 | callbacks.js:53:23:53:30 | source() | callbacks.js:58:10:58:10 | x |
 | capture-flow.js:9:11:9:18 | source() | capture-flow.js:14:10:14:16 | outer() |
+| capture-flow.js:9:11:9:18 | source() | capture-flow.js:19:6:19:16 | outerMost() |
+| capture-flow.js:31:14:31:21 | source() | capture-flow.js:31:6:31:22 | confuse(source()) |
 | captured-sanitizer.js:25:3:25:10 | source() | captured-sanitizer.js:15:10:15:10 | x |
 | constructor-calls.js:4:18:4:25 | source() | constructor-calls.js:18:8:18:14 | c.taint |
 | constructor-calls.js:4:18:4:25 | source() | constructor-calls.js:22:8:22:19 | c_safe.taint |

javascript/ql/test/library-tests/TaintTracking/capture-flow.js

@@ -16,4 +16,16 @@ function outerMost() {
     return outer();
 }
 
-sink(outerMost()); // NOT OK - but missed
+sink(outerMost()); // NOT OK
+
+function confuse(x) {
+    let captured;
+    function f() {
+        captured = x;
+    }
+    f();
+    return captured;
+}
+
+sink(confuse('safe')); // OK
+sink(confuse(source())); // NOT OK