Closed Bug 990293 Opened 6 years ago Closed 6 years ago
Assertion failure: "Wrap
Native shouldn't create a cross-compartment wrapper" (XSLT in iframe)
323 bytes, text/html
12.76 KB, text/plain
2.57 KB, patch
|Details | Diff | Splinter Review|
2.55 KB, patch
|Details | Diff | Splinter Review|
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
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?
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?"
> 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?
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...
Marking sec-high because it sounds bad. Feel free to adjust as desired.
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?
Attachment #8402729 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
This was very recently bitrotted and needs trunk rebasing.
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 :)
This already has Aurora approval (see right before comment 16). Your obsoleting of my bitrotted patch wiped it out. ;)
Yeah yeah... :P https://hg.mozilla.org/releases/mozilla-aurora/rev/a8575f98bb8a
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.
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
You need to log in before you can comment on or make changes to this bug.