354973 - Yet Another PopupBlocker XSS

Status: VERIFIED FIXED

(Core :: Security, defect)

Description

[shutdown]

URI based window lookup is not safe for URIs such as data:, javascript:, etc...
http://lxr.mozilla.org/seamonkey/search?string=requestingWindowURI

steps:
1. make sure the popup blocker is enabled.
2. save the attached testcase to local disk or somewhere not B.M.O.
   this step is required to test the vulnerability correctly.
3. load the saved testcase.
4. open the blocked popup.

Several weeks ago, my HDD crashed and
I have lost some exploit testcases and mails :( ugh...

Comment 1

[shutdown]

Attachment: testcase

works on:
Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8.1)
  Gecko/20060930 BonEcho/2.0
Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8.0.8pre)
  Gecko/20060930 Firefox/1.5.0.8pre
  ("Force links that open new window to open in" needs to be disabled.)

Comment 2

[chris hofmann]

we should try and get this for the next 1.8.x.x releases...

Comment 3

[Daniel Veditz [:dveditz]]

Johnny, any hope for this one?

Comment 4

[Johnny Stenback (:jst)]

Attachment: Fix.

The problem here is that the code in browser.js is too fragile to deal with this case. The browser code uses the requesting URI to find the child-window that the popup was opened on, and in this case there's two child windows with the same data: URI and thus we end up with the wrong one. The one we end up finding has the principals of its containing frame, which are different than that of the frame that was used to open the blocked popup, and thus we end up executing the javascript: URI in a frame the original caller didn't have access to.

This patch makes the browser code pass around the actual window that was used to open the blocked popup, and thus we always end up using the right window when showing a blocked popup and this bug goes away.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Is the target of the event really the correct window? FireAbuseEvents seems to pass the topmost window to FirePopupBlockedEvent.

Comment 6

[Johnny Stenback (:jst)]

Comment on attachment 245621 [details] [diff] [review]
Fix.

Good point, need to get that info through somehow.

Comment 7

[Boris Zbarsky [:bzbarsky]]

The other question I have is how long the report sticks around (in terms of effective-leak stuff).  Do we effectively keep that window object alive forever  (at least while the toplevel Firefox window is open)?

Comment 8

[Boris Zbarsky [:bzbarsky]]

Perhaps we could add branch-only scriptable APIs to:

1)  Get an nsISupports for a principal for a window.
2)  Perform a same-origin check on two principals.

?  On trunk this can already be done....

Comment 9

[Johnny Stenback (:jst)]

Attachment: More elaborate fix.

This makes things more robust over all. This change replaces nsIDOMPopupBlockedEvent's requestingWindowURI with a requestingWindow argument that is the actual requesting window. And it also changes the code that used to do the docshell lookups based on the requestingWindowURI to only attempt to open a popup that came from a given window if the document in that window is still the same, if it's not then we could expose the user to security vulnerabilities.

We'll need to not change the nsIDOMPopupBlockedEvent interface on the branch, but other than that the same change should apply there.

Comment 10

[Asaf Romano (gone)]

Comment on attachment 245720 [details] [diff] [review]
More elaborate fix.

toolkit/ and browser/ changes look OK. I get the following error in http://popuptest.com/popuptest1.html though:
************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'[JavaScript Error: "evt.requestingWindow has no properties" {fil
e: "chrome://global/content/bindings/browser.xml" line: 497}]' when calling meth
od: [nsIDOMEventListener::handleEvent]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAV
ASCRIPT_ERROR_WITH_DETAILS)"  location: "<unknown>"  data: yes]
************************************************************

Comment 11

[Johnny Stenback (:jst)]

I don't see that error, and that property is on the event interface in question. Could this be due to a local build problem or somesuch?

Comment 12

[Asaf Romano (gone)]

Comment on attachment 245720 [details] [diff] [review]
More elaborate fix.

Yeah, I forgot to build in layout/build, r=mano on the toolkit/ and browser/ portions

Comment 13

[Johnny Stenback (:jst)]

(In reply to comment #8)
> Perhaps we could add branch-only scriptable APIs to:
> 
> 1)  Get an nsISupports for a principal for a window.
> 2)  Perform a same-origin check on two principals.

Yeah, we could do that but while that could fix the security problem it wouldn't make the code correct, blocked popups could still be opened using the wrong opener.

I'm happy to do something like that to plug this on the branch if we decide we need to do so, but I think we want the more elaborate fix for the trunk.

Comment 14

[Peter Van der Beken [:peterv]]

Comment on attachment 245720 [details] [diff] [review]
More elaborate fix.

>diff --git a/camino/src/browser/BrowserWindowController.mm b/camino/src/browser/BrowserWindowController.mm
>index fa7a541..fd5c444 100644
>--- a/camino/src/browser/BrowserWindowController.mm
>+++ b/camino/src/browser/BrowserWindowController.mm

>@@ -1893,15 +1894,10 @@ #ifndef MOZILLA_1_8_BRANCH

>+  if (!requestingWindow)
>     return;

No need for this null-check

>+  nsCOMPtr<nsPIDOMWindow> piDomWin = do_QueryInterface(requestingWindow);
>   if (!piDomWin)
>     return;

Comment 15

[Daniel Veditz [:dveditz]]

Clearing blocking flags for the immediate upcoming releases. Still want this fixed as soon as we get something branch-appropriate, but the required user interaction provides some protection.

Comment 16

[Johnny Stenback (:jst)]

Attachment: Updated to current trunk

Same as above, only updated to a current trunk tree.

Comment 17

[Johnny Stenback (:jst)]

Fix checked in on the trunk.

Comment 18

[Jesse Ruderman]

<reed> Jesse: Hi, can you mention in bug 354973 that jst broke Camino?

http://tinderbox.mozilla.org/showbuilds.cgi?tree=Camino

Comment 19

[Robert Sayre]

(In reply to comment #18)
> <reed> Jesse: Hi, can you mention in bug 354973 that jst broke Camino?

I landed a bustage fix with Camino reviewers. Please make sure I didn't miss the point of the security fix:

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/camino/src/browser&command=DIFF_FRAMESET&file=BrowserWindowController.mm&rev1=1.306&rev2=1.307&root=/cvsroot

Comment 20

[Samuel Sidler (old account; do not CC)]

I'm not 100% sure one what behavior I'm supposed to be seeing after this patch landed vs. before it landed. If someone can walk me through what changes I should be seeing, I'll be happy to do a functional test of the changes Robert made for Camino.

Comment 21

[Samuel Sidler (old account; do not CC)]

I walked through this with Gavin.

When pressing "Unblock", Camino now opens a new, blank window/tab. No alert at all appears. Apparently, after testing, this is standard behavior for Camino (to not show the JS alert in that case).

Thus, I think this bug can be marked 'fixed' again.

Comment 22

[Robert Sayre]

Attachment: Patch for Camino including jst's patch and follow-on bustage fix

in case this needs to go on the branch

Comment 23

[Johnny Stenback (:jst)]

Attachment: 1.8 branch diff.

This is the same as above with the exception of some changes to deal with a new interface for the new new/changed things in nsIDOMPopupBlockedEvent. What I did for the event on the branch was to leave the requestingWindowURI property and add the new requestingWindow one, and the old property continues to work. The new requestingWindow property only works if the event was initialized using the new interface.

Other than that this is the same change, but since that's not an absolutely trivial change I'm requesting re-review from peterv on the branch version of this change.

Comment 24

[Peter Van der Beken [:peterv]]

Comment on attachment 251852 [details] [diff] [review]
1.8 branch diff.

Unfortunate that we're exposing PopupBlockedEvent_MOZILLA_1_8_BRANCH to script, oh well.

Comment 25

[Daniel Veditz [:dveditz]]

Comment on attachment 251852 [details] [diff] [review]
1.8 branch diff.

approved for 1.8/1.8.0 branches, a=dveditz for drivers

Comment 26

[Johnny Stenback (:jst)]

Attachment: 1.8.0 branch diff

This is essentially the same as above, except no camino changes here as the camino popup blocking code on the 1.8.0 branch is entirely different than the newer source...

Comment 27

[Johnny Stenback (:jst)]

Fixed on both branches.

Comment 28

[Samuel Sidler (old account; do not CC)]

The checkin on the 1.8 branch broke Camino.

I've also CCed Stuart Morgan to look at making a Camino patch for the 1.8.0 branch.

Comment 29

[Stuart Morgan]

Since the vulnerability only applies to trying to find the requesting window in order to open the popup, and Camino 1.0.x doesn't have a way to open blocked popups, there's nothing to do for Camino on the 1.8.0 branch.

Comment 30

[Samuel Sidler (old account; do not CC)]

sayrer checked in a fix for the bustage on the 1.8 branch.

Comment 31

[Martijn Wargers (dead)]

Verified fixed on trunk.
I can see the bug in a 2007-01-12 build, but not anymore in the 2007-01-13 build.

Also verified that Firefox doesn't allow the XSS anymore with the testcase on the latest branch builds.