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

RESOLVED FIXED in mozilla1.8beta5

Status

()

Core
Security
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: moz_bug_r_a4, Assigned: bz)

Tracking

({fixed-aviary1.0.7, fixed1.7.12, fixed1.8})

Trunk
mozilla1.8beta5
fixed-aviary1.0.7, fixed1.7.12, fixed1.8
Points:
---
Bug Flags:
blocking1.7.12 +
blocking-aviary1.0.7 +
blocking1.8b5 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:fix])

Attachments

(7 attachments)

(Reporter)

Description

12 years ago
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:
(Reporter)

Comment 1

12 years ago
Created attachment 194639 [details]
testcase 1 - window without any chrome UI
(Reporter)

Comment 2

12 years ago
Created attachment 194640 [details]
(browser.xul for 1.0.6)
(Reporter)

Comment 3

12 years ago
Created attachment 194641 [details]
(browser.xul for Deer Park)
(Reporter)

Comment 4

12 years ago
Created attachment 194642 [details]
testcase 2 - browser UI spoofing
Flags: blocking1.8b5?

Updated

12 years ago
Flags: blocking1.8b5? → blocking1.8b5+
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking1.7.12+
Flags: blocking-aviary1.0.7+
Whiteboard: [sg:fix]
(Assignee)

Comment 5

12 years ago
Created attachment 195426 [details]
Another simple testcase
(Assignee)

Comment 6

12 years ago
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?
(Assignee)

Comment 7

12 years ago
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...
(Assignee)

Comment 8

12 years ago
Created attachment 195454 [details] [diff] [review]
Proposed patch
Attachment #195454 - Flags: superreview?(jst)
Attachment #195454 - Flags: review?(jst)
Comment on attachment 195454 [details] [diff] [review]
Proposed patch

Yeah, seems like the right thing to do. r+sr=jst
Attachment #195454 - Flags: superreview?(jst)
Attachment #195454 - Flags: superreview+
Attachment #195454 - Flags: review?(jst)
Attachment #195454 - Flags: review+
(Assignee)

Updated

12 years ago
Assignee: dveditz → bzbarsky
OS: Windows XP → All
Hardware: PC → All
Target Milestone: --- → mozilla1.8beta5
(Assignee)

Comment 10

12 years ago
Fixed on trunk.
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
(Assignee)

Comment 11

12 years ago
Comment on attachment 195454 [details] [diff] [review]
Proposed patch

Requesting branch approvals.  This is a pretty safe and straightforward fix.
Attachment #195454 - Flags: approval1.8b5?
Attachment #195454 - Flags: approval1.7.12?
Flags: blocking1.7.12?
Flags: blocking-aviary1.0.7?
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?
(Assignee)

Comment 13

12 years ago
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.
(Assignee)

Comment 14

12 years ago
Created attachment 195805 [details] [diff] [review]
1.7 branch patch
Attachment #195805 - Flags: superreview?(jst)
Attachment #195805 - Flags: review?(jst)
Attachment #195805 - Flags: approval1.7.12?
Attachment #195805 - Flags: approval-aviary1.0.7?

Updated

12 years ago
Flags: blocking-aviary1.0.7? → blocking-aviary1.0.7+
Flags: blocking1.7.12? → blocking1.7.12+

Updated

12 years ago
Attachment #195454 - Flags: approval1.8b5? → approval1.8b5+

Updated

12 years ago
Attachment #195454 - Flags: approval1.7.13?
Comment on attachment 195805 [details] [diff] [review]
1.7 branch patch

r+sr=jst
Attachment #195805 - Flags: superreview?(jst)
Attachment #195805 - Flags: superreview+
Attachment #195805 - Flags: review?(jst)
Attachment #195805 - Flags: review+
Attachment #195805 - Flags: approval1.7.12?
Attachment #195805 - Flags: approval1.7.12+
Attachment #195805 - Flags: approval-aviary1.0.7?
Attachment #195805 - Flags: approval-aviary1.0.7+
(Assignee)

Comment 16

12 years ago
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.
Keywords: fixed-aviary1.0.7, fixed1.7.12, fixed1.8
Flags: blocking1.7.13+
Flags: blocking-aviary1.0.8+

Comment 17

12 years ago
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
Group: security
You need to log in before you can comment on or make changes to this bug.