Closed Bug 990293 Opened 6 years ago Closed 6 years ago

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

Categories

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

x86_64
macOS
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla31
Tracking Status
firefox29 --- unaffected
firefox30 + fixed
firefox31 + fixed
firefox-esr24 --- unaffected
b2g-v1.2 --- unaffected
b2g-v1.3 --- unaffected
b2g-v1.3T --- unaffected
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: jruderman, Assigned: mccr8)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, sec-high, testcase)

Attachments

(4 files)

Attached file 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
Attached file stack
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?
Flags: needinfo?(continuation)
Flags: needinfo?(bobbyholley)
Do we need to go through an audit other WrapNative callers?
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?"
Flags: needinfo?(continuation)
> This is only safe insofar as the document should already be same-compartment with the
> window

The slightly bogus part there is "the" document.
> yes, why wasn't this callsite changed in that patch?
That callsite was changed, in that AllowWrapping went from being false to being true.
What I meant, was why was the code not changed to preserve the old behavior?  ;)
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?
Flags: needinfo?(bobbyholley)
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...
Blocks: 733636
Assignee: nobody → continuation
Marking sec-high because it sounds bad.  Feel free to adjust as desired.
Keywords: sec-high
This just reverts the changes I made to nsIDocument::WrapObject.

try run: https://tbpl.mozilla.org/?tree=Try&rev=54c2b78e8904
Attachment #8402729 - Flags: review?(bzbarsky)
Comment on attachment 8402729 [details] [diff] [review]
(bitrotted on trunk) Pass false to WrapNative in nsIDocument::WrapObject. r=bz

r=me
Attachment #8402729 - Flags: review?(bzbarsky) → review+
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.
Attachment #8402729 - Flags: sec-approval?
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.
Attachment #8402729 - Flags: sec-approval? → sec-approval+
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
Attachment #8402729 - Flags: approval-mozilla-aurora?
Keywords: checkin-needed
Attachment #8402729 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
This was very recently bitrotted and needs trunk rebasing.
Keywords: checkin-needed
Keywords: checkin-needed
Ms2ger just got backed out on inbound, but in case he relands it, here's the rebased version.
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.
Attachment #8402729 - Attachment is obsolete: true
Comment on attachment 8404724 [details] [diff] [review]
Pass false to WrapNative in nsIDocument::WrapObject. r=bz

carrying forward the r+ from bz
Attachment #8404724 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/a9dd8675e776

Please request Aurora approval on this when you get a chance :)
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
This already has Aurora approval (see right before comment 16).  Your obsoleting of my bitrotted patch wiped it out. ;)
Flags: needinfo?(ryanvm)
Attachment #8402729 - Attachment description: Pass false to WrapNative in nsIDocument::WrapObject. r=bz → (bitrotted on trunk) Pass false to WrapNative in nsIDocument::WrapObject. r=bz
Attachment #8402729 - Attachment is obsolete: false
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.
Flags: needinfo?(jruderman)
Neither Kamil nor I were able to reproduce the original problem, hence marking [qa-] for verification purposes.

Jesse, feel free to weigh in.
QA Whiteboard: [qa-]
WFM on trunk (with testcase changed to point to new location, dom/base/crashtests/458637-1-inner.xhtml)
Status: RESOLVED → VERIFIED
Flags: needinfo?(jruderman)
Group: core-security
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.