Closed Bug 1474541 Opened Last year Closed Last year

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

Details

Attachments

(2 files)

No description provided.
Depends on: 1475177
Priority: -- → P3
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?
Flags: needinfo?(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....
Flags: needinfo?(bzbarsky)
(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.
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).
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 :)
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.
Flags: needinfo?(bzbarsky)
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
> 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.
Flags: needinfo?(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?
(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.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Attachment #8994460 - Flags: review?(bzbarsky)
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 on attachment 8994460 [details] [diff] [review]
Part 1 - Unwrap the object passed to AutoEntryScript in nsFrameMessageManager::ReceiveMessage

r=me
Attachment #8994460 - Flags: review?(bzbarsky) → review+
Comment on attachment 8994461 [details] [diff] [review]
Part 2 - Assert we don't pass cross-compartment wrappers to AutoEntryScript

r=me
Attachment #8994461 - Flags: review?(bzbarsky) → review+
Pushed by jandemooij@gmail.com:
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
https://hg.mozilla.org/mozilla-central/rev/567f701e6617
https://hg.mozilla.org/mozilla-central/rev/f0f7e6c5327a
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.