Closed Bug 354973 Opened 14 years ago Closed 13 years ago

Yet Another PopupBlocker XSS


(Core :: Security, defect)

Not set





(Reporter: sync2d, Assigned: jst)



(Keywords: verified1.8.0.10, verified1.8.1.2, Whiteboard: [sg:moderate])


(6 files, 1 obsolete file)

URI based window lookup is not safe for URIs such as data:, javascript:, etc...

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...
Attached file 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:
  Gecko/20060930 Firefox/
  ("Force links that open new window to open in" needs to be disabled.)
Flags: blocking1.9?
Flags: blocking1.8.1.1?
Flags: blocking1.8.0.9?
Assignee: dveditz → jst
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x+
Whiteboard: [sg:moderate]
we should try and get this for the next 1.8.x.x releases...
Johnny, any hope for this one?
Attached patch Fix. (obsolete) — Splinter Review
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.
Attachment #245621 - Flags: superreview?(dveditz)
Attachment #245621 - Flags: review?(peterv)
Is the target of the event really the correct window? FireAbuseEvents seems to pass the topmost window to FirePopupBlockedEvent.
Comment on attachment 245621 [details] [diff] [review]

Good point, need to get that info through somehow.
Attachment #245621 - Attachment is obsolete: true
Attachment #245621 - Flags: superreview?(dveditz)
Attachment #245621 - Flags: review?(peterv)
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)?
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....
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.
Attachment #245720 - Flags: superreview?(mano)
Attachment #245720 - Flags: review?(peterv)
Comment on attachment 245720 [details] [diff] [review]
More elaborate fix.

toolkit/ and browser/ changes look OK. I get the following error in 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]
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 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
Attachment #245720 - Flags: superreview?(mano) → review+
(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.
Flags: blocking1.8.1.1?
Flags: blocking1.8.1.1+
Flags: blocking1.8.0.9?
Flags: blocking1.8.0.9+
Comment on attachment 245720 [details] [diff] [review]
More elaborate fix.

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

>@@ -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;
Attachment #245720 - Flags: review?(peterv) → review+
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.
Flags: blocking1.8.1.1+
Flags: blocking1.8.0.9+
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x+
Flags: blocking1.8.1.2+
Flags: blocking1.8.0.10+
Same as above, only updated to a current trunk tree.
Fix checked in on the trunk.
Closed: 13 years ago
Resolution: --- → FIXED
<reed> Jesse: Hi, can you mention in bug 354973 that jst broke Camino?
Attachment #251330 - Attachment is patch: true
Attachment #251330 - Attachment mime type: application/octet-stream → text/plain
(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:
Resolution: FIXED → ---
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.
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.
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
in case this needs to go on the branch
Attached patch 1.8 branch diff.Splinter Review
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.
Attachment #251852 - Flags: review?(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.
Attachment #251852 - Flags: review?(peterv) → review+
Comment on attachment 251852 [details] [diff] [review]
1.8 branch diff.

approved for 1.8/1.8.0 branches, a=dveditz for drivers
Attachment #251852 - Flags: approval1.8.1.2+
Attachment #251852 - Flags: approval1.8.0.10+
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...
Fixed on both branches.
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.
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.
sayrer checked in a fix for the bustage on the 1.8 branch.
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.
Flags: blocking1.9?
Group: security
You need to log in before you can comment on or make changes to this bug.