JS: Port PrototypePollutingFunction

2025-12-24 04:36:35 +01:00 · 2023-10-04 21:37:13 +02:00
parent f1f45927b1
commit adf7d5409d
3 changed files with 874 additions and 3019 deletions
--- a/javascript/ql/src/Security/CWE-915/PrototypePollutingFunction.ql
+++ b/javascript/ql/src/Security/CWE-915/PrototypePollutingFunction.ql
@@ -17,11 +17,10 @@
 */

 import javascript
-import DataFlow
-import PathGraph
 import semmle.javascript.DynamicPropertyAccess
 private import semmle.javascript.dataflow.InferredTypes

+// WIN: gained TP in Lucifier/r.js:2757, though not sure why it wasn't flagged to start with.
 /**
 * A call of form `x.split(".")` where `x` is a parameter.
 *
@@ -30,14 +29,14 @@ private import semmle.javascript.dataflow.InferredTypes
 class SplitCall extends StringSplitCall {
  SplitCall() {
    this.getSeparator() = "." and
-    this.getBaseString().getALocalSource() instanceof ParameterNode
+    this.getBaseString().getALocalSource() instanceof DataFlow::ParameterNode
  }
 }

 /**
 * Holds if `pred -> succ` should preserve polluted property names.
 */
-predicate copyArrayStep(SourceNode pred, SourceNode succ) {
+predicate copyArrayStep(DataFlow::SourceNode pred, DataFlow::SourceNode succ) {
  // x -> [...x]
  exists(SpreadElement spread |
    pred.flowsTo(spread.getOperand().flow()) and
@@ -45,7 +44,7 @@ predicate copyArrayStep(SourceNode pred, SourceNode succ) {
  )
  or
  // `x -> y` in `y.push( x[i] )`
-  exists(MethodCallNode push |
+  exists(DataFlow::MethodCallNode push |
    push = succ.getAMethodCall("push") and
    (
      getAnEnumeratedArrayElement(pred).flowsTo(push.getAnArgument())
@@ -55,7 +54,7 @@ predicate copyArrayStep(SourceNode pred, SourceNode succ) {
  )
  or
  // x -> x.concat(...)
-  exists(MethodCallNode concat_ |
+  exists(DataFlow::MethodCallNode concat_ |
    concat_.getMethodName() = "concat" and
    (pred = concat_.getReceiver() or pred = concat_.getAnArgument()) and
    succ = concat_
@@ -66,21 +65,21 @@ predicate copyArrayStep(SourceNode pred, SourceNode succ) {
 * Holds if `node` may refer to a `SplitCall` or a copy thereof, possibly
 * returned through a function call.
 */
-predicate isSplitArray(SourceNode node) {
+predicate isSplitArray(DataFlow::SourceNode node) {
  node instanceof SplitCall
  or
-  exists(SourceNode pred | isSplitArray(pred) |
+  exists(DataFlow::SourceNode pred | isSplitArray(pred) |
    copyArrayStep(pred, node)
    or
-    pred.flowsToExpr(node.(CallNode).getACallee().getAReturnedExpr())
+    pred.flowsToExpr(node.(DataFlow::CallNode).getACallee().getAReturnedExpr())
  )
 }

 /**
 * A property name originating from a `x.split(".")` call.
 */
-class SplitPropName extends SourceNode {
-  SourceNode array;
+class SplitPropName extends DataFlow::SourceNode {
+  DataFlow::SourceNode array;

  SplitPropName() {
    isSplitArray(array) and
@@ -90,7 +89,7 @@ class SplitPropName extends SourceNode {
  /**
   * Gets the array from which this property name was obtained (the result from `split`).
   */
-  SourceNode getArray() { result = array }
+  DataFlow::SourceNode getArray() { result = array }

  /** Gets an element accessed on the same underlying array. */
  SplitPropName getAnAlias() { result.getArray() = this.getArray() }
@@ -117,18 +116,18 @@ predicate isPollutedPropNameSource(DataFlow::Node node) {
 * Holds if `node` may flow from a source of polluted propery names, possibly
 * into function calls (but not returns).
 */
-predicate isPollutedPropName(Node node) {
+predicate isPollutedPropName(DataFlow::Node node) {
  isPollutedPropNameSource(node)
  or
-  exists(Node pred | isPollutedPropName(pred) |
+  exists(DataFlow::Node pred | isPollutedPropName(pred) |
    node = pred.getASuccessor()
    or
-    argumentPassingStep(_, pred, _, node)
+    DataFlow::argumentPassingStep(_, pred, _, node)
    or
    // Handle one level of callbacks
-    exists(FunctionNode function, ParameterNode callback, int i |
+    exists(DataFlow::FunctionNode function, DataFlow::ParameterNode callback, int i |
      pred = callback.getAnInvocation().getArgument(i) and
-      argumentPassingStep(_, function, _, callback) and
+      DataFlow::argumentPassingStep(_, function, _, callback) and
      node = function.getParameter(i)
    )
  )
@@ -138,8 +137,8 @@ predicate isPollutedPropName(Node node) {
 * Holds if `node` may refer to `Object.prototype` obtained through dynamic property
 * read of a property obtained through property enumeration.
 */
-predicate isPotentiallyObjectPrototype(SourceNode node) {
-  exists(Node base, Node key |
+predicate isPotentiallyObjectPrototype(DataFlow::SourceNode node) {
+  exists(DataFlow::Node base, DataFlow::Node key |
    dynamicPropReadStep(base, key, node) and
    isPollutedPropName(key) and
    // Ignore cases where the properties of `base` are enumerated, to avoid FPs
@@ -149,8 +148,8 @@ predicate isPotentiallyObjectPrototype(SourceNode node) {
    not arePropertiesEnumerated(base.getALocalSource())
  )
  or
-  exists(Node use | isPotentiallyObjectPrototype(use.getALocalSource()) |
-    argumentPassingStep(_, use, _, node)
+  exists(DataFlow::Node use | isPotentiallyObjectPrototype(use.getALocalSource()) |
+    DataFlow::argumentPassingStep(_, use, _, node)
  )
 }

@@ -197,7 +196,7 @@ string unsafePropName() {
 * A flow label representing an unsafe property name, or an object obtained
 * by using such a property in a dynamic read.
 */
-class UnsafePropLabel extends FlowLabel {
+class UnsafePropLabel extends DataFlow::FlowLabel {
  UnsafePropLabel() { this = unsafePropName() }
 }

@@ -233,10 +232,10 @@ class UnsafePropLabel extends FlowLabel {
 * for coinciding paths afterwards.  This means this configuration can't be used as
 * a standalone configuration like in most path queries.
 */
-class PropNameTracking extends DataFlow::Configuration {
-  PropNameTracking() { this = "PropNameTracking" }
+module PropNameTrackingConfig implements DataFlow::StateConfigSig {
+  class FlowState = DataFlow::FlowLabel;

-  override predicate isSource(DataFlow::Node node, FlowLabel label) {
+  predicate isSource(DataFlow::Node node, DataFlow::FlowLabel label) {
    label instanceof UnsafePropLabel and
    (
      isPollutedPropNameSource(node)
@@ -245,7 +244,7 @@ class PropNameTracking extends DataFlow::Configuration {
    )
  }

-  override predicate isSink(DataFlow::Node node, FlowLabel label) {
+  predicate isSink(DataFlow::Node node, DataFlow::FlowLabel label) {
    label instanceof UnsafePropLabel and
    (
      dynamicPropWrite(node, _, _) or
@@ -254,14 +253,19 @@ class PropNameTracking extends DataFlow::Configuration {
    )
  }

-  override predicate isAdditionalFlowStep(
-    DataFlow::Node pred, DataFlow::Node succ, FlowLabel predlbl, FlowLabel succlbl
+  predicate isBarrier(DataFlow::Node node, DataFlow::FlowLabel label) {
+    node = DataFlow::MakeLabeledBarrierGuard<BarrierGuard>::getABarrierNode(label)
+  }
+
+  predicate isAdditionalFlowStep(
+    DataFlow::Node pred, DataFlow::FlowLabel predlbl, DataFlow::Node succ,
+    DataFlow::FlowLabel succlbl
  ) {
    predlbl instanceof UnsafePropLabel and
    succlbl = predlbl and
    (
      // Step through `p -> x[p]`
-      exists(PropRead read |
+      exists(DataFlow::PropRead read |
        pred = read.getPropertyNameExpr().flow() and
        not read.(DynamicPropRead).hasDominatingAssignment() and
        succ = read
@@ -276,29 +280,33 @@ class PropNameTracking extends DataFlow::Configuration {
    )
  }

-  override predicate isBarrier(DataFlow::Node node) {
-    super.isBarrier(node)
-    or
-    node instanceof DataFlow::VarAccessBarrier
+  predicate isBarrier(DataFlow::Node node) {
+    node instanceof DataFlow::VarAccessBarrier or
+    node = DataFlow::MakeBarrierGuard<BarrierGuard>::getABarrierNode()
  }
+}

-  override predicate isBarrierGuard(DataFlow::BarrierGuardNode node) {
-    node instanceof DenyListEqualityGuard or
-    node instanceof AllowListEqualityGuard or
-    node instanceof HasOwnPropertyGuard or
-    node instanceof InExprGuard or
-    node instanceof InstanceOfGuard or
-    node instanceof TypeofGuard or
-    node instanceof DenyListInclusionGuard or
-    node instanceof AllowListInclusionGuard or
-    node instanceof IsPlainObjectGuard
-  }
+module PropNameTracking = DataFlow::GlobalWithState<PropNameTrackingConfig>;
+
+/**
+ * A barrier guard for prototype pollution.
+ */
+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 sanitizer guard of form `x === "__proto__"` or `x === "constructor"`.
 */
-class DenyListEqualityGuard extends DataFlow::LabeledBarrierGuardNode, ValueNode {
+class DenyListEqualityGuard extends BarrierGuard, DataFlow::ValueNode {
  override EqualityTest astNode;
  string propName;

@@ -307,7 +315,7 @@ class DenyListEqualityGuard extends DataFlow::LabeledBarrierGuardNode, ValueNode
    propName = unsafePropName()
  }

-  override predicate blocks(boolean outcome, Expr e, FlowLabel label) {
+  override predicate blocksExpr(boolean outcome, Expr e, DataFlow::FlowLabel label) {
    e = astNode.getAnOperand() and
    outcome = astNode.getPolarity().booleanNot() and
    label = propName
@@ -317,7 +325,7 @@ class DenyListEqualityGuard extends DataFlow::LabeledBarrierGuardNode, ValueNode
 /**
 * An equality test with something other than `__proto__` or `constructor`.
 */
-class AllowListEqualityGuard extends DataFlow::LabeledBarrierGuardNode, ValueNode {
+class AllowListEqualityGuard extends BarrierGuard, DataFlow::ValueNode {
  override EqualityTest astNode;

  AllowListEqualityGuard() {
@@ -325,7 +333,7 @@ class AllowListEqualityGuard extends DataFlow::LabeledBarrierGuardNode, ValueNod
    astNode.getAnOperand() instanceof Literal
  }

-  override predicate blocks(boolean outcome, Expr e, FlowLabel label) {
+  override predicate blocksExpr(boolean outcome, Expr e, DataFlow::FlowLabel label) {
    e = astNode.getAnOperand() and
    outcome = astNode.getPolarity() and
    label instanceof UnsafePropLabel
@@ -339,7 +347,7 @@ class AllowListEqualityGuard extends DataFlow::LabeledBarrierGuardNode, ValueNod
 * but the destination object generally doesn't. It is therefore only a sanitizer when
 * used on the destination object.
 */
-class HasOwnPropertyGuard extends DataFlow::BarrierGuardNode instanceof HasOwnPropertyCall {
+class HasOwnPropertyGuard extends BarrierGuard instanceof HasOwnPropertyCall {
  HasOwnPropertyGuard() {
    // Try to avoid `src.hasOwnProperty` by requiring that the receiver
    // does not locally have its properties enumerated. Typically there is no
@@ -347,7 +355,7 @@ class HasOwnPropertyGuard extends DataFlow::BarrierGuardNode instanceof HasOwnPr
    not arePropertiesEnumerated(super.getObject().getALocalSource())
  }

-  override predicate blocks(boolean outcome, Expr e) {
+  override predicate blocksExpr(boolean outcome, Expr e) {
    e = super.getProperty().asExpr() and outcome = true
  }
 }
@@ -358,7 +366,7 @@ class HasOwnPropertyGuard extends DataFlow::BarrierGuardNode instanceof HasOwnPr
 * Since `"__proto__" in obj` and `"constructor" in obj` is true for most objects,
 * this is seen as a sanitizer for `key` in the false outcome.
 */
-class InExprGuard extends DataFlow::BarrierGuardNode, DataFlow::ValueNode {
+class InExprGuard extends BarrierGuard, DataFlow::ValueNode {
  override InExpr astNode;

  InExprGuard() {
@@ -366,7 +374,7 @@ class InExprGuard extends DataFlow::BarrierGuardNode, DataFlow::ValueNode {
    not arePropertiesEnumerated(astNode.getRightOperand().flow().getALocalSource())
  }

-  override predicate blocks(boolean outcome, Expr e) {
+  override predicate blocksExpr(boolean outcome, Expr e) {
    e = astNode.getLeftOperand() and outcome = false
  }
 }
@@ -379,10 +387,10 @@ class InExprGuard extends DataFlow::BarrierGuardNode, DataFlow::ValueNode {
 * It is still possible to get to `Function.prototype` through `constructor.constructor.prototype`
 * so we do not block the `constructor` label.
 */
-class InstanceOfGuard extends DataFlow::LabeledBarrierGuardNode, DataFlow::ValueNode {
+class InstanceOfGuard extends BarrierGuard, DataFlow::ValueNode {
  override InstanceOfExpr astNode;

-  override predicate blocks(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 = "__proto__"
  }
 }
@@ -393,14 +401,14 @@ class InstanceOfGuard extends DataFlow::LabeledBarrierGuardNode, DataFlow::Value
 * The former blocks the `constructor` label as that payload must pass through a function,
 * and the latter blocks the `__proto__` label as that only passes through objects.
 */
-class TypeofGuard extends DataFlow::LabeledBarrierGuardNode, DataFlow::ValueNode {
+class TypeofGuard extends BarrierGuard, DataFlow::ValueNode {
  override EqualityTest astNode;
  Expr operand;
  TypeofTag tag;

  TypeofGuard() { TaintTracking::isTypeofGuard(astNode, operand, tag) }

-  override predicate blocks(boolean outcome, Expr e, DataFlow::FlowLabel label) {
+  override predicate blocksExpr(boolean outcome, Expr e, DataFlow::FlowLabel label) {
    e = operand and
    outcome = astNode.getPolarity() and
    (
@@ -428,7 +436,7 @@ class TypeofGuard extends DataFlow::LabeledBarrierGuardNode, DataFlow::ValueNode
 /**
 * A check of form `["__proto__"].includes(x)` or similar.
 */
-class DenyListInclusionGuard extends DataFlow::LabeledBarrierGuardNode, InclusionTest {
+class DenyListInclusionGuard extends BarrierGuard, InclusionTest {
  UnsafePropLabel label;

  DenyListInclusionGuard() {
@@ -438,7 +446,7 @@ class DenyListInclusionGuard extends DataFlow::LabeledBarrierGuardNode, Inclusio
    )
  }

-  override predicate blocks(boolean outcome, Expr e, DataFlow::FlowLabel lbl) {
+  override predicate blocksExpr(boolean outcome, Expr e, DataFlow::FlowLabel lbl) {
    outcome = this.getPolarity().booleanNot() and
    e = this.getContainedNode().asExpr() and
    label = lbl
@@ -448,7 +456,7 @@ class DenyListInclusionGuard extends DataFlow::LabeledBarrierGuardNode, Inclusio
 /**
 * A check of form `xs.includes(x)` or similar, which sanitizes `x` in the true case.
 */
-class AllowListInclusionGuard extends DataFlow::LabeledBarrierGuardNode {
+class AllowListInclusionGuard extends BarrierGuard {
  AllowListInclusionGuard() {
    this instanceof TaintTracking::PositiveIndexOfSanitizer
    or
@@ -456,7 +464,7 @@ class AllowListInclusionGuard extends DataFlow::LabeledBarrierGuardNode {
    not this = any(MembershipCandidate::ObjectPropertyNameMembershipCandidate c).getTest() // handled with more precision in `HasOwnPropertyGuard`
  }

-  override predicate blocks(boolean outcome, Expr e, DataFlow::FlowLabel lbl) {
+  override predicate blocksExpr(boolean outcome, Expr e, DataFlow::FlowLabel lbl) {
    this.(TaintTracking::AdditionalSanitizerGuardNode).sanitizes(outcome, e) and
    lbl instanceof UnsafePropLabel
  }
@@ -467,14 +475,14 @@ class AllowListInclusionGuard extends DataFlow::LabeledBarrierGuardNode {
 * payload in the true case, since it rejects objects with a non-standard `constructor`
 * property.
 */
-class IsPlainObjectGuard extends DataFlow::LabeledBarrierGuardNode, DataFlow::CallNode {
+class IsPlainObjectGuard extends BarrierGuard, DataFlow::CallNode {
  IsPlainObjectGuard() {
    exists(string name | name = "is-plain-object" or name = "is-extendable" |
-      this = moduleImport(name).getACall()
+      this = DataFlow::moduleImport(name).getACall()
    )
  }

-  override predicate blocks(boolean outcome, Expr e, DataFlow::FlowLabel lbl) {
+  override predicate blocksExpr(boolean outcome, Expr e, DataFlow::FlowLabel lbl) {
    e = this.getArgument(0).asExpr() and
    outcome = true and
    lbl = "constructor"
@@ -507,26 +515,26 @@ string deriveExprName(DataFlow::Node node) {
 * In most cases this will result in an alert, the exception being the case where
 * `base` does not have a prototype at all.
 */
-predicate isPrototypePollutingAssignment(Node base, Node prop, Node rhs, Node propNameSource) {
+predicate isPrototypePollutingAssignment(
+  DataFlow::Node base, DataFlow::Node prop, DataFlow::Node rhs, DataFlow::Node propNameSource
+) {
  dynamicPropWrite(base, prop, rhs) and
  isPollutedPropNameSource(propNameSource) and
-  exists(PropNameTracking cfg |
-    cfg.hasFlow(propNameSource, base) and
-    if propNameSource instanceof EnumeratedPropName
-    then
-      cfg.hasFlow(propNameSource, prop) and
-      cfg.hasFlow([propNameSource, AccessPath::getAnAliasedSourceNode(propNameSource)]
-            .(EnumeratedPropName)
-            .getASourceProp(), rhs)
-    else (
-      cfg.hasFlow(propNameSource.(SplitPropName).getAnAlias(), prop) and
-      rhs.getALocalSource() instanceof ParameterNode
-    )
+  PropNameTracking::flow(propNameSource, base) and
+  if propNameSource instanceof EnumeratedPropName
+  then
+    PropNameTracking::flow(propNameSource, prop) and
+    PropNameTracking::flow([propNameSource, AccessPath::getAnAliasedSourceNode(propNameSource)]
+          .(EnumeratedPropName)
+          .getASourceProp(), rhs)
+  else (
+    PropNameTracking::flow(propNameSource.(SplitPropName).getAnAlias(), prop) and
+    rhs.getALocalSource() instanceof DataFlow::ParameterNode
  )
 }

 /** Gets a data flow node leading to the base of a prototype-polluting assignment. */
-private DataFlow::SourceNode getANodeLeadingToBase(DataFlow::TypeBackTracker t, Node base) {
+private DataFlow::SourceNode getANodeLeadingToBase(DataFlow::TypeBackTracker t, DataFlow::Node base) {
  t.start() and
  isPrototypePollutingAssignment(base, _, _, _) and
  result = base.getALocalSource()
@@ -542,7 +550,9 @@ private DataFlow::SourceNode getANodeLeadingToBase(DataFlow::TypeBackTracker t,
 * This dynamic read is where the reference to a built-in prototype object is obtained,
 * and we need this to ensure that this object actually has a prototype.
 */
-private DataFlow::SourceNode getANodeLeadingToBaseBase(DataFlow::TypeBackTracker t, Node base) {
+private DataFlow::SourceNode getANodeLeadingToBaseBase(
+  DataFlow::TypeBackTracker t, DataFlow::Node base
+) {
  exists(DynamicPropRead read |
    read = getANodeLeadingToBase(t, base) and
    result = read.getBase().getALocalSource()
@@ -553,29 +563,31 @@ private DataFlow::SourceNode getANodeLeadingToBaseBase(DataFlow::TypeBackTracker
  )
 }

-DataFlow::SourceNode getANodeLeadingToBaseBase(Node base) {
+DataFlow::SourceNode getANodeLeadingToBaseBase(DataFlow::Node base) {
  result = getANodeLeadingToBaseBase(DataFlow::TypeBackTracker::end(), base)
 }

 /** A call to `Object.create(null)`. */
-class ObjectCreateNullCall extends CallNode {
+class ObjectCreateNullCall extends DataFlow::CallNode {
  ObjectCreateNullCall() {
-    this = globalVarRef("Object").getAMemberCall("create") and
+    this = DataFlow::globalVarRef("Object").getAMemberCall("create") and
    this.getArgument(0).asExpr() instanceof NullLiteral
  }
 }

+import DataFlow::DeduplicatePathGraph<PropNameTracking::PathNode, PropNameTracking::PathGraph>
+
 from
-  PropNameTracking cfg, DataFlow::PathNode source, DataFlow::PathNode sink, Node propNameSource,
-  Node base, string msg, Node col1, Node col2
+  PathNode source, PathNode sink, DataFlow::Node propNameSource, DataFlow::Node base, string msg,
+  DataFlow::Node col1, DataFlow::Node col2
 where
  isPollutedPropName(propNameSource) and
-  cfg.hasFlowPath(source, sink) and
+  PropNameTracking::flowPath(source.getAnOriginalPathNode(), sink.getAnOriginalPathNode()) and
  isPrototypePollutingAssignment(base, _, _, propNameSource) and
  sink.getNode() = base and
  source.getNode() = propNameSource and
  (
-    getANodeLeadingToBaseBase(base) instanceof ObjectLiteralNode
+    getANodeLeadingToBaseBase(base) instanceof DataFlow::ObjectLiteralNode
    or
    not getANodeLeadingToBaseBase(base) instanceof ObjectCreateNullCall
  ) and
--- a/javascript/ql/test/query-tests/Security/CWE-915/PrototypePollutingFunction/PrototypePollutingFunction.expected
+++ b/javascript/ql/test/query-tests/Security/CWE-915/PrototypePollutingFunction/PrototypePollutingFunction.expected
--- a/javascript/ql/test/query-tests/Security/CWE-915/PrototypePollutingFunction/tests.js
+++ b/javascript/ql/test/query-tests/Security/CWE-915/PrototypePollutingFunction/tests.js
@@ -513,7 +513,7 @@ function usingDefineProperty(dst, src) {
            usingDefineProperty(dst[key], src[key]);
        } else {
            var descriptor = {};
-            descriptor.value = src[key]; 
+            descriptor.value = src[key];
            Object.defineProperty(dst, key, descriptor);  // NOT OK
        }
    }
@@ -587,3 +587,22 @@ function indirectHasOwn(dst, src) {
 function hasOwn(obj, key) {
    return obj.hasOwnProperty(key)
 }
+
+function captureBarrier(obj) {
+	if (!obj || typeof obj !== 'object') {
+		return obj; // 'obj' is captured but should not propagate through here
+	}
+    const fn = () => obj;
+    fn();
+    return "safe";
+}
+
+function merge_captureBarrier(dest, source) {
+    for (const key of Object.keys(source)) {
+        if (dest[key]) {
+            merge_captureBarrier(dest[key], source[key]);
+        } else {
+            dest[key] = captureBarrier(source[key]); // OK - but currently flagged anyway
+        }
+    }
+}