57600 - Security: JavaScript document.write() is denied access to its own window.

Status: RESOLVED FIXED

(Core :: Security, defect, P2)

Description

[bht237]

See attached test case
Works in all other browsers.

Comment 1

[bht237]

Attachment: Simple test case

Comment 2

[David Krause]

-> mstolz

Comment 3

[Asa Dotzler [:asa]]

Confirmed on Mozilla trunk builds
linux 102708 RedHat 6.2
win32 102704 NT 4
mac 102708 Mac OS9

Comment 4

[bht237]

Changed description slightly, added dataloss keyword because a page is not
loading!!!.
While I do accept that I cannot judge the priority of this, I feel that
it is a serious issue when I also look at
http://bugzilla.mozilla.org/show_bug.cgi?id=57636

document.write() is a fundamental tool that is often merely used to code
browser bug workarounds.

With a bug like this, a web developer loses a basic tool in cases that are
difficult enough already. This fundamental method works in all other current
browsers that one can think of today and older versions, too.
If one browser (and even only the first minor release) does not support this,
then web authors are stuffed for the next few years because one of their basic
tools is broken.

The times are over where one could afford JavaScript error messages when
experimenting with frames.
Browsers since version 3 are sophisticated enough to support reliable event
driven applications.
Mozilla/Netscape must not start at a reliability level that is below Internet
Explorer version 3.
Mozilla is an awsome browser but this case shows an extreme disparity.

Comment 5

[John Unruh]

Mass changing QA to ckritzer.

Comment 6

[Mitchell Stoltz (not reading bugmail)]

Mass changing milestones to Moz0.9.1. Many of these bugs are dependent on the
XPConnected DOM and its associated security UI changes.

Comment 7

[Mitchell Stoltz (not reading bugmail)]

This seems to work now, marking Fixed.

Comment 8

[ckritzer (gone)]

Marking VERIFIED FIXED on:
-MacOS91 2001-05-21-15-trunk
-Win98SE 2001-05-22-06-trunk
-LinRH62 2001-05-22-05-trunk

Comment 9

[bht237]

It's back on:

Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.7.1) Gecko/20040707

It's a very bad bug.

Comment 10

[Daniel Veditz [:dveditz]]

This was broken in 1.4 as well: "Permission denied to get property
HTMLDocument.write"

Comment 11

[Brendan Eich [:brendan]]

jst, any hope we can unregress this?

/be

Comment 12

[Johnny Stenback (:jst)]

Attachment: Carry over the caller's principal in document.open().

Comment 13

[Johnny Stenback (:jst)]

Not sure when or how this broke, but with todays code we don't seem to ever set
the principal of the document that's as a result of document.open() + write() to
anything useful, and in this case where the caller is a child frame whose src is
a javascript: URI we end up with a principal generated from the javascript: URI,
which is never right. This patch makes us set the principal of the callee to the
principal of the caller, as this should work AFAICT.

Comment 14

[Brendan Eich [:brendan]]

Go caillon, I'll sr and approve the patch as soon as it gets your r+.

/be

Comment 15

[Daniel Veditz [:dveditz]]

Comment on attachment 159941 [details] [diff] [review]
Carry over the caller's principal in document.open().

>@@ -2106,18 +2106,26 @@ nsHTMLDocument::OpenCommon(nsIURI* aSour
>+  // Grab a reference to the callers principal as it may not be
>+  // reachable past the call to Reset().
>+  nsCOMPtr<nsIPrincipal> callingPrincipal;
>+
>+  if (callingDoc) {
>+    callingPrincipal = callingDoc->GetPrincipal();
>+  }
>+

Why here? Isn't this essentially what the code starting on line 1902 is
doing with securityInfo? Seems like both should happen together just for
conceptual clarity, though I don't know if here or ~1900 is better.

Otherwise looks like the right thing to do r=dveditz, but a double-check from
caillon wouldn't hurt

Comment 16

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 159941 [details] [diff] [review]
Carry over the caller's principal in document.open().

r=caillon if you move this into where the securityInfo is set earlier in the
method.  Also why do we restore conditionally?	Shouldn't we always do that,
like we do for the securityInfo?

Comment 17

[Johnny Stenback (:jst)]

Attachment: More consistent approach.

This changes this code such that when calling document.open() we make the URI
of the callee be that of the codebase in the callers principal, and not the
direct URI of the callee. The two should be the same in all cases except when
the caller's URI is a javascript: or data: URI, and in those cases we want the
URI of the callee to be the callers codebase, not the javascript: or data: URI.
Basically the same thing as my previous patch, except that it doesn't share the
principal instances across documents, just creates new principals based on the
principal of the caller in stead of the document URI of the caller.

Comment 18

[Brendan Eich [:brendan]]

Comment on attachment 160008 [details] [diff] [review]
More consistent approach.

jst: what if the principal has a cert (signed script)?	We should copy that
too.  Maybe we need a Clone operation.

If we fixed the mAnnotations bloat problem, why wouldn't we share a reference
to the caller's principal here, though?  If principals are immutable (as far as
identity goes), that wins by saving memory and cycles.

/be

Comment 19

[Johnny Stenback (:jst)]

I'd propose to take attachment 160008 [details] [diff] [review] for the branches and make us *always*
share principals on the trunk. The reason being that the patch for always
sharing potentially has a greater impact than if we don't share, since we
currently never do in this case. Chris and Brendan, pelase review attachment
160008 [details] [diff] [review] and we'll get that in on the branches and I'll fix this for real on the
trunk...

Comment 20

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 160008 [details] [diff] [review]
More consistent approach.

Fair enough.  r=me, however please make GetSourceCodebaseURI() a void method
since the only caller really doesn't care about an rv, just whether it gets an
nsIURI, and it already makes that check.  You'll need to move the
NS_ENSURE_TRUE inside the null check block.

Comment 21

[Brendan Eich [:brendan]]

Comment on attachment 160008 [details] [diff] [review]
More consistent approach.

sr=[e-mail].

/be

Comment 22

[Johnny Stenback (:jst)]

(In reply to comment #20)
> (From update of attachment 160008 [details] [diff] [review])
> Fair enough.  r=me, however please make GetSourceCodebaseURI() a void method
> since the only caller really doesn't care about an rv, just whether it gets an
> nsIURI, and it already makes that check.  You'll need to move the
> NS_ENSURE_TRUE inside the null check block.

Yeah, I agree, but given that this is a temporary branch-only change I'd rather
keep the change small for the branches, and do it right for the trunk later on.
Thanks for the reviews!

Comment 23

[Asa Dotzler [:asa]]

Comment on attachment 160008 [details] [diff] [review]
More consistent approach.

a=asa for branches checkin.

Comment 24

[Johnny Stenback (:jst)]

Fixed on the aviary and 1.7 brances.

Comment 25

[Johnny Stenback (:jst)]

Attachment: Trunk patch.

Comment 26

[OstGote!]

Please don't forget the trunk patch. Or perhaps the blocking1.8a6? flag should
be set.

Comment 27

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 161587 [details] [diff] [review]
Trunk patch.

r=me, sorry for letting this linger for so long.  Looks good to me, let's get
this in and tested on trunk.

Comment 28

[Will Rickards]

Should the original window properties be accessible in new window object?
See http://forums.mozillazine.org/viewtopic.php?p=1275061
Where it was brought up that IE can access the 'globals' after a document.write
if you specify the global variable without the window reference.
I attached a simple test case.

Comment 29

[Will Rickards]

Attachment: Window Globals Test

Comment 30

[Brendan Eich [:brendan]]

Will, that's bug 114461 -- it has nothing to do with this bug.

/be

Comment 31

[Brendan Eich [:brendan]]

Comment on attachment 161587 [details] [diff] [review]
Trunk patch.

sr=me.

/be

Comment 32

[Johnny Stenback (:jst)]

Fix checked in.

Comment 33

[Boris Zbarsky [:bzbarsky]]

Added a test for this in bug 445004.