Consider freezing Object.prototype for security reasons

[andvgal]

• Version: any
• Platform: any
• Subsystem:

THE PROPOSAL:

1. Add Node.js CLI option to control freezing of Object.prototype on startup
2. As possible, emit a deprecation warning whenever Object.prototype is modified.
3. Over time (e.g. Node.js v11+), enable freezing of Object.prototype by default.

JUSTIFICATION:

The issue formalizes proposal/discussion started here: https://www.reddit.com/r/node/comments/7y341t/quick_cve20183721_proto_from_jsonparse_mitigation/

• It's a known poor practice to modify Object.prototype in production code.
• There are known vulnerabilities related to overriding of __proto__ properties under some conditions.
  • There are also plenty of closed Node.js issues one or another way related to the problem.
• Packages which override toString(), valueOf() or other standard names require just minor modifications.
  • e.g. use ofObject.defineProperty(), assigning a new object to class .prototype with the key already defined or other variation.
• There are known popular libraries like should.js which will break:
  • users can fallback to old behavior through the command line option described above
  • users can migrate to expect/assert or other assertion interface
• As Node.js has already seen Promise-related enforcements, why not to do that for Object.prototype as well?

[devsnek]

related to #18795 and #18773 i guess

imo:
cli option to freeze builtins ... most likely never
default freezing buitins ... never

us caring about lodash having weird security issues ... meh, it isn't our job to take care of npm modules

this could also be an argument for exposing the primordials module from 18795 to user code although i'm kinda iffy on that

As Node.js has already seen Promise-related enforcements

i'm not aware of node enforcing the prototype of Promise, can you elaborate?

[vdeturckheim]

Hey @andvgal,

Thanks for opening this debate. This kind of issue seems to be happening a lot (see nodejs/security-wg#120). However I don't belive there is anything to do on Node.js side to handle it:

• I feel like this would go against ES specification.
• CVE-2018-3721 is, IMHO, very unlikely to be exploited.

When disclosing it, I have tried to assign it the lowest CVSS score.

Therefore, I'll tend to be -1 on this.

[andvgal]

Hi @vdeturckheim

If we are not yet aware how further prototype pollution can be exploited, it does not mean it is not a ticking bomb. I am sure, this vector will continue to be exploited.

When anything like that happens in critical software then fundamental measurements have to be taken in very defensive manner. As far as I can judge, it's far not the first case of __proto__ related problem.

Can you put your reputation on that prototype pollution related security issues never appear again? I doubt that. So, the question: what defensive actions you consider sufficient?

[devsnek]

we can't just randomly freeze prototypes. in fact many part of the spec are designed such that they can be overridden for the purpose of polyfilling. but i don't think thats the issue.

if someone has taken over your code by means of modifying global builtins you should be more careful about the code you run and the dependencies you choose. in this example, people were carelessly using lodash, which has nothing to do with ecmascript spec or node.js. i'm not a member of the security working group for node but i would put my reputation on "prototype pollution related security issues" not being anything worth worrying about.

[bnoordhuis]

To rephrase what @devsnek says, the threat model for Node.js does not include executing untrusted JS code. If your application is compromised to the point where that is possible, prototype pollution is the least of your worries.

Thanks for raising the issue but as there is zero chance of Node.js adopting this proposal, I'll go ahead and close it out.

[andvgal]

Polyfill is valid point. However, it mostly applies to Browser environment, but not Node.js. Responsible developer would update to new Node.js version instead of monkey patching the old one.

Also, I may be wrong, butObject.prototype itself is not so often extended with features suitable for polyfill.

My point for that remains:

Extension of Object.prototype is publicly accepted as bad practice. So, why not to enforce it unless user explicitly wants to go suspicious path.

We can start item 1 & 2 to see how it goes. At least, it should not harm anyone.
Item 3 cannot be decided without true outcome of 1 & 2.

[devsnek]

So, why not to enforce it unless user explicitly wants to go suspicious path.

node will judge you for performance or security, but not code practice. if i want to put all my code methods onto Object.prototype for some strange reason, node can and should allow that.

[andvgal]

Any reason Node.js to be terminated due to unhandled Promise rejection then? You are not consistent here.

The prototype pollution mistake can easily happen in absolutely any place of code if __proto__ key from untrusted data is not handled properly. It's NOT about compromised dependencies, it's about a common programming mistake.

Example:

// 1. Any manual for-in loop where standard deep-merge does not fit.
const data = JSON.parse( ... );
for ( let k in data ) { Object.assign( dst[k], data[k] ); }

// 2. Absolutely different place
const dynamic_flags = {}; // Yes, of course Map or Set would evade the problem

if (dynamic_flags[dynamic_flag]) {
   // trouble here
}

You ask how that can happen? C'mon, people still write ES5 compatible code...

[vdeturckheim]

If we are not yet aware how further prototype pollution can be exploited, it does not mean it is not a ticking bomb. I am sure, this vector will continue to be exploited.

Node.js is not supposed to prevent you from using standardized language features that can be harmful.

When anything like that happens in critical software then fundamental measurements have to be taken in very defensive manner. As far as I can judge, it's far not the first case of proto related problem.

Can you point me any past issue that is actually linked to Node.js and not to JS/ES itself?

Can you put your reputation on that prototype pollution related security issues never appear again? I doubt that. So, the question: what defensive actions you consider sufficient?

Thank you for caring about my reputation. I am sorry if you felt my answer was a bit cold, that was not my intention.

The prototype pollution mistake can easily happen in absolutely any place of code if proto key from untrusted data is not handled properly. It's NOT about compromised dependencies, it's about a common programming mistake.

All examples of exploitation I have seen could have been handled through data validation which is IMHO the first defensive action anyone should take when building a Node.js application (or any application FWIW)

If you want to ensure that prototype pollution is impossible in your application, I suggest you enforce it yourself.

Get me right, I am not trying to say that prototype pollution is not an issue. I was the person who acknowledged this as a security issue in behalf of the ecosystem triage team. I just consider that usual remediations techniques are more than enough to prevent this kind of exploitation. Should I be proven wrong, I would happily change my mind.

[hashseed]

Violating the spec is not an option imo.