693399 - (CVE-2012-0442) "ASSERTION: mDocumentPrincipal prematurely set!" with document.write to multiple documents

Status: VERIFIED FIXED

(Core :: DOM: Navigation, defect)

Description

[Jesse Ruderman]

Attachment: testcase

###!!! ASSERTION: mDocumentPrincipal prematurely set!: 'mDocumentPrincipal == nsnull', file dom/base/nsGlobalWindow.cpp, line 1805

Comment 1

[Jesse Ruderman]

Attachment: stack tracee

Comment 2

[Boris Zbarsky [:bzbarsky]]

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?

Comment 3

[Boris Zbarsky [:bzbarsky]]

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?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Or if GetInnerWindow() is not its outer's current inner, either way.

Comment 5

[Daniel Veditz [:dveditz]]

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?

Comment 6

[Jesse Ruderman]

> Maybe same-origin violations?

My fuzzer found a variant that triggers a "compartment mismatch" assertion, so probably!

Comment 7

[Henri Sivonen (:hsivonen)]

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?

Comment 8

[Boris Zbarsky [:bzbarsky]]

> 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).

Comment 9

[Henri Sivonen (:hsivonen)]

(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.

Comment 10

[Johnny Stenback (:jst)]

Marking this sg:critical based on the compartment mismatch mentioned in comment 6.

Comment 11

[Henri Sivonen (:hsivonen)]

Attachment: Make open() no-op in some cases

Let's fix this the way originally suggested first to make the situation non-critical. We can make the behavior more correct later.

Comment 12

[Boris Zbarsky [: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 13

[Henri Sivonen (:hsivonen)]

Attachment: Make open() no-op in some cases, v2

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 573783 [details] [diff] [review]
Make open() no-op in some cases, v2

r=me

Comment 15

[Henri Sivonen (:hsivonen)]

(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?

Comment 16

[Boris Zbarsky [:bzbarsky]]

I would tend to say no, though maybe aurora is not a bad idea...

Comment 17

[Ian Melven :imelven]

from edmorley : https://hg.mozilla.org/mozilla-central/rev/09ad1943c19a

Comment 18

[Henri Sivonen (:hsivonen)]

Comment on attachment 573783 [details] [diff] [review]
Make open() no-op in some cases, v2

Nominating for Aurora per comment 16.

Comment 19

[Henri Sivonen (:hsivonen)]

Once this bug is made public, it would make sense to land the original test case as a crash test.

Comment 20

[Henri Sivonen (:hsivonen)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1d95a296ebda

Comment 21

[Daniel Veditz [:dveditz]]

This bug affects 3.6.x

Comment 22

[Henri Sivonen (:hsivonen)]

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?

Comment 23

[Johnny Stenback (:jst)]

You still need to request approval on patches, even if a bug is blocking a release.

Comment 24

[Daniel Veditz [:dveditz]]

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

Comment 25

[Daniel Veditz [:dveditz]]

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.

Comment 26

[Daniel Veditz [:dveditz]]

Attachment: 1.9.2 backport

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.

Comment 27

[Daniel Veditz [:dveditz]]

Comment on attachment 590416 [details] [diff] [review]
1.9.2 backport

Approving for 1.9.2.26, a=dveditz

Comment 28

[Daniel Veditz [:dveditz]]

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 29

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 590416 [details] [diff] [review]
1.9.2 backport

r=me

Comment 30

[u279076]

Verified fixed in Firefox 3.6.26.

Comment 31

[u279076]

(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.

Comment 32

[juan becerra [:juanb]]

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.

Comment 33

[Al Billings [:abillings - ex-MoCo]]

Verified in debug nightly.

Comment 34

[u279076]

We missed verifying this on ESR a while back -- I've done so now against 10.0.0esrpre.

Comment 35

[u279076]

(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #34)
> 10.0.0esrpre.

Sorry, that's supposed to read 10.0.8esrpre