Closed Bug 753162 Opened 8 years ago Closed 8 years ago

"Assertion failure: cx->compartment == this" with document.write during DOMAttrModified

Categories

(Core :: XPConnect, defect)

x86_64
macOS
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla16
Tracking Status
firefox15 --- verified
firefox16 + verified
firefox-esr10 --- unaffected

People

(Reporter: jruderman, Assigned: bholley)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, sec-high, testcase, Whiteboard: [sg:high][advisory-tracking+])

Attachments

(3 files)

During DOMAttrModified:

###!!! ASSERTION: nsHTMLDocument::Reset() - Wyciwyg Channel  still exists!: '!mWyciwygChannel', file content/html/document/src/nsHTMLDocument.cpp, line 329

(That first assertion might be bug 675518 or bug 680086.)

After DOMAttrModified:

Assertion failure: cx->compartment == this, at js/src/jscompartment.cpp:124
Attached file stack traces
CC'ing moz_bug_r_a4 in case this compartment confusion leads to interesting discoveries.
Keywords: sec-high
Whiteboard: [sg:high]
Ok, the basic problem here is that the code in CrossCompartmentWrapper::call is assuming that the AutoCompartment restored cx->compartment to the same compartment it had when enter() was called, which isn't necessarily the case. Specifically, the function being call()-ed ends up calling document.write, which triggers a call to SetOuterObject on the outer window. This changes cx->globalObject, and so the resetCompartment() that happens in AutoCompartment::leave() sets cx->compartment to the new scope.

The fix is to just wrap with cx->compartment. Patch coming up.
Attaching a patch, flagging blake for review.
Attachment #634862 - Flags: review?(mrbkap)
Comment on attachment 634862 [details] [diff] [review]
Don't assume that we end up in the same compartment as we started in CrossCompartmentWrapper. v1

IIRC, this has been a longstanding concern... it's always been odd that if I have:

JSAutoRequest ac; ac.enter(cx, compartmentA);

after a call, cx->compartment might not be compartmentA. Will this get better when we remove fake stack frames?

I'm fine with this fix, since it doesn't make anything worse, though.
Attachment #634862 - Flags: review?(mrbkap) → review+
(In reply to Blake Kaplan (:mrbkap) from comment #6)
> Will this get better when we remove fake stack frames?

I don't think so. The fundamental issue is that the document moves compartments during the course of the call. There are two invariants that would be nice:
1 - If we enter from compartment A, we leave in compartment A
2 - If we enter in the compartment of document D, we leave in the compartment of document D

AFAICT we can only guarantee one of them. Though I wonder what will happen when cx->globalObject goes away like I've been hearing it will. Will there be a cx->defaultCompartment?

pushed to m-i: http://hg.mozilla.org/integration/mozilla-inbound/rev/eca1167488ca
Assignee: nobody → bobbyholley+bmo
https://hg.mozilla.org/mozilla-central/rev/eca1167488ca
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
(In reply to Bobby Holley (:bholley) from comment #7)
> 1 - If we enter from compartment A, we leave in compartment A
> 2 - If we enter in the compartment of document D, we leave in the
> compartment of document D
> 
> AFAICT we can only guarantee one of them.

Right. The problem is that right now we have a mix of the two options based on whether or not entering the compartment was a no-op or not and whether there were already stack frames on the context. I think that, in general, option 1 is more important since any code that enters compartment A will still only have objects from compartment A lying around (even if they've been brain transplanted), but all this belongs in some other bug, I suppose.
Is this fallout from cpg or does it affect earlier builds as well?
(In reply to Daniel Veditz [:dveditz] from comment #10)
> Is this fallout from cpg or does it affect earlier builds as well?

The former, I think.
Should we land this in Aurora then?
(In reply to Daniel Veditz [:dveditz] from comment #12)
> Should we land this in Aurora then?

Yes, probably.
Comment on attachment 634862 [details] [diff] [review]
Don't assume that we end up in the same compartment as we started in CrossCompartmentWrapper. v1

Nominating per comment 12.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 650353
User impact if declined: Potential security vulnerabilities.
Testing completed (on m-c, etc.): Baking on m-c.
Risk to taking this patch (and alternatives if risky): Not risky. 
String or UUID changes made by this patch: None.
Attachment #634862 - Flags: approval-mozilla-aurora?
Comment on attachment 634862 [details] [diff] [review]
Don't assume that we end up in the same compartment as we started in CrossCompartmentWrapper. v1

[Triage Comment]
Low risk sg:crit fix. Approved for Aurora 15.
Attachment #634862 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [sg:high] → [sg:high][advisory-tracking+]
Keywords: verifyme
Confirmed testcase crashes/asserts 2012-05-08 Firefox 15.0a1 Nightly Debug.

Does not crash:
2012-06-22 Firefox 16.0a1 Nightly Debug
2012-06-26 Firefox 15.0a2 Aurora Debug
Status: RESOLVED → VERIFIED
Keywords: verifyme
QA Contact: anthony.s.hughes
Group: core-security
You need to log in before you can comment on or make changes to this bug.