806751 - Compartment mismatch with cross-origin captureEvents function, evil prototype chain

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Jesse Ruderman]

Attachment: testcase

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Jesse Ruderman]

Assertion failure: false (compartment mismatched), at js/src/jscntxtinlines.h:204

Comment 3

[Boris Zbarsky [:bzbarsky]]

So the real issue here is that we have an object whose proto is a WindowProxy.  And then we brain-transplant the WindowProxy, changing its compartment, and the object ends up with a proto that's in a different compartment, right?

How is this supposed to work in general anyway?  :(

Comment 4

[Boris Zbarsky [:bzbarsky]]

Except wait.  I though the point of brain transplanting was to NOT change the compartment... What's actually going on?  ;)

Comment 5

[Boris Zbarsky [:bzbarsky]]

In frame #7 (the JS_GetPrototype call there), "objArg" is "_ZTVN3xpc16FilteringWrapperINS_11XrayWrapperIN2js15SecurityWrapperINS2_23CrossCompartmentWrapperEEENS_26XPCWrappedNativeXrayTraitsEEENS_35CrossOriginAccessiblePropertiesOnlyEEE" (sorry about lack of demangling; neither gdb nor c++filt seem to be able to manage it).

Going further up the stack, in XPCCallContext::Init we have mJSContext and obj in the same compartment when we call XPCWrappedNative::GetWrappedNativeOfJSObject.  obj at that point is a js::CrossCompartmentWrapper.

On the other hand, in XPCWrappedNative::GetWrappedNativeOfJSObject the "obj" is NOT same-compartment with the JSContext, and is a plain-vanilla Object.

I wonder whether the issue is that we went through the "if (funobj)" codepath, unwrapped the funobj parent, and then suddenly had things that are not same-compartment?  Or maybe this is the "unsafeObj" unwrapping near the bottom of the loop over cur?  In any case, we seem to be doing unwrapping in GetWrappedNativeOfJSObject without doing any compartment-entering on the JSContext...

Comment 6

[Boris Zbarsky [:bzbarsky]]

Quoting myself from IRC:

js::Proxy::getPrototypeOf does js::CrossCompartmentWrapper::getPrototypeOf which 
does js::assertSameCompartment<JSObject*>.  So before that proto hook was added on proxies, you could get away with JS_GetPrototype(cx, obj) even if cx and obj were not same-compartment.  But now it will assert.  Sometimes.  Depending on what obj is.

Not sure what I think about that last bit.  It may be better to assert it up front in JS_GetPrototype.

Comment 7

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky (:bz) from comment #3)
> So the real issue here is that we have an object whose proto is a
> WindowProxy.  And then we brain-transplant the WindowProxy, changing its
> compartment, and the object ends up with a proto that's in a different
> compartment, right?

No, the original outer window gets replaced with a cross-compartment wrapper during the transplant. So it ends up with a cross-compartment wrapper on the proto chain, which can happen anywhere (just set __proto__ to something from another scope).

I'm totally willing to believe that we screw up compartments in GetWrappedNativeOfJSObject. That's why I want to remove it so bad. ;-)

Comment 8

[Andrew McCreight [:mccr8]]

Feel free to reassign to me if you don't have spare cycles, Bobby.

Comment 9

[Bobby Holley (:bholley)]

Attachment: Pile more garbage into GetWrappedNativeOfJSObject. v1

This is fallout from bug 787856, which caused us to start potentially doing nontrivial work in JS_GetPrototype.

Comment 10

[Bobby Holley (:bholley)]

Attachment: Crashtest. v1

Jesse's testcase formatted as a crashtest.

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 677960 [details] [diff] [review]
Pile more garbage into GetWrappedNativeOfJSObject. v1

The title of this patch is so accurate. This function makes me want to cry.

Comment 12

[Bobby Holley (:bholley)]

Comment on attachment 677960 [details] [diff] [review]
Pile more garbage into GetWrappedNativeOfJSObject. v1

[Security approval request comment]
How easily can the security issue be deduced from the patch?

The potential for a compartment mismatch certainly, but actually figuring out how to trigger it is extremely complicated. And then going from triggering it to exploiting it is also hard.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

No.

Which older supported branches are affected by this flaw?

m-a.

If not all supported branches, which bug introduced the flaw?

bug 787856.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

Same patch should apply.

How likely is this patch to cause regressions; how much testing does it need?

Not regression-risky, unless it turns out to have a performance impact.

Comment 13

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0efae96f3981

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0efae96f3981

Comment 15

[bhavana bajaj [:bajaj]]

Comment on attachment 677960 [details] [diff] [review]
Pile more garbage into GetWrappedNativeOfJSObject. v1

Approving on Aurora .

:bholley can you leave the bug open and sure it does not have a performance impact as suggested in comment 12 after its landings

Comment 16

[Bobby Holley (:bholley)]

(In reply to bhavana bajaj [:bajaj] from comment #15)
> :bholley can you leave the bug open and sure it does not have a performance
> impact as suggested in comment 12 after its landings

Not quite sure what you mean - it's already RESOLVED FIXED, right? Presumably if it regresses Talos, we'll hear about it and back it out, right? Or is there a different way you'd like to proceed?

Comment 17

[Bobby Holley (:bholley)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/34f703a6f232

Comment 18

[Bobby Holley (:bholley)]

Pushed the test:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fdbe3970ed19

Comment 19

[Ryan VanderMeulen [:RyanVM]]

(In reply to Bobby Holley (:bholley) from comment #18)
> Pushed the test:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/fdbe3970ed19

https://hg.mozilla.org/mozilla-central/rev/fdbe3970ed19