CodeQL False Negative - Protototype Pollution

[DSimsek000]

Hi,
I would like to report a possible false negative for SNYK-JS-XASSIGN-1759314.

Relevant code:

const XAssign = {
   assign: function (...args) {
      /**
       * Combine object properties or concat array properties
       *
       * @param {any} acc the target or accumulator
       * @param {any} obj object to apply
       */

      function apply(acc, obj) {
         if (obj == null || typeof obj !== "object") {
            return // ignore non-object args
         }
         Object.keys(obj)
            .forEach((key) => {
               const value = obj[key]
               if (Array.isArray(value)) {
                  acc[key] =
                  acc[key] && Array.isArray(acc[key])
                     ? acc[key].concat(value)
                     : value
               } else if (typeof value === "object") {
                  acc[key] = acc[key] || {}
                  if (Array.isArray(acc[key])) {
                     acc[key] = {} // getting overridden with an Object!
                     apply(acc[key], value)
                  } else if (typeof acc[key] === "object") {
                     apply(acc[key], value)
                  } else {
                     acc[key] = value
                  }
               } else {
                  acc[key] = value
               }
            })
      }

      /**
       * Apply merge for each object argument.
       */
      const result = {}
      args.forEach((obj) => apply(result, obj))
      return result
   },
}

I ran the following query:

/**
 * @name Prototype pollution
 * @description Using externally controlled input to set properties on the prototype of an object can lead to prototype pollution.
 * @severity high
 * @kind path-problem
 * @precision high
 * @id js/prototype-pollution
 * @tags external/cwe/cwe-471 external/cwe/cwe-915
 */

import javascript
import semmle.javascript.dataflow.TaintTracking
import semmle.javascript.security.dataflow.PrototypePollutingAssignmentCustomizations::PrototypePollutingAssignment

module Config implements DataFlow::ConfigSig {
  DataFlow::FlowFeature getAFeature() { result instanceof DataFlow::FeatureHasSourceCallContext }

  predicate isSource(DataFlow::Node source) {
    exists(Function f | f.getName() = "assign" | source.asExpr() = f.getAParameter())
  }

  predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
}

module Flow = TaintTracking::Global<Config>;

import Flow::PathGraph

from Flow::PathNode source, Flow::PathNode sink
where Flow::flowPath(source, sink)
select sink.getNode(), source, sink, ""

By providing the following additional taint steps, I managed to find the vulnerability (though this may be overly broad):

predicate isAdditionalFlowStep(DataFlow::Node fromNode, DataFlow::Node toNode) {
    exists(CallExpr ca | ca = toNode.asExpr() and ca.getAnArgument() = fromNode.asExpr())
    or
    exists(IndexExpr ie | ie = toNode.asExpr() and ie.getIndex() = fromNode.asExpr())
}

[redsun82]

Hi @DSimsek000 thanks a lot for raising this to our attention! I will forward it to our internal team working on javascript analysis.

[asgerf]

Hi 👋

The query js/prototype-pollution-utility (in this file) seems to flag this out of the box. Is there a reason you want a custom query for this?

[DSimsek000]

Hi @asgerf ,
my goal was to analyse a specific entry point and do global taint analysis. I was under the assumption that the above query would import all prototype pollution sink definitions.

[asgerf]

In this case you're probably better off making a copy of PrototypePollutingAssignment.ql and extending the Source class with the specific entry point of interest. Most of the query logic is factored into a reusable .qll file exactly for this type of use-case.

For example, the query would look like this:

/** @kind path-problem */

import javascript
import semmle.javascript.security.dataflow.PrototypePollutingAssignmentQuery
import PrototypePollutingAssignmentFlow::PathGraph

class MyCustomSource extends Source {
  MyCustomSource() {
    this = /* ... */
  }

  override string describe() { result = "custom source" }
}

from
  PrototypePollutingAssignmentFlow::PathNode source, PrototypePollutingAssignmentFlow::PathNode sink
where
  PrototypePollutingAssignmentFlow::flowPath(source, sink) and
  not isIgnoredLibraryFlow(source.getNode(), sink.getNode())
select sink, source, sink,
  "This assignment may alter Object.prototype if a malicious '__proto__' string is injected from $@.",
  source.getNode(), source.getNode().(Source).describe()

[DSimsek000]

Thank you asgerf! Using the above query I was able to find the vulnerability. However I still had to add the following taint step predicate for the Object.keys call when using the parameter node of the function assign as the source:

class TStep extends TaintTracking::AdditionalTaintStep {
  override predicate step(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
    exists(CallExpr ca |
      ca.getCalleeName() = "keys" and
      ca = nodeTo.asExpr() and
      ca.getAnArgument() = nodeFrom.asExpr()
    )
  }
}

It seems Object.{keys,values}() is not specified as taint step in https://github.com/github/codeql/blob/062391334e02fe23a5178204a4d66717761b84cb/javascript/ql/lib/semmle/javascript/dataflow/TaintTracking.qll

Was this a deliberate design choice?

[DSimsek000]

Hi @asgerf,

I was testing the above config against a few CVEs and encountered an issue related to calls to forEach. In the example below the taint path isn't detected in the set method, but it works for the enhanced forloop:

function set(input, value) {
    let config = {};
    let name = input.split(".");
    let length = name.length;
    name.forEach((item, index) => {
        if (index === length - 1) {
            config[item] = value;
        } else {
            config = config[item];
        }
    });
    return config;
}

function set2(input, value) {
    let config = {};
    let name = input.split(".");
    let length = name.length;
    for (const item of name) {
        if (item === name[length - 1]) {
            config[item] = value;
        } else {
            config = config[item];
        }
    }
    return config;
}

I changed the sink predicate to exists(DataFlow::PropWrite write | write.getPropertyNameExpr() = node.asExpr()) which confirmed that this isn't due to the dataflow failing to reach that path.

The working taint path for the set2 method looks like this (taint steps surrounded with **):

1. for (const *item* of name) {
2.      if (item === name[length - 1]) {
3.            config[item] = value
4.        } else {
5.            config = config[item]
6.        }
7.    }

1. for (const item of name) {
2.      if (item === name[length - 1]) {
3.            config[item] = value
4.        } else {
5.            config = config[*item*]
6.        }
7.    }

1. for (const item of name) {
2.      if (item === name[length - 1]) {
3.            config[item] = value
4.        } else {
5.            config = *config[item]* // this should set the label to `FlowState::objectPrototype`
6.        }
7.    }

1. for (const item of name) {
2.      if (item === name[length - 1]) {
3.            config[item] = value
4.        } else {
5.            *config = config[item]* 
6.        }
7.    }

And now it goes back to the loop start and flags the sink:

1. for (const item of name) {
2.      if (item === name[length - 1]) {
3.            *config*[item] = value
4.        } else {
5.            config = config[item]
6.        }
7.    }

I suspect it relates to codeql not covering the last step.

Here the full query:

/**
 * @kind path-problem
 */

import javascript
import semmle.javascript.security.dataflow.PrototypePollutingAssignmentQuery
import PrototypePollutingAssignmentFlow::PathGraph

class MyCustomSource extends Source {
  MyCustomSource() {
    exists(Function f |
      f.getName() = "set" and
      this.asExpr().getEnclosingFunction() = f
    |
      this.asExpr() = f.getAParameter()
    )
  }

  override string describe() { result = "custom source" }
}

from
  PrototypePollutingAssignmentFlow::PathNode source, PrototypePollutingAssignmentFlow::PathNode sink
where
  PrototypePollutingAssignmentFlow::flowPath(source, sink) and
  not isIgnoredLibraryFlow(source.getNode(), sink.getNode())
select sink, source, sink,
  "This assignment may alter Object.prototype if a malicious '__proto__' string is injected from $@.",
  source.getNode(), source.getNode().(Source).describe()