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

How should browsers mitigate Proxy-related security issues? #272

Closed
littledan opened this issue Dec 21, 2015 · 18 comments
Closed

How should browsers mitigate Proxy-related security issues? #272

littledan opened this issue Dec 21, 2015 · 18 comments

Comments

@littledan
Copy link
Member

There's a security issue on the web with ES2015 Proxies where a cross-origin request can be made to load some ECMAScript code, and this request can leak some information across origins due to the existence of Proxies. Details can be found here: https://code.google.com/p/chromium/issues/detail?id=399951 . The short story is that the Proxy can be put in the prototype chain of the global object, and then it's possible to see an identifier-like thing at the beginning of the target page. Making cross-origin requests with non-cooperating sites (by inserting script tags) can leak information which ambient authority signals (like the IP address) grant access to. Telemetry data shows, unfortunately, that it's not web-compatible to lock down mimetype checking for this purpose as I was hoping.

There are a few possible fixes. All require changes to HTML, but they also require deep hooks into ECMAScript.

  1. We could allow an embedding environment to freeze the prototype chain of the global object. This means, on the web, Object.prototype would have a frozen prototype. Currently, the only way to make the prototype of an ordinary object not writable is to call [[PreventExtensions]] on it. (It'd be fine for the exotic window object to have special behavior for [[SetPrototype]]. So the web could either [[PreventExtensions]] on Object.prototype (but this could break the web), or we could add an internal slot in objects which make their prototype specifically frozen, and set that slot to true from HTML (this would probably also be used for objects like window). Firefox is going down this route.
  2. We could disallow property reads from the global object which result in reading from a Proxy at some point in the read. I think Microsoft mentioned that they have a non-web embedding environment where they like to set a Proxy as the global object, so the check would probably have to be a switch that the embedding environment would turn on. This idea came from Toon Verwaest in V8, and we currently have it turned on when users enable the "advanced JavaScript features" flag, to protect early Proxy users. However, we do not plan to ship this yet; we'd rather ship a standardized security policy.
  3. We could specify, in HTML, that if the global object has a Proxy in its prototype chain, then ECMAScript which was loaded from another origin should not be run. To avoid race conditions, the check has to be evaluated immediately before executing the ECMAScript, not before loading it. I think this is a rather fragile and difficult kind of security protection compared to the first two; it just feels ugly to me, the way that there would be this one compound check, and timing bugs could cause security holes.
  4. Edge might not encounter this issue due to some kind of phishing protection. Whatever that is could be standardized.

I like the first two options the best, but I'm really open to any of them, or any additional ideas that people here might have. I'd really like guidance from this committee to move towards one of these, given the tight coupling all of these fixes have with ECMAScript layering.

@claudepache
Copy link
Contributor

I find that the Firefox approach (the first one listed) is easier to reason about. Also, the failure happens at writing time rather than reading time. (BTW, nonextensibility and prototype-immutability happen to be bound today, but it wasn't always the case: before __proto__ was invented, objects were generally both extensible and prototype-immutable.)

@bterlson
Copy link
Member

I think we should try to limit the impact on ECMAScript as much as possible. It would be sad if this issue ended up having wide impacts on ECMAScript when there is no problem for ECMAScript environments that are not the browser.

I personally like the FF approach best too. Assuming it's web compatible, the easiest way to spec is probably to teach ES the difference between non-extensible and proto-frozen. This feature, done right, would require its own proposal as it would likely want to include reflective capabilities. However, in the short term, we can add non-observable machinery for it (I guess a new MOP flag and corresponding updates to 9.1.2 [[SetPrototypeOf]] for ordinary objects).

@annevk
Copy link
Member

annevk commented Dec 29, 2015

I think @bzbarsky and @bholley figured out a way to do this without changes to ECMAScript. But thus far we only discussed the Location object in detail.

@domenic
Copy link
Member

domenic commented Dec 29, 2015

@annevk I am not sure but it seems like you are talking about a completely different thing than what this issue is discussing. For example there are no cross-origin objects involved here, only cross-origin script loads.

@jswalden
Copy link
Contributor

Firefox has shipped fix 1 in nightly builds for several months, Aurora builds for a couple months, and beta builds for a couple weeks. Come January 26, it'll be shipping in release builds. Early to mid-February I'm going to write up a proposal and spec language for what I implemented (or very nearly -- there are one or two expedient fillips I'd change before standardizing).

To briefly sketch the idea. I added a new [[SetImmutablePrototype]] MOP method. Calling the method returns true/false, indicating whether the object's [[Prototype]] is now immutable -- or throwing in cases where the operation doesn't make sense (like on revoked proxies, or on proxies that decided making their prototype immutable doesn't make sense, or in case of internal error). The operation can be invoked by the user using Reflect.setImmutablePrototype (not implemented yet -- we added a testing-only setImmutablePrototype function instead, so as not to add a method sites might start using in advance of standardization). [[SetImmutablePrototype]] isn't currently implemented in Firefox as something user prototypes can reimplement, but that's only because the mechanism's future was uncertain, so why bother spending time on that yet. :-) (Defining semantics for such is near-trivial in any event.) On regular old objects, we store an extra bit indicating whether the prototype is immutable. And of course, in [[SetPrototypeOf]] we consult this bit and return false if the bit is set.

One tweak we don't implement yet, is making [[PreventExtensions]] tail-call [[SetImmutablePrototype]] to finish its work. That and making [[SetImmutablePrototype]] implementable on scripted proxies are the only things I can think of (off the top of my head) that I would additionally propose/standardize but haven't yet implemented.

(For completeness: I would be remiss in noting that all of this is really dancing around the issue, that cross-origin script loads ignore MIME type to run the resulting "script" no matter what. It's beyond TC39's ken, but really we have to prohibit mistyped cross-origin script loads to fully fix this. Doable now? Probably not. But neither was mixed-content blocking. And this is exactly akin to that -- moreso, even, because running someone else's code is much worse than displaying an untrustworthy image. We'll just have to deprecate, announce, ship warning messages, ship harsher messages, etc. until we can fully prohibit.)

@allenwb
Copy link
Member

allenwb commented Dec 30, 2015

Extending the MOP is a high visibility/high impact change and something that should be casually done. Plus, I don't see why a new MOP operations is necessary (or desirable) in order to implement fix 1.

If the HTML provided global object and all objects on its initial prototype chain (including %ObjectPrototype%) are considered to be exotic objects, then their [[SetPrototypeOf]] behavior can be to always return false on attempts to modify their current [[Prototype]] value. It would still be possible for such objects to have a back-channel way to initially set or even to dynamically modify their [[Prototype]] value. But that back channel is part of the implementation (or HTML spec.) rather than part of the ES MOP.

The only thing I see in the current ES spec. that stands in the way of that is the requirement in 19.1.3 that: "The Object prototype object is an ordinary object." That requirement could be easily deleted or replaced with a statement like: "It is implementation defined whether the Object prototype object is an ordinary object or an exotic object."

Some other points:

The current ES spec. currently does not require that the global object's prototype chain include %ObjectPrototype%. So, strictly speaking any code that assumes that it does is not guaranteed to be interoperable across standards-conforming ES implementations. I believe that we left it that way when working on the ES5 spec. because IE's global object did not inherit from %ObjectPrototype%. Perhaps it is now time to add to the ES spec. the requirement that the global object inherits form %ObjectPrototype%.

Is it ever desirable to allow user code to dynamically change the value of %ObjectPrototype%'s [Prototype]]? I dubious that it is. Rather than making [[Prototype]] mutability of %ObjectPrototype% implementation defined we could define a new kind of standard exotic object with an immutable [[Prototype]] and make %ObjectPrototype% that kind of object.

@domenic
Copy link
Member

domenic commented Dec 30, 2015

Rather than making [[Prototype]] mutability of %ObjectPrototype% implementation defined we could define a new kind of standard exotic object with an immutable [[Prototype]] and make %ObjectPrototype% that kind of object.

I strongly support this. I agree that it's probably a good thing to disallow in general. And furthermore I think that as a general guideline, basic things like Object.prototype should behave the same across all environments as much as possible. Making browsers the only ones with this behavior would be a bit sad IMO.

@littledan
Copy link
Member Author

We can make %ObjectPrototype% immutable across all environments while at the same time adding a new MOP operation. @allenwb 's suggestion sounds pretty readily implementable, and it'd fix the security issue, but it also sounds like a one-off exception that makes things more complex with funny edge cases, rather than more regular and predictable. I thought we wanted to move away from explaining builtins as special and towards general, regular behavior when possible.

@allenwb
Copy link
Member

allenwb commented Dec 31, 2015

@littledan One of the primary motivations for the MOP (and its reifaction via Proxy) was to provide a way to explain special case behavior of built-ins and some host objects. This is perfect example of where the current MOP can do exactly that. An object with an immutable prototype reference is simply one where its [[GetPrototypeOf]] internal method always returns the same value and where its [[SetPrototypeOf]] always returns false (except when the value passed is the current value).

We want the MOP to be as narrow and simple as possible. The MOP should only consist of the interfaces that are necessary to support the actual semantics of language features and built-in functions.

So far, in this thread, I haven't seen anything that requires (or motivates) the introduction of a new language feature or built-in. [[GetPrototypeOf]] and [[SetPrototypeOf]] are already sufficient to support the use case discussed here

If you really want to add a [[FreezePrototypeOf]] MOP operation you should start by justifying the addition of an Object.freezePrototypeOf function. In particular, you need to present compelling use cases for why code (probably library code) may need to freeze the prototype reference of arbitrary objects. There may well be a use for such an operation but this the situation addressed in this thread doesn't require it.

@littledan
Copy link
Member Author

@jswalden Have you written any spec text for this idea, or for the simpler idea of just making Object.prototype exotic?

littledan added a commit to littledan/ecma262 that referenced this issue Jan 21, 2016
This patch builds a mechanism to fix the Proxy security issue documented
in bug tc39#272 by locking down the prototype chain of the global object, as
Firefox has experimented with. Although the global object is provided by
the embedding environment, many embedding environments will include Object
in the prototype chain; preventing modification of Object.prototype
addresses the issue by making it impossible to insert a Proxy in that part
of the prototype chain of the global object. Embedding environments that
want to prohibit a Proxy from being in the proto chain of their global
object can make their global object and associated proto chain Immutable
Prototype exotic objects.
littledan added a commit to littledan/ecma262 that referenced this issue Jan 22, 2016
This patch builds a mechanism to fix the Proxy security issue documented
in bug tc39#272 by locking down the prototype chain of the global object, as
Firefox has experimented with. Although the global object is provided by
the embedding environment, many embedding environments will include Object
in the prototype chain; preventing modification of Object.prototype
addresses the issue by making it impossible to insert a Proxy in that part
of the prototype chain of the global object. Embedding environments that
want to prohibit a Proxy from being in the proto chain of their global
object can make their global object and associated proto chain Immutable
Prototype exotic objects.
@jswalden
Copy link
Contributor

While extending the MOP is indeed momentous, I think it's a very reasonable choice here. Conflating extensibility with prototype-mutability was always a dodge, done that way simply because people wanted to be able to depend on prototype identity being constant, and that was the only method at hand whose semantics could be plausibly munged to do it. Splitting the two operations (or rather, having [[PreventExtensions]] delegate prototype-immutability to [[SetImmutablePrototype]]) is perfectly reasonable, to produce objects with normal property behavior but a [[Prototype]] that can be depended upon. I don't think it's adequate to say, "Just override [[SetPrototypeOf]] to throw", because that can only be done using new objects that are proxies: there's no way to perform the operation on a perfectly-suitable existing object, as one might very reasonably want to do inside a constructor function:

function Point(x, y)
{
  this.x = x;
  this.y = y;

  // natural, consistent w/other MOP behavior twiddling on objects
  Object.setImmutablePrototype(this);
}

function DumbPoint(x, y)
{
  this.x = x;
  this.y = y;

  // ...really?  Extra object, extra overhead to use (in the very real case of
  // JITs being unable to boil away overhead), less scrutable in debuggers, etc.
  return new Proxy(this, { setPrototypeOf() { return false; } });
}

@littledan I haven't written up anything yet, either proposal-language or spec-language, no.

@allenwb
Copy link
Member

allenwb commented Jan 22, 2016

@jswalden I'm not taking a position on whether or not there is sufficient utility of aObject.setImmutablePrototype function (regardless of its actual name). I'm just same that it isn't needed to address this issue.

I suggest that we here we stick with whether we want %ObjectPrototype% to have an immutable [[Prototype]] and assuming so, specifying it in the least intrusive why current at our disposal.

You or anyone else should feel free to separately propose a more general mechanism for reflectively controlling prototype mutability.

@littledan
Copy link
Member Author

@jswalden I'm definitely sympathetic here. Setting an immutable prototype could be a good tool for library authors, and is definitely easier for implementations to take advantage of than Proxy-based prototype immutability.

I'd also really like to figure out what our strategy should be for Proxy security to make sure we're not digging ourselves into a hole by releasing something that will have to be locked down somehow later. Do you think you'd be able to come to the TC39 meeting next week to discuss the issue? I put it on the agenda.

@domenic
Copy link
Member

domenic commented Jan 23, 2016

It seems like @littledan's #308 could end up being a subset of a more general set-immutable-prototype hook? That is, if we were to accept #308 for now, and work through a proposal for set-immutable-prototype later, we'd be able to rephrase the definition of Object.prototype from "immutable prototype exotic object" to an ordinary object that has been SetImmutablePrototype-ified, with no change in observable semantics.

@allenwb
Copy link
Member

allenwb commented Jan 23, 2016

@domenic exactly. If we want a quick fix, the exotic object is the way to go for now.

@jswalden
Copy link
Contributor

@littledan I could come, certainly. Ideally not every day, if there are only a couple proposals I could meaningfully comment on, tho -- any idea what's happening when, or should I just say any day works on the registration thing and wing it?

@littledan
Copy link
Member Author

@jswalden "Time constraint: Wednesday only" was written on the agenda in tc39/agendas@e57568f . Does that work for you (maybe it was written with you in mind, just wanted to confirm)?

@jswalden
Copy link
Contributor

@littledan Yep, that was me in mind.

(Technically I'm available any day, but with few proposals I'd meaningfully comment on, it seemed best for productivity to minimize attendance. Wednesday was the day arbitrarily selected for that purpose. In the unlikely event a scheduling issue arises, Tuesday/Thursday would also do.)

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