Closed Bug 806751 Opened 8 years ago Closed 8 years ago
Compartment mismatch with cross-origin capture
Events function, evil prototype chain
457 bytes, text/html
10.52 KB, text/plain
2.53 KB, patch
|Details | Diff | Splinter Review|
1.35 KB, patch
|Details | Diff | Splinter Review|
No description provided.
Assertion failure: false (compartment mismatched), at js/src/jscntxtinlines.h:204
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? :(
Except wait. I though the point of brain transplanting was to NOT change the compartment... What's actually going on? ;)
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...
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.
(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. ;-)
Feel free to reassign to me if you don't have spare cycles, Bobby.
Assignee: nobody → bobbyholley+bmo
This is fallout from bug 787856, which caused us to start potentially doing nontrivial work in JS_GetPrototype.
Attachment #677960 - Flags: review?(mrbkap)
Jesse's testcase formatted as a crashtest.
Attachment #677961 - Flags: review+
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.
Attachment #677960 - Flags: review?(mrbkap) → review+
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.
Attachment #677960 - Flags: sec-approval?
Attachment #677960 - Flags: sec-approval? → sec-approval+
Attachment #677960 - Flags: approval-mozilla-aurora?
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
Attachment #677960 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(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?
Pushed the test: https://hg.mozilla.org/integration/mozilla-inbound/rev/fdbe3970ed19
(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
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.