Merge pull request #2919 from asger-semmle/js/property-barriers

JS: Make sanitizers no longer block taint inside an object

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

@@ -98,6 +98,17 @@ abstract class Configuration extends string {
    */
   predicate isSource(DataFlow::Node source) { none() }
 
+  /**
+   * Gets the flow label to associate with sources added by the 1-argument `isSource` predicate.
+   *
+   * For taint-tracking configurations, this defaults to `taint` and for other data-flow configurations
+   * it defaults to `data`.
+   *
+   * Overriding this predicate is rarely needed, and overriding the 2-argument `isSource` predicate
+   * should be preferred when possible.
+   */
+  FlowLabel getDefaultSourceLabel() { result = FlowLabel::data() }
+
   /**
    * Holds if `source` is a source of flow labeled with `lbl` that is relevant
    * for this configuration.
@@ -256,9 +267,11 @@ abstract class Configuration extends string {
 /**
  * A label describing the kind of information tracked by a flow configuration.
  *
- * There are two standard labels "data" and "taint", the former describing values
- * that directly originate from a flow source, the latter values that are derived
- * from a flow source via one or more transformations (such as string operations).
+ * There are two standard labels "data" and "taint".
+ * - "data" only propagates along value-preserving data flow, such as assignments
+ *   and parameter-passing, and is the default flow source for a `DataFlow::Configuration`.
+ * - "taint" additionally permits flow through transformations such as string operations,
+ *   and is the default flow source for a `TaintTracking::Configuration`.
  */
 abstract class FlowLabel extends string {
   bindingset[this]
@@ -666,7 +679,7 @@ private predicate exploratoryFlowStep(
  */
 private predicate isSource(DataFlow::Node nd, DataFlow::Configuration cfg, FlowLabel lbl) {
   (cfg.isSource(nd) or nd.(AdditionalSource).isSourceFor(cfg)) and
-  lbl = FlowLabel::data()
+  lbl = cfg.getDefaultSourceLabel()
   or
   nd.(AdditionalSource).isSourceFor(cfg, lbl)
   or

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

@@ -48,7 +48,16 @@ module TaintTracking {
     // overridden to provide taint-tracking specific qldoc
     override predicate isSink(DataFlow::Node sink) { super.isSink(sink) }
 
-    /** Holds if the intermediate node `node` is a taint sanitizer. */
+    /**
+     * Holds if the intermediate node `node` is a taint sanitizer, that is,
+     * tainted values can not flow into or out of `node`.
+     *
+     * Note that this only blocks flow through nodes that operate directly on the tainted value.
+     * An object _containing_ a tainted value in a property can still flow into and out of `node`.
+     * To block such objects, override `isBarrier` or use a labeled sanitizer to block the `data` flow label.
+     *
+     * For operations that _check_ if a value is tainted or safe, use `isSanitizerGuard` instead.
+     */
     predicate isSanitizer(DataFlow::Node node) { none() }
 
     /**
@@ -84,25 +93,35 @@ module TaintTracking {
      * For example, if `guard` is the comparison expression in
      * `if(x == 'some-constant'){ ... x ... }`, it could sanitize flow of
      * `x` into the "then" branch.
+     *
+     * Node that this only handles checks that operate directly on the tainted value.
+     * Objects that _contain_ a tainted value in a property may still flow across the check.
+     * To block such objects, use a labeled sanitizer guard to block the `data` label.
      */
     predicate isSanitizerGuard(SanitizerGuardNode guard) { none() }
 
-    final override predicate isBarrier(DataFlow::Node node) {
-      super.isBarrier(node) or
-      isSanitizer(node) or
-      node instanceof DataFlow::VarAccessBarrier
+    override predicate isLabeledBarrier(DataFlow::Node node, DataFlow::FlowLabel lbl) {
+      super.isLabeledBarrier(node, lbl)
+      or
+      isSanitizer(node) and lbl.isTaint()
     }
 
-    final override predicate isBarrierEdge(DataFlow::Node source, DataFlow::Node sink) {
-      super.isBarrierEdge(source, sink) or
-      isSanitizerEdge(source, sink)
+    override predicate isBarrier(DataFlow::Node node) {
+      super.isBarrier(node)
+      or
+      // For variable accesses we block both the data and taint label, as a falsy value
+      // can't be an object, and thus can't have any tainted properties.
+      node instanceof DataFlow::VarAccessBarrier
     }
 
     final override predicate isBarrierEdge(
       DataFlow::Node source, DataFlow::Node sink, DataFlow::FlowLabel lbl
     ) {
-      super.isBarrierEdge(source, sink, lbl) or
+      super.isBarrierEdge(source, sink, lbl)
+      or
       isSanitizerEdge(source, sink, lbl)
+      or
+      isSanitizerEdge(source, sink) and lbl.isTaint()
     }
 
     final override predicate isBarrierGuard(DataFlow::BarrierGuardNode guard) {
@@ -127,6 +146,8 @@ module TaintTracking {
     ) {
       isAdditionalFlowStep(pred, succ) and valuePreserving = false
     }
+
+    override DataFlow::FlowLabel getDefaultSourceLabel() { result.isTaint() }
   }
 
   /**
@@ -157,7 +178,7 @@ module TaintTracking {
    * them.
    */
   abstract class SanitizerGuardNode extends DataFlow::BarrierGuardNode {
-    override predicate blocks(boolean outcome, Expr e) { sanitizes(outcome, e) }
+    override predicate blocks(boolean outcome, Expr e) { none() }
 
     /**
      * Holds if this node sanitizes expression `e`, provided it evaluates
@@ -166,6 +187,8 @@ module TaintTracking {
     abstract predicate sanitizes(boolean outcome, Expr e);
 
     override predicate blocks(boolean outcome, Expr e, DataFlow::FlowLabel label) {
+      sanitizes(outcome, e) and label.isTaint()
+      or
       sanitizes(outcome, e, label)
     }
 
@@ -180,10 +203,6 @@ module TaintTracking {
    * A sanitizer guard node that only blocks specific flow labels.
    */
   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() }
   }
 

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

@@ -23,7 +23,7 @@ module CleartextLogging {
    * A data flow sink for clear-text logging of sensitive information.
    */
   abstract class Sink extends DataFlow::Node {
-    DataFlow::FlowLabel getLabel() { result.isDataOrTaint() }
+    DataFlow::FlowLabel getLabel() { result.isTaint() }
   }
 
   /**
@@ -127,7 +127,7 @@ module CleartextLogging {
 
     override string describe() { result = "an access to " + name }
 
-    override DataFlow::FlowLabel getLabel() { result.isData() }
+    override DataFlow::FlowLabel getLabel() { result.isTaint() }
   }
 
   /** An access to a variable or property that might contain a password. */
@@ -153,7 +153,7 @@ module CleartextLogging {
 
     override string describe() { result = "an access to " + name }
 
-    override DataFlow::FlowLabel getLabel() { result.isData() }
+    override DataFlow::FlowLabel getLabel() { result.isTaint() }
   }
 
   /** A call that might return a password. */
@@ -167,7 +167,7 @@ module CleartextLogging {
 
     override string describe() { result = "a call to " + name }
 
-    override DataFlow::FlowLabel getLabel() { result.isData() }
+    override DataFlow::FlowLabel getLabel() { result.isTaint() }
   }
 
   /** An access to the sensitive object `process.env`. */
@@ -177,7 +177,7 @@ module CleartextLogging {
     override string describe() { result = "process environment" }
 
     override DataFlow::FlowLabel getLabel() {
-      result.isData() or
+      result.isTaint() or
       result instanceof PartiallySensitiveMap
     }
   }

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

@@ -74,7 +74,7 @@ module PrototypePollution {
   private class RemoteFlowAsSource extends Source {
     RemoteFlowAsSource() { this instanceof RemoteFlowSource }
 
-    override DataFlow::FlowLabel getAFlowLabel() { result.isData() }
+    override DataFlow::FlowLabel getAFlowLabel() { result.isTaint() }
   }
 
   /**

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

@@ -53,7 +53,7 @@ module UnsafeDynamicMethodAccess {
         hasUnsafeMethods(read.getBase().getALocalSource()) and
         src = read.getPropertyNameExpr().flow() and
         dst = read and
-        (srclabel = data() or srclabel = taint()) and
+        srclabel.isTaint() and
         dstlabel = unsafeFunction()
       )
       or
@@ -62,7 +62,7 @@ module UnsafeDynamicMethodAccess {
         not PropertyInjection::isPrototypeLessObject(proj.getObject().getALocalSource()) and
         src = proj.getASelector() and
         dst = proj and
-        (srclabel = data() or srclabel = taint()) and
+        srclabel.isTaint() and
         dstlabel = unsafeFunction()
       )
     }

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

@@ -19,7 +19,7 @@ module UnsafeDynamicMethodAccess {
     /**
      * Gets the flow label relevant for this source.
      */
-    DataFlow::FlowLabel getFlowLabel() { result = data() }
+    DataFlow::FlowLabel getFlowLabel() { result = taint() }
   }
 
   /**

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

@@ -40,7 +40,7 @@ module UnvalidatedDynamicMethodCall {
       exists(DataFlow::PropRead read |
         src = read.getPropertyNameExpr().flow() and
         dst = read and
-        (srclabel = data() or srclabel = taint()) and
+        srclabel.isTaint() and
         (
           dstlabel instanceof MaybeNonFunction
           or

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

@@ -19,7 +19,7 @@ module UnvalidatedDynamicMethodCall {
     /**
      * Gets the flow label relevant for this source.
      */
-    DataFlow::FlowLabel getFlowLabel() { result = data() }
+    DataFlow::FlowLabel getFlowLabel() { result = taint() }
   }
 
   /**