Fix for Prototype Pollution

[rockarts]

This commit fixes prototype pollution: https://github.com/HoLyVieR/prototype-pollution-nsec18/blob/master/paper/JavaScript_prototype_pollution_attack_in_NodeJS.pdf

Since so many repos depend on this repo, some may be using it to merge proto and this fix may break them.

[coveralls]

Coverage remained the same at 100.0% when pulling 1678320 on rockarts:master into 28785eb on jaredhanson:master.

[jaredhanson]

It's my understanding that prototype pollution is the result of doing operations (in this case calling merge()) where one argument is unsantized user input. I could see this being an issue in libraries intended to take user input and transform it, such as ORMs.

Why should this bug extend to relatively low-level utilities such as utils-merge, which isn't intended to take user input directly? If this is truly an issue, shouldn't it be fixed at the layer where the issue is caused?

[snowmac]

It's my understanding that prototype pollution is the result of doing operations (in this case calling merge()) where one argument is unsantized user input. I could see this being an issue in libraries intended to take user input and transform it, such as ORMs.

Why should this bug extend to relatively low-level utilities such as utils-merge, which isn't intended to take user input directly? If this is truly an issue, shouldn't it be fixed at the layer where the issue is caused?

There is another report of this issue. Looking at the readme page of the utils-merge it's not clear that this is strictly a low-level utility. Merges the properties from a source object into a destination object. -- could be used for a variety of operations. The example provided shows copying properties, foo bar etc... into a target object.

Furthermore, even if it were low-level why would you want to copy the prototypes? Could we add an optional argument that defaults to false to allow you to copy the prototype when set to true?

[snoopysecurity]

I don' think there is prototype pollution here:

var merge = require('utils-merge');
var a = { foo: 'bar', qux: 'corge' }
      , b = {
        "__proto__": {
          "polluted": "true",
        }
      };
    var o = merge(a, b);
    console.log(o.polluted) //o is polluted but not the global object prototype
    console.log({}.polluted) //if there was prototype pollution, this would be true

You are polluting your own object, not the global object

[snowmac]

I think Snoopy is right, this isn't an issue in the sense of you're only polluting the local object not the global object. Thus in the truest sense of the issue, you're not causing prototype pollution, if you were ALL objects would be affected. I created a gist explaining this with examples

The issue here is that you could if this were called by a user facing API, which expressJS and a lot of other libraries use this to merge things like headers

[jose-nunez]

You might want to remove this console log?

[jose-nunez]

you're only polluting the local object not the global object

I guess the vuln report is actually based on that?

[snoopysecurity]

you're only polluting the local object not the global object

I guess the vuln report is actually based on that?

Yep indeed, the vuln reports that are going around for utils-merge prototype pollution is wrong

[rockarts]

you're only polluting the local object not the global object

I guess the vuln report is actually based on that?

How did the vuln report get filed? I never filed it. This whole PR seemed to get blown out of proportion after it got filed.

[rockarts]

I think Snoopy is right, this isn't an issue in the sense of you're only polluting the local object not the global object. Thus in the truest sense of the issue, you're not causing prototype pollution, if you were ALL objects would be affected. I created a gist explaining this with examples

The issue here is that you could if this were called by a user facing API, which expressJS and a lot of other libraries use this to merge things like headers

There are unit tests on this project which demonstrates this. What good does an example gist do? Why not add more unit tests?