Closed Bug 733636 Opened 8 years ago Closed 6 years ago

Flip security-wrapper default for nsContentUtils::WrapNative

Categories

(Core :: XPConnect, defect)

x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: jaws, Assigned: mccr8)

References

Details

Attachments

(2 files, 3 obsolete files)

Attached file str patch 1 (obsolete) —
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.
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.
Stack?
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?
(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 :-)
(In reply to Bobby Holley (:bholley) from comment #5)
> Blake, this seems pretty foot-gun.

No kidding!  (Might as well call it aAllowCrossCompartmentAssertions ;)
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.
Or perhaps add a new function altogether with a scary name...
Summary: Compartment mismatch where object from WrapNative does not have the same compartment as the global object → Flip security-wrapper default for nsContentUtils::WrapNative
I can take a poke at this.
Assignee: nobody → continuation
Attachment #603539 - Attachment is obsolete: true
Attachment #603572 - Attachment is obsolete: true
Attachment #603540 - Attachment is obsolete: true
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.
Attachment #8375035 - Flags: review?(bobbyholley) → review+
Attachment #8375036 - Flags: review?(bobbyholley) → review+
https://hg.mozilla.org/mozilla-central/rev/387b5a9167b0
https://hg.mozilla.org/mozilla-central/rev/e9378c4abc75
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Duplicate of this bug: 956404
You need to log in before you can comment on or make changes to this bug.