733636 - Flip security-wrapper default for nsContentUtils::WrapNative

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: str patch 1

STR:
1) Apply patch 1
2) Apply patch 2
3) Rebuild the browser
4) Go to http://people.mozilla.org/~mwargers/tests/plugins/flash/flashembed.html
5) Using the doorhanger, activate the plugins on the page.

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: str patch 2

Comment 2

[Luke Wagner [:luke]]

Bobby, the reason I am pretty sure this is an xpconnect bug is that we have a call to WrapNative(cx, scope, supports, vp) where the compartment of cx and scope match but the outgoing vp is in a different compartment.  I thought this might actually be the same issue as some of these c-p-g issues you've been uncovering.

Comment 3

[Bobby Holley (:bholley)]

Stack?

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: stack

Comment 5

[Bobby Holley (:bholley)]

So I stepped through the test case here. As it turns out, the call to nsContentUtils::WrapNative actually takes an optional final argument, aAllowWrapping, which defaults to false. This causes XPCConvert::NativeInterface2JSObject to skip the wrapping phase, causing an unwrapped cross-compartment object to be returned.

So Jared, the easy solution for now is to pass true as the final argument to nsContentUtils::WrapNative.

Blake, this seems pretty foot-gun. Do we have a reason for this parameter, and especially a reason for it to default to false? I can understand it in the pre-compartment days (not wanting security wrappers), but now we really need them to do anything with the JS Object. Is this something we can change?

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Bobby Holley (:bholley) from comment #5)
> So Jared, the easy solution for now is to pass true as the final argument to
> nsContentUtils::WrapNative.

Thanks! That fixed it for me :-)

Comment 7

[Luke Wagner [:luke]]

(In reply to Bobby Holley (:bholley) from comment #5)
> Blake, this seems pretty foot-gun.

No kidding!  (Might as well call it aAllowCrossCompartmentAssertions ;)

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

If I remember correctly, that flag is useful in one case: where we actually want the XPCWrappedNative C++ object for one reason or another. The other can I can think of is PreCreate hooks, where we need the parent object's JS object and not a wrapper around it in order to tell XPConnect which compartment we actually belong in.

We can definitely change the default, though.

Comment 9

[Luke Wagner [:luke]]

Or perhaps add a new function altogether with a scary name...

Comment 10

[Andrew McCreight [:mccr8]]

I can take a poke at this.

Comment 11

[Andrew McCreight [:mccr8]]

Attachment: part 1 - Change the nsContentUtils::WrapNative aAllowWrapping default to true.

Comment 12

[Andrew McCreight [:mccr8]]

Attachment: part 2 - nsXPConnect::WrapNative should pass aAllowWrapping=true.

Comment 13

[Bobby Holley (:bholley)]

Comment on attachment 8375035 [details] [diff] [review]
part 1 - Change the nsContentUtils::WrapNative aAllowWrapping default to true.

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

Thanks for pushing this through Andrew.

Comment 14

[Andrew McCreight [:mccr8]]

try run: https://tbpl.mozilla.org/?tree=Try&rev=5a96fefb4328

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/387b5a9167b0
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e9378c4abc75

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/387b5a9167b0
https://hg.mozilla.org/mozilla-central/rev/e9378c4abc75