False positive for Prototype-polluting function

[dbauszus-glx]

I check for prototype polluting property keys in a set of reserved keys which include __proto__ and constructor.

This shouldn't even be necessary since the key, value come from Object.entries which according to MDN will only iterate own enumerable string-keyd property. ie. never __proto__ or constructor.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/entries

Still the lookup in the set does not work.

  // Iterate through the source own enumerable string-keyed property key-value pairs.
  for (const [key, value] of Object.entries(source)) {

    // This for codeql only. key, value of Object.entries should ensure that only own properties are parsed
    if (!source.hasOwnProperty(key)) continue;

    // The ignoreKeys contain checks against prototype pollution.
    if (new Set(['__proto__', 'constructor', 'mapview']).has(key)) {

      continue;
    }

CodeQL looks at the right place but ignores the check for the set.

https://github.com/GEOLYTIX/xyz/security/code-scanning/217

The only way I can make the issue go away is by doing a === check on the string value like so.

  // Prevent prototype polluting assignment.
  if (key === '__proto__' || key === 'constructor') return true;

Even though I know that this issue can not happen I need to add this extra line to make the CodeQL warning go away.

[mbg]

Hi @dbauszus-glx 👋🏻

Thanks for reporting this! Yes, that does look like a false positive. Fixing these isn't a current product priority, so I can't say when this might get fixed. However, we will be tracking this internally.

Although I appreciate it is not ideal, it seems like you have a workaround for the minute.

[asgerf]

Hi @dbauszus-glx,

In a prototype pollution attack, the payload will be crafted to have special properties like __proto__ as own properties. This means the check source.hasOwnProperty(key) is not an effective check against prototype pollution. Using Object.entries() doesn't guard against prototype pollution either.

As specified in the query help shown on the alert, the recommendation is to check if it's an own property of the destination object (called target in your codebase). The crucial bit is not which parts of the source object you read from, but which parts of the target you end up modifying. When reading a property from target, first make sure the property you're reading from is an own property of target so it can't read back something from the prototype chain.

As for the missed check of form new Set([...]).has(key), that's something we might improve on in the future.

[dbauszus-glx]

@asgerf Thank you for providing more detail to the explanation.

How does this work for new properties on the target?

Let's say my target property is a prototype with no own properties. My source object has the 'foo' boolean [object] flag.

How can I check whether the 'foo' property is safe to be assigned on the target property if the target does not have the ownProperty.

let target = {}
let source = {foo:true}

for (const [key, value] of Object.entries(source)) {
  console.log(target.hasOwnProperty(key)) //false
  target[key] = value
}

[asgerf]

It's usually fine assign to the property without a check. It issue is when reading a property from target. Usually a merge-like function will read from target[key] before doing a recursive call. That's where you need a check. Never read key from the target if it's not an own property.

In the CodeQL alert you can see the point where you're reading from target[key] on step 4: