334891 - Dialog origin spoofing by closing tab that spawned dialog

Status: RESOLVED FIXED

(SeaMonkey :: Tabbed Browser, defect)

Description

[Jesse Ruderman]

A malicious web page can make it look like another page (e.g. a bank) is showing the user a dialog by spawning multiple tabs and closing one while a dialog is visible.  The effect on security is the same as bug 262887 part 2, but the exploit is different.

The exploit below assumes you have "Redirect new-window links into new tabs instead" or whatever it's called this week turned on.

Comment 1

[Jesse Ruderman]

Attachment: demo

Demo for this bug and bug 334893.

Comment 2

[Jesse Ruderman]

Attachment: screenshot (mac os x)

Screenshot showing the attacker's dialog, along with a normal address bar for https://www.wellsfargo.com/.

Comment 3

[Daniel Veditz [:dveditz]]

That tabbify-new-windows isn't the default, or did that change on the trunk?

bug 262887 was before we added the host to the prompt dialog titles which would also minimize the spoofing, at least for users paying attention.

Comment 4

[Jesse Ruderman]

Does that mean you consider bug 334893 to be more serious than this one?  You'd have to be paying quite a bit of attention to not fall for an exploit combining the two, such as the exploit attached to this bug.

Comment 5

[Jesse Ruderman]

tabbify-new-windows is default on trunk, and it's a popular option in 1.5.0.x.

Comment 6

[Jesse Ruderman]

I'd like to think that this is more severe than bug 334893, if only because bug 334893 seems more open-ended and hard to fix.

Comment 7

[Daniel Veditz [:dveditz]]

In the new-window case the modal alert is torn down with the closed window. Is there a way to do the equivalent for tabs? If not, can we prevent .close() on a tab with a modal dialog? That introduces a behavior difference that web-apps may not be able to handle since they can't tell the difference when open() has given them a tab or a window.

Interesting bug your spoof reveals (in 1.5 at least):
  open("data:text/html,<script>alert(document.domain)</script>")

In the new window case that says bugzilla.mozilla.org (correctly). In the new-tab case that says "null". Opened bug 334973 for that unrelated problem.

Comment 8

[Daniel Veditz [:dveditz]]

No patch in sight and requires a non-default setting to work in current (1.8.0.x) releases. Not blocking 1.8.0.5 but want for later releases and would take a safe patch if we get one.

Comment 9

[Mike Schroepfer]

JST - do you have time for this or recommendations on who could look at it?

Comment 10

[Johnny Stenback (:jst)]

Attachment: First stab at a fix.

This seems to fix this bug, at least as far as the testcase goes. What it does it so make the DOM window code keep track of whether a window has modal dialogs opened against it (though just alert, prompt, and confirm dialogs), and if it does it makes window.close() a no-op.

For one, this should be moved into the prompt code (embedding/componentns/windowwatcher/src/nsPrompt.cpp) if we care about blocking close() for other modal dialogs (HTTP-Auth dialogs come to mind), but I want feedback on this approach in general before I write that up.

Thoughts? And Jesse, any way you can apply this to a tree and test it?

Comment 11

[Johnny Stenback (:jst)]

Comment on attachment 227184 [details] [diff] [review]
First stab at a fix.

Ok, I talked to bz about this and we agree that this is the right direction, but as I said in my previous comment we need to push this into nsPrompt.cpp to make sure all our modal dialogs get the same treatments, not just window.alert() and friends. I can do that, but not before next week (unless I manage to find some spare time tonight for this), so if we need this before then someone else needs to step up.

What needs to be done is pretty straight forward, EnterModalState() and LeaveModalState() needs to be pushed into an interface (nsPIDOMWindow most likely) and then used in nsPrompt.cpp around all the calls to open modal dialogs. Oh, and openDialog() might need this too if that's used to open a modal dialog (only callable from chrome code).

Comment 12

[Johnny Stenback (:jst)]

Attachment: Same as above with modal state management moved to nsPrompt.

This does what was discussed above. Should be good to go, but I won't have time to get this into the tree before next week, so if someone wants to jump in and land this sooner, great, otherwise I'll do it next week.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 227361 [details] [diff] [review]
Same as above with modal state management moved to nsPrompt.

>diff --git a/dom/public/base/nsPIDOMWindow.h b/dom/public/base/nsPIDOMWindow.h

>+  virtual void EnterModalState() = 0;
>+  virtual void LeaveModalState() = 0;

So is this the way we expect embeddors to handle this?  Or do we want to expose some other API for them?

>diff --git a/dom/src/base/nsGlobalWindow.cpp b/dom/src/base/nsGlobalWindow.cpp
>+nsGlobalWindow::IsInModalState()

Should we just document that this should only be called on toplevel windows and assert that we're toplevel instead of doing GetTop in opt builds?  Or o we not really care (and this is safer)?

I do think we should document somewhere that embeddors or anyone overriding nsPrompt should deal with this...

sr=bzbarsky in any case, but I do think we should make this a little more embedding-friendly; I'm just not sure how.

Comment 14

[Daniel Veditz [:dveditz]]

Comment on attachment 227361 [details] [diff] [review]
Same as above with modal state management moved to nsPrompt.

r=dveditz

Comment 15

[Johnny Stenback (:jst)]

(In reply to comment #13)
> (From update of attachment 227361 [details] [diff] [review] [edit])
> >diff --git a/dom/public/base/nsPIDOMWindow.h b/dom/public/base/nsPIDOMWindow.h
> 
> >+  virtual void EnterModalState() = 0;
> >+  virtual void LeaveModalState() = 0;
> 
> So is this the way we expect embeddors to handle this?  Or do we want to expose
> some other API for them?

For now this is it. Long term we need a way for embedders to handle this *and* the focusing of tabs when a tab opens a modal dialog etc.

> >diff --git a/dom/src/base/nsGlobalWindow.cpp b/dom/src/base/nsGlobalWindow.cpp
> >+nsGlobalWindow::IsInModalState()
> 
> Should we just document that this should only be called on toplevel windows and
> assert that we're toplevel instead of doing GetTop in opt builds?  Or o we not
> really care (and this is safer)?

I say we don't care. It's cheap enough to make this callable on nested windows, so we may just as well support that as well.

> I do think we should document somewhere that embeddors or anyone overriding
> nsPrompt should deal with this...
> 
> sr=bzbarsky in any case, but I do think we should make this a little more
> embedding-friendly; I'm just not sure how.

Agreed, but I don't have an answer for that yet either.

Comment 16

[Johnny Stenback (:jst)]

Fix landed on the trunk.

Comment 17

[Darin Fisher]

Going to let this bake on the trunk before taking it for the 1.8 branch.  Targeting FF2 beta2.

Comment 18

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 227361 [details] [diff] [review]
Same as above with modal state management moved to nsPrompt.

a=dbaron on behalf of drivers.  Please check in to MOZILLA_1_8_BRANCH and mark fixed1.8.1 once you have.

Comment 19

[Johnny Stenback (:jst)]

Attachment: 1.8 branch version.

Same thing, but w/o changing nsPIDOMWindow directly.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Let's make sure we file a followup bug on embedding APIs for this or something...  Would be nice to fix for 1.9.

Comment 21

[Johnny Stenback (:jst)]

This landed on the 1.8 branch yesterday towards the evening.

Comment 22

[[e-mail]]

I saw discussion on IRC (no bug numbers were mentioned, unfortunately) that pointed out that this breaks goQuitApplication as that assumes that all windows may be closed in the order that nsIWindowMediator::GetEnumerator returns them.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Please get a bug filed on that?  That's a bad assumption that should be fixed.

Comment 24

[Dave Townsend [:mossop]]

(In reply to comment #23)
> Please get a bug filed on that?  That's a bad assumption that should be fixed.

This is actually what is going on in bug 400479. However reordering the closes doesn't help.