Implement a __proto__ check option

[martinheidegger]

Eran Hammer posted an article on proto poisoning and his solution in joi/hapi: https://hueniverse.com/a-tale-of-prototype-poisoning-2610fa170061

@rgrove posted a simple implementation of a fix for this: https://gist.github.com/rgrove/3ea9421b3912235e978f55e291f19d5d

However the fix requires a custom reviver that might slow down the default/valid parsing case, Eran prevented this by using an initial check for __proto__. It might be good to add this as a default to be checked for in body-parser in general that can be switched off... if someone wants to do so.

[dougwilson]

We are working on this as a security issue that was reported.

[dougwilson]

In general unless there is a consensus that JSON.parse is fundamentally unsafe and should not be used, there isn't actually a security issue at hand in this module. The security issue highlighted there is the nature of the language itself, and one could argue that Object.assign is the vulnerable point, as that is the point of failure.

P.S. I didn't mean to close the issue

[dougwilson]

One method to protect your app if you have vulnerable code (and are not willing to fix it), would be to add the following middleware after body parser:

app.use((req, res, next) => {
  if (req.body && typeof req.body === 'object') Bourne.scan(req.body, { your options })
  next()
})

This uses the https://github.com/hapijs/bourne instead of a reviver. Not sure if that is faster or not performance wise, off hand.

[martinheidegger]

In general unless there is a consensus that JSON.parse is fundamentally unsafe and should not be used, there isn't actually a security issue at had in this module. The security issue highlighted there is the nature of the language itself, and one could argue that Object.assign is the vulnerable point, as that is the point of failure.

I was thinking the same thing. Object.assign is definitely inconsistent here. Lets prevent developers from using Object.assign 😉 - ! Sadly not a practical solution.

The argument for fixing this in body-parser to me is regarding unsanitized input. It can be perfectly fine that JSON data contains a __proto__ object if the owner and intents are okay. If an app feeds Json.parse a unsanitized string, however it should check for __proto__. As body-parser is basically a glorified parser for unsanitized content, it seemed like the right repository to open this issue.

[dougwilson]

Sure, but there are many other issues. For example, even constructor should arguably be sanitized out, as modules similar to Object.assign would pollute the prototype with that property name: https://hackerone.com/reports/430291

Where would this end? What specifically makes removing __proto__ necessary and not removing constructor or any other potential property that an extending object module could cause prototype pollution with?

[martinheidegger]

Json.parse() creates an object that - when put through another JavaScript core API - results in an object that doesn't match the expected output. I am not familiar with all the properties that fall under that category but it looks like constructor is also one of those.

[dougwilson]

Right, I get that, but doesn't requiring every module everywhere that does JSON.parse to add this seem like the wrong answer? It seems the root issue here needs to be fixed. What you're describing to me sounds like perhaps there is a real issue here in some new Javascript langauge features that should probably be addressed. If we add this, when will the conversation of the root issue ever happen? Is Object.assign fundamentally flawed? Is JSON.parse ?

[dougwilson]

If this module is unsafe without this change, the Javascript Fetch API had the exact same issue: https://developer.mozilla.org/en-US/docs/Web/API/Body/json

[martinheidegger]

There is a fundamental, not communicated API inconsistency here. Those API's should simply behave same in that they return an enumerable "__proto__" field if an enumerable "__proto__" field is set. I would also say that JOI should look for the enumerability of the proto field to judge if they should validate it - not the current solution. That being said: this is a discrepancy not a bug. It might be intended and as such we need to live with it probably forever.

In my intuition System API > Library API in regard to fetch.json: If you are accessing perfectly safe JSON it shouldn't add this restriction. Library that uses JSON.fetch to access unsanitized data: Then definitely.

[rgrove]

One method to protect your app if you have vulnerable code (and are not willing to fix it), would be to add the following middleware after body parser:

app.use((req, res, next) => {
  if (req.body && typeof req.body === 'object') Bourne.scan(req.body, { your options })
  next()
})

This uses the https://github.com/hapijs/bourne instead of a reviver. Not sure if that is faster or not performance wise, off hand.

Unfortunately using Bourne's scan() function directly like this on an already-parsed object bypasses Bourne's performance improvements. Since Bourne doesn't have access to the JSON string at this point, it can't use a quick RegExp match to skip doing a slower deep scan of the object when possible.

In my testing, using scan() directly like this is quite a bit faster than using a JSON.parse() reviver function, but it's still not as fast as using Bourne.parse() would be. My quick benchmark results of the best-case scenario (using Bourne's no__proto__.js benchmark):

• JSON.parse(): 476,018 ops/sec
• Bourne.parse(): 458,014 ops/sec
• JSON.parse() followed by Bourne.scan(): 380,877 ops/sec
• JSON.parse() with reviver: 183,411 ops/sec

@dougwilson I understand your hesitation to address this in body-parser and in general I agree that this seems like a larger problem that may need to be addressed in Object.assign() or JSON.parse() themselves. But in the meantime, websites do need to protect against prototype poisoning, and currently it's difficult to do that in a performant way while using body-parser.

One option might be for body-parser to accept an optional parse function, defaulting to JSON.parse, which it would call internally when parsing JSON. This would make it possible for users to tell body-parser to use Bourne.parse in order to prevent prototype poisoning.

[dougwilson]

Hi @rgrove I'm not necessarily hesitant to add something like this, but I would like to better understand the actual goal to protect here. It would be one thing if there was actually a vulnerability in this module -- the mere usage of this module caused prototype pollution. That does not seem to be the case here. Instead, the prototype pollution is occurring when one has code which takes the output of this module and does something with it, something which this module cannot control and know of easily.

If the goal is to simply "add a option to remove the a property from an object tree, for example __proto__, then there doesn't seem a point to single out that property -- perhaps a user wants to remove the property super_ from all incoming objects, or whatever. If that is the goal, we should provide a general way in which to remove a property -- which is the existing reviver function.

If the goal is to protect against prototype pollution attacked, then that is fair too, though I don't think simply removing __proto__ and nothing else is the answer here. I linked to one of many HackerOne reports above in which other properties than __proto__ result in prototype pollution. In fact, adding a method to this module to "protect against prototype pollution" means that it is a vulnerability if prototype pollution can still occur when that setting is enabled, as the purpose was to protect you, and being able to circumvent that protection is a vulnerability.

If the goal is to accept custom parsers, for example to provide bounre.parse if one wants, then we can close this issue in favor of the issue tracking that exact feature: #22

[panga]

I've implemented an express middleware to address my concerns with __proto__ or constructor object pollution and also Mongo.js injection.

https://www.npmjs.com/package/node-shield#express-4x-middleware-usage

[damien-git]

@dougwilson I would personally like to see a prototype pollution protection added to express. I doubt there are many legitimate uses of __proto__ and constructor properties in requests parsed by body-parser, and removing them in a new version sounds like an worthy improvement to me. I do some input sanitation in my code, and then sometimes I let external libraries do it (such as sequelize with type validation). I don't feel like checking if these libraries are doing the right thing related to prototype pollution, but I would sleep better knowing that even if they don't handle it right, express will catch this issue before.

[RA80533]

Do any of you personally use Express without prototype sanitization? What areas of your services would suffer as a result of Express handling prototype sanitization?

[JaneJeon]

@rgrove unless if I’m misunderstanding the docs incorrectly, can’t the Bourne check also be done within the body-parser middleware w/o the need to add a bespoke one after body-parser.json()?

express.json({
      verify: (req, res, buf, encoding) => {
        scan(req.body)
      }
    })

[rgrove]

@JaneJeon Good point! I haven't tested this, but it seems like this (or something like it) should work.

[theogravity]

I've published a forked version that replaces the JSON.parse() refs in json.js with a version from secure-json-parse:

https://www.npmjs.com/package/secure-body-parser