Last Comment Bug 326877 - menupopup.showPopup() can be abused
: menupopup.showPopup() can be abused
Status: RESOLVED INVALID
[sg:high] wait for dependencies to be...
: csectype-spoof, fixed1.8.0.10, sec-high, testcase, verified1.8.1.2
Product: Core
Classification: Components
Component: XUL (show other bugs)
: Trunk
: All All
: P2 normal (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
: 339333 (view as bug list)
Depends on: 374569 374570
Blocks: 326633
  Show dependency treegraph
 
Reported: 2006-02-12 01:32 PST by Jesse Ruderman
Modified: 2013-06-09 20:15 PDT (History)
16 users (show)
roc: blocking1.9+
dveditz: blocking1.9a1+
mtschrep: blocking1.8.1-
dveditz: blocking1.8.1.2+
dveditz: blocking1.8.0.4-
dveditz: blocking1.8.0.5-
dveditz: blocking1.8.0.7-
dveditz: blocking1.8.0.10+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
demo: opens a menupopup after 3 seconds (399 bytes, application/vnd.mozilla.xul+xml)
2006-02-12 01:33 PST, Jesse Ruderman
no flags Details
screenshot: menupopup over a Terminal window (7.76 KB, image/png)
2006-02-12 01:34 PST, Jesse Ruderman
no flags Details
demo with multiple menupopups (732 bytes, application/vnd.mozilla.xul+xml)
2006-02-13 23:53 PST, Jesse Ruderman
no flags Details
Don't open popups in unfocused windows (5.60 KB, patch)
2007-01-14 09:23 PST, Neil Deakin
bzbarsky: review-
bzbarsky: superreview-
Details | Diff | Splinter Review
also close popups (9.91 KB, patch)
2007-01-17 11:37 PST, Neil Deakin
bzbarsky: review+
bzbarsky: superreview+
Details | Diff | Splinter Review
branch patch (9.93 KB, patch)
2007-01-25 11:18 PST, Neil Deakin
bzbarsky: review+
bzbarsky: superreview+
jaymoz: approval1.8.1.2+
jaymoz: approval1.8.0.10+
Details | Diff | Splinter Review
CTho's BSOD testcase (2.18 KB, application/vnd.mozilla.xul+xml)
2007-01-28 10:36 PST, Daniel Veditz [:dveditz]
no flags Details
demo patch which shows popups in content area on Mac (5.12 KB, patch)
2007-01-30 07:52 PST, Neil Deakin
no flags Details | Diff | Splinter Review
js for following testcase (896 bytes, application/x-javascript)
2007-04-23 18:25 PDT, Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com]
no flags Details
updated evil bank testcase (880 bytes, application/vnd.mozilla.xul+xml)
2007-04-23 18:29 PDT, Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com]
no flags Details
updated evil bank testcase (884 bytes, application/vnd.mozilla.xul+xml)
2007-04-23 18:36 PDT, Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com]
no flags Details
another part of the following testcase (76.14 KB, image/png)
2007-04-23 18:39 PDT, Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com]
no flags Details
updated evil bank testcase (885 bytes, application/vnd.mozilla.xul+xml)
2007-04-23 18:40 PDT, Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com]
no flags Details

Description Jesse Ruderman 2006-02-12 01:32:53 PST
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 2006-02-12 01:33:40 PST
Created attachment 211571 [details]
demo: opens a menupopup after 3 seconds
Comment 2 Jesse Ruderman 2006-02-12 01:34:55 PST
Created attachment 211572 [details]
screenshot: menupopup over a Terminal window
Comment 3 Daniel Veditz [:dveditz] 2006-02-13 21:53:35 PST
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 2006-02-13 23:53:22 PST
Created attachment 211824 [details]
demo with multiple menupopups
Comment 5 Jesse Ruderman 2006-02-13 23:59:53 PST
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 2006-03-23 12:29:50 PST
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 [:bz] 2006-03-23 13:44:38 PST
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 2006-03-23 13:48:34 PST
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 [:bz] 2006-03-23 14:06:29 PST
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 2006-03-23 14:58:37 PST
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 [:bz] 2006-03-24 10:32:27 PST
Yeah, esp. given that we do want menupopups to stick out of the window.
Comment 12 Daniel Veditz [:dveditz] 2006-05-01 12:11:18 PDT
no comments since March, this obviously isn't making 1.8.0.4

Need a decent owner for this.
Comment 13 Jesse Ruderman 2006-05-26 17:14:22 PDT
*** Bug 339333 has been marked as a duplicate of this bug. ***
Comment 14 georgi - hopefully not receiving bugspam 2006-05-31 08:08:11 PDT
(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] 2006-06-15 14:05:39 PDT
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] 2006-06-20 15:03:37 PDT
Realistically we don't know how to fix this for 1.8.0.5, moving out.
Comment 17 Mike Schroepfer 2006-06-21 10:29:17 PDT
If we can agree on a fix - we'd like it for 1.8.1
Comment 18 Johnny Stenback (:jst, jst@mozilla.com) 2006-07-05 17:00:04 PDT
Sounds like a view problem. Roc, can you have a look at this?
Comment 19 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-07-05 19:47:56 PDT
It's not really a view bug. It's more a XUL/DOM bug.
Comment 20 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-07-05 19:55:44 PDT
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 2006-07-06 02:21:13 PDT
note that xul popups kind of circumvent the window.open() popup blocker.
Comment 22 Matthew Gertner 2006-07-06 04:44:12 PDT
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) (Exited; email my personal email if necessary) 2006-07-06 10:15:31 PDT
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 2006-07-06 11:12:19 PDT
Not going to hold FF2 beta1 for this, but keeping this on the radar for FF2.
Comment 25 Neil Deakin 2006-07-06 11:32:40 PDT
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) (Exited; email my personal email if necessary) 2006-07-06 12:13:37 PDT
The former, although showing them again when it's focused maybe isn't needed.
Comment 27 Jesse Ruderman 2006-07-06 20:39:19 PDT
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) (Exited; email my personal email if necessary) 2006-07-06 22:29:50 PDT
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) (Exited; email my personal email if necessary) 2006-07-06 22:34:00 PDT
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) (Exited; email my personal email if necessary) 2006-07-06 22:38:19 PDT
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] 2006-07-27 16:47:16 PDT
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 2006-08-15 12:37:17 PDT
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] 2006-09-19 15:42:12 PDT
Restoring lost blocking flag
Comment 34 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2006-09-19 18:12:47 PDT
(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 2006-10-08 12:44:33 PDT
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] 2006-12-18 14:14:54 PST
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+2 (mostly busy through August 4; review requests must explain patch) 2006-12-18 16:48:55 PST
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 2006-12-19 08:03:37 PST
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) (Exited; email my personal email if necessary) 2007-01-06 00:39:45 PST
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 2007-01-06 06:00:26 PST
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 2007-01-14 09:23:36 PST
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.
Comment 42 Boris Zbarsky [:bz] 2007-01-14 22:52:06 PST
That patch doesn't allow a subframe of a primary content shell to open popups.  Is that on purpose?
Comment 43 Neil Deakin 2007-01-15 09:07:20 PST
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 [:bz] 2007-01-15 12:40:05 PST
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 [:bz] 2007-01-15 12:42:29 PST
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 2007-01-16 09:18:09 PST
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 [:bz] 2007-01-16 10:23:25 PST
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 2007-01-16 10:58:41 PST
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 [:bz] 2007-01-16 11:04:15 PST
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 2007-01-16 11:20:24 PST
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 [:bz] 2007-01-16 12:30:46 PST
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) (Exited; email my personal email if necessary) 2007-01-16 12:43:40 PST
I think we want the list in bug 324963 to go away in favour of 279703.
Comment 53 Neil Deakin 2007-01-17 11:37:36 PST
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?
Comment 54 Jay Patel [:jay] 2007-01-22 15:09:49 PST
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 55 Boris Zbarsky [:bz] 2007-01-24 12:47:41 PST
*** Bug 368013 has been marked as a duplicate of this bug. ***
Comment 56 Boris Zbarsky [:bz] 2007-01-24 13:02:24 PST
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] 2007-01-24 16:05:10 PST
Does this patch work for the branches or do we need a separate branch-appropriate patch?
Comment 58 Neil Deakin 2007-01-25 07:53:03 PST
(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 [:bz] 2007-01-25 07:59:40 PST
> 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 2007-01-25 11:18:26 PST
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 61 Boris Zbarsky [:bz] 2007-01-25 20:23:29 PST
Comment on attachment 252798 [details] [diff] [review]
branch patch

Looks good.
Comment 62 Daniel Veditz [:dveditz] 2007-01-27 22:33:23 PST
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] 2007-01-28 00:20:31 PST
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 2007-01-28 01:11:37 PST
(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] 2007-01-28 10:36:04 PST
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
Comment 66 Daniel Veditz [:dveditz] 2007-01-28 15:00:00 PST
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 2007-01-28 21:00:44 PST
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 2007-01-30 07:52:36 PST
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.
Comment 69 georgi - hopefully not receiving bugspam 2007-01-30 08:00:01 PST
writing about escaping content area:
<div title="long">longdiv</div>

produces yellow rectangle going outside content area when mouseovered.
Comment 70 Neil Deakin 2007-01-30 08:12:36 PST
Re: comment 69,

That's a tooltip generated from chrome.
Comment 71 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2007-01-30 11:44:32 PST
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] 2007-02-01 14:26:25 PST
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] 2007-02-05 16:58:00 PST
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 2007-02-06 05:35:52 PST
Checked in the patch on both branches
Comment 75 Marcia Knous [:marcia - use ni] 2007-02-08 16:17:19 PST
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 cst@andrew.cmu.edu cst@yecc.com] 2007-03-18 00:07:41 PDT
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] 2007-03-19 22:44:43 PDT
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 cst@andrew.cmu.edu cst@yecc.com] 2007-04-23 18:25:56 PDT
Created attachment 262577 [details]
js for following testcase
Comment 79 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2007-04-23 18:29:23 PDT
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: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 cst@andrew.cmu.edu cst@yecc.com] 2007-04-23 18:36:50 PDT
Created attachment 262581 [details]
updated evil bank testcase

let's try that again...
Comment 81 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2007-04-23 18:39:34 PDT
Created attachment 262582 [details]
another part of the following testcase
Comment 82 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2007-04-23 18:40:44 PDT
Created attachment 262583 [details]
updated evil bank testcase

Sorry for the spam.
Comment 83 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2007-11-07 13:46:42 PST
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) (Exited; email my personal email if necessary) 2007-11-07 18:21:02 PST
We need to sort out what needs to be done here, if anything, before beta2.
Comment 85 Neil Deakin 2007-11-22 08:52:20 PST
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 cst@andrew.cmu.edu cst@yecc.com] 2007-12-03 17:59:07 PST
Filed bug 406686.  

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