Security Fix for Prototype Pollution - huntr.dev

[huntr-helper]

https://huntr.dev/users/alromh87 has fixed the Prototype Pollution vulnerability 🔨. alromh87 has been awarded $25 for fixing the vulnerability through the huntr bug bounty program 💵. 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/y18n/1/README.md

User Comments:

📊 Metadata *

y18n 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-y18n

⚙️ 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.
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 *

Fixed by avoid setting magical attributes.

Note that in this case path is not expanded so constructor.prototype would not cause prototype pollution so filtering this magical attributes is not needed

🐛 Proof of Concept (PoC) *

1. Create the following PoC file:

// poc.js
const y18n = require('y18n')();
var obj = {}
console.log("Before : " + obj.polluted);
y18n.setLocale('__proto__');
y18n.updateLocale({polluted: 'Yes! Its Polluted'});
console.log("After : " + {}.polluted);

2. Execute the following commands in another terminal:

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

3. Check the Output:

Before : undefined
After : Yes! Its Polluted

🔥 Proof of Fix (PoF) *

After fix execution will block prototype pollution and throw an exception

👍 User Acceptance Testing (UAT)

After fix functionality is unafected

[JamieSlome]

@ljharb - let me know if you have any thoughts or questions!

Cheers! 🍰

[ljharb]

This doesn’t have a regression test, and as such shouldn’t qualify as a fix yet.

[ljharb]

Throwing an error doesn’t seem useful here; it seems like the proper fix would be to set a strong property instead of looking up the [[Prototype]].

[bcoe]

@huntr-helper thanks for the patch 👌

To @ljharb's point:

1. we should add a regression test that demonstrates the prototype solution.
2. I don't love throwing, couldn't we just use Object.create(null) to instantiate any objects that are set from inputs?

[JamieSlome]

@bcoe @ljharb - thanks to both for the comments - we will sort out a regression test and look at an alternative to throwing an error.

@alromh87 - any thoughts on this?

[JamieSlome]

@ljharb @bcoe - we have added a regression test plus added a return instead of throwing an error.

Thoughts? 🍰

[ljharb]

after this call, shouldn't expect(i18n.getLocale()).to.have.property('polluted') pass as well?

[JamieSlome]

@ljharb - before the fix, this may have been the case. However, after running the test suite with your suggested test, the following is returned:

If the suggested expect() statement passed, wouldn't this suggest that the prototype has been polluted?

[ljharb]

No, it would suggest that the locale object was properly updated.

I think the solution here isn't to disrupt usage of a string __proto__, but instead, to "salt" each locale name with a character that couldn't conflict with anything built-in. I usually use $.

ie, every get and set of a locale name should quietly stick a "$" in front. That way it'll be impossible to collide with the name of any builtin.

[JamieSlome]

@ljharb - okay, we will take another look at this, and see if we can salt each locale name as suggested with a non-disruptive character.

Cheers! 🍰

[JamieSlome]

@ljharb - after speaking with the team, we would love to hear your thoughts on how a salting approach may be implemented. It seems from our perspective that it could be a fairly sizeable change to the codebase with potentially breaking effects and may only minimally reduce the risk of this type of attack.

From our limited understanding __proto__ is not a legitimate locale and in all likelihood could not be a legitimate locale. So, the detection of this potentially malicious input and preventing a malicious subsequent invocation should be adequate to mediate this risk.

Cheers, and look forward to hearing more! 🍰

[ljharb]

I'm not a maintainer here, so I'd defer to @bcoe for what makes the most sense for this library.

Given that getLocale returns an entire locale object, and updateLocale mutates that object, maybe a simpler approach than salting would be to use Object.defineProperty instead of =, which wouldn't trigger the Object.prototype.__proto__ setter?

[JamieSlome]

@bcoe - any thoughts on the updated approach or @ljharb's comments?