1125015 - Stop unwrapping XrayWrappers in HasPropertyOnPrototype in ESR31

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

Bug 1120261 is fixed by bug 987794, which is in every release branch except ESR31. Backporting that bug would require backporting two large dependencies. The first is bug 787070, which gives Xrays meaningful prototypes. This is necessary so that HasPropertyOnPrototype can reliably query the prototype without unwrapping the XrayWrapper. The second is bug 987111 (and really, bug 856067), which shore up our XrayWrappers for non-DOM objects, including Object.prototype and whatever content sticks on the prototype chain.

Bug 987794 really fixes two hazards - one of which the fact that we unwrap XrayWrappers and invoke scriptable traps on them when performing normal operations on Document and friends. The other is the ability of surreptitious content to substitute DOM elements for regular commonly-used methods by exploiting named element access.

Solving the second problem involves reliably knowing each level of the canonical prototype chain (since content can always just splice in a prototype chain that doesn't have getElementById on it and then shadow it). This could be solved by either backporting bug 787070 (which gives us the JSClass hooks to look up the canonical prototype over Xrays) or hardcoding the names explicitly, neither of which I'm wild about doing.

But the second problem was only ever sec-moderate anyway. The first issue is much more pressing and, per bug 1120261 comment 23, is going to be disclosed Real Soon Now. And I think we can solve the first issue more simply by doing the following:

(1) Modifying XrayWrapper's getPrototypeOf so that it never returns a non-Xrayable object (which, prior to bug 987111 and bug 856067, give us transparent cross-compartment wrappers). In particular, the final Object.prototype gets replaced with the Object.prototype in the XrayWrapper's compartment, which is probably good enough for ESR31.
(2) Removing the unwrap in HasPropertyOnPrototype (which exists because, prior to bug 787070, lookups on an XrayWrapper did not relate to the prototype chain of the XrayWrapper) and replacing it with a manual walk of the prototype chain.

I have patches.

Because of the candid discussion of bug 987794 here, we should embargo this bug until ESR31 EOL.

Comment 1

[Bobby Holley (:bholley)]

Bug 1120261 doesn't reproduce on ESR31 for somewhat orthogonal reasons, but I've verified that these patches, when transplanted to 34, fix the bug there.

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 1 - Prototype non-hasPrototype XrayWrappers with non-Xrayable prototypes to the wrapper's Object.prototype. v1

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 2 - Explicitly climb the Xrayed prototype chain in HasPropertyOnPrototype on esr31. v1

Comment 4

[Bobby Holley (:bholley)]

Attachment: Edits to the test in bug 987794 to reflect the behavior that we're landing on esr31. v1

These aren't for landing, just for reviewing.

Comment 5

[Andrew McCreight [:mccr8]]

[Tracking Requested - why for this release]: Fix for sec-critical bug (bug 1120261) that is going to be disclosed within 60 days.

I'm not sure if this should be sec-critical or sec-high, but I'll just mark it critical for now.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8553551 [details] [diff] [review]
Part 1 - Prototype non-hasPrototype XrayWrappers with non-Xrayable prototypes to the wrapper's Object.prototype. v1

r=me

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8553552 [details] [diff] [review]
Part 2 - Explicitly climb the Xrayed prototype chain in HasPropertyOnPrototype on esr31. v1

>+    if (!js::GetObjectProto(cx, curr, &proto)) {
>+      return true; // Fail safe.

Don't you need to clear the pending exception?

>+    if (!JS_HasPropertyById(cx, proto, id, &hasProp) || hasProp) {

And here?

Past that, I'm not sure my mental model of this stuff is up to date very well; I'd prefer Peter looked this over.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8553553 [details] [diff] [review]
Edits to the test in bug 987794 to reflect the behavior that we're landing on esr31. v1

This will still need to be landed on ESR31, right?  Or is this test not present there?

Why does the value end up undefined instead of the chrome-side hasOwnProperty?

Comment 9

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #7)
> >+    if (!js::GetObjectProto(cx, curr, &proto)) {
> >+      return true; // Fail safe.
> 
> Don't you need to clear the pending exception?

Yep.

> 
> >+    if (!JS_HasPropertyById(cx, proto, id, &hasProp) || hasProp) {
> 
> And here?

And yep. I'll upload a new patch.

Note that the code on trunk also leaves dangling exceptions on cx. I've got a patch to fix that on trunk which I'll upload in another bug someday when I have a free moment.

(In reply to Boris Zbarsky [:bz] from comment #8)
> This will still need to be landed on ESR31, right?  Or is this test not
> present there?

The test isn't checked in anywhere, because of its security-sensitivity.

> Why does the value end up undefined instead of the chrome-side
> hasOwnProperty?

Because XrayWrappers without meaningful prototypes operate with a getPropertyDescriptor trap, which doesn't have any way to resolve stuff from Object.prototype.

Comment 10

[Bobby Holley (:bholley)]

Attachment: Part 2 - Explicitly climb the Xrayed prototype chain in HasPropertyOnPrototype on esr31. v2

Comment 11

[Bobby Holley (:bholley)]

(In reply to Andrew McCreight [:mccr8] from comment #5)
> [Tracking Requested - why for this release]: Fix for sec-critical bug (bug
> 1120261) that is going to be disclosed within 60 days.
> 
> I'm not sure if this should be sec-critical or sec-high, but I'll just mark
> it critical for now.

That's probably overrating it, but we need to fix this on esr31 come hell or high water, so that's fine for now.

Comment 12

[Bobby Holley (:bholley)]

Let's see if we can still get useful try results on esr31: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d0ccae2b4d3b

Comment 13

[Peter Van der Beken [:peterv]]

Comment on attachment 8554197 [details] [diff] [review]
Part 2 - Explicitly climb the Xrayed prototype chain in HasPropertyOnPrototype on esr31. v2

Review of attachment 8554197 [details] [diff] [review]:
-----------------------------------------------------------------

I hope I didn't miss anything, my mental model doesn't correspond to esr31 anymore :-(.

Comment 14

[Bobby Holley (:bholley)]

Comment on attachment 8554197 [details] [diff] [review]
Part 2 - Explicitly climb the Xrayed prototype chain in HasPropertyOnPrototype on esr31. v2

Flagging for ESR31 approval. This is one of two pieces that will fix bug 1125483, as well as bug 1120261 (which will be disclosed at some point not long after the next esr31 release). Moderate risk, but we don't have a choice. No string changes.

Comment 15

[Bobby Holley (:bholley)]

(In reply to Bobby Holley (Busy with media, don't ask for DOM/JS/XPConnect things) from comment #12)
> Let's see if we can still get useful try results on esr31:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=d0ccae2b4d3b

Ryan, I don't know exactly how to read these results, but I _think_ they're ok? I guess we can just land it on the branch and see what happens.

Comment 16

[Bobby Holley (:bholley)]

I have verified locally that these patches fix the testcase in bug 1125483. Do they seem sufficient to you, moz_bug_r_a4?

Comment 17

[Bobby Holley (:bholley)]

ni Ryan just to make sure he sees comment 15 (and for the uplift in general).

Comment 18

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

Yeah, all those failures look like expected ones to me (test suites that were never meant to run on esr31). I'll queue this up to push :)

Comment 19

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/releases/mozilla-esr31/rev/50cad2d9985b
https://hg.mozilla.org/releases/mozilla-esr31/rev/6d7c5ebb94da

Comment 20

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/b49c63911662
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/38cf453a6e2f

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/b3eb74549f59
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/15b74455a4fe

The patches here depend on Window WebIDLification, which landed on 32. Bobby says it's not worth backporting to b2g30.

Comment 21

[moz_bug_r_a4]

(In reply to Bobby Holley (Busy with media, don't ask for DOM/JS/XPConnect things) from comment #16)
> I have verified locally that these patches fix the testcase in bug 1125483.
> Do they seem sufficient to you, moz_bug_r_a4?

Yes, they seem sufficient to fix bug 1120261.  But, there is a null deref crash (bug 1127780).

Comment 22

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/b49c63911662
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/38cf453a6e2f

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/b3eb74549f59
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/15b74455a4fe