306804 - Content can open chrome windows by calling open() on a window that's already closed

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[moz_bug_r_a4]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.11) Gecko/20050728
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.10) Gecko/20050717 Firefox/1.0.6

Content can open chrome windows by calling open() on a window that's already 
closed.

1. Get a closed window.
    f = document.getElementById("iframe");
    w = f.contentWindow;
    f.parentNode.removeChild(f);

2. Call w.open() without url.
    w2 = w.open("");

    w2.toString() is "[object ChromeWindow]".
    w2.location is "about:blank".

note: When url is not empty, open() fails with this error:
Error: uncaught exception: [Exception... "Component returned failure code: 
0x80004005 (NS_ERROR_FAILURE) [nsIDOMJSWindow.open]"  nsresult: "0x80004005 
(NS_ERROR_FAILURE)"  location: "JS frame :: http://127.0.0.1/t1 :: b :: line 18"  
data: no]

That chrome window is completely invisible in the beginning. It is possible to 
load a document into the chrome window by using w2.document.location = "...", 
and make it visible.

This bug could be used to spoof UI.

An attacker can:
* create a window that has not any chrome UI by using <window 
hidechrome="true">.
* create fake browser UI without the real status bar, even though the 
dom.disable_window_open_feature.status pref is true.

Affected:
[Firefox]
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20050901 
Firefox/1.6a1
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050901 
Firefox/1.0+
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.10) Gecko/20050717 
Firefox/1.0.6

[Mozilla suite]
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.11) Gecko/20050728


Reproducible: Always

Steps to Reproduce:

Comment 1

[moz_bug_r_a4]

Attachment: testcase 1 - window without any chrome UI

Comment 2

[moz_bug_r_a4]

Attachment: (browser.xul for 1.0.6)

Comment 3

[moz_bug_r_a4]

Attachment: (browser.xul for Deer Park)

Comment 4

[moz_bug_r_a4]

Attachment: testcase 2 - browser UI spoofing

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Another simple testcase

Comment 6

[Boris Zbarsky [:bzbarsky]]

So here's what's going on:

1) The empty URI means we never call nsGlobalWindow::SecurityCheckURL (since
there's nothing to security check).  Unfortunately, our check for a non-null
mDocument is in SecurityCheckURL (this is what throws NS_ERROR_FAILURE in the
nonempty URI case).
2) We call nsWindowWatcher::OpenWindowJS and pass |this| as the parent.
3) OpenWindowJS gets the parent's tree owner (null in this case)
4) Then it does do_GetInterface on the tree owner to get the nsIWebBrowserChrome
   (which comes out null here, since the tree owner is null).
5) This null nsIWebBrowserChrome is what's passed as aParent to
   CreateChromeWindow2
6) CreateChromeWindow2 doesn't have a parent, so it just creates a toplevel
   window and returns it instead of creating a new browser window and returning
   its rendering area.

So there are several things that I think are wrong in this sequence:

1)  I think that no mDocument should mean immediate failure in window.open in
all cases, not just cases when the URI is nonempty.
2)  I think lack of a tree owner on the parent should mean immediate failure in
the window watcher.

Can anyone think of anything those two changes would break?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Well, I can't really change window watcher, since the API nsGlobalWindow uses is
frozen.  I guess I'll just document what's going on with it...

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

Comment 9

[Johnny Stenback (:jst)]

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

Yeah, seems like the right thing to do. r+sr=jst

Comment 10

[Boris Zbarsky [:bzbarsky]]

Fixed on trunk.

Comment 11

[Boris Zbarsky [:bzbarsky]]

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

Requesting branch approvals.  This is a pretty safe and straightforward fix.

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So how safe is it really?  Would you be comfortable shipping it in a
quickly-tested 1.7.x security release without having it in a trunk / 1.8 alpha
or beta release?

Comment 13

[Boris Zbarsky [:bzbarsky]]

I think I would be, yes.  The only cases that might possibly get broken by it
(start throwing exceptions) are exactly the cases we're trying to prevent.

If we want extra safety, we could only do the check if |this| is a non-chrome
window.  That would make very very sure not to break any extensions and such. 
IF you'd like, I'll post a patch that does that.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: 1.7 branch patch

Comment 15

[Johnny Stenback (:jst)]

Comment on attachment 195805 [details] [diff] [review]
1.7 branch patch

r+sr=jst

Comment 16

[Boris Zbarsky [:bzbarsky]]

Checked attachment 195805 [details] [diff] [review] in to MOZILLA_1_7_BRANCH and AVIARY_1_0_1_20050124_BRANCH.

Checked attachment 195454 [details] [diff] [review] in to and MOZILLA_1_8_BRANCH.

Comment 17

[Bob Clary [:bc] (inactive)]

ff 1.0.6/winxp fails testcase1, testcase2, another simple testcase
ff 1.0.7/winxp; ff 1.5b/winxp (20050914) passes testcase1, testcase2, another
simple testcase