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

VERIFIED FIXED in Firefox 15

Status

()

defect
--
critical
VERIFIED FIXED
7 years ago
7 years ago

People

(Reporter: jruderman, Assigned: bholley)

Tracking

(Blocks 2 bugs, {assertion, sec-high, testcase})

Trunk
mozilla16
x86_64
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox15 verified, firefox16+ verified, firefox-esr10 unaffected)

Details

(Whiteboard: [sg:high][advisory-tracking+])

Attachments

(3 attachments)

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
Posted 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: 7 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+]
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.