Bobby Holley (:bholley) Assignee
	Description • 10 years ago

I was talking with jorendorff, and started getting worried about DOM objects with named accessors fooling the XrayWrapper by either:

(a) shadowing a native property (i.e. getElementById)
or
(b) shadowing an ES property.

We basically had this issue for cross-origin windows in bug 860494.

I'm getting a bug on file here so that I can write some tests, then we can assess whether it's actually a problem or not.

	Boris Zbarsky [:bzbarsky]
	Comment 1 • 10 years ago

Shadowing an ES property on which?

The named accessor bits in HTMLDocumentBinding are conditioned on:

  if (!(flags & JSRESOLVE_ASSIGNING) && (!isXray || !HasPropertyOnPrototype(cx, proxy, id))) {

	Boris Zbarsky [:bzbarsky]
	Comment 2 • 10 years ago

So over Xrays, all DOM proxies act like ones that don't have [OverrideBuiltins], basically.

	Boris Zbarsky [:bzbarsky]
	Comment 3 • 10 years ago

Oh, and that code was in getOwnPropertyDescriptor.  The other key part is this bit in get():

  MOZ_ASSERT(!xpc::WrapperFactory::IsXrayWrapper(proxy),
              "Should not have a XrayWrapper here");

	Bobby Holley (:bholley) Assignee
	Comment 4 • 10 years ago

Ok, this sounds pretty sane then. I'll work up a mochitest after lunch just to make sure.

	Bobby Holley (:bholley) Assignee
	Comment 5 • 10 years ago

(In reply to Boris Zbarsky [:bz] from comment #1)
> Shadowing an ES property on which?
> 
> The named accessor bits in HTMLDocumentBinding are conditioned on:
> 
>   if (!(flags & JSRESOLVE_ASSIGNING) && (!isXray ||
> !HasPropertyOnPrototype(cx, proxy, id))) {

So, thinking about this, this security mechanism doesn't actually work, because content can fool it by simply deleting properties. I'll attach the mochitest-chrome test I was writing up.

	Bobby Holley (:bholley) Assignee
	Comment 6 • 10 years ago

Both "even after deleting it" tests fail.

	Bobby Holley (:bholley) Assignee
	Comment 7 • 10 years ago

I think we basically need meaningful DOM Xray prototypes to implement this security check properly. Setting the dep.

	Bobby Holley (:bholley) Assignee
	Comment 8 • 10 years ago

Including another failing test, to make sure that we hook up DOM Xray
prototypes to the JSXrayed window[0].Object.prototype.

	Bobby Holley (:bholley) Assignee
	Comment 9 • 10 years ago

I will fix this one the deps are fixed.

	Bobby Holley (:bholley) Assignee
	Comment 10 • 10 years ago

(note to self - local branch is named_getter_xray).

	Boris Zbarsky [:bzbarsky]
	Comment 11 • 10 years ago

> because content can fool it by simply deleting properties.

Ugh.  We should presumably be walking the Xrays of the proto chain (or the proto chain of the Xrays?) instead?

	Boris Zbarsky [:bzbarsky]
	Comment 12 • 10 years ago

As in, do we need a more short-term fix that we can backport too?

	Bobby Holley (:bholley) Assignee
	Comment 13 • 10 years ago

(In reply to Boris Zbarsky [:bz] from comment #11)
> Ugh.  We should presumably be walking the Xrays of the proto chain (or the
> proto chain of the Xrays?) instead?

Right.

(In reply to Boris Zbarsky [:bz] from comment #12)
> As in, do we need a more short-term fix that we can backport too?

Hm. I'd initially thought that we needed bug 787070 for this, but maybe we don't really. For WebIDL Xrays, we have a proper prototype chain, right? So I guess we can fix this properly when we have both of the following:

* Window WebIDL (assuming there are no other significant named getters on XPCWN bindings)
* Xrays to Object.prototype (bug 987111).

Those should both be done this cycle, which means that5

	Bobby Holley (:bholley) Assignee
	Comment 14 • 10 years ago

(In reply to Bobby Holley (:bholley) from comment #13)
> Those should both be done this cycle, which means that

(Accidentally hit enter)

which means that the fix here can ideally land on ESR31. Given that it's sec-moderate, I don't know if we should worry too much about the other stuff.

	Boris Zbarsky [:bzbarsky]
	Comment 15 • 10 years ago

> * Window WebIDL

Window's named props over an Xray are already safe, since they don't use the generated handler (because there is none; Window is not a proxy binding) or the classinfo bits.  Fo a non-Xray they're handled by the GSP; for an Xray they're done directly in the Xray code, after resolveNativeProperty, so can't shadow it.

> (assuming there are no other significant named getters on XPCWN bindings)

There is a named getter on nsStorage2SH, but it never resolves props if ObjectIsNativeWrapper(cx, obj).

That's the only named getter left on XPCWN bindings.

> * Xrays to Object.prototype (bug 987111).

Why is this relevant?  If we just walk the proto chain of the Xray, we'll make sure we don't shadow any of the standard Object.prototype props, no?

	Bobby Holley (:bholley) Assignee
	Comment 16 • 10 years ago

(In reply to Boris Zbarsky [:bz] from comment #15)
> Why is this relevant?  If we just walk the proto chain of the Xray, we'll
> make sure we don't shadow any of the standard Object.prototype props, no?

The Xray chain for DOM Xrays leads to contentWindow.Object.prototype, which is currently a transparent wrapper from the Xray side.

	Boris Zbarsky [:bzbarsky]
	Comment 17 • 10 years ago

Ah, so content could still shadow the things that normally live on Object.prototype?

But it can do that without named getters too, just by modifying the values of those props, no?

	Bobby Holley (:bholley) Assignee
	Comment 18 • 10 years ago

(In reply to Boris Zbarsky [:bz] from comment #17)
> Ah, so content could still shadow the things that normally live on
> Object.prototype?

By deleting properties to defeat the shadowing detection, yes.

> But it can do that without named getters too, just by modifying the values
> of those props, no?

No, because without bug 787070, we don't actually consider the prototype without resolving Xray properties. But that's a good reason for bug 787070 to block on Object Xrays, actually. /me marks that.

	Boris Zbarsky [:bzbarsky]
	Comment 19 • 10 years ago

I'm not following.  If I do .toString on an Xray for an HTMLDivElement, say, where is that toString function found right now?  Can content affect the function that's found?

	Bobby Holley (:bholley) Assignee
	Comment 20 • 10 years ago

(In reply to Boris Zbarsky [:bz] from comment #19)
> I'm not following.  If I do .toString on an Xray for an HTMLDivElement, say,
> where is that toString function found right now?  Can content affect the
> function that's found?

Currently, toString is handled as a special-case by Xrays. Other things from Object.prototype (like hasOwnProperty) just resolve to undefined. This will change with bug 787070 (when we start considering the Xray's prototype chain for lookups). So we need Xrays to contentWindow.Object.prototype before we do that.

	Boris Zbarsky [:bzbarsky]
	Comment 21 • 10 years ago

Ah, I see.

So another option, btw, is to just make HasPropertyOnPrototype hardcode-return true for the list of built-in Object.prototype properties that we have floating around anyway.

	Bobby Holley (:bholley) Assignee
	Comment 22 • 10 years ago

(In reply to Boris Zbarsky [:bz] from comment #21)
> Ah, I see.
> 
> So another option, btw, is to just make HasPropertyOnPrototype
> hardcode-return true for the list of built-in Object.prototype properties
> that we have floating around anyway.

Yes. Though note that the opposite attack still exists - Xrays can be fooled into thinking that a named property isn't there, by having content add arbitrary properties to Object.prototype. This is probably much less problematic FWIW.

I'm going to focus my energy on bug 987111, which I hope to get done this cycle. I'd happily review a patch here in the meantime if you want to write it, though.

	Boris Zbarsky [:bzbarsky]
	Comment 23 • 10 years ago

Right, content can make named props go away just by changing named/ids.

I don't think it's worth doing something here unless we plan to backport, if you're planning to finish 987111 anyway.

	Bobby Holley (:bholley) Assignee
	Comment 24 • 10 years ago

(In reply to Boris Zbarsky [:bz] from comment #1)
> Shadowing an ES property on which?
> 
> The named accessor bits in HTMLDocumentBinding are conditioned on:
> 
>   if (!(flags & JSRESOLVE_ASSIGNING) && (!isXray ||
> !HasPropertyOnPrototype(cx, proxy, id))) {

This actually doesn't work, because HasPropertyOnPrototype unwraps XrayWrappers. :-(

I'll try removing that and see what breaks.

	Bobby Holley (:bholley) Assignee
	Comment 25 • 10 years ago

Hm. Actually that still isn't enough, because without bug 787070, we're not guaranteed a sane prototype chain. Even though we Xray to Document.prototype, we only see that on the proto chain if content hasn't screwed up the prototype in some other way.

So yeah, we really need bug 787070 here.

	Boris Zbarsky [:bzbarsky]
	Comment 29 • 10 years ago

Comment on attachment 8442548 [details] [diff] [review]
Tests. v3

r=me.  I assume we fail some of these on tip, right?

	Boris Zbarsky [:bzbarsky]
	Comment 30 • 10 years ago

Comment on attachment 8498082 [details] [diff] [review]
Don't unwrap XrayWrappers in HasPropertyOnPrototype. v2

r=me

	Bobby Holley (:bholley) Assignee
	Comment 31 • 10 years ago

(In reply to Boris Zbarsky [:bz] from comment #29)
> Comment on attachment 8442548 [details] [diff] [review]
> Tests. v3
> 
> r=me.  I assume we fail some of these on tip, right?

Correct.

	Bobby Holley (:bholley) Assignee
	Comment 32 • 10 years ago

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=00a92f4b43cf

	Bobby Holley (:bholley) Assignee
	Comment 33 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/74871c4df0a7

	Ed Morley [:emorley]
	Comment 34 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/74871c4df0a7

	Daniel Veditz [:dveditz]
	Comment 35 • 10 years ago

This patch appears to have fixed sec-critical bug 1120261

	Bobby Holley (:bholley) Assignee
	Comment 36 • 10 years ago

Given the involvement of bug 1120261, we need to fix this on esr31. I have patches.

	Bobby Holley (:bholley) Assignee
	Comment 37 • 10 years ago

(In reply to Bobby Holley (Busy with media, don't ask for DOM/JS/XPConnect things) from comment #40)
> Given the involvement of bug 1120261, we need to fix this on esr31. I have
> patches.

Actually, I don't think we can fix this bug as-filed on ESR31. I've filed bug 1125015 to handle the issue relevant to bug 1120261.