266371 - [FIX]Possible to close user-opened windows by setting window.name

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

STEPS TO REPRODUCE: Click on the link in the URL field.

EXPECTED RESULTS:  Nothing happens.

ACTUAL RESULTS:  Window closes.

Marking confidential for now, since this could be easily abused by sites...

Credit for initially finding this goes to Omar Khan (see bug 265921).

Comment 1

[Asa Dotzler [:asa]]

->danm for a possible trunk fix. 

Comment 2

[Johnny Stenback (:jst)]

*** Bug 279529 has been marked as a duplicate of this bug. ***

Comment 3

[Boris Zbarsky [:bzbarsky]]

Requesting blocking....

Perhaps the way to do this is to keep track of the originalOpener, not just the
opener.  And then allow window.close() calls to succeed only if:

1)  There is an original opener
2)  The calling code is allowed to access the original opener (we may have to use
    a URI, not a window object, for the original opener...)

Comment 4

[Benjamin Smedberg]

Just for kicks, why is script allowed to set/change window.name?

Comment 5

[timeless]

we have to deal with this in our commercial code. but there's clearly a reason.

Comment 6

[Boris Zbarsky [:bzbarsky]]

OK.  Being closed down is keeping this security bug from being fixed, so I'm
going to open it up and hope someone steps up... :(

Comment 7

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Not blocking 1.5, since it's merely a DoS.

Comment 8

[Lachlan Hunt]

Boris Zbarsky referred me to this bug as a possible cause for the problem I'm experiencing, whereby a script can close a window/tab it didn't actually open when the "Force links that open in new windows to open in the same tab/window" preference is enabled.  Here's a test case.

http://lachy.id.au/dev/2005/11/window-open-close-test

This is really annoying for me when scripts close tabs I don't want closed.  There should be a way stop scripts from closing any windows/tabs at all.

Comment 9

[Martijn Wargers (dead)]

Attachment: patch

Maybe something like this?
I haven't tested this much, but if fixes the testcase, and I tested with a regular window.close() testcase and that closed just fine.

Comment 10

[Martijn Wargers (dead)]

Attachment: testcase

And the patch doesn't regress this testcase:
With https://bugzilla.mozilla.org/attachment.cgi?id=127485 (from bug 212298), you can't close the window anymore with the patch by clicking on the link, but when you open it in a new window (with this testcase), you can close the window by clicking on the link.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 208975 [details] [diff] [review]
patch

>Index: dom/src/base/nsGlobalWindow.cpp
> void
> nsGlobalWindow::SetOpenerWindow(nsIDOMWindowInternal* aOpener)

I'm not sure I follow the changes to this function. I would think that the right thing to do would be to set mOriginalOpener the first time SetOpenerWindow is called (and set a boolean indicating that we've done that).  After that, we never set mOriginalOpener.

I definitely don't see why we're setting mOriginalOpener to the mOriginalOpener of aOpener.

>@@ -2278,16 +2284,21 @@ nsGlobalWindow::SetOpener(nsIDOMWindowIn

Similar issues here.  I'm not sure that mOpenerWasCleared is actually needed if we have the mOriginalOpener thing, for what it's worth.

Comment 12

[Martijn Wargers (dead)]

Attachment: patch2

So you mean something like this?
By the way, nsGlobalWindow::SetOpener doesn't seem to be used at all:
http://lxr.mozilla.org/seamonkey/search?string=SetOpener

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 209233 [details] [diff] [review]
patch2

That doesn't look right either, since it allows a window to go from being "not an opened window" to "an opened window" if a window gets SetOpenerWindow called at some point "after" the window is first opened, no?

As for SetOpener, it's a scriptable method exposed to JS.  It's used in JS.

Comment 14

[Martijn Wargers (dead)]

Comment on attachment 209233 [details] [diff] [review]
patch2

> since it allows a window to go from being "not
> an opened window" to "an opened window" if a window gets SetOpenerWindow called
> at some point "after" the window is first opened, no?
When would that happen? 
I can't think of a case when that happens.

Comment 15

[Boris Zbarsky [:bzbarsky]]

The testcase in the URL bar does just that.  The only reason it works with your patch is that it has aOpener == this.  But consider something like:

javascript:window.name = "foo"; window.open('javascript:window.open("", "foo"); opener.close(); close()')

That should close the window it's run in, both with a trunk build (just tested) and with your patch (check?).

The point is, SetOpenerWindow() is called any time the window is the target of a window.open().  Unfortunately, it can be called on both windows that have openers and windows that do not, and the problem is telling a just-opened window that the window watcher is still setting up from a window that got opened by someone else.

Perhaps we can look at whether we have an extant document when SetOpenerWindow is called?  Or something?

Comment 16

[Martijn Wargers (dead)]

(Grr, my internet connection suddenly died yesterday)
> That should close the window it's run in, both with a trunk build (just tested)
> and with your patch (check?).
Indeed, that would not be solved by this patch.

> Perhaps we can look at whether we have an extant document when SetOpenerWindow
> is called?  Or something?
No idea.
Also not sure yet, how your suggestion in comment 11 can work. 

Comment 17

[Boris Zbarsky [:bzbarsky]]

I don't think it quite can, since SetOpenerWindow is not called on, say, new tabs that are opened.  :(

Comment 18

[Boris Zbarsky [:bzbarsky]]

Attachment: Sorta-1.8-friendly patch

This is just for posterity... There's no way this will apply to branch; we need to do something slightly different there diff-wise, since SetOpenerWindow wasn't even on nsPIDOMWindow.  ;)  So I'll just write separate trunk and branch patches, I guess.  And the branch patch will happen after bug 326006 lands on branch.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Attachment: Real 1.8-friendly patch

Comment 20

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed trunk patch

Comment 21

[Johnny Stenback (:jst)]

Comment on attachment 211619 [details] [diff] [review]
Proposed trunk patch

sr=jst

Comment 22

[Boris Zbarsky [:bzbarsky]]

Attachment: Part of original patch I forgot to attach...

Comment 23

[Boris Zbarsky [:bzbarsky]]

Attachment: 1.8 branch patch

jst, mind giving this a once-over?  Merging kinda sucked.  :(

Comment 24

[Boris Zbarsky [:bzbarsky]]

Fixed on trunk.

Comment 25

[Peter Van der Beken [:peterv]]

This turned balsa orange:

###!!! ASSERTION: aOriginalOpener is true, but we already have a document loaded: '!aOriginalOpener || !mDocument', file /builds/tinderbox/Firefox-gcc3.4/Linux_2.4.7-10_Depend/mozilla/dom/src/base/nsGlobalWindow.cpp, line 1473

Comment 26

[Boris Zbarsky [:bzbarsky]]

I removed that assert, since it seems it will fire in various cases depending on what window creator and window provider do (eg if they preload about:blank).

Comment 27

[Johnny Stenback (:jst)]

Comment on attachment 211945 [details] [diff] [review]
1.8 branch patch

r=jst

Comment 28

[Boris Zbarsky [:bzbarsky]]

Fixed on 1.8 branch.

Comment 29

[Boris Zbarsky [:bzbarsky]]

I forgot to remove the assert on the branch.  See bug 341276, where I'll remove it.

Comment 30

[Martijn Wargers (dead)]

*** Bug 345162 has been marked as a duplicate of this bug. ***