Merge pull request #1356 from asger-semmle/tainted-path-cherry-picked

JS: Refactor LabelledBarrierGuard

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

@@ -147,9 +147,8 @@ abstract class Configuration extends string {
    */
   predicate isBarrier(DataFlow::Node node) {
     exists(BarrierGuardNode guard |
-      not guard instanceof LabeledBarrierGuardNode and
       isBarrierGuard(guard) and
-      guard.blocks(node)
+      guard.internalBlocks(node, "")
     )
   }
 
@@ -167,10 +166,9 @@ abstract class Configuration extends string {
    * Holds if flow with label `lbl` cannot flow into `node`.
    */
   predicate isLabeledBarrier(DataFlow::Node node, FlowLabel lbl) {
-    exists(LabeledBarrierGuardNode guard |
-      lbl = guard.getALabel() and
+    exists(BarrierGuardNode guard |
       isBarrierGuard(guard) and
-      guard.blocks(node)
+      guard.internalBlocks(node, lbl)
     )
     or
     none() // relax type inference to account for overriding
@@ -254,43 +252,81 @@ module FlowLabel {
  */
 abstract class BarrierGuardNode extends DataFlow::Node {
   /**
-   * Holds if data flow node `nd` acts as a barrier for data flow.
+   * Holds if data flow node `nd` acts as a barrier for data flow, possibly due to aliasing
+   * through an access path.
+   *
+   * `label` is bound to the blocked label, or the empty string if all labels should be blocked.
    *
    * INTERNAL: this predicate should only be used from within `blocks(boolean, Expr)`.
    */
-  predicate blocks(DataFlow::Node nd) {
+  predicate internalBlocks(DataFlow::Node nd, string label) {
     // 1) `nd` is a use of a refinement node that blocks its input variable
-    exists(SsaRefinementNode ref |
+    exists(SsaRefinementNode ref, boolean outcome |
       nd = DataFlow::ssaDefinitionNode(ref) and
       forex(SsaVariable input | input = ref.getAnInput() |
         asExpr() = ref.getGuard().getTest() and
-        blocks(ref.getGuard().(ConditionGuardNode).getOutcome(), input.getAUse())
+        outcome = ref.getGuard().(ConditionGuardNode).getOutcome() and
+        internalBlocksExpr(outcome, input.getAUse(), label)
       )
     )
     or
     // 2) `nd` is an instance of an access path `p`, and dominated by a barrier for `p`
-    exists(AccessPath p, BasicBlock bb, ConditionGuardNode cond |
+    exists(AccessPath p, BasicBlock bb, ConditionGuardNode cond, boolean outcome |
       nd = DataFlow::valueNode(p.getAnInstanceIn(bb)) and
       asExpr() = cond.getTest() and
-      blocks(cond.getOutcome(), p.getAnInstance()) and
+      outcome = cond.getOutcome() and
+      internalBlocksAccessPath(outcome, p, label) and
       cond.dominates(bb)
     )
   }
 
+  /**
+   * Holds if data flow node `nd` acts as a barrier for data flow.
+   *
+   * `label` is bound to the blocked label, or the empty string if all labels should be blocked.
+   */
+  private predicate internalBlocksExpr(boolean outcome, Expr test, string label) {
+    blocks(outcome, test) and label = ""
+    or
+    blocks(outcome, test, label)
+  }
+
+  /**
+   * Holds if data flow node `nd` acts as a barrier for data flow due to aliasing through
+   * an access path.
+   *
+   * `label` is bound to the blocked label, or the empty string if all labels should be blocked.
+   */
+  pragma[noinline]
+  private predicate internalBlocksAccessPath(boolean outcome, AccessPath ap, string label) {
+    internalBlocksExpr(outcome, ap.getAnInstance(), label)
+  }
+
   /**
    * Holds if this node blocks expression `e` provided it evaluates to `outcome`.
+   *
+   * This will block all flow labels.
    */
   abstract predicate blocks(boolean outcome, Expr e);
+
+  /**
+   * Holds if this node blocks expression `e` from flow of type `label`, provided it evaluates to `outcome`.
+   */
+  predicate blocks(boolean outcome, Expr e, FlowLabel label) { none() }
 }
 
 /**
  * A guard node that only blocks specific labels.
  */
 abstract class LabeledBarrierGuardNode extends BarrierGuardNode {
+  override predicate blocks(boolean outcome, Expr e) { none() }
+
   /**
-   * Get a flow label blocked by this guard node.
+   * DEPRECATED: Use `blocks(outcome, e, label)` or `sanitizes(outcome, e, label)` instead.
+   *
+   * Overriding this predicate has no effect.
    */
-  abstract FlowLabel getALabel();
+  deprecated FlowLabel getALabel() { none() }
 }
 
 /**

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

@@ -129,20 +129,35 @@ module TaintTracking {
    * A node that can act as a sanitizer when appearing in a condition.
    */
   abstract class SanitizerGuardNode extends DataFlow::BarrierGuardNode {
-    final override predicate blocks(boolean outcome, Expr e) { sanitizes(outcome, e) }
+    override predicate blocks(boolean outcome, Expr e) { sanitizes(outcome, e) }
 
     /**
      * Holds if this node sanitizes expression `e`, provided it evaluates
      * to `outcome`.
      */
     abstract predicate sanitizes(boolean outcome, Expr e);
+
+    override predicate blocks(boolean outcome, Expr e, DataFlow::FlowLabel label) {
+      sanitizes(outcome, e, label)
+    }
+
+    /**
+     * Holds if this node sanitizes expression `e`, provided it evaluates
+     * to `outcome`.
+     */
+    predicate sanitizes(boolean outcome, Expr e, DataFlow::FlowLabel label) { none() }
   }
 
   /**
    * A sanitizer guard node that only blocks specific flow labels.
    */
-  abstract class LabeledSanitizerGuardNode extends SanitizerGuardNode,
-    DataFlow::LabeledBarrierGuardNode { }
+  abstract class LabeledSanitizerGuardNode extends SanitizerGuardNode, DataFlow::BarrierGuardNode {
+    final override predicate blocks(boolean outcome, Expr e, DataFlow::FlowLabel label) {
+      sanitizes(outcome, e, label)
+    }
+
+    override predicate sanitizes(boolean outcome, Expr e) { none() }
+  }
 
   /**
    * A taint-propagating data flow edge that should be added to all taint tracking

javascript/ql/src/semmle/javascript/security/TaintedObject.qll

@@ -84,7 +84,6 @@ module TaintedObject {
    * Sanitizer guard that blocks deep object taint.
    */
   abstract class SanitizerGuard extends TaintTracking::LabeledSanitizerGuardNode {
-    override FlowLabel getALabel() { result = label() }
   }
 
   /**
@@ -110,9 +109,10 @@ module TaintedObject {
       )
     }
 
-    override predicate sanitizes(boolean outcome, Expr e) {
+    override predicate sanitizes(boolean outcome, Expr e, FlowLabel label) {
       polarity = outcome and
-      e = typeof.getOperand()
+      e = typeof.getOperand() and
+      label = label()
     }
   }
 }

javascript/ql/src/semmle/javascript/security/dataflow/UnvalidatedDynamicMethodCall.qll

@@ -142,11 +142,10 @@ module UnvalidatedDynamicMethodCall {
       astNode.getAnOperand().getUnderlyingValue() = t
     }
 
-    override predicate sanitizes(boolean outcome, Expr e) {
+    override predicate sanitizes(boolean outcome, Expr e, DataFlow::FlowLabel label) {
       outcome = astNode.getPolarity() and
-      e = t.getOperand().getUnderlyingValue()
+      e = t.getOperand().getUnderlyingValue() and
+      label instanceof MaybeNonFunction
     }
-
-    override DataFlow::FlowLabel getALabel() { result instanceof MaybeNonFunction }
   }
 }

javascript/ql/test/library-tests/LabelledBarrierGuards/LabelledBarrierGuards.expected

@@ -0,0 +1,6 @@
+| tst.js:2:11:2:18 | source() | tst.js:8:12:8:12 | x |
+| tst.js:2:11:2:18 | source() | tst.js:12:12:12:12 | x |
+| tst.js:2:11:2:18 | source() | tst.js:14:12:14:12 | x |
+| tst.js:2:11:2:18 | source() | tst.js:18:12:18:12 | x |
+| tst.js:2:11:2:18 | source() | tst.js:20:12:20:12 | x |
+| tst.js:2:11:2:18 | source() | tst.js:26:12:26:12 | x |

javascript/ql/test/library-tests/LabelledBarrierGuards/LabelledBarrierGuards.ql

@@ -0,0 +1,68 @@
+import javascript
+
+class CustomFlowLabel extends DataFlow::FlowLabel {
+  CustomFlowLabel() {
+    this = "A" or this = "B"
+  }
+}
+
+class Config extends TaintTracking::Configuration {
+  Config() { this = "Config" }
+
+  override predicate isSource(DataFlow::Node node, DataFlow::FlowLabel lbl) {
+    node.(DataFlow::CallNode).getCalleeName() = "source" and
+    lbl instanceof CustomFlowLabel
+  }
+
+  override predicate isSink(DataFlow::Node node, DataFlow::FlowLabel lbl) {
+    exists(DataFlow::CallNode call |
+      call.getCalleeName() = "sink" and
+      node = call.getAnArgument() and
+      lbl instanceof CustomFlowLabel
+    )
+  }
+
+  override predicate isSanitizerGuard(TaintTracking::SanitizerGuardNode node) {
+    node instanceof IsTypeAGuard or
+    node instanceof IsSanitizedGuard
+  }
+}
+
+/**
+ * A condition that checks what kind of value the input is. Not enough to
+ * sanitize the value, but later sanitizers only need to handle the relevant case.
+ */
+class IsTypeAGuard extends TaintTracking::LabeledSanitizerGuardNode, DataFlow::CallNode {
+  IsTypeAGuard() {
+    getCalleeName() = "isTypeA"
+  }
+
+  override predicate sanitizes(boolean outcome, Expr e, DataFlow::FlowLabel lbl) {
+    e = getArgument(0).asExpr() and
+    (
+      outcome = true and lbl = "B"
+      or
+      outcome = false and lbl = "A"
+    )
+  }
+}
+
+class IsSanitizedGuard extends TaintTracking::LabeledSanitizerGuardNode, DataFlow::CallNode {
+  IsSanitizedGuard() {
+    getCalleeName() = "sanitizeA" or getCalleeName() = "sanitizeB"
+  }
+
+  override predicate sanitizes(boolean outcome, Expr e, DataFlow::FlowLabel lbl) {
+    e = getArgument(0).asExpr() and
+    outcome = true and
+    (
+      getCalleeName() = "sanitizeA" and lbl = "A"
+      or
+      getCalleeName() = "sanitizeB" and lbl = "B"
+    )
+  }
+}
+
+from Config cfg, DataFlow::Node source, DataFlow::Node sink
+where cfg.hasFlow(source, sink)
+select source, sink

javascript/ql/test/library-tests/LabelledBarrierGuards/tst.js

@@ -0,0 +1,33 @@
+function test() {
+  let x = source();
+
+  if (isTypeA(x)) {
+    if (sanitizeA(x)) {
+      sink(x); // OK
+    } else {
+      sink(x); // NOT OK
+    }
+
+    if (sanitizeB(x)) {
+      sink(x); // NOT OK
+    } else {
+      sink(x); // NOT OK
+    }
+  } else {
+    if (sanitizeA(x)) {
+      sink(x); // NOT OK
+    } else {
+      sink(x); // NOT OK
+    }
+
+    if (sanitizeB(x)) {
+      sink(x); // OK
+    } else {
+      sink(x); // NOT OK
+    }
+  }
+
+  if (sanitizeA(x) && sanitizeB(x)) {
+    sink(x); // OK
+  }
+}