JS: Port PrototypePollutingAssignment

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

@@ -38,6 +38,30 @@ module PrototypePollutingAssignment {
    */
   abstract class Sanitizer extends DataFlow::Node { }
 
+  /**
+   * A barrier guard for prototype-polluting assignments.
+   */
+  abstract class BarrierGuard extends DataFlow::Node {
+    /**
+     * Holds if this node acts as a barrier for data flow, blocking further flow from `e` if `this` evaluates to `outcome`.
+     */
+    predicate blocksExpr(boolean outcome, Expr e) { none() }
+
+    /**
+     * Holds if this node acts as a barrier for `label`, blocking further flow from `e` if `this` evaluates to `outcome`.
+     */
+    predicate blocksExpr(boolean outcome, Expr e, DataFlow::FlowLabel label) { none() }
+  }
+
+  /** A subclass of `BarrierGuard` that is used for backward compatibility with the old data flow library. */
+  abstract class BarrierGuardLegacy extends BarrierGuard, TaintTracking::SanitizerGuardNode {
+    override predicate sanitizes(boolean outcome, Expr e) { this.blocksExpr(outcome, e) }
+
+    override predicate sanitizes(boolean outcome, Expr e, DataFlow::FlowLabel label) {
+      this.blocksExpr(outcome, e, label)
+    }
+  }
+
   /** A flow label representing the `Object.prototype` value. */
   abstract class ObjectPrototype extends DataFlow::FlowLabel {
     ObjectPrototype() { this = "Object.prototype" }
@@ -46,7 +70,9 @@ module PrototypePollutingAssignment {
   /** The base of an assignment or extend call, as a sink for `Object.prototype` references. */
   private class DefaultSink extends Sink {
     DefaultSink() {
-      this = any(DataFlow::PropWrite write).getBase()
+      // Avoid using PropWrite here as we only want assignments that can mutate a pre-existing object,
+      // so not object literals or array literals.
+      this = any(AssignExpr assign).getTarget().(PropAccess).getBase().flow()
       or
       this = any(ExtendCall c).getDestinationOperand()
       or
@@ -67,7 +93,9 @@ module PrototypePollutingAssignment {
    * A parameter of an exported function, seen as a source prototype-polluting assignment.
    */
   class ExternalInputSource extends Source {
-    ExternalInputSource() { this = Exports::getALibraryInputParameter() }
+    ExternalInputSource() {
+      this = Exports::getALibraryInputParameter() and not this instanceof RemoteFlowSource
+    }
 
     override string describe() { result = "library input" }
   }

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

@@ -19,16 +19,18 @@ private class ConcreteObjectPrototype extends ObjectPrototype {
 }
 
 /** A taint-tracking configuration for reasoning about prototype-polluting assignments. */
-class Configuration extends TaintTracking::Configuration {
-  Configuration() { this = "PrototypePollutingAssignment" }
+module PrototypePollutingAssignmentConfig implements DataFlow::StateConfigSig {
+  class FlowState = DataFlow::FlowLabel;
 
-  override predicate isSource(DataFlow::Node node) { node instanceof Source }
+  predicate isSource(DataFlow::Node node, DataFlow::FlowLabel label) {
+    node instanceof Source and label.isTaint()
+  }
 
-  override predicate isSink(DataFlow::Node node, DataFlow::FlowLabel lbl) {
+  predicate isSink(DataFlow::Node node, DataFlow::FlowLabel lbl) {
     node.(Sink).getAFlowLabel() = lbl
   }
 
-  override predicate isSanitizer(DataFlow::Node node) {
+  predicate isBarrier(DataFlow::Node node) {
     node instanceof Sanitizer
     or
     // Concatenating with a string will in practice prevent the string `__proto__` from arising.
@@ -53,17 +55,24 @@ class Configuration extends TaintTracking::Configuration {
         not replace.getRawReplacement().getStringValue() = ""
       )
     )
+    or
+    node = DataFlow::MakeBarrierGuard<BarrierGuard>::getABarrierNode()
   }
 
-  override predicate isSanitizerOut(DataFlow::Node node, DataFlow::FlowLabel lbl) {
+  predicate isBarrierOut(DataFlow::Node node, DataFlow::FlowLabel lbl) {
     // Suppress the value-preserving step src -> dst in `extend(dst, src)`. This is modeled as a value-preserving
     // step because it preserves all properties, but the destination is not actually Object.prototype.
     node = any(ExtendCall call).getASourceOperand() and
     lbl instanceof ObjectPrototype
   }
 
-  override predicate isAdditionalFlowStep(
-    DataFlow::Node pred, DataFlow::Node succ, DataFlow::FlowLabel inlbl, DataFlow::FlowLabel outlbl
+  predicate isBarrierIn(DataFlow::Node node, DataFlow::FlowLabel lbl) {
+    // FIXME: This should only be an in-barrier for the corresponding flow state, but flow-state specific in-barriers are not supported right now.
+    isSource(node, lbl)
+  }
+
+  predicate isAdditionalFlowStep(
+    DataFlow::Node pred, DataFlow::FlowLabel inlbl, DataFlow::Node succ, DataFlow::FlowLabel outlbl
   ) {
     // Step from x -> obj[x] while switching to the ObjectPrototype label
     // (If `x` can have the value `__proto__` then the result can be Object.prototype)
@@ -91,7 +100,80 @@ class Configuration extends TaintTracking::Configuration {
       outlbl instanceof ObjectPrototype
     )
     or
-    DataFlow::localFieldStep(pred, succ) and inlbl = outlbl
+    // TODO: local field step becomes a jump step, resulting in FPs (closure-lib)
+    // TODO: localFieldStep is too expensive with dataflow2
+    // DataFlow::localFieldStep(pred, succ)
+    none()
+    or
+    inlbl.isTaint() and
+    TaintTracking::defaultTaintStep(pred, succ) and
+    inlbl = outlbl
+  }
+
+  DataFlow::FlowFeature getAFeature() { result instanceof DataFlow::FeatureHasSourceCallContext }
+
+  predicate isBarrier(DataFlow::Node node, DataFlow::FlowLabel lbl) {
+    lbl.isTaint() and
+    TaintTracking::defaultSanitizer(node)
+    or
+    // Don't propagate into the receiver, as the method lookups will generally fail on Object.prototype.
+    node instanceof DataFlow::ThisNode and
+    lbl instanceof ObjectPrototype
+    or
+    node = DataFlow::MakeLabeledBarrierGuard<BarrierGuard>::getABarrierNode(lbl)
+  }
+}
+
+/** Taint-tracking for reasoning about prototype-polluting assignments. */
+module PrototypePollutingAssignmentFlow =
+  DataFlow::GlobalWithState<PrototypePollutingAssignmentConfig>;
+
+/**
+ * Holds if the given `source, sink` pair should not be reported, as we don't have enough
+ * confidence in the alert given that source is a library input.
+ */
+bindingset[source, sink]
+predicate isIgnoredLibraryFlow(ExternalInputSource source, Sink sink) {
+  exists(source) and
+  // filter away paths that start with library inputs and end with a write to a fixed property.
+  exists(DataFlow::PropWrite write | sink = write.getBase() |
+    // fixed property name
+    exists(write.getPropertyName())
+    or
+    // non-string property name (likely number)
+    exists(Expr prop | prop = write.getPropertyNameExpr() |
+      not prop.analyze().getAType() = TTString()
+    )
+  )
+}
+
+/**
+ * DEPRECATED. Use the `PrototypePollutingAssignmentFlow` module instead.
+ */
+deprecated class Configuration extends TaintTracking::Configuration {
+  Configuration() { this = "PrototypePollutingAssignment" }
+
+  override predicate isSource(DataFlow::Node node) { node instanceof Source }
+
+  override predicate isSink(DataFlow::Node node, DataFlow::FlowLabel lbl) {
+    node.(Sink).getAFlowLabel() = lbl
+  }
+
+  override predicate isSanitizer(DataFlow::Node node) {
+    PrototypePollutingAssignmentConfig::isBarrier(node)
+  }
+
+  override predicate isSanitizerOut(DataFlow::Node node, DataFlow::FlowLabel lbl) {
+    // Suppress the value-preserving step src -> dst in `extend(dst, src)`. This is modeled as a value-preserving
+    // step because it preserves all properties, but the destination is not actually Object.prototype.
+    node = any(ExtendCall call).getASourceOperand() and
+    lbl instanceof ObjectPrototype
+  }
+
+  override predicate isAdditionalFlowStep(
+    DataFlow::Node pred, DataFlow::Node succ, DataFlow::FlowLabel inlbl, DataFlow::FlowLabel outlbl
+  ) {
+    PrototypePollutingAssignmentConfig::isAdditionalFlowStep(pred, inlbl, succ, outlbl)
   }
 
   override predicate hasFlowPath(DataFlow::SourcePathNode source, DataFlow::SinkPathNode sink) {
@@ -174,9 +256,7 @@ private predicate isPropertyPresentOnObjectPrototype(string prop) {
 }
 
 /** A check of form `e.prop` where `prop` is not present on `Object.prototype`. */
-private class PropertyPresenceCheck extends TaintTracking::LabeledSanitizerGuardNode,
-  DataFlow::ValueNode
-{
+private class PropertyPresenceCheck extends BarrierGuardLegacy, DataFlow::ValueNode {
   override PropAccess astNode;
 
   PropertyPresenceCheck() {
@@ -184,7 +264,7 @@ private class PropertyPresenceCheck extends TaintTracking::LabeledSanitizerGuard
     not isPropertyPresentOnObjectPrototype(astNode.getPropertyName())
   }
 
-  override predicate sanitizes(boolean outcome, Expr e, DataFlow::FlowLabel label) {
+  override predicate blocksExpr(boolean outcome, Expr e, DataFlow::FlowLabel label) {
     e = astNode.getBase() and
     outcome = true and
     label instanceof ObjectPrototype
@@ -192,14 +272,14 @@ private class PropertyPresenceCheck extends TaintTracking::LabeledSanitizerGuard
 }
 
 /** A check of form `"prop" in e` where `prop` is not present on `Object.prototype`. */
-private class InExprCheck extends TaintTracking::LabeledSanitizerGuardNode, DataFlow::ValueNode {
+private class InExprCheck extends BarrierGuardLegacy, DataFlow::ValueNode {
   override InExpr astNode;
 
   InExprCheck() {
     not isPropertyPresentOnObjectPrototype(astNode.getLeftOperand().getStringValue())
   }
 
-  override predicate sanitizes(boolean outcome, Expr e, DataFlow::FlowLabel label) {
+  override predicate blocksExpr(boolean outcome, Expr e, DataFlow::FlowLabel label) {
     e = astNode.getRightOperand() and
     outcome = true and
     label instanceof ObjectPrototype
@@ -207,10 +287,10 @@ private class InExprCheck extends TaintTracking::LabeledSanitizerGuardNode, Data
 }
 
 /** A check of form `e instanceof X`, which is always false for `Object.prototype`. */
-private class InstanceofCheck extends TaintTracking::LabeledSanitizerGuardNode, DataFlow::ValueNode {
+private class InstanceofCheck extends BarrierGuardLegacy, DataFlow::ValueNode {
   override InstanceofExpr astNode;
 
-  override predicate sanitizes(boolean outcome, Expr e, DataFlow::FlowLabel label) {
+  override predicate blocksExpr(boolean outcome, Expr e, DataFlow::FlowLabel label) {
     e = astNode.getLeftOperand() and
     outcome = true and
     label instanceof ObjectPrototype
@@ -218,7 +298,7 @@ private class InstanceofCheck extends TaintTracking::LabeledSanitizerGuardNode,
 }
 
 /** A check of form `typeof e === "string"`. */
-private class TypeofCheck extends TaintTracking::LabeledSanitizerGuardNode, DataFlow::ValueNode {
+private class TypeofCheck extends BarrierGuardLegacy, DataFlow::ValueNode {
   override EqualityTest astNode;
   Expr operand;
   boolean polarity;
@@ -231,7 +311,7 @@ private class TypeofCheck extends TaintTracking::LabeledSanitizerGuardNode, Data
     )
   }
 
-  override predicate sanitizes(boolean outcome, Expr e, DataFlow::FlowLabel label) {
+  override predicate blocksExpr(boolean outcome, Expr e, DataFlow::FlowLabel label) {
     polarity = outcome and
     e = operand and
     label instanceof ObjectPrototype
@@ -239,20 +319,20 @@ private class TypeofCheck extends TaintTracking::LabeledSanitizerGuardNode, Data
 }
 
 /** A guard that checks whether `x` is a number. */
-class NumberGuard extends TaintTracking::SanitizerGuardNode instanceof DataFlow::CallNode {
+class NumberGuard extends BarrierGuardLegacy instanceof DataFlow::CallNode {
   Expr x;
   boolean polarity;
 
   NumberGuard() { TaintTracking::isNumberGuard(this, x, polarity) }
 
-  override predicate sanitizes(boolean outcome, Expr e) { e = x and outcome = polarity }
+  override predicate blocksExpr(boolean outcome, Expr e) { e = x and outcome = polarity }
 }
 
 /** A call to `Array.isArray`, which is false for `Object.prototype`. */
-private class IsArrayCheck extends TaintTracking::LabeledSanitizerGuardNode, DataFlow::CallNode {
+private class IsArrayCheck extends BarrierGuardLegacy, DataFlow::CallNode {
   IsArrayCheck() { this = DataFlow::globalVarRef("Array").getAMemberCall("isArray") }
 
-  override predicate sanitizes(boolean outcome, Expr e, DataFlow::FlowLabel label) {
+  override predicate blocksExpr(boolean outcome, Expr e, DataFlow::FlowLabel label) {
     e = this.getArgument(0).asExpr() and
     outcome = true and
     label instanceof ObjectPrototype
@@ -262,12 +342,12 @@ private class IsArrayCheck extends TaintTracking::LabeledSanitizerGuardNode, Dat
 /**
  * Sanitizer guard of form `x !== "__proto__"`.
  */
-private class EqualityCheck extends TaintTracking::SanitizerGuardNode, DataFlow::ValueNode {
+private class EqualityCheck extends BarrierGuardLegacy, DataFlow::ValueNode {
   override EqualityTest astNode;
 
   EqualityCheck() { astNode.getAnOperand().getStringValue() = "__proto__" }
 
-  override predicate sanitizes(boolean outcome, Expr e) {
+  override predicate blocksExpr(boolean outcome, Expr e) {
     e = astNode.getAnOperand() and
     outcome = astNode.getPolarity().booleanNot()
   }
@@ -276,10 +356,10 @@ private class EqualityCheck extends TaintTracking::SanitizerGuardNode, DataFlow:
 /**
  * Sanitizer guard of the form `x.includes("__proto__")`.
  */
-private class IncludesCheck extends TaintTracking::LabeledSanitizerGuardNode, InclusionTest {
+private class IncludesCheck extends BarrierGuardLegacy, InclusionTest {
   IncludesCheck() { this.getContainedNode().mayHaveStringValue("__proto__") }
 
-  override predicate sanitizes(boolean outcome, Expr e) {
+  override predicate blocksExpr(boolean outcome, Expr e) {
     e = this.getContainerNode().asExpr() and
     outcome = this.getPolarity().booleanNot()
   }
@@ -288,7 +368,7 @@ private class IncludesCheck extends TaintTracking::LabeledSanitizerGuardNode, In
 /**
  * A sanitizer guard that checks tests whether `x` is included in a list like `["__proto__"].includes(x)`.
  */
-private class DenyListInclusionGuard extends TaintTracking::SanitizerGuardNode, InclusionTest {
+private class DenyListInclusionGuard extends BarrierGuardLegacy, InclusionTest {
   DenyListInclusionGuard() {
     this.getContainerNode()
         .getALocalSource()
@@ -297,7 +377,7 @@ private class DenyListInclusionGuard extends TaintTracking::SanitizerGuardNode,
         .mayHaveStringValue("__proto__")
   }
 
-  override predicate sanitizes(boolean outcome, Expr e) {
+  override predicate blocksExpr(boolean outcome, Expr e) {
     e = this.getContainedNode().asExpr() and
     outcome = super.getPolarity().booleanNot()
   }