399 bytes, application/vnd.mozilla.xul+xml
7.76 KB, image/png
732 bytes, application/vnd.mozilla.xul+xml
9.91 KB, patch
|Details | Diff | Splinter Review|
9.93 KB, patch
|Details | Diff | Splinter Review|
2.18 KB, application/vnd.mozilla.xul+xml
5.12 KB, patch
|Details | Diff | Splinter Review|
76.14 KB, image/png
885 bytes, application/vnd.mozilla.xul+xml
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.
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?
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.)
jst, bz: any thoughts on what it would take to fix this bug? any idea who might be able to do the work?
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?
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.
That doesn't fix the issue Jesse is having with the popup covering up other dialogs (which are in other windows).
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.
Yeah, esp. given that we do want menupopups to stick out of the window.
no comments since March, this obviously isn't making 22.214.171.124 Need a decent owner for this.
*** Bug 339333 has been marked as a duplicate of this bug. ***
(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.
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?
Realistically we don't know how to fix this for 126.96.36.199, moving out.
If we can agree on a fix - we'd like it for 1.8.1
Sounds like a view problem. Roc, can you have a look at this?
It's not really a view bug. It's more a XUL/DOM bug.
How about we just refuse to allow popups to be visible for windows that don't have focus?
note that xul popups kind of circumvent the window.open() popup blocker.
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?
Sure, that wouldn't be hard. If we go this way, maybe this is a job for one of the Neils.
Not going to hold FF2 beta1 for this, but keeping this on the radar for FF2.
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?
The former, although showing them again when it's focused maybe isn't needed.
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?
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.
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".
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.
Would love a fix, so plussing 188.8.131.52 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.
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.
Restoring lost blocking flag
(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.
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.
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).
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.
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.
Neil, do you want to take this? If not, would you mind posting what you did to make popups not be popups?
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.
Created attachment 251455 [details] [diff] [review] 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.
That patch doesn't allow a subframe of a primary content shell to open popups. Is that on purpose?
No, that wasn't intended. Is it sufficient to retrieve the shell's sameTypeRootTreeItem and check if that is a primary frame?
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 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.
Hmmm, no it doesn't seem to prevent popups during a pagehide event. Is there a way to detect this state?
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()?
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.
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...
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.
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?
I think we want the list in bug 324963 to go away in favour of 279703.
Created attachment 251809 [details] [diff] [review] 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?
Neil: Anyone else around to review your patch? bz seems a bit busy and we would like to get this in for 184.108.40.206/220.127.116.11.
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?
Does this patch work for the branches or do we need a separate branch-appropriate patch?
(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?
> 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.
Created attachment 252798 [details] [diff] [review] branch patch The branch patch is the same changes, although the existing code in PageHide is different which is just indented.
Comment on attachment 252798 [details] [diff] [review] branch patch Looks good.
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
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.
(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.
Created attachment 253104 [details] 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
Removing approval request until we get more information on whether we need an additional patch or different approach (see comment 62 - 64)
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.
Created attachment 253335 [details] [diff] [review] 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.
writing about escaping content area: <div title="long">longdiv</div> produces yellow rectangle going outside content area when mouseovered.
Re: comment 69, That's a tooltip generated from chrome.
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.
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 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?).
Checked in the patch on both branches
verified fixed on the 1.8 branch using Build identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:18.104.22.168pre) Gecko/2007020807 BonEcho/22.214.171.124pre. 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.
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).
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
Created attachment 262579 [details] 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:126.96.36.199) Gecko/2007030919 Firefox/188.8.131.52 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.
Created attachment 262581 [details] updated evil bank testcase let's try that again...
Created attachment 262582 [details] another part of the following testcase
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 fixed184.108.40.206 and verified220.127.116.11 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?
We need to sort out what needs to be done here, if anything, before beta2.
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.
Filed bug 406686.