987794 - (CVE-2014-8636) named getters can fool Xrays

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

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.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Shadowing an ES property on which?

The named accessor bits in HTMLDocumentBinding are conditioned on:

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

Comment 2

[Boris Zbarsky [:bzbarsky]]

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

Comment 3

[Boris Zbarsky [:bzbarsky]]

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");

Comment 4

[Bobby Holley (:bholley)]

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

Comment 5

[Bobby Holley (:bholley)]

(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.

Comment 6

[Bobby Holley (:bholley)]

Attachment: Tests. v1

Both "even after deleting it" tests fail.

Comment 7

[Bobby Holley (:bholley)]

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

Comment 8

[Bobby Holley (:bholley)]

Attachment: Tests. v2

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

Comment 9

[Bobby Holley (:bholley)]

I will fix this one the deps are fixed.

Comment 10

[Bobby Holley (:bholley)]

(note to self - local branch is named_getter_xray).

Comment 11

[Boris Zbarsky [:bzbarsky]]

> 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?

Comment 12

[Boris Zbarsky [:bzbarsky]]

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

Comment 13

[Bobby Holley (:bholley)]

(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

Comment 14

[Bobby Holley (:bholley)]

(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.

Comment 15

[Boris Zbarsky [:bzbarsky]]

> * 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?

Comment 16

[Bobby Holley (:bholley)]

(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.

Comment 17

[Boris Zbarsky [:bzbarsky]]

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?

Comment 18

[Bobby Holley (:bholley)]

(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.

Comment 19

[Boris Zbarsky [:bzbarsky]]

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?

Comment 20

[Bobby Holley (:bholley)]

(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.

Comment 21

[Boris Zbarsky [:bzbarsky]]

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.

Comment 22

[Bobby Holley (:bholley)]

(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.

Comment 23

[Boris Zbarsky [:bzbarsky]]

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.

Comment 24

[Bobby Holley (:bholley)]

(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.

Comment 25

[Bobby Holley (:bholley)]

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.

Comment 26

[Bobby Holley (:bholley)]

Attachment: Tests. v3

Comment 27

[Bobby Holley (:bholley)]

Attachment: Don't unwrap XrayWrappers in HasPropertyOnPrototype. v1

Comment 28

[Bobby Holley (:bholley)]

Attachment: Don't unwrap XrayWrappers in HasPropertyOnPrototype. v2

Comment 29

[Boris Zbarsky [:bzbarsky]]

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

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

Comment 30

[Boris Zbarsky [:bzbarsky]]

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

r=me

Comment 31

[Bobby Holley (:bholley)]

(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.

Comment 32

[Bobby Holley (:bholley)]

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

Comment 33

[Bobby Holley (:bholley)]

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

Comment 34

[Ed Morley [:emorley]]

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

Comment 35

[Daniel Veditz [:dveditz]]

This patch appears to have fixed sec-critical bug 1120261

Comment 40

[Bobby Holley (:bholley)]

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

Comment 42

[Bobby Holley (:bholley)]

(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.