Security Fix for Prototype Pollution - huntr.dev

[huntr-helper]

https://huntr.dev/users/arjunshibu has fixed the Prototype Pollution vulnerability 🔨. Think you could fix a vulnerability like this?

Get involved at https://huntr.dev/

Q | A
Version Affected | ALL
Bug Fix | YES
Original Pull Request | 418sec#1
Vulnerability README | https://github.com/418sec/huntr/blob/master/bounties/npm/rfc6902/1/README.md

User Comments:

📊 Metadata *

rfc6902 is vulnerable to Prototype Pollution. This package allowing for modification of prototype behavior, which may result in Information Disclosure/DoS/RCE.

Bounty URL: https://www.huntr.dev/bounties/1-npm-rfc6902

⚙️ Description *

Prototype Pollution refers to the ability to inject properties into existing JavaScript language construct prototypes, such as objects.
JavaScript allows all Object attributes to be altered, including their magical attributes such as proto, constructor and prototype.
An attacker manipulates these attributes to overwrite, or pollute, a JavaScript application object prototype of the base object by injecting other values. Properties on the Object.prototype are then inherited by all the JavaScript objects through the prototype chain.

💻 Technical Description *

Fix implemented by not allowing to modify object prototype.

🐛 Proof of Concept (PoC) *

• Create the following PoC file:

// poc.js
var rfc6902 = require("rfc6902")
var obj = {}
console.log("Before : " + {}.polluted);
rfc6902.applyPatch(obj, [{ op: 'add', path: "/__proto__/polluted", value: "Yes! Its Polluted"}]);
console.log("After : " + {}.polluted);

• Execute the following commands in another terminal:

npm i rfc6902 # Install affected module
node poc.js #  Run the PoC

• Check the Output:

Before : undefined
After : Yes! Its Polluted

🔥 Proof of Fix (PoF) *

Prototype pollution is fixed as seen below.

👍 User Acceptance Testing (UAT)

• I've executed unit tests.
• After fix the functionality is unaffected.

[JamieSlome]

@chbrown - let me know if you have any thoughts or questions, cheers! 🍰

[ddimitrioglo]

['__proto__', 'constructor', 'prototype'].includes(key)?

[JamieSlome]

Resolved, thanks! 🍰

[ddimitrioglo]

parent?.[key]

[JamieSlome]

@ddimitrioglo - in place of the entire conditional statement?

[ddimitrioglo]

value = (parent || {})[key]

=>

value = parent?.[key]

[JamieSlome]

Was looking at the wrong line!

Was looking at 72 and 73! 😝

Will commit and push now...

[JamieSlome]

Done! 🍰

[delucis]

@chbrown Anything needed to help get this patch merged?

[jake-arkinstall]

This PR (well, the lack of merge after so long) has gained some fame.

https://thedailywtf.com/articles/patching-over-your-problem

[JBaldwinWGU]

@agnihotriwgu Seems like there's already a fix for the vulnerability we're seeing in Veracode -- just not merged in after close to a year (!). @chbrown is there anything else that needs to be done for this PR?

[chbrown]

I was putting this off because I thought there might be a better solution that didn't just skip over these polluting keys but didn't throw an error outright... but I couldn't summon the persistence (interest) to find it.

I still don't know why this "vulnerability" is such a big deal but I figure the behavior is broken either way, so if c006ce9 assuages some fears out there, sure, why not.

Re: "anything else that needs to be done for this PR?" — it wasn't that hard to cherry-pick out the right commit and assimilate it, but for the record:

1. Match the style of the codebase you're PR'ing! This is clearly a no-semicolon, no-implicit-blocks codebase.
2. If you're going to change something to be less straightforward, especially in a hot code path like this one, you're gonna need to demonstrate some advantage. But in the case of 2e7266a, it's all downside. Quick naive benchmark says key == '__proto__' || key == 'constructor' || key == 'prototype' is 4x faster than ['__proto__', 'constructor', 'prototype'].includes(key) which is about what I would have guessed.
3. Leave unrelated changes (611c6f7) for another PR. Especially if they use syntax not otherwise used in the codebase.

Anyhow, fixed in v5.0.0. See https://github.com/chbrown/rfc6902#changelog.

[delucis]

Thanks for resolving this @chbrown!