Merge pull request github#13719 from asgerf/js/barrier-inout

JS: Replace barrier edges with barrier nodes

Author: asgerf

javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/XssATM.qll

@@ -23,18 +23,15 @@ class DomBasedXssAtmConfig extends AtmConfig {
 
   override predicate isSanitizer(DataFlow::Node node) {
     super.isSanitizer(node) or
-    node instanceof DomBasedXss::Sanitizer
+    node instanceof DomBasedXss::Sanitizer or
+    DomBasedXss::isOptionallySanitizedNode(node)
   }
 
   override predicate isSanitizerGuard(TaintTracking::SanitizerGuardNode guard) {
     guard instanceof PrefixStringSanitizerActivated or
     guard instanceof QuoteGuard or
     guard instanceof ContainsHtmlGuard
   }
-
-  override predicate isSanitizerEdge(DataFlow::Node pred, DataFlow::Node succ) {
-    DomBasedXss::isOptionallySanitizedEdge(pred, succ)
-  }
 }
 
 private import semmle.javascript.security.dataflow.Xss::Shared as Shared

javascript/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/XssThroughDomATM.qll

@@ -23,7 +23,8 @@ class XssThroughDomAtmConfig extends AtmConfig {
 
   override predicate isSanitizer(DataFlow::Node node) {
     super.isSanitizer(node) or
-    node instanceof DomBasedXss::Sanitizer
+    node instanceof DomBasedXss::Sanitizer or
+    DomBasedXss::isOptionallySanitizedNode(node)
   }
 
   override predicate isSanitizerGuard(TaintTracking::SanitizerGuardNode guard) {
@@ -34,10 +35,6 @@ class XssThroughDomAtmConfig extends AtmConfig {
     guard instanceof QuoteGuard or
     guard instanceof ContainsHtmlGuard
   }
-
-  override predicate isSanitizerEdge(DataFlow::Node pred, DataFlow::Node succ) {
-    DomBasedXss::isOptionallySanitizedEdge(pred, succ)
-  }
 }
 
 /**

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

@@ -166,6 +166,26 @@ abstract class Configuration extends string {
     )
   }
 
+  /**
+   * Holds if flow into `node` is prohibited.
+   */
+  predicate isBarrierIn(DataFlow::Node node) { none() }
+
+  /**
+   * Holds if flow out `node` is prohibited.
+   */
+  predicate isBarrierOut(DataFlow::Node node) { none() }
+
+  /**
+   * Holds if flow into `node` is prohibited for the flow label `lbl`.
+   */
+  predicate isBarrierIn(DataFlow::Node node, FlowLabel lbl) { none() }
+
+  /**
+   * Holds if flow out `node` is prohibited for the flow label `lbl`.
+   */
+  predicate isBarrierOut(DataFlow::Node node, FlowLabel lbl) { none() }
+
   /**
    * Holds if flow from `pred` to `succ` is prohibited.
    */
@@ -494,7 +514,7 @@ private BasicBlock getADominatedBasicBlock(BarrierGuardNode guard, ConditionGuar
  *
  * Only holds for barriers that should apply to all flow labels.
  */
-private predicate isBarrierEdge(Configuration cfg, DataFlow::Node pred, DataFlow::Node succ) {
+private predicate isBarrierEdgeRaw(Configuration cfg, DataFlow::Node pred, DataFlow::Node succ) {
   cfg.isBarrierEdge(pred, succ)
   or
   exists(DataFlow::BarrierGuardNode guard |
@@ -503,11 +523,26 @@ private predicate isBarrierEdge(Configuration cfg, DataFlow::Node pred, DataFlow
   )
 }
 
+/**
+ * Holds if there is a barrier edge `pred -> succ` in `cfg` either through an explicit barrier edge
+ * or one implied by a barrier guard, or by an out/in barrier for `pred` or `succ`, respectively.
+ *
+ * Only holds for barriers that should apply to all flow labels.
+ */
+pragma[inline]
+private predicate isBarrierEdge(Configuration cfg, DataFlow::Node pred, DataFlow::Node succ) {
+  isBarrierEdgeRaw(cfg, pred, succ)
+  or
+  cfg.isBarrierOut(pred)
+  or
+  cfg.isBarrierIn(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(
+private predicate isLabeledBarrierEdgeRaw(
   Configuration cfg, DataFlow::Node pred, DataFlow::Node succ, DataFlow::FlowLabel label
 ) {
   cfg.isBarrierEdge(pred, succ, label)
@@ -518,6 +553,21 @@ private predicate isLabeledBarrierEdge(
   )
 }
 
+/**
+ * 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, or by an out/in barrier for `pred` or `succ`, respectively.
+ */
+pragma[inline]
+private predicate isLabeledBarrierEdge(
+  Configuration cfg, DataFlow::Node pred, DataFlow::Node succ, DataFlow::FlowLabel label
+) {
+  isLabeledBarrierEdgeRaw(cfg, pred, succ, label)
+  or
+  cfg.isBarrierOut(pred, label)
+  or
+  cfg.isBarrierIn(succ, label)
+}
+
 /**
  * A guard node that only blocks specific labels.
  */

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

@@ -62,6 +62,26 @@ module TaintTracking {
      */
     predicate isSanitizer(DataFlow::Node node) { none() }
 
+    /**
+     * Holds if flow into `node` is prohibited.
+     */
+    predicate isSanitizerIn(DataFlow::Node node) { none() }
+
+    /**
+     * Holds if flow out `node` is prohibited.
+     */
+    predicate isSanitizerOut(DataFlow::Node node) { none() }
+
+    /**
+     * Holds if flow into `node` is prohibited for the flow label `lbl`.
+     */
+    predicate isSanitizerIn(DataFlow::Node node, DataFlow::FlowLabel lbl) { none() }
+
+    /**
+     * Holds if flow out `node` is prohibited for the flow label `lbl`.
+     */
+    predicate isSanitizerOut(DataFlow::Node node, DataFlow::FlowLabel lbl) { none() }
+
     /** Holds if the edge from `pred` to `succ` is a taint sanitizer. */
     predicate isSanitizerEdge(DataFlow::Node pred, DataFlow::Node succ) { none() }
 
@@ -108,6 +128,22 @@ module TaintTracking {
       this.isSanitizerEdge(source, sink) and lbl.isTaint()
     }
 
+    final override predicate isBarrierIn(DataFlow::Node node) { none() }
+
+    final override predicate isBarrierOut(DataFlow::Node node) { none() }
+
+    final override predicate isBarrierIn(DataFlow::Node node, DataFlow::FlowLabel lbl) {
+      this.isSanitizerIn(node, lbl)
+      or
+      this.isSanitizerIn(node) and lbl.isTaint()
+    }
+
+    final override predicate isBarrierOut(DataFlow::Node node, DataFlow::FlowLabel lbl) {
+      this.isSanitizerOut(node, lbl)
+      or
+      this.isSanitizerOut(node) and lbl.isTaint()
+    }
+
     final override predicate isBarrierGuard(DataFlow::BarrierGuardNode guard) {
       super.isBarrierGuard(guard) or
       guard.(AdditionalSanitizerGuardNode).appliesTo(this) or

javascript/ql/lib/semmle/javascript/security/dataflow/BuildArtifactLeakQuery.qll

@@ -27,10 +27,6 @@ class Configuration extends TaintTracking::Configuration {
 
   override predicate isSanitizer(DataFlow::Node node) { node instanceof CleartextLogging::Barrier }
 
-  override predicate isSanitizerEdge(DataFlow::Node pred, DataFlow::Node succ) {
-    CleartextLogging::isSanitizerEdge(pred, succ)
-  }
-
   override predicate isAdditionalTaintStep(DataFlow::Node src, DataFlow::Node trg) {
     CleartextLogging::isAdditionalTaintStep(src, trg)
   }

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

@@ -175,12 +175,24 @@ module CleartextLogging {
   }
 
   /**
+   * DEPRECATED. Use `Barrier` instead, sanitized have been replaced by sanitized nodes.
+   *
    * Holds if the edge `pred` -> `succ` should be sanitized for clear-text logging of sensitive information.
    */
-  predicate isSanitizerEdge(DataFlow::Node pred, DataFlow::Node succ) {
+  deprecated predicate isSanitizerEdge(DataFlow::Node pred, DataFlow::Node succ) {
     succ.(DataFlow::PropRead).getBase() = pred
   }
 
+  private class PropReadAsBarrier extends Barrier {
+    PropReadAsBarrier() {
+      this = any(DataFlow::PropRead read).getBase() and
+      // the 'foo' in 'foo.bar()' may have flow, we only want to suppress plain property reads
+      not this = any(DataFlow::MethodCallNode call).getReceiver() and
+      // do not block custom taint steps from this node
+      not isAdditionalTaintStep(this, _)
+    }
+  }
+
   /**
    * Holds if the edge `src` -> `trg` is an additional taint-step for clear-text logging of sensitive information.
    */

javascript/ql/lib/semmle/javascript/security/dataflow/CleartextLoggingQuery.qll

@@ -33,10 +33,6 @@ class Configuration extends TaintTracking::Configuration {
 
   override predicate isSanitizer(DataFlow::Node node) { node instanceof Barrier }
 
-  override predicate isSanitizerEdge(DataFlow::Node pred, DataFlow::Node succ) {
-    CleartextLogging::isSanitizerEdge(pred, succ)
-  }
-
   override predicate isAdditionalTaintStep(DataFlow::Node src, DataFlow::Node trg) {
     CleartextLogging::isAdditionalTaintStep(src, trg)
   }

javascript/ql/lib/semmle/javascript/security/dataflow/ClientSideRequestForgeryQuery.qll

@@ -31,9 +31,7 @@ class Configuration extends TaintTracking::Configuration {
     node instanceof Sanitizer
   }
 
-  override predicate isSanitizerEdge(DataFlow::Node source, DataFlow::Node sink) {
-    sanitizingPrefixEdge(source, sink)
-  }
+  override predicate isSanitizerOut(DataFlow::Node node) { sanitizingPrefixEdge(node, _) }
 
   override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
     isAdditionalRequestForgeryStep(pred, succ)

javascript/ql/lib/semmle/javascript/security/dataflow/ClientSideUrlRedirectQuery.qll

@@ -33,9 +33,7 @@ class Configuration extends TaintTracking::Configuration {
     node instanceof Sanitizer
   }
 
-  override predicate isSanitizerEdge(DataFlow::Node source, DataFlow::Node sink) {
-    hostnameSanitizingPrefixEdge(source, sink)
-  }
+  override predicate isSanitizerOut(DataFlow::Node node) { hostnameSanitizingPrefixEdge(node, _) }
 
   override predicate isAdditionalFlowStep(
     DataFlow::Node pred, DataFlow::Node succ, DataFlow::FlowLabel f, DataFlow::FlowLabel g

javascript/ql/lib/semmle/javascript/security/dataflow/DomBasedXssCustomizations.qll

@@ -290,9 +290,13 @@ module DomBasedXss {
   private class HtmlSanitizerAsSanitizer extends Sanitizer instanceof HtmlSanitizerCall { }
 
   /**
+   * DEPRECATED. Use `isOptionallySanitizedNode` instead.
+   *
    * Holds if there exists two dataflow edges to `succ`, where one edges is sanitized, and the other edge starts with `pred`.
    */
-  predicate isOptionallySanitizedEdge(DataFlow::Node pred, DataFlow::Node succ) {
+  deprecated predicate isOptionallySanitizedEdge = isOptionallySanitizedEdgeInternal/2;
+
+  private predicate isOptionallySanitizedEdgeInternal(DataFlow::Node pred, DataFlow::Node succ) {
     exists(HtmlSanitizerCall sanitizer |
       // sanitized = sanitize ? sanitizer(source) : source;
       exists(ConditionalExpr branch, Variable var, VarAccess access |
@@ -319,6 +323,17 @@ module DomBasedXss {
     )
   }
 
+  /**
+   * Holds if `node` should be considered optionally sanitized as it occurs in a branch
+   * that controls whether sanitization is enabled.
+   *
+   * For example, in `sanitized = sanitize ? sanitizer(source) : source`, the right-hand `source` expression
+   * is considered an optionally sanitized node.
+   */
+  predicate isOptionallySanitizedNode(DataFlow::Node node) {
+    isOptionallySanitizedEdgeInternal(_, node)
+  }
+
   /** A source of remote user input, considered as a flow source for DOM-based XSS. */
   class RemoteFlowSourceAsSource extends Source instanceof RemoteFlowSource { }