Bug 987794 (CVE-2014-8636)

named getters can fool Xrays

RESOLVED FIXED in Firefox 35

Status

()

defect
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

({sec-moderate})

unspecified
mozilla35
x86
macOS
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox34 wontfix, firefox35 fixed, firefox-esr31 wontfix)

Details

(Whiteboard: [adv-main35+])

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

5 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.
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 over Xrays, all DOM proxies act like ones that don't have [OverrideBuiltins], basically.
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");
(Assignee)

Comment 4

5 years ago
Ok, this sounds pretty sane then. I'll work up a mochitest after lunch just to make sure.
(Assignee)

Comment 5

5 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.
(Assignee)

Comment 6

5 years ago
Posted patch Tests. v1 (obsolete) — Splinter Review
Both "even after deleting it" tests fail.
(Assignee)

Updated

5 years ago
Keywords: sec-auditsec-moderate
Summary: Make sure that named getters can't fool Xrays → named getters can fool Xrays
(Assignee)

Comment 7

5 years ago
I think we basically need meaningful DOM Xray prototypes to implement this security check properly. Setting the dep.
Depends on: 787070
(Assignee)

Comment 8

5 years ago
Posted patch Tests. v2 (obsolete) — Splinter Review
Including another failing test, to make sure that we hook up DOM Xray
prototypes to the JSXrayed window[0].Object.prototype.
Attachment #8396708 - Attachment is obsolete: true
(Assignee)

Comment 9

5 years ago
I will fix this one the deps are fixed.
Assignee: nobody → bobbyholley
Depends on: 987111
(Assignee)

Comment 10

5 years ago
(note to self - local branch is named_getter_xray).
> 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?
As in, do we need a more short-term fix that we can backport too?
(Assignee)

Comment 13

5 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
(Assignee)

Comment 14

5 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.
(Assignee)

Updated

5 years ago
Depends on: 789261
No longer depends on: 787070
> * 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?
(Assignee)

Comment 16

5 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.
No longer depends on: 789261
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?
(Assignee)

Comment 18

5 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.
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?
(Assignee)

Comment 20

5 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.
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.
(Assignee)

Comment 22

5 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.
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.
Group: core-security
(Assignee)

Comment 24

5 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.
(Assignee)

Comment 25

5 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.
Depends on: 787070
(Assignee)

Comment 26

5 years ago
Posted patch Tests. v3Splinter Review
Attachment #8396714 - Attachment is obsolete: true
Group: dom-core-security
(Assignee)

Updated

5 years ago
Flags: in-testsuite?
(Assignee)

Comment 28

5 years ago
Attachment #8442549 - Attachment is obsolete: true
Attachment #8498082 - Flags: review?(bzbarsky)
(Assignee)

Updated

5 years ago
Attachment #8442548 - Flags: review?(bzbarsky)
Comment on attachment 8442548 [details] [diff] [review]
Tests. v3

r=me.  I assume we fail some of these on tip, right?
Attachment #8442548 - Flags: review?(bzbarsky) → review+
Comment on attachment 8498082 [details] [diff] [review]
Don't unwrap XrayWrappers in HasPropertyOnPrototype. v2

r=me
Attachment #8498082 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 31

5 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.
https://hg.mozilla.org/mozilla-central/rev/74871c4df0a7
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Whiteboard: [adv-main35+]
Alias: CVE-2014-8636
This patch appears to have fixed sec-critical bug 1120261
Blocks: 1120261
(Assignee)

Updated

4 years ago
Flags: needinfo?(bzbarsky)
(Assignee)

Updated

4 years ago
Flags: needinfo?(bzbarsky) → needinfo?(bobbyholley)
(Assignee)

Comment 40

4 years ago
Given the involvement of bug 1120261, we need to fix this on esr31. I have patches.
Flags: needinfo?(bobbyholley)
(Assignee)

Comment 42

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

Updated

4 years ago
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.