github / github/codeql

CodeQL False Negative - Protototype Pollution

Open
#18,665 6 comments 0 reactions 0 assignees View on GitHub
question
Dominant language
CodeQL
Stars
10.1k
Forks
2.1k
Avg merge
2d 15h
Merged PRs (30d)
141

Description

Hi,
I would like to report a possible false negative for [SNYK-JS-XASSIGN-1759314](https://security.snyk.io/vuln/SNYK-JS-XASSIGN-1759314).

Relevant code:
```js
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:
```ql
/**
* @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;

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):
```ql
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())
}
```

Contributor guide

Open the contributing guide

Research direction

Start with the supplied Config, isSource, isSink, and isAdditionalFlowStep predicates and reproduce the flow using the JavaScript snippet and query. Compare the result with the PrototypePollutingAssignment sink; done means determining whether the missing flow is expected and capturing a regression case or a justified query change.

Written by the indexing model from the issue text.

Assessment

Tech stack
javascript
Domain
security
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
38/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.