Node: 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.

[devsnek]

to this point, TC39 is working on getting rid of annex b and putting all the things contained within (more or less) in the main body of the spec.

[ChALkeR]

@devnek what is the planned status of __proto__ in that spec? I'm guessing it would be required, but let's reconfirm =).

[bmeck]

@ChALkeR merge it but it remains able to be removed and virtually all hardening systems are expected to remove it.

I'd note just because something is in the spec, doesn't mean it needs to be preserved/encouraged in all scenarios. Sloppy mode for example is permanently in the spec, but not encouraged. Having a sane set of default hardening behaviors is prudent and is how a variety of systems work, see CSP limitations on eval etc.

[jasnell]

What I'd propose at this point is either:

(a) An initial experimental CLI flag for Node.js that disables __proto__ as suggested with the option of generating trace output when uses are caught (e.g. similar to --trace-sync).

(b) Development of a preload module that essentially does the same thing.

This would allow us to begin verifying what would break and help the ecosystem move things along.

My preference is for option (a). Once we collect information about what breaks, we can decide how to proceed on from there.

[addaleax]

I’m personally a fan of b) because I don’t quite see the need to do something in core.

[ChALkeR]

I think that (a) is better is a sense that it would make testing that easier for the ecosystem users.
We can label that as experimental (as in: can be reverted at any time). It would still make sense to revert only on major versions unless something huge happens though.

[kanongil]

For any fans of b), I decided to create a protofree module that can remove the __proto__ behavior through a simple preload. For good measure, I added 2 extra variants.

FYI, I found a use of __proto__ setter in the massively popular chalk library.

[bmeck]

@kanongil I've opened up a PR (it also removed a linting error to migrate that code)

[ChALkeR]

Babel at some previous versions apparently produced code which included:

if (superClass) subClass.__proto__ = superClass

(which is now changed to attempting setPrototypeOf first),
and many packages out there are processed with Babel (including those old versions of it).

This might be hard due to those.

[bmeck]

As a flag I do think this allows people to opt-in to breakage and shouldn't be thought of as breaking code without users expressing consent for the breakage.

[awwright]

Here's another one to add to the list:
https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2020-7598
https://snyk.io/vuln/SNYK-JS-MINIMIST-559764
I'd like to prohibit __proto__ (or at least its special behavior), I really think there should be an easy way to disable it.

[ljharb]

That particular CVE imo is nonsense; the vulnerability is that you can attack yourself on purpose.

[bmeck]

Deno unconditionally removes __proto__ as of https://github.com/denoland/deno/pull/4341

[addaleax]

@bmeck Deno is also not really concerned about full compatibility with Node.js, as I understand it?

[bmeck]

@addaleax correct, this is just noting a different environment doing something similar for similar reasons.

If an increased number of environments disable __proto__ it is less reliable for the JS ecosystem as a whole which is partially being discussed here, brings up a discussion about how much node doing this will break, and adds a discussion point of why the breakage was accepted by other environments.