Bug 693399 (CVE-2012-0442)

"ASSERTION: mDocumentPrincipal prematurely set!" with document.write to multiple documents

VERIFIED FIXED in Firefox 10

Status

()

defect
VERIFIED FIXED
8 years ago
7 years ago

People

(Reporter: jruderman, Assigned: hsivonen)

Tracking

(Blocks 1 bug, {assertion, testcase, verified1.9.2})

Trunk
mozilla11
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox8- wontfix, firefox9+ wontfix, firefox10+ verified, firefox11+ verified, firefox-esr1010+ verified, blocking1.9.2 .26+, status1.9.2 .26-fixed)

Details

(Whiteboard: [sg:critical][qa!])

Attachments

(4 attachments, 1 obsolete attachment)

Posted file testcase
###!!! ASSERTION: mDocumentPrincipal prematurely set!: 'mDocumentPrincipal == nsnull', file dom/base/nsGlobalWindow.cpp, line 1805
Posted file stack tracee
So the key part here is a write() call on a document that is no longer the currently-loaded document in a window?  Is that not throwing?
Open() has this:

  // check whether we're in the middle of unload.  If so, ignore this call.
  nsCOMPtr<nsIDocShell> shell = do_QueryReferent(mDocumentContainer);
  if (!shell) {
    // We won't be able to create a parser anyway.
    return NS_OK;

but of course |shell| ends up non-null in this case.

mScriptGlobalObject == mWindow == null, but we of course have a non-null GetInnerWindow().

Seems to me like we should just no-op out of here if !mScriptGlobalObject, no?
Or if GetInnerWindow() is not its outer's current inner, either way.
Assigning to hsivonen per jst. Can you or bz take a stab at whether this really is a potential security problem and if so how bad? maybe same-origin violations?
Assignee: nobody → hsivonen
> Maybe same-origin violations?

My fuzzer found a variant that triggers a "compartment mismatch" assertion, so probably!
Doing what bz said in comment 3 and comment 4 indeed makes the assertions go away with this test case and the test case in bug 693250.

But is it the right fix? Shouldn't this test case either work as in Opera if having the data: URL involved is not considered to cause a same-origin violation or throw an exception as in Chrome if having the data: URL involved is considered a same-origin violation. (It's unclear to me what the current thinking on data: URL origin inheritance is.)

Why shouldn't .open() and .write() on a detached document work?
> Why shouldn't .open() and .write() on a detached document work?

This is a good question.  I believe our current setup doesn't work for that situation (e.g. the HTML parser still depends on having a docshell, right?).  Past that, there is no a priori reason it shouldn't work; we just have to actually make it work correctly (e.g. not mess with the window we used to be in if we're now detached).
(In reply to Boris Zbarsky (:bz) from comment #8)
> (e.g. the HTML parser still depends on having a docshell, right?).

Not once bug 651072 has landed.
Marking this sg:critical based on the compartment mismatch mentioned in comment 6.
Whiteboard: [sg:critical]
Let's fix this the way originally suggested first to make the situation non-critical. We can make the behavior more correct later.
Attachment #572442 - Flags: review?(bzbarsky)
Comment on attachment 572442 [details] [diff] [review]
Make open() no-op in some cases

Wouldn't it make more sense to check that our inner is the current inner of our outer?
Comment on attachment 573783 [details] [diff] [review]
Make open() no-op in some cases, v2

r=me
Attachment #573783 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky (:bz) from comment #14)
> r=me

Thanks. Landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/09ad1943c19a

Should this go on any other channels?
I would tend to say no, though maybe aurora is not a bad idea...
Comment on attachment 573783 [details] [diff] [review]
Make open() no-op in some cases, v2

Nominating for Aurora per comment 16.
Attachment #573783 - Flags: approval-mozilla-aurora?
Once this bug is made public, it would make sense to land the original test case as a crash test.
Status: NEW → RESOLVED
Closed: 8 years ago
Flags: in-testsuite?
OS: Mac OS X → All
Hardware: x86_64 → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
Attachment #573783 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [sg:critical] → [sg:critical][qa+]
This bug affects 3.6.x
blocking1.9.2: --- → ?
blocking1.9.2: ? → .26+
Comment on attachment 573783 [details] [diff] [review]
Make open() no-op in some cases, v2

Does blocking1.9.2: .26+ imply approval or do I need to now request approval separately like the tree rules seem to say?
Attachment #573783 - Flags: approval1.9.2.26?
You still need to request approval on patches, even if a bug is blocking a release.
Comment on attachment 573783 [details] [diff] [review]
Make open() no-op in some cases, v2

Approval to land for 1.9.2.26, a=dveditz for release-drivers
Attachment #573783 - Flags: approval1.9.2.26? → approval1.9.2.26+
Alias: CVE-2012-0442
Comment on attachment 573783 [details] [diff] [review]
Make open() no-op in some cases, v2

This doesn't apply to the 1.9.2 branch, approval removed.
Attachment #573783 - Flags: approval1.9.2.26+ → approval1.9.2.26-
The obvious context munging, seems to fix the symptoms. I don't know if the changes we made to window wrappers between 1.9.2 and 2.0 (fx4) would make this the wrong fix.
Attachment #590416 - Flags: review?(bzbarsky)
Attachment #590416 - Flags: approval1.9.2.26?
Attachment #590416 - Attachment is patch: true
Comment on attachment 590416 [details] [diff] [review]
1.9.2 backport

Approving for 1.9.2.26, a=dveditz
Attachment #590416 - Flags: approval1.9.2.26? → approval1.9.2.26+
Assuming bz's r+ since it's a trivial merge and the patch fixes the bug; we'll have time to back out if there's a problem

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/e04e018f9ae6
Comment on attachment 590416 [details] [diff] [review]
1.9.2 backport

r=me
Attachment #590416 - Flags: review?(bzbarsky) → review+
Verified fixed in Firefox 3.6.26.
Keywords: verified1.9.2
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #30)
> Verified fixed in Firefox 3.6.26.

FYI, tested using the attached testcase in mozilla-1.9.2-debug from 2012-01-11 to reproduce the assertion and mozilla-1.9.2-debug from 2012-01-24 to verify it fixed.
Verified on Fx10 and Fx11 using recent debug builds. Previously I would get an assertion in the shell when loading the testcase in comment #0 but recent debug builds show a warning.
Whiteboard: [sg:critical][qa+] → [sg:critical][qa!]
Verified in debug nightly.
Status: RESOLVED → VERIFIED
Group: core-security
We missed verifying this on ESR a while back -- I've done so now against 10.0.0esrpre.
(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #34)
> 10.0.0esrpre.

Sorry, that's supposed to read 10.0.8esrpre
You need to log in before you can comment on or make changes to this bug.