JS: address comments and introduce LabeledBarrierGuardNode

Author: asger-semmle

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

@@ -93,7 +93,7 @@ abstract class Configuration extends string {
   }
 
   /**
-   * Holds if `source` is a source of flow labelled with `lbl` that is relevant
+   * Holds if `source` is a source of flow labeled with `lbl` that is relevant
    * for this configuration.
    */
   predicate isSource(DataFlow::Node source, FlowLabel lbl) {
@@ -108,7 +108,7 @@ abstract class Configuration extends string {
   }
 
   /**
-   * Holds if `sink` is a sink of flow labelled with `lbl` that is relevant
+   * Holds if `sink` is a sink of flow labeled with `lbl` that is relevant
    * for this configuration.
    */
   predicate isSink(DataFlow::Node sink, FlowLabel lbl) {
@@ -146,7 +146,7 @@ abstract class Configuration extends string {
    */
   predicate isBarrier(DataFlow::Node node) {
     exists (BarrierGuardNode guard |
-      guard.blocksAllLabels() and
+      not guard instanceof LabeledBarrierGuardNode and
       isBarrierGuard(guard) and
       guard.blocks(node)
     )
@@ -165,9 +165,9 @@ abstract class Configuration extends string {
   /**
    * Holds if flow with label `lbl` cannot flow into `node`.
    */
-  predicate isBarrierForLabel(DataFlow::Node node, FlowLabel lbl) {
-    exists (BarrierGuardNode guard |
-      guard.blocksSpecificLabel(lbl) and
+  predicate isLabeledBarrier(DataFlow::Node node, FlowLabel lbl) {
+    exists (LabeledBarrierGuardNode guard |
+      lbl = guard.getALabel() and
       isBarrierGuard(guard) and
       guard.blocks(node)
     )
@@ -309,21 +309,16 @@ abstract class BarrierGuardNode extends DataFlow::Node {
    * Holds if this node blocks expression `e` provided it evaluates to `outcome`.
    */
   abstract predicate blocks(boolean outcome, Expr e);
+}
 
+/**
+ * A guard node that only blocks specific labels.
+ */
+abstract class LabeledBarrierGuardNode extends BarrierGuardNode {
   /**
-   * Holds if this barrier guard should block all labels.
-   *
-   * To block specific labels only, subclasses should override this with `none()` and
-   * also override `blocksSpecificLabel`.
-   */
-  predicate blocksAllLabels() { any() }
-
-  /**
-   * Holds if this barrier guard only blocks specific labels, and `label` is one of them.
-   *
-   * Subclasses that override this predicate should also override `blocksAllLabels`.
+   * Get a flow label blocked by this guard node.
    */
-  predicate blocksSpecificLabel(FlowLabel label) { none() }
+  abstract FlowLabel getALabel();
 }
 
 /**
@@ -597,7 +592,7 @@ private predicate flowThroughCall(DataFlow::Node input, DataFlow::Node invk,
     calls(invk, f) and // Do not consider partial calls
     reachableFromInput(f, invk, input, ret, cfg, summary) and
     not cfg.isBarrier(ret, invk) and
-    not cfg.isBarrierForLabel(invk, summary.getEndLabel())
+    not cfg.isLabeledBarrier(invk, summary.getEndLabel())
   )
 }
 
@@ -669,7 +664,7 @@ private predicate flowStep(DataFlow::Node pred, DataFlow::Configuration cfg,
   ) and
   not cfg.isBarrier(succ) and
   not cfg.isBarrier(pred, succ) and
-  not cfg.isBarrierForLabel(succ, summary.getEndLabel())
+  not cfg.isLabeledBarrier(succ, summary.getEndLabel())
 }
 
 /**
@@ -694,7 +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
+    not cfg.isLabeledBarrier(nd, lbl) and
     summary = MkPathSummary(false, false, lbl, lbl)
   )
   or
@@ -714,7 +709,7 @@ private predicate onPath(DataFlow::Node nd, DataFlow::Configuration cfg,
   reachableFromSource(nd, cfg, summary) and
   isSink(nd, cfg, summary.getEndLabel()) and
   not cfg.isBarrier(nd) and
-  not cfg.isBarrierForLabel(nd, summary.getEndLabel())
+  not cfg.isLabeledBarrier(nd, summary.getEndLabel())
   or
   exists (DataFlow::Node mid, PathSummary stepSummary |
     reachableFromSource(nd, cfg, summary) and

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

@@ -159,6 +159,12 @@ module TaintTracking {
 
   }
 
+  /**
+   * A sanitizer guard node that only blocks specific flow labels.
+   */
+  abstract class LabeledSanitizerGuardNode extends SanitizerGuardNode, DataFlow::LabeledBarrierGuardNode {
+  }
+
   /**
    * DEPRECATED: Override `Configuration::isAdditionalTaintStep` or use
    * `AdditionalTaintStep` instead.

javascript/ql/src/semmle/javascript/frameworks/Express.qll

@@ -489,18 +489,18 @@ module Express {
       result = kind
     }
 
-    override predicate isDeepObject() {
+    override predicate isUserControlledObject() {
       kind = "body" and
       exists (ExpressLibraries::BodyParser bodyParser, RouteHandlerExpr expr |
         expr.getBody() = rh and
-        bodyParser.isDeepObject() and
+        bodyParser.producesUserControlledObjects() and
         bodyParser.flowsToExpr(expr.getAMatchingAncestor())
       )
       or
       // If we can't find the middlewares for the route handler,
       // but all known body parsers are deep, assume req.body is a deep object.
       kind = "body" and
-      forall(ExpressLibraries::BodyParser bodyParser | bodyParser.isDeepObject())
+      forall(ExpressLibraries::BodyParser bodyParser | bodyParser.producesUserControlledObjects())
       or
       kind = "parameter" and
       exists (DataFlow::Node request | request = DataFlow::valueNode(rh.getARequestExpr()) |

javascript/ql/src/semmle/javascript/frameworks/ExpressModules.qll

@@ -271,9 +271,10 @@ module ExpressLibraries {
     }
 
     /**
-     * Holds if this parses the input as JSON or extended URL-encoding.
+     * Holds if this parses the input as JSON or extended URL-encoding, resulting
+     * in user-controlled objects (as opposed to user-controlled strings).
      */
-    predicate isDeepObject() {
+    predicate producesUserControlledObjects() {
       isJson() or isExtendedUrlEncoded()
     }
   }

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

@@ -24,7 +24,7 @@ module TaintedObject {
   /**
    * Gets the flow label representing a deeply tainted object.
    *
-   * A "tainted object" is an array or object whose properties values are all assumed to be tainted as well.
+   * A "tainted object" is an array or object whose property values are all assumed to be tainted as well.
    *
    * Note that the presence of the this label generally implies the presence of the `taint` label as well.
    */
@@ -70,25 +70,23 @@ module TaintedObject {
   }
 
   /**
-   * A source of a user-controlled deep object object.
+   * A source of a user-controlled deep object.
    */
   abstract class Source extends DataFlow::Node {}
 
   /** Request input accesses as a JSON source. */
   private class RequestInputAsSource extends Source {
     RequestInputAsSource() {
-      this.(HTTP::RequestInputAccess).isDeepObject()
+      this.(HTTP::RequestInputAccess).isUserControlledObject()
     }
   }
 
   /**
    * Sanitizer guard that blocks deep object taint.
    */
-  abstract class SanitizerGuard extends TaintTracking::SanitizerGuardNode {
-    override predicate blocksAllLabels() { none() }
-
-    override predicate blocksSpecificLabel(FlowLabel label) {
-      label = label()
+  abstract class SanitizerGuard extends TaintTracking::LabeledSanitizerGuardNode {
+    override FlowLabel getALabel() {
+      result = label()
     }
   }
 

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

@@ -12,9 +12,9 @@ abstract class RemoteFlowSource extends DataFlow::Node {
   abstract string getSourceType();
 
   /**
-   * Holds if this can be a user-controlled deep object, such as a JSON object parsed from user-controlled data.
+   * Holds if this can be a user-controlled object, such as a JSON object parsed from user-controlled data.
    */
-  predicate isDeepObject() { none() }
+  predicate isUserControlledObject() { none() }
 }
 
 /**