Disable __proto__

[mcollina]

There have been quite a few CVE related to __proto__ in the last while. I think it would be good to have a flag to enable/disable it.

A quick example:

const payload = '{"__proto__": null}'
const a = {}
console.log("Before : " + a) // this works
Object.assign(a, JSON.parse(payload))
console.log("After : " + a) // this crashes

(It's not strictly related to JSON, as it can also apply to multipart data or other serialization format).

Some vulnerabilities:

• https://www.npmjs.com/advisories/1480
• https://snyk.io/vuln/npm:hoek:20180212
• https://www.npmjs.com/advisories/577
• (There are probably way more)

I don't know if this is fixable / manageable on our side (vs V8), but __proto__ still causes significant vulnerabilities.

---

Note that there are some modules to help with this, including https://github.com/hapijs/bourne.

[mcollina]

cc @nodejs/tsc @nodejs/security @nodejs/v8

[hueniverse]

From a security / dev perspective, there really is no reason not to disable the getter/setter for __proro__ on Object. If this is done in a major breaking release, modules that use it can easily move to better ways of manipulating the prototype.

The core issue here is that even the most security-aware developers still get caught not thinking about it when using external inputs. It's impossible to completely seal internal logic from some external input coming in that contains __proto__ directly or after some string manipulation.

I am seriously considering deleting the getter/setter when hapi loads and being done with it, letting any other non-hapi module that requires it to fail and get people to fix it. But this would be much better dealt with for everyone at the node core level (or v8). It's just a getter/setter on the Object prototype.

[devsnek]

There are two parts to __proto__:

• Object.prototype.__proto__
• Object literals with __proto__

The second one is extremely popular for creating object literals with a null prototype, so I doubt we could disable it. I don't think the Object.prototype.__proto__ has anywhere near as much usage, but I would guess it is still a lot more than we could deal with.

[jasnell]

I'm not convinced there's anything reasonable we can do at the Node.js level here by default without causing massive backwards compat issues. I wouldn't be opposed to running an experiment tho so see if that's wrong.

[ljharb]

It's part of JS; I don't think node dot JS should ever be in the position of disabling parts of the language, even with a flag.

(also most of these vulnerabilities are due to doing dangerous things with unsanitized user input, which is a bad practice that causes problems with or without __proto__)

[bakkot]

Drive-by comment:

I don't know if this is fixable / manageable on our side

For the vulnerabilities listed, which are about the setter on Object.prototype, I think it would be straightforward for Node to just delete that setter during startup when the flag was set. (Possibly it would be a little more complicated to propagate that behavior to new contexts; I lack the relevant knowledge of the internals.)

Object.defineProperty(Object.prototype, '__proto__', { set: void 0 });

[mscdex]

IMO there isn't anything node can/should do about this. Developers just need to implement better patterns. The typical scenario I've seen that relates to this kind of issue is people using {} as a data storage mechanism instead of a Map or Object.create(null), both of which have been available for quite some time now and are more suitable for storing arbitrary data.

Additionally, I'm not entirely sure why someone would be attempting the kind of operation in the OP ('string' + nullProtoObj). Outside of edge cases like that, objects with null prototypes work just fine, even when calling JSON.stringify():

> JSON.stringify({ foo: Object.create(null) })
'{"foo":{}}'

[vdeturckheim]

For context, here is a real life RCE in Kibana using prototype pollution: https://research.securitum.com/prototype-pollution-rce-kibana-cve-2019-7609/

[bmeck]

I know we have --frozen-intrinsics and --experimental-policies, perhaps we could make a more cohesive single set of defaults instead of a flag for each. This would be similar to the difficulty TypeScript faces with all its flags, and strict mode for TS opts into the desired but breaking defaults.

[kanongil]

I recently looked into the feasibility of doing a delete Object.prototype.__proto__ for my serverside projects. It can work, but it is definitely still in use. This was evidenced when I tried to benchmark if it caused any changes in performance, and the benchmark tool failed.

It's part of JS; I don't think node dot JS should ever be in the position of disabling parts of the language, even with a flag.

It's not really, though. At least, as it is currently specced in the optional for non-browsers section.

There are two parts to __proto__:

• Object.prototype.__proto__
• Object literals with __proto__

The second one is extremely popular for creating object literals with a null prototype, so I doubt we could disable it. I don't think the Object.prototype.__proto__ has anywhere near as much usage, but I would guess it is still a lot more than we could deal with.

The second form could still be valid, and is not a security concern, as you can't accidentally get user input into a literal.

[guybedford]

Note that the specific JSON issue quoted in the issue is blocked when enabling the --frozen-intrinsics flag, as it is not possible to write to any builtin __proto__ properties with this.

Edit: it's just --frozen-intrinsics

[Jack-Works]

It's part of JS; I don't think node dot JS should ever be in the position of disabling parts of the language, even with a flag.

(also most of these vulnerabilities are due to doing dangerous things with unsanitized user input, which is a bad practice that causes problems with or without __proto__)

It's not part of JS. __proto__ is defined in section B and Node.JS is not a browser so it's safe even __proto__ is not implemented🤔

When the ECMAScript host is a web browser the following additional properties of the standard built-in objects are defined. (section B)

[kanongil]

The --frozen-intrinsics flag doesn't really work. It does prevent the more serious issue, where the global prototype is modified. It does not prevent changing an existing object prototype through assignment to the obj.__proto__ property itself.

[ljharb]

In practice, most of Annex B is not really optional if you want to write or use portable code, but yes, that would be the loophole that would allow node to remove it, using the method described upthread.