Closed Bug 857238 Opened 7 years ago Closed 5 years ago

compartment mismatch with location in nsWindowSH::NewResolve with Xrays

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED INCOMPLETE

People

(Reporter: mccr8, Unassigned)

References

Details

(Keywords: sec-other)

I saw a couple of these on Aurora

DefinePropertyById
LocationSetter<nsIDOMWindow>
JSCompartment::wrap
nsWindowSH::NewResolve
xpc::XPCWrappedNativeXrayTraits::resolveOwnProperty

https://crash-stats.mozilla.com/report/index/d726d7fe-ae3f-45a2-abf5-0d37d2130326
https://crash-stats.mozilla.com/report/index/b37cf30c-57ae-41ff-a1aa-f721a2130326
Hm, so I think part of that stack is garbage, but my guess is that the call to JS_DefinePropertyById in the (sLocation_id == id) branch of nsWindowSH::NewResolve is busted in the Xray case:

http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDOMClassInfo.cpp#5305

What happens in this function _looks_ highly suspicious when accessed over Xrays, but I can't actually find anything wrong with it. |cx| and |obj| should be same-compartment, |id| should be an interned atom, and we appear to be wrapping |v| into the cx compartment right before the DefineProperty call.

bz, see anything I don't in that code block?
Flags: needinfo?(bzbarsky)
The obvious thing I think I see is that if WrapNative depends on cx and scope being same-compartment then there's no reason they would be in this case, right?

But apart from that, this code looks sane....

I wish the same-compartment assert were broken up into several separate asserts for the obj, value, id, etc...  then we'd know which one is failing.  :(
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky (:bz) from comment #2)
> The obvious thing I think I see is that if WrapNative depends on cx and
> scope being same-compartment then there's no reason they would be in this
> case, right?

Yeah, my thoughts exactly. It looks like the WrapNative immediately invokes nsXPConnect::WrapNativeToJSVal, which invokes the static NativeInterface2JSObject doppelganger in nsXPConnect.cpp, which, as its first action, enters the compartment of aScope on the cx:

http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/nsXPConnect.cpp#1144


> I wish the same-compartment assert were broken up into several separate
> asserts for the obj, value, id, etc...  then we'd know which one is failing.
> :(

Me too!
Well, we can change that and see what crash-stats has to say for itself...

Or I guess we could try to reproduce, right?
who can own this between you three? :)
Assignee: nobody → continuation
The obvious zapping location with Xrays didn't cause any badness, so this is probably just some weird addon.  Given that none of the three of us can identify any problem here by inspection, I'm going to downgrade this to sec-other.  If it shows up some more, I'll consider landing some diagnostics, like maybe just splitting up the same compartment assertion.
Assignee: continuation → nobody
This has changed enough in the last few years that I think it isn't worth keeping this open.
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → INCOMPLETE
Group: core-security → core-security-release
Component: DOM → DOM: Core & HTML
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.