Last Comment Bug 354973 - Yet Another PopupBlocker XSS
: Yet Another PopupBlocker XSS
Status: VERIFIED FIXED
[sg:moderate]
: verified1.8.0.10, verified1.8.1.2
Product: Core
Classification: Components
Component: Security (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Johnny Stenback (:jst, jst@mozilla.com)
:
Mentors:
Depends on:
Blocks: 352465
  Show dependency treegraph
 
Reported: 2006-09-30 12:48 PDT by shutdown
Modified: 2007-02-23 13:37 PST (History)
10 users (show)
dveditz: blocking1.8.1.2+
dveditz: blocking1.8.0.10+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (505 bytes, text/html)
2006-09-30 13:24 PDT, shutdown
no flags Details
Fix. (2.72 KB, patch)
2006-11-14 16:29 PST, Johnny Stenback (:jst, jst@mozilla.com)
no flags Details | Diff | Splinter Review
More elaborate fix. (12.96 KB, patch)
2006-11-15 18:29 PST, Johnny Stenback (:jst, jst@mozilla.com)
peterv: review+
asaf: review+
Details | Diff | Splinter Review
Updated to current trunk (13.14 KB, patch)
2007-01-12 17:06 PST, Johnny Stenback (:jst, jst@mozilla.com)
no flags Details | Diff | Splinter Review
Patch for Camino including jst's patch and follow-on bustage fix (3.35 KB, patch)
2007-01-16 12:37 PST, Robert Sayre
no flags Details | Diff | Splinter Review
1.8 branch diff. (15.82 KB, patch)
2007-01-17 17:32 PST, Johnny Stenback (:jst, jst@mozilla.com)
peterv: review+
dveditz: approval1.8.1.2+
dveditz: approval1.8.0.10+
Details | Diff | Splinter Review
1.8.0 branch diff (12.36 KB, patch)
2007-01-25 17:11 PST, Johnny Stenback (:jst, jst@mozilla.com)
no flags Details | Diff | Splinter Review

Description shutdown 2006-09-30 12:48:51 PDT
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 2006-09-30 13:24:40 PDT
Created attachment 240754 [details]
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 2006-11-07 16:46:56 PST
we should try and get this for the next 1.8.x.x releases...
Comment 3 Daniel Veditz [:dveditz] 2006-11-10 11:01:22 PST
Johnny, any hope for this one?
Comment 4 Johnny Stenback (:jst, jst@mozilla.com) 2006-11-14 16:29:36 PST
Created attachment 245621 [details] [diff] [review]
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: gavin@gavinsharp.com] 2006-11-14 16:40:54 PST
Is the target of the event really the correct window? FireAbuseEvents seems to pass the topmost window to FirePopupBlockedEvent.
Comment 6 Johnny Stenback (:jst, jst@mozilla.com) 2006-11-14 16:46:17 PST
Comment on attachment 245621 [details] [diff] [review]
Fix.

Good point, need to get that info through somehow.
Comment 7 Boris Zbarsky [:bz] 2006-11-14 20:53:48 PST
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 [:bz] 2006-11-14 20:55:35 PST
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, jst@mozilla.com) 2006-11-15 18:29:42 PST
Created attachment 245720 [details] [diff] [review]
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 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2006-11-16 01:17:03 PST
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, jst@mozilla.com) 2006-11-16 11:46:53 PST
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 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2006-11-16 13:43:29 PST
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, jst@mozilla.com) 2006-11-16 17:03:00 PST
(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] 2006-11-20 21:10:30 PST
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] 2006-11-28 12:17:46 PST
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, jst@mozilla.com) 2007-01-12 17:06:42 PST
Created attachment 251330 [details] [diff] [review]
Updated to current trunk

Same as above, only updated to a current trunk tree.
Comment 17 Johnny Stenback (:jst, jst@mozilla.com) 2007-01-12 17:09:13 PST
Fix checked in on the trunk.
Comment 18 Jesse Ruderman 2007-01-12 21:00:09 PST
<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 2007-01-12 23:08:43 PST
(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) 2007-01-15 21:39:36 PST
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) 2007-01-16 11:53:23 PST
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 2007-01-16 12:37:02 PST
Created attachment 251675 [details] [diff] [review]
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, jst@mozilla.com) 2007-01-17 17:32:49 PST
Created attachment 251852 [details] [diff] [review]
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] 2007-01-23 06:38:57 PST
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] 2007-01-24 16:11:42 PST
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, jst@mozilla.com) 2007-01-25 17:11:06 PST
Created attachment 252851 [details] [diff] [review]
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, jst@mozilla.com) 2007-01-25 17:11:50 PST
Fixed on both branches.
Comment 28 Samuel Sidler (old account; do not CC) 2007-01-25 21:29:18 PST
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 2007-01-26 07:09:42 PST
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) 2007-01-26 09:44:35 PST
sayrer checked in a fix for the bustage on the 1.8 branch.
Comment 31 Martijn Wargers [:mwargers] (not working for Mozilla) 2007-02-07 16:10:46 PST
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.

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