JS: support label-specific sanitizer guards

Author: asger-semmle

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

@@ -146,6 +146,7 @@ abstract class Configuration extends string {
    */
   predicate isBarrier(DataFlow::Node node) {
     exists (BarrierGuardNode guard |
+      guard.blocksAllLabels() and
       isBarrierGuard(guard) and
       guard.blocks(node)
     )
@@ -161,6 +162,17 @@ abstract class Configuration extends string {
    */
   predicate isBarrier(DataFlow::Node src, DataFlow::Node trg, FlowLabel lbl) { none() }
 
+  /**
+   * Holds if flow with label `lbl` cannot flow into `node`.
+   */
+  predicate isBarrierForLabel(DataFlow::Node node, FlowLabel lbl) {
+    exists (BarrierGuardNode guard |
+      guard.blocksSpecificLabel(lbl) and
+      isBarrierGuard(guard) and
+      guard.blocks(node)
+    )
+  }
+
   /**
    * Holds if data flow node `guard` can act as a barrier when appearing
    * in a condition.
@@ -298,6 +310,15 @@ abstract class BarrierGuardNode extends DataFlow::Node {
    */
   abstract predicate blocks(boolean outcome, Expr e);
 
+  /**
+   * Holds if this barrier guard blocks all labels.
+   */
+  predicate blocksAllLabels() { any() }
+
+  /**
+   * Holds if this barrier guard only blocks specific labels, and `label` is one of them.
+   */
+  predicate blocksSpecificLabel(FlowLabel label) { none() }
 }
 
 /**
@@ -570,7 +591,8 @@ private predicate flowThroughCall(DataFlow::Node input, DataFlow::Node invk,
     ret.asExpr() = f.getAReturnedExpr() and
     calls(invk, f) and // Do not consider partial calls
     reachableFromInput(f, invk, input, ret, cfg, summary) and
-    not cfg.isBarrier(ret, invk)
+    not cfg.isBarrier(ret, invk) and
+    not cfg.isBarrierForLabel(invk, summary.getEndLabel())
   )
 }
 
@@ -641,7 +663,8 @@ private predicate flowStep(DataFlow::Node pred, DataFlow::Configuration cfg,
    flowThroughProperty(pred, succ, cfg, summary)
   ) and
   not cfg.isBarrier(succ) and
-  not cfg.isBarrier(pred, succ)
+  not cfg.isBarrier(pred, succ) and
+  not cfg.isBarrierForLabel(succ, summary.getEndLabel())
 }
 
 /**
@@ -666,6 +689,7 @@ private predicate reachableFromSource(DataFlow::Node nd, DataFlow::Configuration
   exists (FlowLabel lbl |
     isSource(nd, cfg, lbl) and
     not cfg.isBarrier(nd) and
+    not cfg.isBarrierForLabel(nd, lbl) and
     summary = MkPathSummary(false, false, lbl, lbl)
   )
   or
@@ -684,7 +708,8 @@ private predicate onPath(DataFlow::Node nd, DataFlow::Configuration cfg,
                          PathSummary summary) {
   reachableFromSource(nd, cfg, summary) and
   isSink(nd, cfg, summary.getEndLabel()) and
-  not cfg.isBarrier(nd)
+  not cfg.isBarrier(nd) and
+  not cfg.isBarrierForLabel(nd, summary.getEndLabel())
   or
   exists (DataFlow::Node mid, PathSummary stepSummary |
     reachableFromSource(nd, cfg, summary) and

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

@@ -10,6 +10,7 @@
  * 1. One or more sinks associated with the label `TaintedObject::label()`.
  * 2. The sources from `TaintedObject::isSource`.
  * 3. The flow steps from `TaintedObject::step`.
+ * 4. The sanitizing guards `TaintedObject::SanitizerGuard`.
  */
 import javascript
 
@@ -80,5 +81,41 @@ module TaintedObject {
     }
   }
 
-  // TODO: string tests should be classified as sanitizer guards; need support for flow labels on guards
+  /**
+   * Sanitizer guard that blocks deep object taint.
+   */
+  abstract class SanitizerGuard extends TaintTracking::SanitizerGuardNode {
+    override predicate blocksAllLabels() { none() }
+
+    override predicate blocksSpecificLabel(FlowLabel label) {
+      label = label()
+    }
+  }
+
+  /**
+   * A test of form `typeof x === "something"`, preventing `x` from being an object in some cases.
+   */
+  private class TypeTestGuard extends SanitizerGuard, ValueNode {
+    override EqualityTest astNode;
+    TypeofExpr typeof;
+    boolean polarity;
+
+    TypeTestGuard() {
+      astNode.getAnOperand() = typeof and
+      (
+        // typeof x === "object" sanitizes `x` when it evaluates to false
+        astNode.getAnOperand().getStringValue() = "object" and
+        polarity = astNode.getPolarity().booleanNot()
+        or
+        // typeof x === "string" sanitizes `x` when it evaluates to true
+        astNode.getAnOperand().getStringValue() != "object" and
+        polarity = astNode.getPolarity()
+      )
+    }
+
+    override predicate sanitizes(boolean outcome, Expr e) {
+      polarity = outcome and
+      e = typeof.getOperand()
+    }
+  }
 }

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

@@ -54,6 +54,10 @@ module NosqlInjection {
       node instanceof Sanitizer
     }
 
+    override predicate isSanitizerGuard(TaintTracking::SanitizerGuardNode guard) {
+      guard instanceof TaintedObject::SanitizerGuard
+    }
+
     override predicate isAdditionalFlowStep(DataFlow::Node src, DataFlow::Node trg, DataFlow::FlowLabel inlbl, DataFlow::FlowLabel outlbl) {
       TaintedObject::step(src, trg, inlbl, outlbl)
       or

javascript/ql/test/query-tests/Security/CWE-089/SqlInjection.expected

@@ -1,5 +1,6 @@
 | mongodb.js:18:16:18:20 | query | This query depends on $@. | mongodb.js:13:19:13:26 | req.body | a user-provided value |
-| mongodb.js:45:16:45:20 | query | This query depends on $@. | mongodb.js:40:19:40:33 | req.query.title | a user-provided value |
+| mongodb.js:32:18:32:45 | { title ... itle) } | This query depends on $@. | mongodb.js:26:19:26:26 | req.body | a user-provided value |
+| mongodb.js:54:16:54:20 | query | This query depends on $@. | mongodb.js:49:19:49:33 | req.query.title | a user-provided value |
 | mongodb_bodySafe.js:29:16:29:20 | query | This query depends on $@. | mongodb_bodySafe.js:24:19:24:33 | req.query.title | a user-provided value |
 | mongoose.js:27:20:27:24 | query | This query depends on $@. | mongoose.js:21:19:21:26 | req.body | a user-provided value |
 | mongoose.js:30:25:30:29 | query | This query depends on $@. | mongoose.js:21:19:21:26 | req.body | a user-provided value |

javascript/ql/test/query-tests/Security/CWE-089/mongodb.js

@@ -22,6 +22,15 @@ app.post('/documents/find', (req, res) => {
 
       // OK: throws unless user-data is a string
       doc.find({ title: query.body.title.substr(1) });
+
+      let title = req.body.title;
+      if (typeof title === "string") {
+        // OK: input checked to be a string
+        doc.find({ title: title });
+
+        // NOT OK: input is parsed as JSON after string check
+        doc.find({ title: JSON.parse(title) });
+      }
     });
 });