Merge pull request #2435 from asger-semmle/phi-edge-barrier-guards

JS: Phi edge barrier guards

Author: max-schaefer

change-notes/1.24/analysis-javascript.md

@@ -7,6 +7,8 @@
 * Imports with the `.js` extension can now be resolved to a TypeScript file,
   when the import refers to a file generated by TypeScript.
 
+- The analysis of sanitizer guards has improved, leading to fewer false-positive results from the security queries.
+
 * Support for the following frameworks and libraries has been improved:
   - [react](https://www.npmjs.com/package/react)
   - [typeahead.js](https://www.npmjs.com/package/typeahead.js)

javascript/ql/src/semmle/javascript/SSA.qll

@@ -639,8 +639,16 @@ abstract class SsaPseudoDefinition extends SsaImplicitDefinition {
  * would be visible.
  */
 class SsaPhiNode extends SsaPseudoDefinition, TPhi {
+  /**
+   * Gets the input to this phi node coming from the given predecessor block.
+   */
+  SsaVariable getInputFromBlock(BasicBlock bb) {
+    bb = getBasicBlock().getAPredecessor() and
+    result = getDefReachingEndOf(bb, getSourceVariable())
+  }
+
   override SsaVariable getAnInput() {
-    result = getDefReachingEndOf(getBasicBlock().getAPredecessor(), getSourceVariable())
+    result = getInputFromBlock(_)
   }
 
   override predicate definesAt(ReachableBasicBlock bb, int i, SsaSourceVariable v) {

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

@@ -363,6 +363,51 @@ private predicate barrierGuardBlocksAccessPath(BarrierGuardNode guard, boolean o
   barrierGuardBlocksExpr(guard, outcome, ap.getAnInstance(), label)
 }
 
+/**
+  * Holds if `guard` should block flow along the edge `pred -> succ`.
+  *
+  * `label` is bound to the blocked label, or the empty string if all labels should be blocked.
+  */
+private predicate barrierGuardBlocksEdge(BarrierGuardNode guard, DataFlow::Node pred, DataFlow::Node succ, string label) {
+  exists(SsaVariable input, SsaPhiNode phi, BasicBlock bb, ConditionGuardNode cond, boolean outcome |
+    pred = DataFlow::ssaDefinitionNode(input) and
+    succ = DataFlow::ssaDefinitionNode(phi) and
+    input = phi.getInputFromBlock(bb) and
+    guard.getEnclosingExpr() = cond.getTest() and
+    outcome = cond.getOutcome() and
+    barrierGuardBlocksExpr(guard, outcome, input.getAUse(), label) and
+    cond.dominates(bb)
+  )
+}
+
+/**
+ * Holds if there is a barrier edge `pred -> succ` in `cfg` either through an explicit barrier edge
+ * or one implied by a barrier guard.
+ *
+ * Only holds for barriers that should apply to all flow labels.
+ */
+private predicate isBarrierEdge(Configuration cfg, DataFlow::Node pred, DataFlow::Node succ) {
+  cfg.isBarrierEdge(pred, succ)
+  or
+  exists(DataFlow::BarrierGuardNode guard |
+    cfg.isBarrierGuard(guard) and
+    barrierGuardBlocksEdge(guard, pred, succ, "")
+  )
+}
+
+/**
+ * Holds if there is a labeled barrier edge `pred -> succ` in `cfg` either through an explicit barrier edge
+ * or one implied by a barrier guard.
+ */
+private predicate isLabeledBarrierEdge(Configuration cfg, DataFlow::Node pred, DataFlow::Node succ, DataFlow::FlowLabel label) {
+  cfg.isBarrierEdge(pred, succ, label)
+  or
+  exists(DataFlow::BarrierGuardNode guard |
+    cfg.isBarrierGuard(guard) and
+    barrierGuardBlocksEdge(guard, pred, succ, label)
+  )
+}
+
 /**
  * A guard node that only blocks specific labels.
  */
@@ -470,7 +515,8 @@ private predicate basicFlowStep(
     // Local flow
     exists(FlowLabel predlbl, FlowLabel succlbl |
       localFlowStep(pred, succ, cfg, predlbl, succlbl) and
-      not cfg.isBarrierEdge(pred, succ, predlbl) and
+      not isLabeledBarrierEdge(cfg, pred, succ, predlbl) and
+      not isBarrierEdge(cfg, pred, succ) and
       summary = MkPathSummary(false, false, predlbl, succlbl)
     )
     or
@@ -584,7 +630,7 @@ private predicate callInputStep(
     )
   ) and
   not cfg.isBarrier(succ) and
-  not cfg.isBarrierEdge(pred, succ)
+  not isBarrierEdge(cfg, pred, succ)
 }
 
 /**
@@ -638,7 +684,8 @@ private predicate flowThroughCall(
     ret.asExpr() = f.getAReturnedExpr() and
     calls(output, f) and // Do not consider partial calls
     reachableFromInput(f, output, input, ret, cfg, summary) and
-    not cfg.isBarrierEdge(ret, output) and
+    not isBarrierEdge(cfg, ret, output) and
+    not isLabeledBarrierEdge(cfg, ret, output, summary.getEndLabel()) and
     not cfg.isLabeledBarrier(output, summary.getEndLabel())
   )
   or
@@ -647,7 +694,8 @@ private predicate flowThroughCall(
     DataFlow::exceptionalInvocationReturnNode(output, invk.asExpr()) and
     calls(invk, f) and
     reachableFromInput(f, invk, input, ret, cfg, summary) and
-    not cfg.isBarrierEdge(ret, output) and
+    not isBarrierEdge(cfg, ret, output) and
+    not isLabeledBarrierEdge(cfg, ret, output, summary.getEndLabel()) and
     not cfg.isLabeledBarrier(output, summary.getEndLabel())
   )
 }
@@ -886,7 +934,8 @@ private predicate flowStep(
     flowIntoHigherOrderCall(pred, succ, cfg, summary)
   ) and
   not cfg.isBarrier(succ) and
-  not cfg.isBarrierEdge(pred, succ) and
+  not isBarrierEdge(cfg, pred, succ) and
+  not isLabeledBarrierEdge(cfg, pred, succ, summary.getEndLabel()) and
   not cfg.isLabeledBarrier(succ, summary.getEndLabel())
 }
 

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

@@ -77,6 +77,7 @@ typeInferenceMismatch
 | sanitizer-guards.js:13:14:13:21 | source() | sanitizer-guards.js:26:9:26:14 | this.x |
 | sanitizer-guards.js:43:11:43:18 | source() | sanitizer-guards.js:45:8:45:8 | x |
 | sanitizer-guards.js:43:11:43:18 | source() | sanitizer-guards.js:48:10:48:10 | x |
+| sanitizer-guards.js:68:11:68:18 | source() | sanitizer-guards.js:75:8:75:8 | x |
 | spread.js:2:15:2:22 | source() | spread.js:4:8:4:19 | { ...taint } |
 | spread.js:2:15:2:22 | source() | spread.js:5:8:5:43 | { f: 'h ... orld' } |
 | spread.js:2:15:2:22 | source() | spread.js:7:8:7:19 | [ ...taint ] |

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

@@ -53,6 +53,7 @@
 | sanitizer-guards.js:43:11:43:18 | source() | sanitizer-guards.js:45:8:45:8 | x |
 | sanitizer-guards.js:43:11:43:18 | source() | sanitizer-guards.js:48:10:48:10 | x |
 | sanitizer-guards.js:43:11:43:18 | source() | sanitizer-guards.js:52:10:52:10 | x |
+| sanitizer-guards.js:68:11:68:18 | source() | sanitizer-guards.js:75:8:75:8 | x |
 | thisAssignments.js:4:17:4:24 | source() | thisAssignments.js:5:10:5:18 | obj.field |
 | thisAssignments.js:7:19:7:26 | source() | thisAssignments.js:8:10:8:20 | this.field2 |
 | tst.js:2:13:2:20 | source() | tst.js:4:10:4:10 | x |

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

@@ -52,3 +52,25 @@ function reflective() {
     sink(x); // OK
   }
 }
+
+function phi() {
+  let x = source();
+
+  if (something(x) && isSafe(x)) {
+    // this input to the phi node for 'x' should be sanitized
+  } else {
+    x = null;
+  }
+  sink(x); // OK
+}
+
+function phi2() {
+  let x = source();
+
+  if (something(x) || isSafe(x)) {
+    // this input to the phi node for 'x' is not fully sanitized
+  } else {
+    x = null;
+  }
+  sink(x); // NOT OK
+}