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/shvl/1/README.md

User Comments:

📊 Metadata *

shvl is vulnerable to Prototype Pollution.

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

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

1. Create the following PoC file:

// poc.js
var shvl = require("shvl")
let obj = {}
console.log("Before: " + {}.polluted)
shvl.set(obj, '__proto__.polluted', 'Yes! Its Polluted')
console.log("After: " + {}.polluted)

2. Execute the following commands in terminal:

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

3. Check the Output:

Before: undefined
After: Yes! Its Polluted

🔥 Proof of Fix (PoF) *

+1 User Acceptance Testing (UAT)

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

[JamieSlome]

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

[robinvdvleuten]

@JamieSlome thanks for your effort! I tried to fix the issue myself in v2.0.2, how does your solution differs?

[JamieSlome]

@robinvdvleuten - this does a comparison against the __proto__ which should never be a modifiable property and evaluates if this is matched against p.

Cheers! 🍰

[JamieSlome]

@ljharb - since our discussion on Prototype Pollution yesterday, does this fix look good to you? 🍰

Perhaps we don't care so much about prototype and constructor but certainly __proto__?

[ljharb]

i'd argue it is incorrect to omit constructor/prototype from this, but __proto__ is correct here.

Also, .includes would not be available in an ES5 environment, which this package appears to work on, so you may want to use .indexOf here (or use three === comparisons)

[yoshino-s]

We do need to consider about constructor and prototype, because we have this poc which doesn't use __proto__ but also make an prototype pollution.

// poc.js
var shvl = require("shvl")
let obj = {}
console.log("Before: " + {}.polluted)
shvl.set(obj, 'constructor.prototype.polluted', 'Yes! Its Polluted')
console.log("After: " + {}.polluted)

and i have an idea about how to fix it, which doesn't introduce new functions.

export function set  (object, path, val, obj) {
  return !/__proto__|constructor|prototype/.test(path) && ((path = path.split ? path.split('.') : path.slice(0)).slice(0, -1).reduce(function (obj, p) {
    return obj[p] = obj[p] || {};
  }, obj = object)[path.pop()] = val), object;
};

Should we use this fix as quick as possible?

[ljharb]

That precise fix would also block “.constructorx” which should be allowed, but the general approach is reasonable. However, i don’t think these are actually bugs. If the user explicitly intended to pollute the prototype, then that is a correct usage if the library, and if a different package is doing that with untrusted user input, that is a bug in the caller and not this library.

[yoshino-s]

I think it indeed a vulnerability because if we use shvl.set() with user input arguments,such as this situation:

const store = {};
app.use("/test", (req, res) => {
  const k = req.query.key;
  const v = req.query.value;
  shvl.set(store, k, v);
});

In this application malcioud input it will lead to a disaster.

Also Prototype Pollution is a defined CWE type. https://cwe.mitre.org/data/definitions/1321.html

And this may be a more precious fix:

export function set  (object, path, val, obj) {
  return !/^(__proto__|constructor|prototype)$/.test(path) && ((path = path.split ? path.split('.') : path.slice(0)).slice(0, -1).reduce(function (obj, p) {
    return obj[p] = obj[p] || {};
  }, obj = object)[path.pop()] = val), object;
};

[ljharb]

The bug is using unsanitized user input. It’s not shlvl’s fault that someone uses it unsafely, just like it’s not “rm”’s fault if someone runs rm -rf /.

[robinvdvleuten]

Thanks for all the input! The fix will be released in v2.0.3