Merge pull request #1272 from asger-semmle/access-path-capture

Approved by xiemaisi

Author: semmle-qlci

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

@@ -43,20 +43,38 @@ private SsaVariable getRefinedVariable(SsaVariable variable) {
   result = variable.getDefinition().(SsaRefinementNode).getAnInput()
 }
 
-private SsaVariable getARefinementOf(SsaVariable variable) {
-  variable = getRefinedVariable(result)
-}
+private SsaVariable getARefinementOf(SsaVariable variable) { variable = getRefinedVariable(result) }
 
 /**
- * A representation of a (nested) property access on an SSA variable
+ * A representation of a (nested) property access on an SSA variable or captured variable
  * where each property name is either constant or itself an SSA variable.
  */
 private newtype TAccessPath =
+  /**
+   * An access path rooted in an SSA variable.
+   *
+   * Refinement nodes are treated as no-ops so that all uses of a refined value are
+   * given the same access path. Refinement nodes are therefore never treated as roots.
+   */
   MkSsaRoot(SsaVariable var) {
-    not exists(getRefinedVariable(var))
-  }
-  or
+    not exists(getRefinedVariable(var)) and
+    not var.getSourceVariable().isCaptured() // Handled by MkCapturedRoot
+  } or
+  /**
+   * An access path rooted in a captured variable.
+   *
+   * The SSA form for captured variables is too conservative for constructing
+   * access paths across function boundaries, so in this case we use the source
+   * variable as the root.
+   */
+  MkCapturedRoot(LocalVariable var) { var.isCaptured() } or
+  /**
+   * An access path rooted in the receiver of a function.
+   */
   MkThisRoot(Function function) { function.getThisBinder() = function } or
+  /**
+   * A property access on an access path.
+   */
   MkAccessStep(AccessPath base, PropertyName name) {
     exists(PropAccess pacc |
       pacc.getBase() = base.getAnInstance() and
@@ -65,7 +83,7 @@ private newtype TAccessPath =
   }
 
 /**
- * A representation of a (nested) property access on an SSA variable
+ * A representation of a (nested) property access on an SSA variable or captured variable
  * where each property name is either constant or itself an SSA variable.
  */
 class AccessPath extends TAccessPath {
@@ -78,6 +96,12 @@ class AccessPath extends TAccessPath {
       result = getARefinementOf*(var).getAUseIn(bb)
     )
     or
+    exists(Variable var |
+      this = MkCapturedRoot(var) and
+      result = var.getAnAccess() and
+      result.getBasicBlock() = bb
+    )
+    or
     exists(ThisExpr this_ |
       this = MkThisRoot(this_.getBinder()) and
       result = this_ and

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

@@ -12,6 +12,7 @@
 | callbacks.js:44:17:44:24 | source() | callbacks.js:41:10:41:10 | x |
 | callbacks.js:50:18:50:25 | source() | callbacks.js:30:29:30:29 | y |
 | callbacks.js:51:18:51:25 | source() | callbacks.js:30:29:30:29 | y |
+| captured-sanitizer.js:25:3:25:10 | source() | captured-sanitizer.js:15:10:15:10 | x |
 | closure.js:6:15:6:22 | source() | closure.js:8:8:8:31 | string. ... (taint) |
 | closure.js:6:15:6:22 | source() | closure.js:9:8:9:25 | string.trim(taint) |
 | closure.js:6:15:6:22 | source() | closure.js:10:8:10:33 | string. ... nt, 50) |

javascript/ql/test/library-tests/TaintTracking/captured-sanitizer.js

@@ -0,0 +1,25 @@
+import * as dummy from 'dummy';
+
+function f(x) {
+  useVar();
+  useVar();
+  mutateVar();
+  mutateVar();
+
+  function useVar() {
+    if (isSafe(x)) {
+      causeReCapture();
+      causeReCapture();
+      sink(x); // OK
+    }
+    sink(x); // NOT OK
+  }
+
+  function causeReCapture() {}
+
+  function mutateVar() {
+    x = null;
+  }
+}
+
+f(source());