Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

suggestion: make Object.prototype exotically reject new properties #1

Open
bakkot opened this issue Nov 28, 2022 · 19 comments
Open

suggestion: make Object.prototype exotically reject new properties #1

bakkot opened this issue Nov 28, 2022 · 19 comments

Comments

@bakkot
Copy link

bakkot commented Nov 28, 2022

You can't freeze Object.prototype without triggering the override mistake. But you could, in principle, make Object.prototype an exotic object whose [[Set]] fails (or, I guess, throws).

(Maybe also make Array.prototype an exotic whose [[Set]] fails for valid array indices.)

It's possible that would be viable without a new mode, so it's a lot more palatable to me than the ideas here. Would that solve enough of the problem, do you figure?

@ljharb
Copy link
Member

ljharb commented Nov 28, 2022

This would certainly break a number of my packages' test suites, but I suspect it would be unlikely to break actual deployed code.

@syg
Copy link
Collaborator

syg commented Nov 28, 2022

I guess the assumption here is that people don't add polyfills to Object.prototype? My gut reaction is that that seems like a smaller scope and also riskier move to make (since there's no opt-in). It's smaller scope in that the proposed different mode solution aims to prevent all prototype "spooky action at a distance" pollution, even for user prototypes. There is desire to prevent that action at a distance even for user prototypes, but I suspect preventing Object.prototype does go a long way.

My take here is that if we have appetite for backwards breaking opt-in modes that we have some ideas for how to ease the adoption of (see this section), we should try to get the most bang for our buck.

@bakkot Can you say more about why the opt-in mode is a lot less palatable?

Also cc @salcho for thoughts and data points from his experience trying to mitigate this stuff.

@bakkot
Copy link
Author

bakkot commented Nov 28, 2022

I guess the assumption here is that people don't add polyfills to Object.prototype?

Yup. We haven't added a new property to Object.prototype since ES5, and I expect (/sincerely hope) we never will, so there's not much need for polyfills. (I'm assuming that the exotic [[Set]] would still allow you to replace existing properties so that you can patch Object.prototype.toString etc.)

My take here is that if we have appetite for backwards breaking opt-in modes that we have some ideas for how to ease the adoption of (see this section), we should try to get the most bang for our buck.

Agreed; my suggestion would only be an improvement over the alternatives in this proposal if we could do it unconditionally instead of needing an opt-in for another route.

Can you say more about why the opt-in mode is a lot less palatable?

New modes which change fundamental semantics are a massive burden for learners, tooling, and libraries which try to be mode-agnostic. Also, I doubt this proposal's proposed modes would see widespread adoption, so like Symbol.species it would be another one of those "this rarely-used feature can be used to violate underlying assumptions" things which so often lead to bugs and vulnerabilities.

@syg
Copy link
Collaborator

syg commented Nov 29, 2022

New modes which change fundamental semantics are a massive burden for learners, tooling, and libraries which try to be mode-agnostic. Also, I doubt this proposal's proposed modes would see widespread adoption, so like Symbol.species it would be another one of those "this rarely-used feature can be used to violate underlying assumptions" things which so often lead to bugs and vulnerabilities.

Part of the hope is an out-of-band opt-in, perhaps via CSP or something similar, coupled with automatic rewriting by the JS engine's parser that doesn't require any code changes in the common case (that is, in the case where your code doesn't depend on having computed property access to a prototype, and only use .prototype and reflection APIs).

As for violating the underlying assumptions in the engines thing: great point and agreed on the risk. I think how we'd implement this is that the prototype properties would always be refactored to be these special symbols or only as internal slots only reachable via new reflection APIs, and whether string keys get magically aliased to them would be implemented on top depends on the mode. It's still messy, but more enumerable than Symbol.species. This consideration probably also plays into whether, in the current design, you want to cut off access to prototypes via "constructor" or via "prototype". There are a lot fewer places of Get(obj, "prototype") in the spec, and I suspect in implementations.

@bakkot
Copy link
Author

bakkot commented Nov 29, 2022

that is, in the case where your code doesn't depend on having computed property access to a prototype

I thought this was going to be impractical because of minifiers; I was expecting to find that minifiers would replace

x.prototype.f1 = 0;
x.prototype.f2 = 0;
x.prototype.f3 = 0;
x.prototype.f4 = 0;

with

let _t = 'prototype';
x[_t].f1 = 0;
x[_t].f2 = 0;
x[_t].f3 = 0;
x[_t].f4 = 0;

But on actually checking, apparently they don't. (Though I've certainly done that transformation manually, so it's not like it never happens.) So maybe?

Still, having to learn that computed property access is not the same as normal property access when the key is specifically "prototype" is going to be a lot of pain.

@syg
Copy link
Collaborator

syg commented Nov 29, 2022

Still, having to learn that computed property access is not the same as normal property access when the key is specifically "prototype" is going to be a lot of pain.

Yeah, in general breaking property access symmetry is really terrible, and it is a downside of the automatic rewriting approach. In this case the contention is that the payoff is worth it for moving the needle on this attack vector. And the hope is that people don't really need to learn this in practice. Not saying that nobody out there will hit the asymmetry, but that it'll both be rare and should be cause for pause. Out of the code that internal folks have audited, IIRC getting to the prototype via computed property access is always unintended.

@bakkot
Copy link
Author

bakkot commented Nov 29, 2022

Out of the code that internal folks have audited, IIRC getting to the prototype via computed property access is always unintended.

Keep in mind that it's not just the input code that matters; you've got to look code which is produced by tooling as well. Minifiers apparently aren't relevant, but they're far from the only tools - for example, one of our internal tools would in fact translate x.prototype into x[z] in many cases.

@ljharb
Copy link
Member

ljharb commented Nov 29, 2022

Doesn't Google Closure Compiler prefer x['prototype'] over x.prototype in a general sense?

@salcho
Copy link
Collaborator

salcho commented Nov 29, 2022

You can't freeze Object.prototype without triggering the override mistake. But you could, in principle, make Object.prototype an exotic object whose [[Set]] fails (or, I guess, throws).

This would make freeze a lot more usable, but developers will still run into the problem of knowing when to run the freeze. There often isn't an obvious point at runtime where prototypes have settled, which is somewhere after polyfills + prototype-modifying framework code finishes running (think libraries that hot swap code in dev mode) but before application code starts processing input. Even if you were to get this right, future code changes are likely to move this boundary up and down. I suspect freeze will still see low adoption because of this fiddly context-dependent restriction.

Keep in mind that it's not just the input code that matters; you've got to look code which is produced by tooling as well.

Yes, this is a good point that could weaken automatic refactoring in the browser. FWIW, we did a small study on code that goes through our toolchain (compilation, bundling, minification, etc) looking for breaking transformations and got optimistic results that suggest that these are likely not very common. The handful of occurrences we found were caused by constant folding (var p = "__proto__"; obj[p]), which AFAIU seem tractable from a lexer/parser's point of view.

And the hope is that people don't really need to learn this in practice.

Just a +1 to this point. Our preference would be on landing a mechanism that is transparent to developers and automatic refactoring seems like a good vehicle for that.

@bakkot
Copy link
Author

bakkot commented Nov 29, 2022

This would make freeze a lot more usable, but developers will still run into the problem of knowing when to run the freeze.

Sorry, I think you've misunderstood the suggestion. I'm not suggesting to make a new kind of freezing available and asking developers to figure out when use it. I'm suggesting we unconditionally change the language so that code can't add new properties to Object.prototype.

FWIW, we did a small study on code that goes through our toolchain (compilation, bundling, minification, etc) looking for breaking transformations and got optimistic results that suggest that these are likely not very common.

Looking within a single company isn't very useful, because that means you're only looking at a single toolchain. You'd really need to analyze different toolchains to see the outputs. (And as I mentioned, our toolchain does end up outputting uses of "prototype" and "constructor" as a computed key; more generally it can end up translating any static property access to a computed property access.)

@zloirock
Copy link

Good luck.

image

@syg
Copy link
Collaborator

syg commented Nov 29, 2022

The toolchain issue seems like a dealbreaker only in the context of non-opt-in. An opt-in is a pretty big hammer; again, if palatable, I imagine toolchains would update to be aware of this asymmetry if the tradeoff is deemed acceptable.

@bakkot
Copy link
Author

bakkot commented Nov 29, 2022

@zloirock What's that from?

@zloirock
Copy link

@bakkot ancient core-js versions, now it's just 0.1% of downloads - but even this is about 250k / month. This is just the first example that came to mind.

@bakkot
Copy link
Author

bakkot commented Nov 29, 2022

@zloirock and what is the value of the _ binding in that screenshot? Is it a string or a symbol? If it's a symbol we could have Object.prototype only reject string-named properties and be fine.

Either way though it would only break if people were actually using the property, which is likely to be rare. (Assuming the [[Set]] fails silently rather than throwing.)

@zloirock
Copy link

@bakkot it's an object convertible to a randomly generated string for avoiding name collisions - it was before symbols. And I saw a similar approach in some other libraries.

@salcho
Copy link
Collaborator

salcho commented Nov 30, 2022

I'm suggesting we unconditionally change the language so that code can't add new properties to Object.prototype.

Ah, sorry for the confusion and thanks for the clarification. In that case I'd only echo @syg comments: this suggestion would have a narrower scope (it protects only the global prototype and not intermediate/application-defined prototypes) and works out of the box for applications that don't use polyfills or add properties to Object.prototype.

I think this is a strong solution, both in terms of security and (from experimental data) number of compatible codebases, but I'm afraid it doesn't fully engineer away this class of bugs and is still backwards breaking, so it seems like we should exhaust all options that can give us full security coverage before falling back to Object.prototype only.

As a side thought, I wonder if we could reduce the complexity introduced by the new symbols in this proposal by instead making all prototypes be exotic (with a failing/throwing [[Set]]), unless it's called from a set of reflection APIs, e.g. setPrototypeOf.

@ljharb
Copy link
Member

ljharb commented Nov 30, 2022

Every object is a potential prototype, so I'm not sure how that would work.

@salcho
Copy link
Collaborator

salcho commented Mar 17, 2023

Hey everyone, we've done some work in the last few weeks to gather data on how likely it is that a given toolchain will produce code that is incompatible with secure mode. I've committed a new version of the proposal to reflect that, please take a look again!

Kevin's original proposal on this issue continues being feasible if we limit ourselves to solving 'global prototype pollution' (pollution of top level prototypes only), but leaves all intermediate prototypes unprotected. Of all pollution vulnerabilities we've seen at Google, only one relied on polluting intermediate prototypes and the functions responsible for the vulnerability almost always dealt with raw objects (as opposed to application types), which suggests that protecting top-level prototypes only will go a very long way.

To call out explicitly what limiting this proposal to top-level prototypes means: we run the risk of protecting an attack technique that is popular today (because it is the shortest path to exploitation), but that might evolve in the future. If that risk materialises, a subset of all vulnerable codebases nowadays will continue to be vulnerable.

It would be very valuable to hear feedback on 1) the updated content of the proposal, now with some backing data and 2) where delegates stand in terms of limiting this proposal to top-level prototypes, if the suggested 'secure mode' is not palatable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants