Security fix for prototype pollution in js-ini

[arjunshibu]

📊 Metadata *

js-ini is vulnerable to Prototype Pollution.

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

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

Prototype pollution if fixed by not allowing to modify object prototype.

🐛 Proof of Concept (PoC)

1. Create the following PoC and INI files:

// poc.js
var fs = require('fs')
var ini = require('js-ini')
console.log("Before : " + {}.polluted);
var parsed = ini.parse(fs.readFileSync('./payload.ini', 'utf-8'))
console.log("After : " + {}.polluted);

//payload.ini
[__proto__]
polluted = "Yes! Its Polluted"

2. Execute the following commands in terminal:

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

3. 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.

[mzfr]

LGTM!

[Mik317]

The fix is applied to the line 75 which is enclosed in a if statement.
However, in line 89 there's another dangerous line which could lead to prototype pollution in case the first condition isn't respected but the 2' yep.

I would suggest adding a check before the various if clauses in order to avoid this kind of problems 😄

If I missed something or you think I'm not correct please let me know it 👍

Cheers,
Mik

[arjunshibu]

The fix is applied to the line 75 which is enclosed in a if statement.
However, in line 89 there's another dangerous line which could lead to prototype pollution in case the first condition isn't respected but the 2' yep.

@Mik317 I assume you are refering to the else statement in the below code

if (currentSection !== '') {
  (<IIniObjectSection>result[currentSection])[name] = val;
} else {
  result[name] = val;
}

The setting of property is done in the if condition. The currentSection variable will hold the value of the header in the payload .ini file, in this case __proto__. It will only hit else if the .ini file have an empty header []. So the payload file will have to be something like

[]
polluted = "Yes! Its Polluted"

In that case, name variable has the value 'polluted'. So there won't be prototype pollution since the object key is not a magic attribute.

I have improved the fix by using function to make the code cleaner.

[Mik317]

LGTM 😄 🍰

@arjunshibu thanks for the explanation 👍
I was thinking about the possibility to use just the polluted = 'string' line to create an array as value and set the name of that property to __proto__ which theorically would have worked but actually can't do it since IDK the INI syntax lol (I worked on it for a hour then I just checked if it was possible array transformation and it doesn't seem)

Cheers,
Mik

[huntr-helper]

Congratulations arjunshibu - your fix has been selected! 🎉

Thanks for being part of the community & helping secure the world's open source code.
If you have any questions, please respond in the comments section, or hit us up on Discord. Your bounty is on its way - keep hunting!

Come join us on Discord