Last Comment Bug 306804 - Content can open chrome windows by calling open() on a window that's already closed
: Content can open chrome windows by calling open() on a window that's already ...
Status: RESOLVED FIXED
[sg:fix]
: fixed-aviary1.0.7, fixed1.7.12, fixed1.8
Product: Core
Classification: Components
Component: Security (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla1.8beta5
Assigned To: Boris Zbarsky [:bz]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2005-09-01 23:45 PDT by moz_bug_r_a4
Modified: 2006-03-12 18:52 PST (History)
13 users (show)
dbaron: blocking1.7.12+
mtschrep: blocking‑aviary1.0.7+
mtschrep: blocking1.8b5+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase 1 - window without any chrome UI (1.30 KB, text/html)
2005-09-01 23:48 PDT, moz_bug_r_a4
no flags Details
(browser.xul for 1.0.6) (72.21 KB, application/vnd.mozilla.xul+xml)
2005-09-01 23:54 PDT, moz_bug_r_a4
no flags Details
(browser.xul for Deer Park) (73.93 KB, application/vnd.mozilla.xul+xml)
2005-09-01 23:56 PDT, moz_bug_r_a4
no flags Details
testcase 2 - browser UI spoofing (4.76 KB, text/html)
2005-09-02 00:00 PDT, moz_bug_r_a4
no flags Details
Another simple testcase (1.23 KB, text/html)
2005-09-09 09:53 PDT, Boris Zbarsky [:bz]
no flags Details
Proposed patch (4.33 KB, patch)
2005-09-09 12:58 PDT, Boris Zbarsky [:bz]
jst: review+
jst: superreview+
asa: approval1.8b5+
Details | Diff | Splinter Review
1.7 branch patch (5.90 KB, patch)
2005-09-12 15:26 PDT, Boris Zbarsky [:bz]
jst: review+
jst: superreview+
dbaron: approval‑aviary1.0.7+
dbaron: approval1.7.12+
Details | Diff | Splinter Review

Description moz_bug_r_a4 2005-09-01 23:45:36 PDT
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 2005-09-01 23:48:34 PDT
Created attachment 194639 [details]
testcase 1 - window without any chrome UI
Comment 2 moz_bug_r_a4 2005-09-01 23:54:20 PDT
Created attachment 194640 [details]
(browser.xul for 1.0.6)
Comment 3 moz_bug_r_a4 2005-09-01 23:56:05 PDT
Created attachment 194641 [details]
(browser.xul for Deer Park)
Comment 4 moz_bug_r_a4 2005-09-02 00:00:05 PDT
Created attachment 194642 [details]
testcase 2 - browser UI spoofing
Comment 5 Boris Zbarsky [:bz] 2005-09-09 09:53:42 PDT
Created attachment 195426 [details]
Another simple testcase
Comment 6 Boris Zbarsky [:bz] 2005-09-09 10:04:06 PDT
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 [:bz] 2005-09-09 12:28:34 PDT
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 [:bz] 2005-09-09 12:58:09 PDT
Created attachment 195454 [details] [diff] [review]
Proposed patch
Comment 9 Johnny Stenback (:jst, jst@mozilla.com) 2005-09-09 17:06:22 PDT
Comment on attachment 195454 [details] [diff] [review]
Proposed patch

Yeah, seems like the right thing to do. r+sr=jst
Comment 10 Boris Zbarsky [:bz] 2005-09-09 22:07:12 PDT
Fixed on trunk.
Comment 11 Boris Zbarsky [:bz] 2005-09-09 22:07:30 PDT
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+2 (mostly busy through August 4; review requests must explain patch) 2005-09-12 11:06:28 PDT
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 [:bz] 2005-09-12 11:11:22 PDT
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 [:bz] 2005-09-12 15:26:50 PDT
Created attachment 195805 [details] [diff] [review]
1.7 branch patch
Comment 15 Johnny Stenback (:jst, jst@mozilla.com) 2005-09-12 17:33:04 PDT
Comment on attachment 195805 [details] [diff] [review]
1.7 branch patch

r+sr=jst
Comment 16 Boris Zbarsky [:bz] 2005-09-12 18:28:01 PDT
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:] 2005-09-14 17:17:57 PDT
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

Note You need to log in before you can comment on or make changes to this bug.