JS: Make barrier guards work with use-use flow

Author: asgerf

javascript/ql/lib/semmle/javascript/dataflow/internal/BarrierGuards.qll

@@ -6,6 +6,8 @@
 
 private import javascript
 private import semmle.javascript.dataflow.internal.AccessPaths
+private import semmle.javascript.dataflow.internal.DataFlowPrivate as DataFlowPrivate
+private import semmle.javascript.dataflow.internal.sharedlib.Ssa as Ssa2
 
 private signature class BarrierGuardSig extends DataFlow::Node {
   /**
@@ -282,6 +284,22 @@ module MakeStateBarrierGuard<
     )
   }
 
+  private predicate ssa2GuardChecks(
+    Ssa2::SsaDataflowInput::Guard guard, Ssa2::SsaDataflowInput::Expr test, boolean branch,
+    FlowState state
+  ) {
+    exists(BarrierGuard g |
+      g.asExpr() = guard and
+      g.blocksExpr(branch, test, state)
+    )
+  }
+
+  private module Ssa2Barrier = Ssa2::BarrierGuardWithState<FlowState, ssa2GuardChecks/4>;
+
+  private predicate ssa2BlocksNode(DataFlow::Node node, FlowState state) {
+    node = DataFlowPrivate::getNodeFromSsa2(Ssa2Barrier::getABarrierNode(state))
+  }
+
   /** Holds if a barrier guard blocks uses of `ap` in basic blocks dominated by `cond`. */
   pragma[nomagic]
   private predicate barrierGuardBlocksAccessPathIn(
@@ -323,6 +341,8 @@ module MakeStateBarrierGuard<
       barrierGuardBlocksAccessPathUse(use, state) and
       nd = DataFlow::valueNode(use)
     )
+    or
+    ssa2BlocksNode(nd, state)
   }
 
   /**

javascript/ql/lib/semmle/javascript/dataflow/internal/DataFlowPrivate.qll

@@ -1062,7 +1062,8 @@ private predicate samePhi(SsaPhiNode legacyPhi, Ssa2::PhiNode newPhi) {
   )
 }
 
-private Node getNodeFromSsa2(Ssa2::Node node) {
+cached
+Node getNodeFromSsa2(Ssa2::Node node) {
   result = TSsaUseNode(node.(Ssa2::ExprNode).getExpr())
   or
   result = TExprPostUpdateNode(node.(Ssa2::ExprPostUpdateNode).getExpr())

javascript/ql/lib/semmle/javascript/dataflow/internal/TaintTrackingPrivate.qll

@@ -5,6 +5,7 @@ private import semmle.javascript.dataflow.internal.Contents::Public
 private import semmle.javascript.dataflow.internal.sharedlib.FlowSummaryImpl as FlowSummaryImpl
 private import semmle.javascript.dataflow.internal.FlowSummaryPrivate as FlowSummaryPrivate
 private import semmle.javascript.dataflow.internal.BarrierGuards
+private import semmle.javascript.dataflow.internal.sharedlib.Ssa as Ssa2
 
 cached
 predicate defaultAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) {
@@ -44,13 +45,51 @@ private class SanitizerGuardAdapter extends DataFlow::Node instanceof TaintTrack
   predicate blocksExpr(boolean outcome, Expr e) { super.sanitizes(outcome, e) }
 }
 
+bindingset[node]
+pragma[inline_late]
+private BasicBlock getBasicBlockFromSsa2(Ssa2::Node node) {
+  result = node.(Ssa2::ExprNode).getExpr().getBasicBlock()
+  or
+  node.(Ssa2::SsaInputNode).isInputInto(_, result)
+}
+
+/**
+ * Holds if `node` should act as a taint barrier, as it occurs after a variable has been checked to be falsy.
+ *
+ * For example:
+ * ```js
+ * if (!x) {
+ *   use(x); // <-- 'x' is a varAccessBarrier
+ * }
+ * ```
+ *
+ * This is particularly important for ensuring that query-specific barrier guards work when they
+ * occur after a truthiness-check:
+ * ```js
+ * if (x && !isSafe(x)) {
+ *   throw new Error()
+ * }
+ * use(x); // both inputs to the phi-read for 'x' are blocked (one by varAccessBarrier, one by isSafe(x))
+ * ```
+ */
+private predicate varAccessBarrier(DataFlow::Node node) {
+  exists(ConditionGuardNode guard, Ssa2::ExprNode nodeFrom, Ssa2::Node nodeTo |
+    guard.getOutcome() = false and
+    guard.getTest().(VarAccess) = nodeFrom.getExpr() and
+    Ssa2::localFlowStep(_, nodeFrom, nodeTo, true) and
+    guard.dominates(getBasicBlockFromSsa2(nodeTo)) and
+    node = getNodeFromSsa2(nodeTo)
+  )
+}
+
 /**
  * Holds if `node` should be a sanitizer in all global taint flow configurations
  * but not in local taint.
  */
 cached
 predicate defaultTaintSanitizer(DataFlow::Node node) {
   node instanceof DataFlow::VarAccessBarrier or
+  varAccessBarrier(node) or
   node = MakeBarrierGuard<SanitizerGuardAdapter>::getABarrierNode()
 }
 

javascript/ql/lib/semmle/javascript/dataflow/internal/sharedlib/Ssa.qll

@@ -53,7 +53,7 @@ module SsaConfig implements InputSig<js::DbLocation> {
 
 import Make<js::DbLocation, SsaConfig>
 
-private module SsaDataflowInput implements DataFlowIntegrationInputSig {
+module SsaDataflowInput implements DataFlowIntegrationInputSig {
   class Expr extends js::ControlFlowNode {
     Expr() { this = any(SsaConfig::SourceVariable v).getAUse() }
 
@@ -66,11 +66,28 @@ private module SsaDataflowInput implements DataFlowIntegrationInputSig {
 
   predicate ssaDefInitializesParam(WriteDefinition def, Parameter p) { none() } // Not handled here
 
-  abstract class Guard extends Expr { } // empty class
+  class Guard extends js::ControlFlowNode {
+    Guard() { this = any(js::ConditionGuardNode g).getTest() }
 
-  predicate guardControlsBlock(Guard guard, js::BasicBlock bb, boolean branch) { none() }
+    predicate hasCfgNode(js::BasicBlock bb, int i) { this = bb.getNode(i) }
+  }
+
+  pragma[inline]
+  predicate guardControlsBlock(Guard guard, js::BasicBlock bb, boolean branch) {
+    exists(js::ConditionGuardNode g |
+      g.getTest() = guard and
+      g.dominates(bb) and
+      branch = g.getOutcome()
+    )
+  }
 
-  js::BasicBlock getAConditionalBasicBlockSuccessor(js::BasicBlock bb, boolean branch) { none() }
+  js::BasicBlock getAConditionalBasicBlockSuccessor(js::BasicBlock bb, boolean branch) {
+    exists(js::ConditionGuardNode g |
+      bb = g.getTest().getBasicBlock() and
+      result = g.getBasicBlock() and
+      branch = g.getOutcome()
+    )
+  }
 }
 
 import DataFlowIntegration<SsaDataflowInput>

javascript/ql/test/library-tests/TripleDot/useuse.js

@@ -111,3 +111,34 @@ function t7() {
     const c = new Sub(source('t7.1'));
     sink(c.field); // $ hasTaintFlow=t7.1
 }
+
+function t8() {
+    function foo(x) {
+        const obj = {};
+        obj.field = x;
+
+        sink(obj.field); // $ hasTaintFlow=t8.1
+
+        if (obj) {
+            sink(obj.field); // $ hasTaintFlow=t8.1
+        } else {
+            sink(obj.field);
+        }
+
+        if (!obj) {
+            sink(obj.field);
+        } else {
+            sink(obj.field); // $ hasTaintFlow=t8.1
+        }
+
+        if (!obj || !obj) {
+            sink(obj.field);
+        } else {
+            sink(obj.field); // $ hasTaintFlow=t8.1
+        }
+    }
+
+    // The guards used above are specific to taint-tracking, to ensure only taint flows in
+    const taint = source('t8.1') + ' taint';
+    foo(taint);
+}