253951 - [FIXr]Dialogs from a loading page appear above previous page's content

Status: RESOLVED FIXED

(Core :: Layout, defect, P1)

Description

[Jesse Ruderman]

If a page puts up a dialog while it is loading, the dialog can appear above the
content of the previous page.  This works with at least alert() and
enablePrivilege().  Luckily, the address bar usually (???) shows the attacker's URL.

Comment 1

[Jesse Ruderman]

Attachment: demo

Comment 2

[Daniel Veditz [:dveditz]]

We need to clear the contents before we run any script from the new page

Comment 3

[Johnny Stenback (:jst)]

Jesse, I get nothing other than a button that does nothing when loading your
demo. What am I missing?

Comment 4

[Jesse Ruderman]

I can still repro with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.3)
Gecko/20041004 Firefox/0.10

You have to have "Force links that open in new windows..." unchecked and you
have to have codebase principals enabled.

Comment 5

[Daniel Veditz [:dveditz]]

I could repro in 1.7.3, but in the latest firefox nightly the "google" page
never shows, it's just blank.

The alert dialog part of this works (on 1.7.3) whether you have codebase
principals allowed or not. Lot of branch changes since 1.7.3, maybe it's fixed?
I'll try the trunk, too, as soon as I get out of here.

Comment 6

[Daniel Veditz [:dveditz]]

(In reply to comment #5)
> I could repro in 1.7.3, but in the latest firefox nightly the "google" page
> never shows, it's just blank.

Nevermind, I must have had some of the default script options turned off. I
still see this in Firefox 1.0.3 and Suite trunk (2005041905)

Comment 7

[Jesse Ruderman]

I see this bug on Mac too.  Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O;
en-US; rv:1.8b4) Gecko/20050810 Firefox/1.0+.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

Comment 9

[Boris Zbarsky [:bzbarsky]]

Note that that patch will NOT work for the 1.7 branch.  If we need this fixed
there it'll take a good bit more work at best.

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 193633 [details] [diff] [review]
Proposed patch

>Index: content/html/document/src/nsHTMLContentSink.cpp
>+    PRBool notify = ((aType & Flush_SinkNotifications) != 0);

The parens around the rhs are not necessary.
r=mrbkap

Comment 11

[Johnny Stenback (:jst)]

Comment on attachment 193633 [details] [diff] [review]
Proposed patch

sr=jst

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 193633 [details] [diff] [review]
Proposed patch

We should take this on 1.8 branch...

Do we need a 1.7 branch fix for this also?

Comment 13

[Asa Dotzler [:asa]]

Comment on attachment 193633 [details] [diff] [review]
Proposed patch

please land on the trunk and make sure it looks good before moving up to the
branch. thanks.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Requesting 1.7 branch blocking so we remember to evaluate whether we need a fix
there.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Fixed on trunk.  Will land on branch in a few days.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Checked in on 1.8 branch.

Comment 17

[Boris Zbarsky [:bzbarsky]]

This caused bug 306660.

Comment 18

[Boris Zbarsky [:bzbarsky]]

For what it's worth, checking this in on 1.7 branch will be a bear -- it required some ... interesting ... content sink changes to sort out the regressions, and I'm not sure we've kept track of them all well enough... :(

Comment 19

[Daniel Veditz [:dveditz]]

Can this be used for worse than the spoofing in the demo? The latest branch builds do show the real source of the alerts in the title bar, and the permission dialog does show the other site.

As long as the script can't interact with the page contents (covered by other splitwindow-related bugs) we can probably live with the spoofing in 1.0.x rather than risk the regressions