504862 - (CVE-2009-3988) XSS due to window.dialogArguments accessible cross-domain (MSVR-09-0047 / ZDI-CAN-535)

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P1)

Description

[Brandon Sterne (:bsterne)]

Attachment: 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)]

Attachment: testcase reads window.dialogArguments from other domain

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Do window arguments have to be a variant? Could we clamp them to be a string?

Comment 3

[Johnny Stenback (:jst)]

They have to be a variant.

Comment 4

[Johnny Stenback (:jst)]

Attachment: 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)]

Attachment: 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 [:bzbarsky]]

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) (inactive)]

Boris: How would we deal with the returnValue bit, then?

Comment 8

[Boris Zbarsky [:bzbarsky]]

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) (inactive)]

(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)]

FYI, ZDI TippingPoint reported this to us today via security@m.o as ZDI-CAN-535.

Comment 11

[Johnny Stenback (:jst)]

Attachment: 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 [:bzbarsky]]

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 [:bzbarsky]]

With that check reversed as just discussed, looks good.

Comment 14

[Johnny Stenback (:jst)]

Fix checked in.

http://hg.mozilla.org/mozilla-central/rev/105d89f1a33b

Comment 15

[Johnny Stenback (:jst)]

The fix didn't stick, mochitest failures. Must be due to a recent change in the patch...

Comment 16

[Reed Loden [:reed]]

(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)]

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)]

Attachment: 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 [:bzbarsky]]

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)]

Attachment: 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 [:bzbarsky]]

Comment on attachment 404945 [details] [diff] [review]
Better additional fix.

r=bzbarsky

Comment 22

[Johnny Stenback (:jst)]

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)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/273f3d3d2936

Comment 24

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

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)]

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)]

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)]

Attachment: 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)]

Oh, and passes mochitest locally when applied to 1.9.1.

Comment 29

[Reed Loden [:reed]]

(In reply to comment #27)
What about 1.9.0?

Comment 30

[Daniel Veditz [:dveditz]]

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)]

Johnny: Please don't forget a 1.9.0 patch here.

Comment 32

[Johnny Stenback (:jst)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/7b5e903dffe1

Comment 33

[Johnny Stenback (:jst)]

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)]

Restoring flags. Johnny was misinformed. :)

Comment 36

[Johnny Stenback (:jst)]

Attachment: 1.9.0 patch

This is going through try server right now, passes the relevant tests locally.

Comment 37

[Johnny Stenback (:jst)]

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]]

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 - ex-MoCo]]

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)]

This caused a leak on 1.9.1: bug 528133.

Comment 41

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

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)]

Backed out in all those 4 places :(

Comment 43

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

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)]

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]]

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)]

Thanks Peter for figuring that out!

Comment 47

[Daniel Veditz [:dveditz]]

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]]

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)]

(dveditz of course meant 1.9.0.17 as evidenced by the flag.)

Comment 50

[Johnny Stenback (:jst)]

Fixed for 1.9.1.8 and 1.9.0.18.

Comment 51

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(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 - ex-MoCo]]

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 - ex-MoCo]]

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