326877 - menupopup.showPopup() can be abused

Status: RESOLVED INVALID

(Core :: XUL, defect, P2)

Description

[Jesse Ruderman]

menupopup.showPopup() lets a site bring up a menu above other tabs, above windows from other applications, etc.

I don't know whether the menus are limited in appearance (text menu items only).  If they are limited, this bug is mostly an annoyance and possible ad vector; if they aren't limited, this bug could be used to spoof things.

Comment 1

[Jesse Ruderman]

Attachment: demo: opens a menupopup after 3 seconds

Comment 2

[Jesse Ruderman]

Attachment: screenshot: menupopup over a Terminal window

Comment 3

[Daniel Veditz [:dveditz]]

You can definitely add images (and then take away the text) and style them any way you like. Hard to say what you could really do with this other than have someone click on it and perform some action back in a tab they weren't viewing at the time. Or obscuring/blocking the site they were on.

Should this be opened up?

Comment 4

[Jesse Ruderman]

Attachment: demo with multiple menupopups

Comment 5

[Jesse Ruderman]

A site can open multiple menupopups at once (even it it's only open in a single browser tab).  With four or more menupopups, it could completely surround the "Install" button of a security dialog.  According to comment 3, the stuff surrounding the button could be an arbitrary image.

Bug 260560 fixed the button enabling delay (introduced in bug 162020) to take focus into account, but menupopups neither take focus nor steal clicks (except that the newest one steals some clicks, which doesn't hurt an attacker much).

That means this is a bug where a clever attacker could convince a user to click "Install" in an unseen security dialog.  The severity of these kinds of bugs has ranged from sg:low (bug 260560) to sg:critical (bug 162020).  Bug 260560's severity was sg:low because browser window borders and status bars could arouse suspicion.  Here, there are only menu borders.  On the two platforms I tested, I don't think window borders would arouse enough suspicion:

* On Mac, there are no menu borders.  There are shadows under menus, but an attacker using a dark background could cancel those out in most directions by using a reverse gradient exactly where the shadows will appear.

* On Windows 98, menu borders are 1px dark gray.  This border could easily be uesd as part of a spoof.

Since a spoof attack against a security dialog could be almost perfect on Windows 98 and Mac, I think this should be sg:critical.

(Note: I used Mac for most of the testing in this comment.  If Windows differs in an important way, for example in how it handles focus, this might not be sg:critical on Windows.)

Comment 6

[Darin Fisher]

jst, bz: any thoughts on what it would take to fix this bug?  any idea who might be able to do the work?

Comment 7

[Boris Zbarsky [:bzbarsky]]

What would constitute a "fix"?  That is, what do we consider safe behavior and how much functionality are we willing to remove?

For example, we could make showPopup() throw (or do nothing) from untrusted code. That fixes this bug; is the cost worth it?

Comment 8

[Darin Fisher]

It would seem to me that we should try to treat this like the way we treat alerts.  Just select and focus the tab that calls showPopup.

Comment 9

[Boris Zbarsky [:bzbarsky]]

That doesn't fix the issue Jesse is having with the popup covering up other dialogs (which are in other windows).

Comment 10

[Darin Fisher]

Hrm.. good point.  It's as if we really want the menupopup to be implemented more like a floating div would be implemented, but that doesn't sound like a trivial fix at all.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Yeah, esp. given that we do want menupopups to stick out of the window.

Comment 12

[Daniel Veditz [:dveditz]]

no comments since March, this obviously isn't making 1.8.0.4

Need a decent owner for this.

Comment 13

[Jesse Ruderman]

*** Bug 339333 has been marked as a duplicate of this bug. ***

Comment 14

[georgi - hopefully not receiving bugspam]

(In reply to comment #7)
> 
> For example, we could make showPopup() throw (or do nothing) from untrusted
> code. That fixes this bug; is the cost worth it?
> 

i believe this is the real solution.

Comment 15

[Daniel Veditz [:dveditz]]

Blocking explicit showPopup() from untrusted code wouldn't be so bad if the automatic behavior of button, menu, menulist, and popupset continued to work. Assuming those don't respond to synthetic click events that might be safe enough and still provide enough functionality.

Since XBL isn't trusted code, widgets using showPopup() would presumably break in non-chrome (remote) XUL. Looks like that includes autocomplete, colorpicker, popup, tree, richlistbox, and notification. Tooltip extends popup. That breaks just about everything, right?

Comment 16

[Daniel Veditz [:dveditz]]

Realistically we don't know how to fix this for 1.8.0.5, moving out.

Comment 17

[Mike Schroepfer]

If we can agree on a fix - we'd like it for 1.8.1

Comment 18

[Johnny Stenback (:jst)]

Sounds like a view problem. Roc, can you have a look at this?

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It's not really a view bug. It's more a XUL/DOM bug.

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

How about we just refuse to allow popups to be visible for windows that don't have focus?

Comment 21

[georgi - hopefully not receiving bugspam]

note that xul popups kind of circumvent the window.open() popup blocker.

Comment 22

[Matthew Gertner]

It's possible that blocking popups for non-focused windows would be an issue for https://bugzilla.mozilla.org/show_bug.cgi?id=325353. If this is the chosen route, perhaps it shouldn't apply to those with chrome privileges?

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Sure, that wouldn't be hard.

If we go this way, maybe this is a job for one of the Neils.

Comment 24

[Darin Fisher]

Not going to hold FF2 beta1 for this, but keeping this on the radar for FF2.

Comment 25

[Neil Deakin]

Do you mean for unprivileged content, hide popups when the containing window/tab isn't focused, and then show the popups again when it is? Or just prevent opening popups in background windows?

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The former, although showing them again when it's focused maybe isn't needed.

Comment 27

[Jesse Ruderman]

If we go with that solution (rather than blocking showPopup() entirely), will it be possible for pages to make popups appear way outside of the content area?  Without borders?

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yes, so it's still possible to spoof other windows. But then, it's generally possible to do that just by displaying a PNG in the content area.

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Hmm, I was going to say that it does prevents people from clicking "Install" or "OK" on some security dialog where popups are hiding the informational parts of the dialog, but I guess that's still possible. E.g. "open window 1, trigger security dialog, open window 2 (taking focus), window 2 opens popups covering parts of security dialog".

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Maybe instead we should try having untrusted popups just be displayed as regular, positioned, child content. This might be as simple as hacking XUL popup frames to not have "floating" nsIViews and to create a child widget instead of a popup widget.

Comment 31

[Daniel Veditz [:dveditz]]

Would love a fix, so plussing 1.8.0.6 to match the 1.8.1 state -- but I'm not very confident we'll actually get a fix we can take on the branch :-(

comment #30 sounds promising though.

Comment 32

[Mike Schroepfer]

Doesn't look like we are on a path to get a fix in time for 1.8.1 - so I'm taking off the list.  If we get something together which is safe enough please re-nom.

Comment 33

[Daniel Veditz [:dveditz]]

Restoring lost blocking flag

Comment 34

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

(In reply to comment #3)
> You can definitely add images (and then take away the text) and style them any
> way you like. Hard to say what you could really do with this other than have
> someone click on it and perform some action back in a tab they weren't viewing
> at the time. Or obscuring/blocking the site they were on.
> 
> Should this be opened up?

It's possible to be very evil given the default behavior of Firefox 2 of opening new windows in new tabs: http://ctho.ath.cx/tmp/moz/326877evil.xul

It's also possible to be extremely obnoxious: http://ctho.ath.cx/tmp/crash.xul  If you put the iframe back (view the source) and remove the button it can actually get pretty difficult to get rid of the popup.

Comment 35

[georgi - hopefully not receiving bugspam]

supporting bz blocking1.9+

the m$ <CENSORED> suffered from this iirc.

the bugzilla replied:

 You tried to grant blocking1.9. Only a sufficiently empowered user can make this change.

please excuse my lack of empoweredness, sorry.

Comment 36

[Daniel Veditz [:dveditz]]

Is there no way to kill the popups when the page loses focus? ctho's testcase in comment 34 shows how evil this can be (it's not perfect, but scary enough).

Comment 37

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

Another possible solution -- make the menupopups use more normal widgets for content (rather than chrome).  In other words, forbid content from making menupopups that stick out of the window or show up in other contexts.

Comment 38

[Neil Deakin]

Any solution involving closing popups on blur would be simple, but because the platform-specific widget code only keeps track of one popup at a time, it's very easy to get into a state where a popup hangs on screen. I'm not sure how hard it would be to keep track of all popups for 1.8

Getting popups to be regular non-popup widgets is fairly easy. I just tried it and it worked. Getting them to close is harder. Currently the widget code handles closing popups when a click outside them occurs, so this would need be translated into something that works when there isn't.

Comment 39

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Neil, do you want to take this? If not, would you mind posting what you did to make popups not be popups?

Comment 40

[Neil Deakin]

I just commented out the call to CreateWidgetForPopup in nsMenuPopupFrame::Init, but it should only be done in a non-chrome context.

That fixes the 'abuse' part but doesn't make the popups work properly because they don't close any more.

Comment 41

[Neil Deakin]

Attachment: Don't open popups in unfocused windows

This patch prevents opening popups from content docshells that aren't the primary one and aren't in the active window.

Comment 42

[Boris Zbarsky [:bzbarsky]]

That patch doesn't allow a subframe of a primary content shell to open popups.  Is that on purpose?

Comment 43

[Neil Deakin]

No, that wasn't intended. Is it sufficient to retrieve the shell's sameTypeRootTreeItem and check if that is a primary frame?

Comment 44

[Boris Zbarsky [:bzbarsky]]

Yeah, getting the sameTypeRootTreeItem is what we would want for that.

But I thought about this a bit more, and checking whether something is the primary shell is NOT what we want here.  That happens to work for Firefox and Seamonkey, but breaks in any embedding app.

What we probably want instead is the same check that we use for nsGenericElement::ShouldFocus, nsGlobalWindow::GetContent,  nsGlobalWindow::Focus, etc -- checking GetVisibility on the docshell QIed to nsIBaseWindow.  This can be done on the subframe just fine.  In fact, it can be done by just QIing the presshell to nsIViewObserver and calling IsVisible() on it.

Comment 45

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 251455 [details] [diff] [review]
Don't open popups in unfocused windows

Two more comments:

1) I assume the focus controller thing works in embeddin apps too?  Get a Camino someone to test?

2) Test carefully with bfcache.  I _think_ this should work correctly because we null out the container on bfcached prefcontexts, but still.  I'd especially like to see tests for showPopup() being called in onPageShow and onPageHide handlers as one navigates back and forward in history.

Comment 46

[Neil Deakin]

Hmmm, no it doesn't seem to prevent popups during a pagehide event. Is there a way to detect this state?

Comment 47

[Boris Zbarsky [:bzbarsky]]

Hmm.  Do we then proceed to hide the page, so that the popup looks like it's coming from a different page?

If so, do we have the same problem with onunload?

Does PresShell::Freeze() get called before or after onpagehide?  If the latter (as I suspect), would it make sense to take down popups in Freeze()?

Comment 48

[Neil Deakin]

When the document is destroyed after an unload event, the popups disappear because the views for their frames are deleted. The pagehide doesn't destroy the frames, but it should be hiding all the views. I suppose the Freeze() call should iterate over the view tree and call SetViewVisibility on ones that are associated with popups.

Comment 49

[Boris Zbarsky [:bzbarsky]]

Or it could just iterate the popup lists for the various popupsets, no?

On trunk we might need another solution too, because we want views to go away...

Comment 50

[Neil Deakin]

Assuming you mean nsPopupSetFrame, no, as the popups associated with menus aren't in one.

For the trunk, we might be able to use the popup list added in bug 324963, although looking at the patch it only seems to keep track of menu popups. Of course, when the big patch in 279703 lands, we'll be able to just use the list the popup manager maintains.

Comment 51

[Boris Zbarsky [:bzbarsky]]

OK.  Should probably just do whatever is simplest, then.

How do things behave right now if a page is navigated away from while it has a popup up?

Comment 52

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think we want the list in bug 324963 to go away in favour of 279703.

Comment 53

[Neil Deakin]

Attachment: also close popups

This patch implements comment 44. Also it closes the open popups when the document is hidden/unloaded.

The location in DocumentViewerImpl::PageHide seems to work properly, Freeze doesn't seem to be called all the time. Is it only called for html and not xhtml?

Comment 54

[Jay Patel [:jay]]

Neil: Anyone else around to review your patch?  bz seems a bit busy and we would like to get this in for 1.8.1.2/1.8.0.10.  

Comment 56

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 251809 [details] [diff] [review]
also close popups

>Index: layout/base/nsDocumentViewer.cpp

Is nsIMenuParent available if MOZ_XUL is not defined?

If not, please put the relevant part of the changes to this file inside MOZ_XUL.

Add a comment saying that we want to close popups after the unload handler has fired, in case it opens some.

r+sr=bzbarsky with that.

Though on a side note, do we want to close the popups even on some of the error conditions in PageHide?  That is, do we want to move that event-firing code into a DoPageHide or something and close the popups after that no matter what it returns?

Comment 57

[Daniel Veditz [:dveditz]]

Does this patch work for the branches or do we need a separate branch-appropriate patch?

Comment 58

[Neil Deakin]

(In reply to comment #56)
> (From update of attachment 251809 [details] [diff] [review])
> >Index: layout/base/nsDocumentViewer.cpp
> 
> Is nsIMenuParent available if MOZ_XUL is not defined?
> 
nsIMenuParent appears to still be built, but menupopups aren't, so maybe this should be inside a MOZ_XUL anyway.

> Though on a side note, do we want to close the popups even on some of the error
> conditions in PageHide?  That is, do we want to move that event-firing code
> into a DoPageHide or something and close the popups after that no matter what
> it returns?

Would there be a case where the document or window isn't present yet the presshell is still valid?

Comment 59

[Boris Zbarsky [:bzbarsky]]

> Would there be a case where the document or window isn't present yet the
> presshell is still valid?

I don't think so, no.

Comment 60

[Neil Deakin]

Attachment: branch patch

The branch patch is the same changes, although the existing code in PageHide is different which is just indented.

Comment 61

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 252798 [details] [diff] [review]
branch patch

Looks good.

Comment 62

[Daniel Veditz [:dveditz]]

The branch patch doesn't fully fix the testcases in this bug, haven't tried the trunk to see if it's different.

Fixed:
  The original demo (attachment 211571 [details])
  CTho's evil.xul (national Bank hack)

Not Fixed:
  multi-popup demo (attachment 211824 [details])
  CTho's crash.xul[*]

[*] At least I don't think we ought to be allowing popups to fill the whole screen

Comment 63

[Daniel Veditz [:dveditz]]

Same results on trunk.

On the multi-popup demo one of the three popups closed when I switched to another window, but the remaining two still bled through. I bet that means we could get CTho's Bank phish working again even with the patch by opening two popups on top of each other.

Comment 64

[georgi - hopefully not receiving bugspam]

(In reply to comment #62)
> [*] At least I don't think we ought to be allowing popups to fill the whole
> screen

imho user popups should not be allowed outside content area.
 

Comment 65

[Daniel Veditz [:dveditz]]

Attachment: CTho's BSOD testcase

Preserving CTho's "crash.xul" testcase in bugzilla

Letting user stuff leak outside the content area leads to things like bug 361298

Comment 66

[Daniel Veditz [:dveditz]]

Removing approval request until we get more information on whether we need an additional patch or different approach (see comment 62 - 64)

Comment 67

[Neil Deakin]

OK, so I guess the only real fix here is to just make content popups have regular views then so that they don't extend outside of the content area.

Comment 68

[Neil Deakin]

Attachment: demo patch which shows popups in content area on Mac

Not actually sure why it works, but it seems to do. I won't have a windows machine for a couple of weeks. Maybe roc can comment on how to get a child widget on non-mac platforms.

Comment 69

[georgi - hopefully not receiving bugspam]

writing about escaping content area:
<div title="long">longdiv</div>

produces yellow rectangle going outside content area when mouseovered.

Comment 70

[Neil Deakin]

Re: comment 69,

That's a tooltip generated from chrome.

Comment 71

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Try changing this:
  widgetData.mWindowType = eWindowType_popup;

Georgi: That's pretty hard to abuse since you can only get text in a rectangle, with colors chosen by the theme. Unless there's a bug in our tooltip code.

Comment 72

[Daniel Veditz [:dveditz]]

The single popup case is fixed, which includes things like CTho's "evil" testcase. We definitely want this fixed.

Still two remaining issues:
1) Could we simply prevent multiple calls to showPopup(), either failing if a popup is already open or closing the old one before opening the new one? Don't much care if this is a blanket restriction or content-only. It's probably easier to do that restriction than to change the code to track the other popups.

2) The other issue is the size, and it seems like it should be pretty trivial to ensure the dimensions don't go outside the content window. Might be nice to keep frame popups inside the frame borders, too, but I could live with telling sites "don't frame other people's content if you don't trust them".

Comment 73

[Jay Patel [:jay]]

Comment on attachment 252798 [details] [diff] [review]
branch patch

Approved for both branches, a=jay.  Let's get the patch we have landed and verify that it fixes the cases we are most worried about.  We can deal with the remaining cases later (maybe in separate bugs?).

Comment 74

[Neil Deakin]

Checked in the patch on both branches

Comment 75

[Marcia Knous [:marcia]]

verified fixed on the 1.8 branch using Build identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.2pre) Gecko/2007020807 BonEcho/2.0.0.2pre. I verified this on mac because of Jesse's comment. I can verify what dan sees in Comment 62 - the first testcase when I switch to a new window or minimize the popup vanishes. But with (https://bugzilla.mozilla.org/attachment.cgi?id=211824), if I minimize or switch to the another window, the popup lingers and I have to close the tab, close the window or click on the popups in succession to get them to vanish.

Comment 76

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

http://ctho.ath.cx/tmp/moz/326877evil2.xul
Decreasing the 500ms timeout in byeBye() makes it difficult for the user to do things like resize the browser window.  This doesn't really test anything that crash.xul doesn't, but it's a more "security"-type testcase (inspired by bug 361298).

Comment 77

[Daniel Veditz [:dveditz]]

That's why I haven't issued an advisory for this partial fix, we still need to deal with the issues in comment 72. I guess I should file a new bug or two to make sure they don't get lost.

comment 72 issue 1: bug 374569
comment 72 issue 2: bug 374570

Comment 78

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: js for following testcase

Comment 79

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: updated evil bank testcase

It's still possible to do bad things.  I tested this with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.3) Gecko/2007030919 Firefox/2.0.0.3

The only real change from the old testcase is that now I show the popup before opening the new tab - opening a new tab doesn't kill the popup reliably (the behavior seems focus dependent).  I also made the popup small initially to make it more convincing, but that's not functionally relevant.  To get the popup to stay, just click the testcase when it loads, then don't do anything until the new document has loaded completely.  Clicking stuff during load sometimes causes the popup to disappear when the load finishes.

Comment 80

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: updated evil bank testcase

let's try that again...

Comment 81

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: another part of the following testcase

Comment 82

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: updated evil bank testcase

Sorry for the spam.

Comment 83

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

So is the trunk at least in the same state as the branch, or are there things we've fixed on the branch that we haven't fixed on the trunk?  I see fixed1.8.0.10 and verified1.8.1.2 keywords.  If the same thing that was fixed there is also fixed on the trunk, could you mark this bug fixed and open a new one for issues that remain?

Comment 84

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We need to sort out what needs to be done here, if anything, before beta2.

Comment 85

[Neil Deakin]

In fact, what needs to be done is marking this bug invalid. The problem described in comment 1 is fixed, and is bug 374570. I've kept asking for what the other security problems with popups are and have only got vague and incomplete answers, or with confusing testcases. If someone provides a real simplified testcase (the testcases in this bug are not) showing an actual problem, please open a new bug.

Comment 86

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Filed bug 406686.