1474541 - Make sure AutoEntryScript::AutoEntryScript is not called with a CCW

Status: RESOLVED FIXED

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

Comment 1

[Jan de Mooij [:jandem]]

So in this bug the hard one is nsFrameMessageManager::ReceiveMessage:

https://searchfox.org/mozilla-central/rev/8384a6519437f5eefbe522196f9ddf5c8b1d3fb4/dom/base/nsFrameMessageManager.cpp#725

Here |object| can be webIDLListener->CallbackOrNull() and it looks like that's just the object passed to addMessageListener (then there's also the weak-listener case). I guess the object will usually be a plain function but it could also be a CCW.

Could we do a CheckedUnwrap here or something?

Comment 2

[Boris Zbarsky [:bzbarsky]]

Using the CallbackOrNull of a WebIDL callback as an AutoEntryScript arg like this code does doesn't make any sense.   AutoEntryScript should represent the script being entered.   The "normal" way we call callbacks, via CallSetup, does a CheckedUnwrap before doing the AutoEntryScript.

Doing a CheckedUnwrap in the message manager when setting up the AutoEntryScript makes sense to me.

That said, there is the separate question of what realm should be used when doing the JS_NewPlainObject, aCpows->ToObject(), aCloneData->Read(), etc.  If strings are involved and we have a CCW and we do all this stuff in the underlying compartment of the CCW, then we will copy the strings twice when doing the call through the CCW, right?  I think that's OK, though....

Comment 3

[Jan de Mooij [:jandem]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #2)
> If strings are involved and we have a CCW and we do all this stuff in the
> underlying compartment of the CCW, then we will copy the strings twice when
> doing the call through the CCW, right?  I think that's OK, though....

Strings are per-zone so we only copy them when we cross a zone boundary..

Just to make sure: the *current code* does the extra wrapping, right, because we enter the wrapper's realm (the AutoEntryScript), allocate a bunch of things, then call into the call-a-wrapper machinery where we'll wrap arguments as needed. If we change this code to enter the unwrapped object's realm instead, then everything will get allocated directly in the correct realm/compartment.

I'll Try server the CheckedUnwrap patch and see if that works.

Comment 4

[Jan de Mooij [:jandem]]

Oh you meant using the unwrapped object only for the AutoEntryScript but still calling through the wrapper. Hm that will be more annoying because the allocated objects will have to be wrapped into the wrapper's compartment (because they got allocated into the wrapper's target realm), but how? We currently don't have a good way to do that because we can't enter a wrapper's realm (or at least we want to fix that).

Comment 5

[Jan de Mooij [:jandem]]

And CallSetup has a similar issue where it uses JSAutoRealm to enter the *wrapper's* realm:

https://searchfox.org/mozilla-central/rev/ad36eff63e208b37bc9441b91b7cea7291d82890/dom/bindings/CallbackObject.cpp#251-254

More dragons ahead :)

Comment 6

[Jan de Mooij [:jandem]]

For nsFrameMessageManager::ReceiveMessage we could do one of:

(1) Use UncheckedUnwrap for the AutoEntryScript, then enter an arbitrary realm in the wrapper's compartment. This is what JSAutoRealm will do, for now (we still have to audit that and then we would have to make that more explicit).

(2) Store a global object in MessageListener so we can enter that realm after the AutoEntryScript (the wrapper and the global would be same-compartment). CallbackObject already stores an mIncumbentJSGlobal, but that seems optional.

So I wonder if fixing this properly is a bigger design question that also affects CallSetup. For this bug I could go with (1) to move on, but we will have to revisit it later when we do the JSAutoRealm audit.

Comment 7

[Jan de Mooij [:jandem]]

XPCWrappedJSClass has a similar pattern, AutoEntryScript (without unwrapping) followed by JSAutoRealm:

https://searchfox.org/mozilla-central/rev/ad36eff63e208b37bc9441b91b7cea7291d82890/js/xpconnect/src/XPCWrappedJSClass.cpp#945,971

Comment 8

[Boris Zbarsky [:bzbarsky]]

> Oh you meant using the unwrapped object only for the AutoEntryScript but still calling through the wrapper.

Right, because we need to enforce our wrappers' security policies on the call.

> because the allocated objects will have to be wrapped into the wrapper's compartment (because they got allocated into the wrapper's target realm), but how?

Hmm, good question.  And as you noticed, CallSetup has the same issue.

The answer may well be that a CallbackObject needs to be handed and store a global to enter before doing stuff, in addition to the thing it's trying to call.  I guess that's your option (2) from comment 6.

For this bug, let's do (1) for now and file a followup to do (2).

> XPCWrappedJSClass has a similar pattern

Right, though it does AutoEntryScript on a global.  But the thing it calls js::GetGlobalForObjectCrossCompartment from there might well be a CCW itself...

I suspect we'll need to change nsXPCWrappedJS to store a global too, or something.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Oh, and I can probably write the patch for (2) if you want; it's all going to be codegen and DOM code.  Let me know, OK?

Comment 10

[Jan de Mooij [:jandem]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #9)
> Oh, and I can probably write the patch for (2) if you want; it's all going
> to be codegen and DOM code.  Let me know, OK?

Excellent. If I do it, it will likely require more of your time than if you do it yourself. Filed bug 1477923.

Comment 11

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Unwrap the object passed to AutoEntryScript in nsFrameMessageManager::ReceiveMessage

Comment 12

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Assert we don't pass cross-compartment wrappers to AutoEntryScript

Comment 13

[Jan de Mooij [:jandem]]

Comment on attachment 8994461 [details] [diff] [review]
Part 2 - Assert we don't pass cross-compartment wrappers to AutoEntryScript

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

::: dom/script/ScriptSettings.cpp
@@ +670,5 @@
>                                   const char* aReason,
>                                   bool aIsMainThread)
>    : AutoEntryScript(xpc::NativeGlobal(aObject), aReason, aIsMainThread)
>  {
> +  MOZ_ASSERT(!js::IsCrossCompartmentWrapper(aObject));

I forgot to mention: I know this is a bit awkward because it follows the NativeGlobal call, but my plan for NativeGlobal is to split it in (1) NativeGlobal that asserts the argument is a global and (2) NativeNonCCWObjectGlobal or something. Here we would use (2) and then we can just remove this redundant assertion.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8994460 [details] [diff] [review]
Part 1 - Unwrap the object passed to AutoEntryScript in nsFrameMessageManager::ReceiveMessage

r=me

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8994461 [details] [diff] [review]
Part 2 - Assert we don't pass cross-compartment wrappers to AutoEntryScript

r=me

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/567f701e6617
part 1 - Unwrap the object passed to AutoEntryScript in nsFrameMessageManager::ReceiveMessage. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/f0f7e6c5327a
part 2 - Assert we don't pass cross-compartment wrappers to AutoEntryScript. r=bz

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/567f701e6617
https://hg.mozilla.org/mozilla-central/rev/f0f7e6c5327a