990293 - Assertion failure: "WrapNative shouldn't create a cross-compartment wrapper" (XSLT in iframe)

Status: VERIFIED FIXED

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

Description

[Jesse Ruderman]

Attachment: testcase

1. Fix paths in testcase.
2. Create a profile with security.fileuri.strict_origin_policy set to false
3. Load the testcase

Result:

Assertion failure: &winVal.toObject() == js::UncheckedUnwrap(&winVal.toObject()) (WrapNative shouldn't create a cross-compartment wrapper), at content/base/src/nsDocument.cpp:12134

This assertion was added in:
https://hg.mozilla.org/mozilla-central/rev/387b5a9167b0

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Boris Zbarsky [:bzbarsky]]

Er... yes, why wasn't this callsite changed in that patch?  And why was that assertion added, when it makes no sense given the code?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Do we need to go through an audit other WrapNative callers?

Comment 4

[Andrew McCreight [:mccr8]]

I audited all of the callers in bug 956404, so you can look there for previous analysis.  This document one was a place I wasn't sure of.  Bobby said: "This is only safe insofar as the document should already be same-compartment with the window. But I think it's playing with fire. Let's remove the explicit |false| and replace it with an assertion that the result is not a cross-compartment wrapper?"

Comment 5

[Boris Zbarsky [:bzbarsky]]

> This is only safe insofar as the document should already be same-compartment with the
> window

The slightly bogus part there is "the" document.

Comment 6

[Andrew McCreight [:mccr8]]

> yes, why wasn't this callsite changed in that patch?
That callsite was changed, in that AllowWrapping went from being false to being true.

Comment 7

[Boris Zbarsky [:bzbarsky]]

What I meant, was why was the code not changed to preserve the old behavior?  ;)

Comment 8

[Bobby Holley (:bholley)]

I don't get it. In the old behavior, we do WrapNative with aAllowWrapping=false, and then immediately use that value in a JS_DefineUCProperty call. Any situation where we're hitting this assertion today is a compartment mismatch in the old world, right?

Comment 9

[Boris Zbarsky [:bzbarsky]]

That's a very interesting question, so I checked.

So what happens in this code is the following:  win is not the current inner of its outer.  win->mJSObject is same-compartment with "obj".  So before bug we did the wrap, got back the JSObject for the XPCWrappedNative for "win" in winVal (and in particular, that handed us win->mJSObject), and went on our merry way.

But now on trunk we ask WrapNative to create proxies for us.  This does a JS_WrapObject, which screws things up in two ways.  First of all, it outerizes, which is most definitely NOT desired here.  Then, once it's outerized it has to create a cross-compartment wrapper, since the inner is not current and hence not same-compartment with the outer.  And that triggers the assertion in question.

So the answer to your question is "no, not right", thanks to outerization.  ;)

Conclusion: Passing "true" for WrapNative() is almost certainly the wrong thing to do when wrapping an inner window due to the outerization behavior; we should double-check callers for that.

On a separate note, I wonder if we can just assume that mJSObject is set on win and use FastGetGlobalJSObject or some such.  Or just restore the aAllowWrapping=false for now, and wait for this code to go away with WebIDL window...

Comment 10

[Andrew McCreight [:mccr8]]

Marking sec-high because it sounds bad.  Feel free to adjust as desired.

Comment 11

[Andrew McCreight [:mccr8]]

Attachment: (bitrotted on trunk) Pass false to WrapNative in nsIDocument::WrapObject. r=bz

This just reverts the changes I made to nsIDocument::WrapObject.

try run: https://tbpl.mozilla.org/?tree=Try&rev=54c2b78e8904

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8402729 [details] [diff] [review]
(bitrotted on trunk) Pass false to WrapNative in nsIDocument::WrapObject. r=bz

r=me

Comment 13

[Andrew McCreight [:mccr8]]

Comment on attachment 8402729 [details] [diff] [review]
(bitrotted on trunk) Pass false to WrapNative in nsIDocument::WrapObject. r=bz

[Security approval request comment]
How easily could an exploit be constructed based on the patch? It probably isn't too obvious.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No.

Which older supported branches are affected by this flaw? Just 30.

If not all supported branches, which bug introduced the flaw? bug 733636

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? it should be identical

How likely is this patch to cause regressions; how much testing does it need? should be fairly safe, I'm just reverting to the behavior in 29.

Comment 14

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8402729 [details] [diff] [review]
(bitrotted on trunk) Pass false to WrapNative in nsIDocument::WrapObject. r=bz

Sec-approval+ for trunk. If you nominate your Aurora patch (reversion?), I can approve it as well.

Comment 15

[Andrew McCreight [:mccr8]]

Comment on attachment 8402729 [details] [diff] [review]
(bitrotted on trunk) Pass false to WrapNative in nsIDocument::WrapObject. r=bz

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 733636
User impact if declined: sec-high
Testing completed (on m-c, etc.): try push
Risk to taking this patch (and alternatives if risky): low, just reverting part of the patch
String or IDL/UUID changes made by this patch: none

Comment 16

[Ryan VanderMeulen [:RyanVM]]

This was very recently bitrotted and needs trunk rebasing.

Comment 17

[Andrew McCreight [:mccr8]]

Attachment: Pass false to WrapNative in nsIDocument::WrapObject. r=bz

Ms2ger just got backed out on inbound, but in case he relands it, here's the rebased version.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8402729 [details] [diff] [review]
(bitrotted on trunk) Pass false to WrapNative in nsIDocument::WrapObject. r=bz

Yeah, I still need to use the rebased patch.

Comment 19

[Andrew McCreight [:mccr8]]

Comment on attachment 8404724 [details] [diff] [review]
Pass false to WrapNative in nsIDocument::WrapObject. r=bz

carrying forward the r+ from bz

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a9dd8675e776

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a9dd8675e776

Please request Aurora approval on this when you get a chance :)

Comment 22

[Andrew McCreight [:mccr8]]

This already has Aurora approval (see right before comment 16).  Your obsoleting of my bitrotted patch wiped it out. ;)

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Yeah yeah... :P

https://hg.mozilla.org/releases/mozilla-aurora/rev/a8575f98bb8a

Comment 24

[Matt Wobensmith [:mwobensmith][:matt:]]

Hi Jesse - I'm not able to reproduce the original assert on builds of Fx30/31 from 2014-03-31. 

Can you confirm that your config no longer produces an assert, with a fixed build of Fx30 and/or Fx31? Thank you.

Comment 25

[Matt Wobensmith [:mwobensmith][:matt:]]

Neither Kamil nor I were able to reproduce the original problem, hence marking [qa-] for verification purposes.

Jesse, feel free to weigh in.

Comment 26

[Jesse Ruderman]

WFM on trunk (with testcase changed to point to new location, dom/base/crashtests/458637-1-inner.xhtml)