Yet Another PopupBlocker XSS

VERIFIED FIXED

Status

()

Core
Security
VERIFIED FIXED
11 years ago
10 years ago

People

(Reporter: shutdown, Assigned: jst)

Tracking

({verified1.8.0.10, verified1.8.1.2})

Trunk
verified1.8.0.10, verified1.8.1.2
Points:
---
Bug Flags:
blocking1.8.1.2 +
blocking1.8.0.10 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:moderate])

Attachments

(6 attachments, 1 obsolete attachment)

(Reporter)

Description

11 years ago
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...
(Reporter)

Comment 1

11 years ago
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.)
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]

Comment 2

11 years ago
we should try and get this for the next 1.8.x.x releases...
Johnny, any hope for this one?
(Assignee)

Comment 4

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

Comment 6

11 years ago
Comment on attachment 245621 [details] [diff] [review]
Fix.

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

Comment 9

11 years ago
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.
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 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]
************************************************************
(Assignee)

Comment 11

11 years ago
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+
(Assignee)

Comment 13

11 years ago
(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/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;
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+
Blocks: 352465
(Assignee)

Comment 16

10 years ago
Created attachment 251330 [details] [diff] [review]
Updated to current trunk

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

Comment 17

10 years ago
Fix checked in on the trunk.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Comment 18

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

http://tinderbox.mozilla.org/showbuilds.cgi?tree=Camino
Attachment #251330 - Attachment is patch: true
Attachment #251330 - Attachment mime type: application/octet-stream → text/plain

Comment 19

10 years ago
(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

Updated

10 years ago
Status: RESOLVED → REOPENED
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.

Updated

10 years ago
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago10 years ago
Resolution: --- → FIXED

Comment 22

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

Comment 23

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

Comment 26

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

Comment 27

10 years ago
Fixed on both branches.
Keywords: fixed1.8.0.10, fixed1.8.1.2
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

10 years ago
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.
Status: RESOLVED → VERIFIED
Flags: blocking1.9?
Keywords: fixed1.8.0.10, fixed1.8.1.2 → verified1.8.0.10, verified1.8.1.2
Group: security
You need to log in before you can comment on or make changes to this bug.