Last Comment Bug 504862 - (CVE-2009-3988) XSS due to window.dialogArguments accessible cross-domain (MSVR-09-0047 / ZDI-CAN-535)
(CVE-2009-3988)
: XSS due to window.dialogArguments accessible cross-domain (MSVR-09-0047 / ZDI...
Status: RESOLVED FIXED
[sg:moderate][needs 1.9.1 landing]
: verified1.9.0.18, verified1.9.1
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: P1 normal (vote)
: ---
Assigned To: Johnny Stenback (:jst, jst@mozilla.com)
:
Mentors:
Depends on: 508774 528133 530447
Blocks:
  Show dependency treegraph
 
Reported: 2009-07-17 11:09 PDT by Brandon Sterne (:bsterne)
Modified: 2010-03-18 11:01 PDT (History)
15 users (show)
benjamin: blocking1.9.2+
samuel.sidler+old: blocking1.9.0.18+
samuel.sidler+old: wanted1.9.0.x+
dveditz: wanted1.8.1.x-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
beta1-fixed
.8+
.8-fixed


Attachments
testcase sets window.dialogArguments (405 bytes, text/html)
2009-07-17 11:09 PDT, Brandon Sterne (:bsterne)
no flags Details
testcase reads window.dialogArguments from other domain (947 bytes, text/html)
2009-07-17 11:10 PDT, Brandon Sterne (:bsterne)
no flags Details
Fix, still needs some cleaning up. (5.07 KB, patch)
2009-07-17 17:06 PDT, Johnny Stenback (:jst, jst@mozilla.com)
no flags Details | Diff | Review
Check in the window watcher code instead. (3.66 KB, patch)
2009-07-20 17:58 PDT, Johnny Stenback (:jst, jst@mozilla.com)
bzbarsky: superreview-
Details | Diff | Review
Updated fix. (22.39 KB, patch)
2009-08-17 17:36 PDT, Johnny Stenback (:jst, jst@mozilla.com)
mrbkap: review+
bzbarsky: superreview+
Details | Diff | Review
Additional change required for this to pass mochitest (710 bytes, patch)
2009-10-01 14:06 PDT, Johnny Stenback (:jst, jst@mozilla.com)
no flags Details | Diff | Review
Better additional fix. (1.12 KB, patch)
2009-10-06 15:45 PDT, Johnny Stenback (:jst, jst@mozilla.com)
bzbarsky: review+
Details | Diff | Review
Fix for 1.9.1 (20.19 KB, patch)
2009-11-06 16:32 PST, Johnny Stenback (:jst, jst@mozilla.com)
jst: review+
jst: superreview+
dveditz: approval1.9.1.8+
Details | Diff | Review
1.9.0 patch (32.56 KB, patch)
2009-11-11 19:04 PST, Johnny Stenback (:jst, jst@mozilla.com)
dveditz: approval1.9.0.18+
Details | Diff | Review

Description Brandon Sterne (:bsterne) 2009-07-17 11:09:32 PDT
Created attachment 389157 [details]
testcase sets window.dialogArguments

This issue was reported to security@m.o by Microsoft Vulnerability Research.  Credit should go to Hidetake Jo and Microsoft Vulnerability Research.

I can confirm in Firefox 3.5 that the properties of window.dialogArguments are accessible by script across domains.  I will attach the testcases shortly.  From the email:

-----

Title:

Name Cross-Domain window.dialogVaribles Initialization in Firefox 3.5 [MSVR-0047]

Description:

General mechanism for Cross-site scripting 

It appears that Firefox 3.5 permits cross-domain initialization of window.dialogVariables.  This behavior is unlike that of IE and Safari, and would allow exploit of XSS in web pages which assume that the page's dialogVariable can't be initialized in this way.

Technical details of exploit:

1. Find a page on the internet that is reading and using value from window.dialogArguments.

2. On evil.com create a page that calls window.showModalDialog (http://victim.com/page.xx <http://victim.com/page.xx> , ...);

2a. This allows you to initialize the window.dialogArguments on the victim domain.

3. Lure victim to the exploit page on evil.com

Finder(s):

Hidetake Jo and Microsoft Vulnerability Research.

Type of Issue:

Cross-domain initialization of window.dialogArguments enabling XSS exploits

Product(s) and version(s) that contain the bug:

Firefox 3.5

The finders have not tested against other versions of Firefox or against platforms other than Windows Vista Sp1

Special configuration requirements:

No special hardware or configuration is needed. The PoC has been demonstrated on a system with Windows Vista Sp1 + latest security updates (as of July 2009)
Comment 1 Brandon Sterne (:bsterne) 2009-07-17 11:10:43 PDT
Created attachment 389158 [details]
testcase reads window.dialogArguments from other domain
Comment 2 Blake Kaplan (:mrbkap) (please use needinfo!) 2009-07-17 12:15:57 PDT
Do window arguments have to be a variant? Could we clamp them to be a string?
Comment 3 Johnny Stenback (:jst, jst@mozilla.com) 2009-07-17 17:04:59 PDT
They have to be a variant.
Comment 4 Johnny Stenback (:jst, jst@mozilla.com) 2009-07-17 17:06:33 PDT
Created attachment 389232 [details] [diff] [review]
Fix, still needs some cleaning up.

This makes our behavior match IE's where we don't pass any arguments if the URI of the dialog is not same origin with the caller. This copies some ugly URI related code from the window watcher which I'd like to avoid, but I haven't found a cleaner way yet...
Comment 5 Johnny Stenback (:jst, jst@mozilla.com) 2009-07-20 17:58:48 PDT
Created attachment 389593 [details] [diff] [review]
Check in the window watcher code instead.

This is still a bit ugly, but a lot better, and probably about as good as it'll get here.
Comment 6 Boris Zbarsky [:bz] 2009-07-20 18:11:54 PDT
Hrm.  Is there a reason for doing it that way?  Wouldn't it make more sense to store the principal the arguments come from as part of SetNewArguments and then do a check any time we're actually binding the arguments on the inner window, against the principal of said inner window?  That would make sure that the arguments are set on an inner if and only if we're same-origin with it anyway (and hence could have just set it ourselves).  Is there an obvious problem with that, other than needing a signature change on SetNewArguments to take the principal?
Comment 7 Blake Kaplan (:mrbkap) (please use needinfo!) 2009-07-22 18:15:17 PDT
Boris: How would we deal with the returnValue bit, then?
Comment 8 Boris Zbarsky [:bz] 2009-07-22 19:22:52 PDT
Can we not examine the principal of the modal window after ShowAsModal returns and clear the returnValue if it's not same-origin with the opener principal?

Seems like that would be the smart thing to do anyway, in case the modal window gets navigated to another origin which then sets returnValue.

That said, if the modal origin is different, will caller even be able to get returnValue?
Comment 9 Blake Kaplan (:mrbkap) (please use needinfo!) 2009-07-22 23:52:30 PDT
(In reply to comment #8)
> Can we not examine the principal of the modal window after ShowAsModal returns
> and clear the returnValue if it's not same-origin with the opener principal?

Ah, right. I sort of forgot that we actually have a window by that point.

> That said, if the modal origin is different, will caller even be able to get
> returnValue?

Said getting happens in C++ (in ShowModalDialog) and is the return value from that function, so yes.
Comment 10 Brandon Sterne (:bsterne) 2009-08-06 14:56:19 PDT
FYI, ZDI TippingPoint reported this to us today via security@m.o as ZDI-CAN-535.
Comment 11 Johnny Stenback (:jst, jst@mozilla.com) 2009-08-17 17:36:33 PDT
Created attachment 394965 [details] [diff] [review]
Updated fix.

This does what bz suggested, it stores the principal where the arguments came from, and checks when the dialog is dismissed whether the caller can access the last loaded document in the dialog before getting the return value out of it. Making those changes got a bit nasty. I had to pass the principals along with the arguments, which meant changing the interface nsIScriptGlobalObject which is what had the SetNewArguments() method, which IMO doesn't belong in that interface at all. So since I was changing that interface anyways, I decided to move that method to nsPIDOMWindow, and rename it to SetArguments(), as there's nothing new about the arguments AFAICT. Getting things to work right in the nsGlobalWindow where we create new inner windows and migrate arguments from one to another was fun too.

But this patch does work, fixes the security bug and some other bad bugs in this code as well, and I think this is the way we'd want to go here for now.
Comment 12 Boris Zbarsky [:bz] 2009-08-26 15:18:41 PDT
Comment on attachment 394965 [details] [diff] [review]
Updated fix.

>@@ -8988,7 +9033,14 @@ nsGlobalModalWindow::GetDialogArguments(
>-  *aArguments = mArguments;

Good thing we're changing this code, eh?  ;)

>+  if (mArgumentsOrigin &&
>+      NS_SUCCEEDED(mArgumentsOrigin->Subsumes(GetPrincipal(), &subsumes)) &&
>+      subsumes) {

Why this particular direction for the security check?  When is this code even called?
Comment 13 Boris Zbarsky [:bz] 2009-08-26 16:03:24 PDT
With that check reversed as just discussed, looks good.
Comment 14 Johnny Stenback (:jst, jst@mozilla.com) 2009-08-31 14:43:38 PDT
Fix checked in.

http://hg.mozilla.org/mozilla-central/rev/105d89f1a33b
Comment 15 Johnny Stenback (:jst, jst@mozilla.com) 2009-08-31 15:06:42 PDT
The fix didn't stick, mochitest failures. Must be due to a recent change in the patch...
Comment 16 Reed Loden [:reed] (use needinfo?) 2009-09-11 00:13:23 PDT
(In reply to comment #15)
> The fix didn't stick, mochitest failures. Must be due to a recent change in the
> patch...

Any update on this?
Comment 17 Damon Sicore (:damons) 2009-09-29 16:41:26 PDT
Sounds like this would be an easy thing to pick off right away.  Can we get this patch updated and landed, please?
Comment 18 Johnny Stenback (:jst, jst@mozilla.com) 2009-10-01 14:06:27 PDT
Created attachment 404118 [details] [diff] [review]
Additional change required for this to pass mochitest

This is needed in addition to the previous patch for this to pass our tests. The reason for this is that there's a difference in the order of events when we're opening a modal dialog whose URL is a javascript: URI vs not. In the javascript: URI case we end up executing the JS in the URI before we've called SetNewDocument() on the global window, so a call to GetDialogArguments() can happen before the arguments are moved from the outer window to the inner window, whereas in the regular case where the content of a loaded URI asks for the dialog arguments we've already moved them to the inner.

This stuff really needs cleaning up at some point. Ideally we wouldn't attempt to share members with the dialog argument code we use for XUL dialogs, which I suspect would simplify this code a fair bit, or we should make some saner decisions regarding the time of availability of these arguments, etc. But for now, I'd just like to land this to get this blocker out of the way...
Comment 19 Boris Zbarsky [:bz] 2009-10-02 11:11:18 PDT
So with that last patch, if called on an outer we won't forward to the inner, right?  That seems wrong somehow.  Wouldn't we want to only skip forwarding if we have no inner?

I wonder whether it would make sense to have javascript: URIs trigger inner window creation before executing script.  I'm a little surprised they don't already!  But that can be a separate bug.
Comment 20 Johnny Stenback (:jst, jst@mozilla.com) 2009-10-06 15:45:15 PDT
Created attachment 404945 [details] [diff] [review]
Better additional fix.

After talking this over with bz (and sorting out some confusion regarding whether we need any additional changes at all), I think this is the way to go here. This makes it so that when opening a modal dialog and calling SetArguments() on it we make sure that we at that point set the arguments on the inner window if it has already been created. This passes mochitest.
Comment 21 Boris Zbarsky [:bz] 2009-10-06 15:48:17 PDT
Comment on attachment 404945 [details] [diff] [review]
Better additional fix.

r=bzbarsky
Comment 22 Johnny Stenback (:jst, jst@mozilla.com) 2009-10-06 17:41:38 PDT
Fixed on trunk.

http://hg.mozilla.org/mozilla-central/rev/526f516cbd46
http://hg.mozilla.org/mozilla-central/rev/d96ffc8c2749 (forgot to add the tests)
Comment 23 Johnny Stenback (:jst, jst@mozilla.com) 2009-10-07 12:28:35 PDT
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/273f3d3d2936
Comment 24 Samuel Sidler (old account; do not CC) 2009-10-14 16:41:37 PDT
Johnny, can you work up a combined 1.9.1 patch for this bug (or just check if the 1.9.2 one applies)?
Comment 25 Brandon Sterne (:bsterne) 2009-10-16 10:20:34 PDT
This also affects 1.9.0.  Johnny, can you please land the fix there as well?
Comment 26 Samuel Sidler (old account; do not CC) 2009-11-04 15:38:58 PST
Johnny: We really need a patch here. Code freeze for 1.9.0 and 1.9.1 is November 10 at 11:59pm.
Comment 27 Johnny Stenback (:jst, jst@mozilla.com) 2009-11-06 16:32:18 PST
Created attachment 410897 [details] [diff] [review]
Fix for 1.9.1

This is applied w/o any real problems to 1.9.1, but I had to create a new nsPIDOMWindow_1_9_1 interface to maintain interface compatibility.
Comment 28 Johnny Stenback (:jst, jst@mozilla.com) 2009-11-06 16:32:47 PST
Oh, and passes mochitest locally when applied to 1.9.1.
Comment 29 Reed Loden [:reed] (use needinfo?) 2009-11-09 10:12:32 PST
(In reply to comment #27)
What about 1.9.0?
Comment 30 Daniel Veditz [:dveditz] 2009-11-09 14:52:04 PST
Comment on attachment 410897 [details] [diff] [review]
Fix for 1.9.1

Approved for 1.9.1.6, a=dveditz for release-drivers
Comment 31 Samuel Sidler (old account; do not CC) 2009-11-09 14:52:45 PST
Johnny: Please don't forget a 1.9.0 patch here.
Comment 32 Johnny Stenback (:jst, jst@mozilla.com) 2009-11-10 17:21:03 PST
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/7b5e903dffe1
Comment 33 Johnny Stenback (:jst, jst@mozilla.com) 2009-11-11 16:51:37 PST
Clearing 1.9.0 blocking flag since this feature didn't ship until 1.9.1.
Comment 34 Samuel Sidler (old account; do not CC) 2009-11-11 17:13:13 PST
Restoring flags. Johnny was misinformed. :)
Comment 36 Johnny Stenback (:jst, jst@mozilla.com) 2009-11-11 19:04:16 PST
Created attachment 411872 [details] [diff] [review]
1.9.0 patch

This is going through try server right now, passes the relevant tests locally.
Comment 37 Johnny Stenback (:jst, jst@mozilla.com) 2009-11-13 15:53:41 PST
Comment on attachment 411872 [details] [diff] [review]
1.9.0 patch

Requesting approval for this patch, even though I just actually landed this thinking that it was already approved. So, let me know if this is all good, or if I should back this out of CVS.
Comment 38 Daniel Veditz [:dveditz] 2009-11-13 16:10:52 PST
Comment on attachment 411872 [details] [diff] [review]
1.9.0 patch

Approved for 1.9.0.16, a=dveditz for release-drivers
Comment 39 Al Billings [:abillings] 2009-11-20 12:45:21 PST
Verified fixed for 1.9.1.6 using testcase and build Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.6pre) Gecko/20091119 Shiretoko/3.5.6pre (.NET CLR 3.5.30729). 

Verified for 1.9.0.16 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.16pre) Gecko/2009111921 GranParadiso/3.0.16pre (.NET CLR 3.5.30729).
Comment 40 Samuel Sidler (old account; do not CC) 2009-12-01 11:55:25 PST
This caused a leak on 1.9.1: bug 528133.
Comment 41 Samuel Sidler (old account; do not CC) 2009-12-01 14:43:27 PST
Talked this over with Johnny. We need to backout this fix across all branches and respin both releases without it. (This *should* be causing a leak on 1.9.0, per him, but isn't for some reason.)

Johnny, please back this out on... 1.9.1, 1.9.0, GECKO1916_20091130_RELBRANCH (hg), and GECKO190_20091130_RELBRANCH (cvs).
Comment 42 Johnny Stenback (:jst, jst@mozilla.com) 2009-12-01 15:21:05 PST
Backed out in all those 4 places :(
Comment 43 Samuel Sidler (old account; do not CC) 2009-12-01 15:43:10 PST
Johnny, you should make sure this doesn't affect 1.9.2 somehow (despite not seeing it right now) because if we need to back it out there, we need to do so ASAP.
Comment 44 Johnny Stenback (:jst, jst@mozilla.com) 2009-12-01 16:00:17 PST
Yeah, but I don't think I can do that before I understand this leak, as there is no evidence on 1.9.2 of a leak like the one we see in 1.9.1 with this patch.
Comment 45 Peter Van der Beken [:peterv] 2009-12-02 03:35:10 PST
The leaks are due to bug 508774, which was fixed on trunk before 1.9.2 branched, but after 1.9.1. I've asked for approval on the patch in bug 508774.
Comment 46 Johnny Stenback (:jst, jst@mozilla.com) 2009-12-02 10:01:32 PST
Thanks Peter for figuring that out!
Comment 47 Daniel Veditz [:dveditz] 2009-12-02 15:13:49 PST
Comment on attachment 410897 [details] [diff] [review]
Fix for 1.9.1

Approved for 1.9.1.7, a=dveditz for release-drivers
Comment 48 Daniel Veditz [:dveditz] 2009-12-02 15:14:16 PST
Comment on attachment 411872 [details] [diff] [review]
1.9.0 patch

Approved for 1.9.0.16, a=dveditz for release-drivers

Please land bug 508774 first
Comment 49 Samuel Sidler (old account; do not CC) 2009-12-02 19:13:31 PST
(dveditz of course meant 1.9.0.17 as evidenced by the flag.)
Comment 50 Johnny Stenback (:jst, jst@mozilla.com) 2010-01-15 13:52:20 PST
Fixed for 1.9.1.8 and 1.9.0.18.
Comment 51 Henrik Skupin (:whimboo) 2010-01-28 13:45:42 PST
(In reply to comment #39)
> Verified fixed for 1.9.1.6 using testcase and build Mozilla/5.0 (Windows; U;
> Windows NT 5.1; en-US; rv:1.9.1.6pre) Gecko/20091119 Shiretoko/3.5.6pre (.NET
> CLR 3.5.30729). 

Al, I have problems to reproduce this issue with Firefox 3.5.7. Given the revoked flags we have backed-out the fix for 3.5.7 and the failure should be visible. Can you please verify it with 3.5.8pre?
Comment 52 Al Billings [:abillings] 2010-02-01 17:05:53 PST
The first testcase repros cleanly in 1.9.1.7 but the second one ("testcase reads window.dialogArguments from other domain") returns "undefined" for both values. This is the fixed behavior. The first testcase does the same in 1.9.1.8 (and no longer displays the dialog about injection).

This is fixed in 1.9.1.8, verified with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.8pre) Gecko/20100201 Shiretoko/3.5.8pre (.NET CLR 3.5.30729).
Comment 53 Al Billings [:abillings] 2010-02-10 17:36:15 PST
The same behavior happens on 1.9.0 as what I saw in 1.9.1. Verified fixed in 1.9.1.8 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.16pre) Gecko/2010020215 GranParadiso/3.0.18pre (.NET CLR 3.5.30729).

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