Last Comment Bug 575294 - (CVE-2012-3984) Navigation away from a page with an active <select> dropdown menu can be used for URL spoofing, other evil
(CVE-2012-3984)
: Navigation away from a page with an active <select> dropdown menu can be used...
Status: RESOLVED FIXED
[sg:critical][advisory-tracking+] cro...
: csectype-spoof, sec-critical
Product: Core
Classification: Components
Component: Layout: Form Controls (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla16
Assigned To: Mats Palmgren (:mats)
:
Mentors:
jar:https://bugzilla.mozilla.org/atta...
: 308278 591861 691014 (view as bug list)
Depends on: 1199886 759243 760946 775958 780661 783405 803995 805153 806364 807174 820921 836384
Blocks: 691014 726264
  Show dependency treegraph
 
Reported: 2010-06-28 09:41 PDT by David Bloom
Modified: 2015-08-28 23:33 PDT (History)
25 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
wontfix
+
wontfix
+
wontfix
+
wontfix
+
fixed
-
wontfix
.x+
wontfix
wontfix


Attachments
simple proof of concept of the vulnerability (2.91 KB, application/java-archive)
2010-06-28 09:41 PDT, David Bloom
no flags Details
wip (5.06 KB, patch)
2010-06-29 09:54 PDT, Olli Pettay [:smaug] (vacation Aug 25-28)
no flags Details | Diff | Splinter Review
v0.1, don't show select popup over location bar (21.97 KB, patch)
2010-11-30 10:33 PST, Olli Pettay [:smaug] (vacation Aug 25-28)
no flags Details | Diff | Splinter Review
Simple part (6.08 KB, patch)
2010-12-14 14:50 PST, Olli Pettay [:smaug] (vacation Aug 25-28)
roc: review+
Details | Diff | Splinter Review
even safer "simple part" (6.27 KB, patch)
2010-12-15 09:06 PST, Olli Pettay [:smaug] (vacation Aug 25-28)
no flags Details | Diff | Splinter Review
close popup when unloading (6.27 KB, patch)
2010-12-27 03:01 PST, Olli Pettay [:smaug] (vacation Aug 25-28)
roc: review+
Details | Diff | Splinter Review
Ensure that popup is under chrome (20.40 KB, patch)
2010-12-27 03:03 PST, Olli Pettay [:smaug] (vacation Aug 25-28)
no flags Details | Diff | Splinter Review
Ensure that popup is under chrome (20.43 KB, patch)
2010-12-28 00:32 PST, Olli Pettay [:smaug] (vacation Aug 25-28)
no flags Details | Diff | Splinter Review
v6 (19.50 KB, patch)
2011-01-03 09:37 PST, Olli Pettay [:smaug] (vacation Aug 25-28)
no flags Details | Diff | Splinter Review
v7 (22.08 KB, patch)
2011-01-20 11:50 PST, Olli Pettay [:smaug] (vacation Aug 25-28)
no flags Details | Diff | Splinter Review
part 1/3, send a blur (1.77 KB, patch)
2012-04-05 13:44 PDT, Mats Palmgren (:mats)
enndeakin: review+
Details | Diff | Splinter Review
part 2/3, never place the dropdown above the top of the root frame (4.24 KB, patch)
2012-04-05 13:47 PDT, Mats Palmgren (:mats)
bugs: review-
Details | Diff | Splinter Review
part 3/3, minor cleanup (5.84 KB, patch)
2012-04-05 13:48 PDT, Mats Palmgren (:mats)
bugs: review+
Details | Diff | Splinter Review
Screenshot, with patches (130.10 KB, image/png)
2012-04-06 15:53 PDT, Mats Palmgren (:mats)
no flags Details
part 1/5, send a blur (1.75 KB, patch)
2012-04-12 12:39 PDT, Mats Palmgren (:mats)
mats: review+
Details | Diff | Splinter Review
part 2/3, resize the dropdown to fit the larger available space above/below (17.60 KB, patch)
2012-04-12 13:00 PDT, Mats Palmgren (:mats)
bugs: review+
Details | Diff | Splinter Review
part 3/3, minor cleanup (4.69 KB, patch)
2012-04-12 13:01 PDT, Mats Palmgren (:mats)
mats: review+
Details | Diff | Splinter Review
part 2/5, resize the dropdown to fit the larger available space above/below (26.48 KB, patch)
2012-05-19 16:10 PDT, Mats Palmgren (:mats)
bugs: review+
roc: review+
Details | Diff | Splinter Review
part 3/4, call nsIRollupListener::NotifyGeometryChange when dropdown moves (14.51 KB, patch)
2012-05-19 16:18 PDT, Mats Palmgren (:mats)
bugs: review-
Details | Diff | Splinter Review
part 5/5, minor cleanup (4.73 KB, patch)
2012-05-19 16:21 PDT, Mats Palmgren (:mats)
mats: review+
Details | Diff | Splinter Review
part 4/5, call nsIRollupListener::NotifyGeometryChange when dropdown moves (16.17 KB, patch)
2012-05-27 11:06 PDT, Mats Palmgren (:mats)
bugs: review+
roc: review+
Details | Diff | Splinter Review
part=3/5, use exact height when it fits (1.82 KB, patch)
2012-06-22 11:20 PDT, Mats Palmgren (:mats)
roc: review+
Details | Diff | Splinter Review

Description David Bloom 2010-06-28 09:41:12 PDT
User-Agent:       Opera/9.80 (X11; Linux x86_64; U; en) Presto/2.6.30 Version/10.60
Build Identifier: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.3) Gecko/20100423 Ubuntu/10.04 (lucid) Firefox/3.6.3

Firefox shows the dropdown menu for <select> elements as an always-on-top chromeless window. It also allows arbitrary HTML content to be rendered in the <option> elements within the <select>.

Navigating away from a page with an active <select> menu does not remove the chromeless window containing the menu from view. By opening another menu programatically (using XUL, in my proof-of-concept page), it is possible to make the <select> menu persist through mouse interactions with the browser. By using absolute positioning/scrolling, an attacker can essentially cover arbitrary portions of a user's display with whatever they want.

I think the correct fix here would probably be to automatically dismiss <select> menus when navigating away from pages.

Reproducible: Always
Comment 1 David Bloom 2010-06-28 09:41:57 PDT
Created attachment 454538 [details]
simple proof of concept of the vulnerability
Comment 2 Daniel Veditz [:dveditz] 2010-06-28 14:01:26 PDT
The select can be moved to cover chrome, didn't see that the first time I ran the testcase.
Comment 3 Olli Pettay [:smaug] (vacation Aug 25-28) 2010-06-29 09:54:09 PDT
Created attachment 454879 [details] [diff] [review]
wip

...but I'm investigating still few things
Comment 4 David Bloom 2010-06-29 10:12:11 PDT
The select is always-on-top too. For example, it could cover the contents of a warning dialog for installing an XPI (and instead make it say something like "Click OK to update Firefox security updates")
Comment 5 David Bloom 2010-07-26 08:15:14 PDT
Any updates?
Comment 6 Olli Pettay [:smaug] (vacation Aug 25-28) 2010-07-26 08:56:13 PDT
(Sorry about the delay. There was the Summit and then I was on vacation.)

Roc, I think the patch isn't enough (it is needed, but just not enough).
Should we close the <select> when the page is scrolled?
What would be the best or easiest way to do that?
Or could we disallow moving the native widget which is showing the popup?
Comment 7 Olli Pettay [:smaug] (vacation Aug 25-28) 2010-07-26 09:02:06 PDT
Ah, nsComboboxControlFrame::AbsolutelyPositionDropDown might need some
tweaking.
Comment 8 David Bloom 2010-07-26 09:14:57 PDT
Another defense-in-depth idea...have you considered blocking HTML content in <select> elements? All other browsers only show plain text in <select>, and Firefox already converts to plaintext in some situations (such as setting <select>.innerHTML).
Comment 9 Olli Pettay [:smaug] (vacation Aug 25-28) 2010-07-26 13:10:36 PDT
select.innerHTML isn't handled in any special way.

IMO allowing html content <option> elements is quite handy.
Comment 10 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-07-26 15:55:51 PDT
Maybe we should just clamp the select position to the viewport when determining the location of the dropdown?
Comment 11 David Bloom 2010-07-26 16:00:47 PDT
(In reply to comment #10)
> Maybe we should just clamp the select position to the viewport when determining
> the location of the dropdown?

Remember that the dropdown is an always-on-top window. The attacker could window.open a new browser window that is positioned lower, so that the evil <select> dropdown from the original window would overlap the address bar of the new window.
Comment 12 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-07-27 16:33:32 PDT
True.

We can mitigate this by hiding the dropdown more aggressively, but I think probably it would be better to not render arbitrary HTML in <selects>s. The HTML5 parser is already ignoring most content in <select>: http://www.w3.org/TR/html5/tokenization.html#parsing-main-inselect

One way to do this would be to modify nsListControlFrame::BuildDisplayList to throw away any display items that aren't text.
Comment 13 David Bloom 2010-08-04 07:40:38 PDT
Would that take care of generated content and other CSS styling as well?
Comment 14 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-08-04 15:48:47 PDT
You'd still be able to use generated content, but any non-text content wouldn't be rendered.

Hmm, maybe that still leaves us vulnerable to someone cobbling together glyphs with various colors, using table layout say, to produce an arbitrary display. We can put "color:inherit !important" in the UA style sheet to lock the text color.

Perhaps a more robust option is to modify the frame constructor to only allow inline and text frames inside a dropdown.
Comment 15 David Bloom 2010-08-09 08:46:53 PDT
(In reply to comment #14)
> You'd still be able to use generated content, but any non-text content wouldn't
> be rendered.
> 
> Hmm, maybe that still leaves us vulnerable to someone cobbling together glyphs
> with various colors, using table layout say, to produce an arbitrary display.
An attacker could still use a custom font too, right?
> We can put "color:inherit !important" in the UA style sheet to lock the text
> color.
Locking the color would be helpful, as it would make it more difficult to spoof HTTPS convincingly.
> 
> Perhaps a more robust option is to modify the frame constructor to only allow
> inline and text frames inside a dropdown.
Comment 16 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-08-09 16:07:33 PDT
> An attacker could still use a custom font too, right?

Yes. It would be easy to lock the font using a !important rule, though.
Comment 17 Joe Drew (not getting mail) 2010-08-11 08:38:16 PDT
Would totally take a patch, but we have enough sg:crits that already block.
Comment 18 Jesse Ruderman 2010-09-14 13:36:41 PDT
*** Bug 308278 has been marked as a duplicate of this bug. ***
Comment 19 Johnny Stenback (:jst, jst@mozilla.com) 2010-11-16 13:53:06 PST
Olli, could you get back to this sg:high bug? It's been sitting for quite a
while now...
Comment 20 Olli Pettay [:smaug] (vacation Aug 25-28) 2010-11-16 13:54:30 PST
My blocker list is now quite short, so yes.
Sorry about the delay!
Comment 21 Olli Pettay [:smaug] (vacation Aug 25-28) 2010-11-17 06:48:59 PST
Oh, webkit, or at least Chromium has a very similar bug.
I need to report that to them, unless you David have already done that?
Comment 22 David Bloom 2010-11-17 07:25:31 PST
I haven't reported it...did you find a way to make it the menu to persist when navigating away from the page, or to make it persist when the user clicks outside of it?
Comment 23 Olli Pettay [:smaug] (vacation Aug 25-28) 2010-11-17 07:55:54 PST
No, but able to spoof location bar. It is similar, though probably not
as serious as this one.
Comment 24 Boris Zbarsky [:bz] 2010-11-17 08:13:22 PST
> Perhaps a more robust option is to modify the frame constructor to only allow
> inline and text frames inside a dropdown.

I was considering this anyway, since other browsers seem to do something like this.  Would need some testing to determine what's actually interoperable now....
Comment 25 David Bloom 2010-11-22 13:31:49 PST
(In reply to comment #23)
> No, but able to spoof location bar. It is similar, though probably not
> as serious as this one.

I just figured out how to make it not disappear after clicking in Chrome. I'll take care of filing a bug for that.
Comment 26 Olli Pettay [:smaug] (vacation Aug 25-28) 2010-11-23 03:05:47 PST
Thanks. I'll inform Opera.
Haven't tested Safari nor IE.
Comment 27 Daniel Veditz [:dveditz] 2010-11-23 08:46:02 PST
> Thanks. I'll inform Opera.

This is the first mention of Opera in the bug (other than David's UA when reporting). Do you see this behavior in Opera, too?
Comment 28 Olli Pettay [:smaug] (vacation Aug 25-28) 2010-11-23 08:54:31 PST
I didn't see exactly this bug in Chrome or Opera, but something similar related
select's popup over location bar.
Comment 29 David Bloom 2010-11-23 09:02:39 PST
Behaviors seen in Opera, and possibly IE/Safari: Possible to cover address bar with a <select> dropdown. The dropdown disappears when the user clicks outside of it, so it's pretty hard to use this in an actual spoofing attack. Fixing this would be a defense-in-depth measure.

Behaviors seen in Firefox, Chrome: Possible to cover address bar with a <select> dropdown, *and* make the dropdown stay open when the user clicks outside of it. This could potentially be used for an actual spoofing attack.

The proof of concept attached to this bug doesn't work in Chrome...the technique for making the dropdown menu persist in Chrome is completely different than the one used in Firefox.
Comment 30 Olli Pettay [:smaug] (vacation Aug 25-28) 2010-11-23 09:08:38 PST
(In reply to comment #29)
> Behaviors seen in Opera, and possibly IE/Safari: Possible to cover address bar
> with a <select> dropdown. The dropdown disappears when the user clicks outside
> of it, so it's pretty hard to use this in an actual spoofing attack.
Don't know about IE/Safari, but in Opera if user uses only keyboard, web page
can capture key events when popup is open. So it may look like
user it typing for example username and password for some service, although
the page is something quite different.
Comment 31 David Bloom 2010-11-23 09:10:50 PST
Ah, I forgot to test that in Opera!

Yes, you definitely should file an Opera bug then :-)
Comment 32 Olli Pettay [:smaug] (vacation Aug 25-28) 2010-11-30 10:33:18 PST
Created attachment 494039 [details] [diff] [review]
v0.1, don't show select popup over location bar

...and never move the visible popup.

This is very much not my area of code, but it has been quite useful and even fun
to learn new things. But anyway, before I write more code, better to ask some
feedback.

Note, we do already close the popup when window moves, so no need to add
code for that.
And the extra layout flush is just moved to be in a useful place.
I don't know if we could somehow get rid of the flush.

I had another approach where I basically disabled images inside <select> (using css), but
that isn't enough.
Comment 33 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-11-30 12:15:13 PST
+  nsPresContext* pc = PresContext();
+  if (pc->IsChrome()) {
+    return 0;
+  }

nscoord_MIN? on multimonitor setups screen coordinates can be negative.

I get the feeling this is more complex than necessary. When we flip the combobox dropdown vertically, can't we just check whether there's enough space below the top of the toplevel content window and not flip if there isn't enough space? I don't understand the need for mPreviousRootY and mAvailHeight.

A separate patch for doing CloseOpenPopup in PageHide would be good.
Comment 34 Olli Pettay [:smaug] (vacation Aug 25-28) 2010-11-30 12:30:08 PST
(In reply to comment #33)
> nscoord_MIN? on multimonitor setups screen coordinates can be negative.
oh.


> I get the feeling this is more complex than necessary. When we flip the
> combobox dropdown vertically, can't we just check whether there's enough space
> below the top of the toplevel content window and not flip if there isn't enough
> space?
But what if the popup is reflowed for larger height? It would be possible to
have too large popup where part of the scrollbar would be below the bottom
of the screen.
I'm trying to keep the default 20 rows height, when possible,
and decrease it only when really needed.

> I don't understand the need for mPreviousRootY and mAvailHeight.
mPreviousRootY is the location of the topmost content window when
popup was activated (but before the flush, so the "previous". Better name could be mRootYWhenPopupActivated).
mAvailHeight tells to the popup what is the maximum height it can use.

> A separate patch for doing CloseOpenPopup in PageHide would be good.
That is rather trivial part of the patch, but I can do a separate patch.
Comment 35 Olli Pettay [:smaug] (vacation Aug 25-28) 2010-11-30 12:32:51 PST
(In reply to comment #34)

> But what if the popup is reflowed for larger height? It would be possible to
> have too large popup where part of the scrollbar would be below the bottom
> of the screen.
Scrollbar or the bottommost option elements
Comment 36 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-11-30 12:41:28 PST
(In reply to comment #34)
> But what if the popup is reflowed for larger height? It would be possible to
> have too large popup where part of the scrollbar would be below the bottom
> of the screen.
> I'm trying to keep the default 20 rows height, when possible,
> and decrease it only when really needed.

OK.

> > I don't understand the need for mPreviousRootY and mAvailHeight.
> mPreviousRootY is the location of the topmost content window when
> popup was activated (but before the flush, so the "previous". Better name could
> be mRootYWhenPopupActivated).
> mAvailHeight tells to the popup what is the maximum height it can use.

I still don't really follow.

Suppose we modify AbsolutelyPositionDropDown to ensure that the combobox is either a) completely above the window top or b) completely below the toplevel content window top.

Suppose we ensure that once opened, the dropdown widget cannot change its height or position.

You say that we already ensure moving the browser window will close the dropdown.

If we do all of those things, then the problem is fixed, right? And none of those things require extra fields in the frames.
Comment 37 Jesse Ruderman 2010-11-30 15:56:25 PST
CCing Jordi, who found the somewhat similar bug 613935.
Comment 38 Olli Pettay [:smaug] (vacation Aug 25-28) 2010-12-14 14:50:30 PST
Created attachment 497629 [details] [diff] [review]
Simple part

This is the simple part of the patch - just close the popup when unloading a page.
Comment 39 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-12-14 16:38:58 PST
Comment on attachment 497629 [details] [diff] [review]
Simple part

@@ -734,18 +741,22 @@ nsComboboxControlFrame::ShowDropDown(PRB
   if (eventStates.HasState(NS_EVENT_STATE_DISABLED)) {
     return;
   }
 
   if (!mDroppedDown && aDoDropDown) {
     if (mListControlFrame) {
       mListControlFrame->SyncViewWithFrame();
     }
+    sOpened = this;

Assert that sOpened is currently null?

+      nsComboboxControlFrame* f = sOpened;
+      sOpened = nsnull;
+      f->ShowDropDown(PR_FALSE);

Why clear sOpened here?
Comment 40 Olli Pettay [:smaug] (vacation Aug 25-28) 2010-12-15 09:06:59 PST
Created attachment 497785 [details] [diff] [review]
even safer "simple part"
Comment 41 Olli Pettay [:smaug] (vacation Aug 25-28) 2010-12-27 03:01:41 PST
Created attachment 499811 [details] [diff] [review]
close popup when unloading
Comment 42 Olli Pettay [:smaug] (vacation Aug 25-28) 2010-12-27 03:03:46 PST
Created attachment 499812 [details] [diff] [review]
Ensure that popup is under chrome
Comment 43 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-12-27 12:30:48 PST
Why can't we just have nsComboboxControlFrame::AllowPositionChange return false for all dropped-down views?

In fact, why can't it just return false always? In that case we'd only need a flag, not a callback.
Comment 44 Timothy Nikkel (:tnikkel) 2010-12-27 19:00:11 PST
*** Bug 591861 has been marked as a duplicate of this bug. ***
Comment 45 Olli Pettay [:smaug] (vacation Aug 25-28) 2010-12-27 23:02:03 PST
An early version of the patch just returned false always and had a flag, but
that broke event handling/hit testing, since all the view/widget offsets would 
need to be updated.
Comment 46 Olli Pettay [:smaug] (vacation Aug 25-28) 2010-12-27 23:03:57 PST
...so I decided to implement an easier (IMO) approach.
Comment 47 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-12-27 23:44:18 PST
Doesn't that mean that event handling/hit testing is broken when this method returns false?
Comment 48 Olli Pettay [:smaug] (vacation Aug 25-28) 2010-12-28 00:27:14 PST
Comment on attachment 499812 [details] [diff] [review]
Ensure that popup is under chrome

Bah, found a bug - apparently I copy-pasted something wrong when
cleaning the patch.
Comment 49 Olli Pettay [:smaug] (vacation Aug 25-28) 2010-12-28 00:32:08 PST
Created attachment 499994 [details] [diff] [review]
Ensure that popup is under chrome

So if popup is open and <select> is scrolled too much up (so that it isn't 
visible), the popup is closed.
If, however the <select> is still visible but the popup overlaps chrome,
the popup is repositioned using AbsolutelyPositionDropDown().
Event handling seem to work ok that way.
Comment 50 Timothy Nikkel (:tnikkel) 2010-12-28 11:53:50 PST
Comment on attachment 499994 [details] [diff] [review]
Ensure that popup is under chrome

>+nsComboboxControlFrame::RootY()
>+  nsCOMPtr<nsISupports> container = pc->GetContainer();
>+  nsCOMPtr<nsIDocShellTreeItem> treeItem = do_QueryInterface(container);
>+  if (treeItem) {
>+    nsCOMPtr<nsIDocShellTreeItem> root;
>+    treeItem->GetSameTypeRootTreeItem(getter_AddRefs(root));
>+    nsCOMPtr<nsIDocShell> ds = do_QueryInterface(root);
>+    if (ds) {
>+      nsCOMPtr<nsIPresShell> shell;
>+      ds->GetPresShell(getter_AddRefs(shell));
>+      if (shell) {
>+        return shell->GetRootFrame()->GetScreenRectInAppUnits().y;

This return value is in app units of a different document, so could have a different scale. It probably doesn't matter but you should probably convert the result using NSCoordScale(result, shell->PresContext()->AppUnitsPerDevPixel(), PresContext()->AppUnitsPerDevPixel()).
Comment 51 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-12-29 13:38:00 PST
Calling AbsolutelyPositionDropDown() during DoResetWidgetBounds does not seem safe. But I'd still like to find a way to avoid the viewpositionobserver altogether. We could add a method nsIViewObserver::DidChangeFloatingViewPosition(nsIView*) which gets called on floating views (nsIView::GetFloating()), and have that method dispatch an event to asynchronously reflow the <select> to reposition the dropdown. Couldn't we?

Why keep mMinPopupY? Can't we just call RootY when we need it?
Comment 52 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-01-03 09:37:35 PST
Created attachment 500834 [details] [diff] [review]
v6

No mMinPopupY, and NSCoordScale, and async AbsolutelyPositionDropDown.

But I could still investigate the DidChangeFloatingViewPosition approach.
(Not that I like dispatching an nsGUIEvent for this case)
Comment 53 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-01-03 22:42:48 PST
You don't need to dispatch an nsGUIEvent. You'd just call through mObserver directly.

Can AllowPositionChange always return true now?

+      if (frame->GetScreenRectInAppUnits().YMost() < frame->RootY()) {
+        frame->ShowList(PR_FALSE);
+      } else {
+        frame->AbsolutelyPositionDropDown();

Why not just call AbsolutelyPositionDropDown unconditionally?
Comment 54 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-01-04 01:53:19 PST
(In reply to comment #53)
> Why not just call AbsolutelyPositionDropDown unconditionally?
Because there is really no place to re-position the popup always.
Comment 55 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-01-04 02:02:36 PST
So why not just do whatever AbsolutelyPositionDropDown would do in that situation?
Comment 56 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-01-04 02:13:26 PST
Well, it doesn't do anything useful in that situation.
I could ofc change AbsolutelyPositionDropDown.
Comment 57 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-01-04 02:13:59 PST
...so that it closes the popup.
Comment 58 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-01-04 02:41:48 PST
It seems like it should do something, so that attempts to open the popup where we can't open it don't just do something random.
Comment 59 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-01-12 04:45:33 PST
Roc, I'm not quite sure what all I should do with this to get the patch
good enough.
Comment 60 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-01-12 05:02:19 PST
Tell me what should happen if the user tries to open a popup but we can't find a safe place to put it.
Comment 61 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-01-12 06:15:01 PST
we shouldn't open the popup at all.

I could add a check for it to AbsolutelyPositionDropDown
Comment 62 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-01-12 14:14:12 PST
If you add the check in AbsolutelyPositionDropDown, then AllowPositionChange can call AbsolutelyPositionDropDown unconditionally and always return true, can't it? Then we don't need a return value at all.
Comment 63 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-01-12 14:14:34 PST
... so we should just change the callback to a "notify widget moved" callback.
Comment 64 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-01-20 11:50:25 PST
Created attachment 505477 [details] [diff] [review]
v7

This is very close to v6. AllowPositionChange does still return PRBool, since
if the widget is moved, that causes some painting artifacts.

I don't know what else I could do here to make this work reasonable well.
Someone more familiar with views and layout could perhaps come up with simpler approach.
In E10s we can make the popup to stay in the content process or something.
Comment 65 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-01-21 08:43:14 PST
Apparently the redrawing problems might be because of some other regression.
Not yet sure though.
Comment 66 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-01-23 19:59:35 PST
(In reply to comment #65)
> Apparently the redrawing problems might be because of some other regression.
> Not yet sure though.

Are you still checking that?

+nsComboboxControlFrame::AbsolutelyPositionDropDownInternal(PRBool aNoNeedForAsyncClose)

Can we do async close always?
Comment 67 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-01-24 02:43:11 PST
The async close was causing painting problems in some cases.
But I need to retest.
Comment 68 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-01-24 02:56:36 PST
Though, I haven't seen any bug reports about painting problems on linux.
Joe and someone else, IIRC, were discussing about something like that
on IRC.
Comment 69 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-01-24 06:49:56 PST
Apparently Bug 627399 could be related. Will test.
Comment 70 Mike Beltzner [:beltzner, not reading bugmail] 2011-03-03 07:21:03 PST
** PRODUCT DRIVERS PLEASE NOTE **

This bug is one of 7 automatically changed from blocking2.0:final+ to blocking2.0:.x during the endgame of Firefox 4 for the following reasons:

 - it was marked as a soft blocking issue without a requirement for beta coverage
Comment 71 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-03-28 05:02:18 PDT
Comment on attachment 505477 [details] [diff] [review]
v7

Will need to update this.
Comment 72 David Bloom 2011-05-02 09:29:03 PDT
The Chromium bug for this issue has been marked as non-security. (Mitigating factors: there is a lot of user-interaction required on Chromium, and it's not a complete spoof, since it is not possible to inject arbitrary images into the <select> dropdown)
http://code.google.com/p/chromium/issues/detail?id=64118
Comment 73 Jordi Chancel 2011-09-30 19:06:19 PDT
I think this bug is more critical than high.
we can use <select> element for make a clickjacking attack, on a Java applet.

Look this video => http://www.youtube.com/watch?v=LQhD4wandzk

(result after 3 click on a webpage + one click for download and execute calc.exe)
Comment 74 David Bloom 2011-09-30 19:09:56 PDT
See also, my original comment here: https://bugzilla.mozilla.org/show_bug.cgi?id=575294#c4

There's a lot of spoofing you can do with a chromeless always-on-top rect filled with attacker controlled content.
Comment 75 Jordi Chancel 2011-09-30 19:13:07 PDT
So this vulnerability is more high than critical for you or not?
Comment 76 David Bloom 2011-09-30 19:24:52 PDT
I wish that Mozilla could fix the low-hanging fruit here (forcing <select> content to be plaintext) more quickly. That would go a long way. You can probably still do some spoofing using a custom web font, but it would be much more limited and would look a little less convincing.
Comment 77 Jesse Ruderman 2011-09-30 19:35:01 PDT
Jordi, that sounds more like a critical security hole in Java. It should not enable an install-software button in a dialog that is covered by another window. Can you file another bug report on that issue, though?
Comment 78 Jordi Chancel 2011-09-30 23:43:01 PDT
Yes I can! Can i report a new issue or post my PoC here?
Comment 79 Jesse Ruderman 2011-10-01 00:08:55 PDT
Please report a new issue -- it's likely to require a different fix (maybe even an API change between Firefox and Java).
Comment 82 Jordi Chancel 2012-02-11 03:00:48 PST
I've found a similare vulnerability that works on 10.0.1 without XUL.

https://bugzilla.mozilla.org/show_bug.cgi?id=726264
Comment 83 Benjamin Smedberg [:bsmedberg] 2012-02-14 05:55:52 PST
smaug, are you still working on this?
Comment 84 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-02-14 05:59:19 PST
It would be better if some layout dev could take this.
And we should decide what kind of UI we want.
Comment 85 Jordi Chancel 2012-02-14 06:01:35 PST
@Olli Petay and @Benjamin smedberg : I've found a similare vulnerability that works on 10.0.1 without XUL.

https://bugzilla.mozilla.org/show_bug.cgi?id=726264
Comment 86 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-02-14 06:04:26 PST
This bug has nothing to do with XUL.
Comment 87 Jordi Chancel 2012-02-14 06:06:35 PST
Bug 575294 use XUL and works only on 3.6.X ! ! !

My new vulnerability works on 10.0.1 without xuln ! ! !
Comment 88 Daniel Veditz [:dveditz] 2012-03-15 15:44:11 PDT
Raising the severity of this bug because Jordi has proved in bug 691014 and bug 726264 that it can be abused to do worse than mere "spoofing". Attention is mis-focused when the symptoms are marked critical rather than the underlying bug.
Comment 89 Jet Villegas (:jet) 2012-03-16 11:27:30 PDT
Mats: Please see if the diagnosis for 726264 applies here. I'd like to knock both bugs out with the same fix.
Comment 90 Daniel Veditz [:dveditz] 2012-04-05 13:23:43 PDT
*** Bug 742687 has been marked as a duplicate of this bug. ***
Comment 91 Mats Palmgren (:mats) 2012-04-05 13:39:30 PDT
There's a couple of issues to fix here (independently of bug 726264).
The root of the problem is that the content doesn't get a blur event
when a new window is opened (the combobox closes the dropdown on blur).
Also, there's the problem of positioning the dropdown over chrome UI.
Comment 92 Mats Palmgren (:mats) 2012-04-05 13:44:24 PDT
Created attachment 612674 [details] [diff] [review]
part 1/3, send a blur

Send a blur event when changing the focus state on the old content
when a new window is opened.
Comment 93 Mats Palmgren (:mats) 2012-04-05 13:47:19 PDT
Created attachment 612675 [details] [diff] [review]
part 2/3, never place the dropdown above the top of the root frame

Don't allow the dropdown to be above the top of the root frame.
If it's placed below the combobox button, then auto-close it (async).
If it would be clipped by bottom of the screen, then only place it above
the combobox button if it's below the top of the root - otherwise just
leave it below and let it be clipped.

And, copied from your earlier patches:
-  if (eventStates.HasState(NS_EVENT_STATE_DISABLED)) {
+  if (aDoDropDown && eventStates.HasState(NS_EVENT_STATE_DISABLED)) {
     return;

I think this is right, we don't want to prevent *closing* the
dropdown menu just because the element is now disabled. (r=me)
Comment 94 Mats Palmgren (:mats) 2012-04-05 13:48:38 PDT
Created attachment 612676 [details] [diff] [review]
part 3/3, minor cleanup

s/mFocused/sFocused/ and some indentation fixes
Comment 95 Mats Palmgren (:mats) 2012-04-05 13:51:57 PDT
Try build (including bug 726264) is green on all platforms.
The builds are available for testing here:
https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mpalmgren@mozilla.com-748972c39205/
Comment 96 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-04-06 02:33:01 PDT
Comment on attachment 612675 [details] [diff] [review]
part 2/3, never place the dropdown above the top of the root frame

>+  // Don't allow the drop-down to be placed above the top of the root frame
>+  // since that can be used to spoof UI.
This is perhaps a bit too exact comment.


>+  nsIFrame* root = PresContext()->PresShell()->GetRootFrame();
>+  nscoord rootScreenY =
>+    PresContext()->IsChrome() ? nscoord_MIN : root->GetScreenRectInAppUnits().y;
>+  nsRect thisScreenRect = GetScreenRectInAppUnits();
>+  if (thisScreenRect.y + translation.y + dropdownYOffset < rootScreenY) {
>+    NS_DispatchToCurrentThread(new nsAsyncRollup(this));
>+    return;
>+  }
>+
>+  // If the drop-down list is clipped by the bottom of the screen then position
>+  // it above the combobox button instead, unless that would place it above
>+  // the top of the root frame.
>   nsRect screen = nsFormControlFrame::GetUsableScreenRect(PresContext());
>-
>-  // Check to see if the drop-down list will go offscreen
>-  if ((GetScreenRectInAppUnits() + translation).YMost() + dropdownSize.height > screen.YMost()) {
>-    // move the dropdown list up
>-    dropdownYOffset = - (dropdownSize.height);
>+  if (thisScreenRect.YMost() + translation.y + dropdownSize.height > screen.YMost() &&
>+      thisScreenRect.y + translation.y - dropdownSize.height >= rootScreenY) {
>+    dropdownYOffset = -dropdownSize.height;

So what if the dropdown is huge, and <select> is somewhere bottom of the page?
Comment 97 Mats Palmgren (:mats) 2012-04-06 04:45:35 PDT
> This is perhaps a bit too exact comment.

OK, I'll drop the second line.

> So what if the dropdown is huge, and <select> is somewhere bottom of the page?

The same requirement applies - the drop-down isn't allowed to protrude above
the top of the page, if it does, then it's displayed downward as normal
(and will be clipped by the screen edge).  Are you worried about spoofing
something below the viewport in the status bar or outside the window?
Comment 98 Mats Palmgren (:mats) 2012-04-06 04:54:02 PDT
s/status bar/add-on bar/
Comment 99 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-04-06 08:43:39 PDT
(In reply to Mats Palmgren [:mats] from comment #97)
> The same requirement applies - the drop-down isn't allowed to protrude above
> the top of the page, if it does, then it's displayed downward as normal
> (and will be clipped by the screen edge).  Are you worried about spoofing
> something below the viewport in the status bar or outside the window?

I'm worried that the drop down is really really tiny.
Comment 100 Mats Palmgren (:mats) 2012-04-06 11:33:27 PDT
I don't understand, this method doesn't change the size of the dropdown,
it just changes its position.  Can you elaborate please?
Comment 101 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-04-06 15:19:53 PDT
If the select is right at the bottom of the page, and it has lots of <options> and the window
size is reasonable small. Is there anything which guarantees that something is shown to the user?

Make the window height to be just 100px or so, and load 
http://tinyurl.com/bnlurrf
What happens to the popup? Right now part of it ends up covering the chrome.
Comment 102 Mats Palmgren (:mats) 2012-04-06 15:53:25 PDT
Created attachment 613012 [details]
Screenshot, with patches

This is what your testcase looks like, at two different positions on
the screen.  (the gray bar at the bottom is the desktop toolbar, the
bottom of the screenshot is the bottom of the screen)
Comment 103 Jordi Chancel 2012-04-07 07:06:48 PDT
It's really a cross browser issue , i've found a similar vulnerability on Opera 11.61 (now patched) : http://www.opera.com/support/kb/view/1011/

And i've found a better way for exploit it on Google Chrome (SecSeverity:Low) : http://code.google.com/p/chromium/issues/detail?id=116425&thanks=116425&ts=1330634097
Comment 104 Jordi Chancel 2012-04-07 07:07:47 PDT
Mozilla Firefox : CRITICAL
Opera : HIGH
Google Chrome : Low/Moderate
Comment 105 Neil Deakin 2012-04-09 11:26:23 PDT
Comment on attachment 612674 [details] [diff] [review]
part 1/3, send a blur

>+    if (mActiveWindow)
>+      window->UpdateCommands(NS_LITERAL_STRING("focus"));

Since a check was made for the focused window, mActiveWindow should always be set I would think.
Comment 106 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-04-09 23:18:09 PDT
Comment on attachment 612675 [details] [diff] [review]
part 2/3, never place the dropdown above the top of the root frame

So, I think we need to resize the popup.
Comment 107 Mats Palmgren (:mats) 2012-04-12 12:39:30 PDT
Created attachment 614503 [details] [diff] [review]
part 1/5, send a blur

> Since a check was made for the focused window, mActiveWindow should always
> be set I would think.

OK, fixed.
Comment 108 Mats Palmgren (:mats) 2012-04-12 13:00:19 PDT
Created attachment 614519 [details] [diff] [review]
part 2/3, resize the dropdown to fit the larger available space above/below

> So, I think we need to resize the popup.

OK, this patch schedules an async resize reflow in these cases:
 1. if the space is too small and the dropdown can shrink (ie, it currently
    displays 2 or more rows),
 2. if the available space is now larger and the dropdown can grow (ie, it
    was clipped by the last reflow and it currently displays less than
    the maximum rows (20)).
Comment 109 Mats Palmgren (:mats) 2012-04-12 13:01:19 PDT
Created attachment 614522 [details] [diff] [review]
part 3/3, minor cleanup
Comment 110 Mats Palmgren (:mats) 2012-04-12 13:08:26 PDT
Try builds for testing:
https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mpalmgren@mozilla.com-c136b2637d1c/

I've tested this on WinXP, Linux, OSX and it works fine *for me*.
I would very much appreciate additional testing by others.
Comment 111 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-04-12 23:55:05 PDT
Comment on attachment 614519 [details] [diff] [review]
part 2/3, resize the dropdown to fit the larger available space above/below

This is getting so layout'y that some layout peer should review this too.


>-nsComboboxControlFrame::AbsolutelyPositionDropDown()
>+nsComboboxControlFrame::GetAvailableDropdownSpace(nscoord* aAbove,
>+                                                  nscoord* aBelow,
>+                                                  nsPoint* aTranslation)
> {
>-   // Position the dropdown list. It is positioned below the display frame if there is enough
>-   // room on the screen to display the entire list. Otherwise it is placed above the display
>-   // frame.
>-
>-   // Note: As first glance, it appears that you could simply get the absolute bounding box for the
>-   // dropdown list by first getting its view, then getting the view's nsIWidget, then asking the nsIWidget
>-   // for it's AbsoluteBounds. The problem with this approach, is that the dropdown lists y location can
>-   // change based on whether the dropdown is placed below or above the display frame.
>-   // The approach, taken here is to get use the absolute position of the display frame and use it's location
>-   // to determine if the dropdown will go offscreen.
>+  // Note: As first glance, it appears that you could simply get the absolute
>+  // bounding box for the dropdown list by first getting its view, then getting
>+  // the view's nsIWidget, then asking the nsIWidget for its AbsoluteBounds.
>+  // The problem with this approach, is that the dropdown lists y location can
>+  // change based on whether the dropdown is placed below or above the display
>+  // frame.  The approach, taken here is to get the absolute position of the
>+  // display frame and use its location to determine if the dropdown will go
>+  // offscreen.
> 
>   // Normal frame geometry (eg GetOffsetTo, mRect) doesn't include transforms.
>   // In the special case that our transform is only a 2D translation we
>   // introduce this hack so that the dropdown will show up in the right place.
>-  nsPoint translation = GetCSSTransformTranslation();
>+  *aTranslation = GetCSSTransformTranslation();
>+  *aAbove = 0;
>+  *aBelow = 0;
>+  
>+  nsRect thisScreenRect = GetScreenRectInAppUnits();
>+  nsRect screen = nsFormControlFrame::GetUsableScreenRect(PresContext());
>+  nscoord dropdownY = thisScreenRect.YMost() + aTranslation->y;
so dropdownY isn't really about dropdown by about 'this'. Perhaps some other name could be
better.

>-   // Use the height calculated for the area frame so it includes both
>-   // the display and button heights.
>-  nscoord dropdownYOffset = GetRect().height;
>-  nsSize dropdownSize = mDropdownFrame->GetSize();
>+  nscoord minY;
>+  if (!PresContext()->IsChrome()) {
>+    nsIFrame* root = PresContext()->PresShell()->GetRootFrame();
>+    minY = root->GetScreenRectInAppUnits().y;
>+    if (dropdownY < root->GetScreenRectInAppUnits().y) {
>+      // Don't allow the drop-down to be placed above the top of the root frame.
>+      return;
>+    }
>+  } else {
>+    minY = screen.y;
>+  }
>+  
>+  nscoord below = screen.YMost() - dropdownY;
>+  nscoord above = thisScreenRect.y + aTranslation->y - minY;
> 
>-  nsRect screen = nsFormControlFrame::GetUsableScreenRect(PresContext());
>-
>-  // Check to see if the drop-down list will go offscreen
>-  if ((GetScreenRectInAppUnits() + translation).YMost() + dropdownSize.height > screen.YMost()) {
>-    // move the dropdown list up
>-    dropdownYOffset = - (dropdownSize.height);
>+  // If the space above is less than a row height more than below,
>+  // then we favor the space below.
Strange comment. less than - more than

>+void
>+nsComboboxControlFrame::AbsolutelyPositionDropDown()
>+{
>+  nsPoint translation;
>+  nscoord above, below;
>+  GetAvailableDropdownSpace(&above, &below, &translation);
>+  if (above <= 0 && below <= 0) {
>+    NS_DispatchToCurrentThread(new nsAsyncRollup(this));
>+    return;
>+  }
>+
>+  nsSize dropdownSize = mDropdownFrame->GetSize();
>+  nscoord height = NS_MAX(above, below);
>+  nsListControlFrame* lcf = static_cast<nsListControlFrame*>(mDropdownFrame);
>+  if (height < dropdownSize.height) {
>+    if (lcf->GetNumDisplayRows() > 1) {
>+      // The drop-down doesn't fit and currently shows more than 1 row -
>+      // schedule a resize to show fewer rows.
>+      NS_DispatchToCurrentThread(new nsAsyncResize(lcf));
I wonder if scriptrunner could work here. Truly async may cause some flickering, I think.
But, I'm not sure if we have scriptblocker on stack at this point.
Comment 112 Al Billings [:abillings] 2012-05-03 14:09:07 PDT
> Comment on attachment 614519 [details] [diff] [review]
> part 2/3, resize the dropdown to fit the larger available space above/below
> 
> This is getting so layout'y that some layout peer should review this too.

Are we going to get a layout peer to review this? 

This has been sitting for a few weeks and is sg:critical. I know bug 726264 is waiting on this one as well and it is also critical.
Comment 113 Al Billings [:abillings] 2012-05-18 17:56:37 PDT
(In reply to Al Billings [:abillings] from comment #112)

> Are we going to get a layout peer to review this? 
> 
> This has been sitting for a few weeks and is sg:critical. I know bug 726264
> is waiting on this one as well and it is also critical.

Two more weeks now. Can we get a review?
Comment 114 Mats Palmgren (:mats) 2012-05-18 18:02:25 PDT
I have updated the patches to address Olli's concern regarding flickering so the
current patches are obsolete.  Sorry, for the delay.  I hope to attach new patches
later today or tomorrow.
Comment 115 Mats Palmgren (:mats) 2012-05-19 16:10:59 PDT
Created attachment 625447 [details] [diff] [review]
part 2/5, resize the dropdown to fit the larger available space above/below

I found that positioning/sizing the dropdown from DidReflow gave the wrong
results since the frame isn't positioned at that point, it may be what
caused the flicker you saw.  I changed it to use a post-reflow callback
instead.  I'll ask roc for a review of parts 3 and 4 too.
Comment 116 Mats Palmgren (:mats) 2012-05-19 16:18:56 PDT
Created attachment 625448 [details] [diff] [review]
part 3/4, call nsIRollupListener::NotifyGeometryChange when dropdown moves

This is similar to your idea of a View position observer.  To fix spoofing
I think it's enough to track the one and only dropped down menu so I'm
adding a method in nsIRollupListener for that.
Comment 117 Mats Palmgren (:mats) 2012-05-19 16:21:09 PDT
Created attachment 625450 [details] [diff] [review]
part 5/5, minor cleanup
Comment 118 Mats Palmgren (:mats) 2012-05-19 16:21:56 PDT
Try builds for testing:
https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mpalmgren@mozilla.com-052391e8f60d/
Comment 119 Mats Palmgren (:mats) 2012-05-19 16:25:51 PDT
> I'll ask roc for a review of parts 3 and 4 too.

I meant parts 2 and 3.
Comment 120 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-05-21 06:34:37 PDT
Comment on attachment 625447 [details] [diff] [review]
part 2/5, resize the dropdown to fit the larger available space above/below


>+class nsResizeDropdownAtFinalPosition : public nsIReflowCallback
>+{
>+public:
>+  nsResizeDropdownAtFinalPosition(nsComboboxControlFrame* aFrame)
>+    : mFrame(aFrame) {}
>+
>+  virtual bool ReflowFinished()
>+  {
>+    if (mFrame.IsAlive()) {
>+      static_cast<nsComboboxControlFrame*>(mFrame.GetFrame())->
>+        AbsolutelyPositionDropDown();
>+    }
>+    return false;
>+  }
>+
>+  virtual void ReflowCallbackCanceled()
>+  {
>+    delete this;
>+  }
Shouldn't you delete nsResizeDropdownAtFinalPosition object also in ReflowFinished?
Comment 121 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-05-21 06:41:08 PDT
Comment on attachment 625448 [details] [diff] [review]
part 3/4, call nsIRollupListener::NotifyGeometryChange when dropdown moves


>-class nsResizeDropdownAtFinalPosition : public nsIReflowCallback
>+class nsResizeDropdownAtFinalPosition
>+  : public nsIReflowCallback, public nsRunnable
So who owns the object and when is it deleted?
It is refcounted in some cases, when used as runnable, but not when used as reflowcallback?
Comment 122 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-05-21 06:43:08 PDT
Comment on attachment 625448 [details] [diff] [review]
part 3/4, call nsIRollupListener::NotifyGeometryChange when dropdown moves

Please add some ctor/dtor counters (whatever the macro names are) so that
possible leak can be detected easily.
Also, manual delete should be removed and ADDREF_THIS and RELEASE_THIS
should be used instead.
Comment 123 Alex Keybl [:akeybl] 2012-05-21 10:41:31 PDT
Mats let us know this is likely too risky to get into FF13. Marking 14+ for the ESR.
Comment 124 Mats Palmgren (:mats) 2012-05-27 11:06:39 PDT
Created attachment 627572 [details] [diff] [review]
part 4/5, call nsIRollupListener::NotifyGeometryChange when dropdown moves

> Shouldn't you delete nsResizeDropdownAtFinalPosition object also in ReflowFinished?

Oops, good catch.  I've incorporated your feedback regarding ownership,
so nsResizeDropdownAtFinalPosition is a always ref-counted now.  When
used as a reflow callback I'm doing a forget() after successfully adding
it to the queue, that's later Released when we get the ReflowFinished / 
ReflowCallbackCanceled callback (it's guaranteed one will be called).

https://tbpl.mozilla.org/?usebuildbot=1&tree=Try&rev=f5181fc9b7cc
Comment 126 Phil Ringnalda (:philor) 2012-05-28 22:54:01 PDT
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/8a8c0d8b1ac7 - something in that push caused bug 759243, where test_hiddenitems.xul failed every run but didn't get counted as a failure unless something else in the run failed.
Comment 127 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-05-29 01:55:00 PDT
(In reply to Mats Palmgren [:mats] from comment #125)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/a5444e23c668
> https://hg.mozilla.org/integration/mozilla-inbound/rev/205d2e0d297c
> https://hg.mozilla.org/integration/mozilla-inbound/rev/8622c5680fb3
> https://hg.mozilla.org/integration/mozilla-inbound/rev/deac5d31bc12

Oh, hmm, did this ever get a review from a layout peer.
I don't trust my layout knowledge enough ;)
Comment 128 Mats Palmgren (:mats) 2012-05-29 14:14:22 PDT
Fortunately there's nothing wrong with the patches here - the mochitest failure
(bug 759243) was just a broken test that depended on the bug that part 1 fixes.
Comment 129 Mats Palmgren (:mats) 2012-05-29 14:30:46 PDT
Comment on attachment 625447 [details] [diff] [review]
part 2/5, resize the dropdown to fit the larger available space above/below

roc, you can skip over the nsResizeDropdownAtFinalPosition class in this patch -
the final version is in part 3.  (Olli has already reviewed once)
Comment 132 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-06-03 07:20:50 PDT
This appears to have caused the topcrash in bug 760946.
Comment 133 Mats Palmgren (:mats) 2012-06-03 09:11:53 PDT
Backed out bug 575294 and bug 726264:
https://tbpl.mozilla.org/?usebuildbot=1&rev=e7ca047b71b2
Comment 134 Daniel Veditz [:dveditz] 2012-06-21 13:33:33 PDT
so now what? any new patch in the works?
Comment 135 Mats Palmgren (:mats) 2012-06-22 07:40:26 PDT
Yes, I'm working on a fix for the regression.  The problem was that
for a dropdown with <option>s of different heights setting the
computed height to 'mNumDisplayRows * heightOfARow' is too small.

This is happens for example in the review flags in Bugzilla.
I filed bug 767374 for that, but it can happen also when that
bug is fixed when the <option>s have different height for
other [valid] reasons.
Comment 136 Mats Palmgren (:mats) 2012-06-22 11:20:48 PDT
Created attachment 635824 [details] [diff] [review]
part=3/5, use exact height when it fits

Use the exact height when the dropdown fits in the available height.
mNumDisplayRows * heightOfARow is only approximation we use when
clamping the number of items we can show.

Tested on Linux/Win7/OSX.
https://tbpl.mozilla.org/?usebuildbot=1&tree=Try&rev=bf9cb39842a5
Comment 137 Alex Keybl [:akeybl] 2012-06-22 13:41:39 PDT
This bug does not have a patch landed on m-c, so clearing the ESR tracking flag for now.
Comment 140 Daniel Veditz [:dveditz] 2012-07-19 16:43:01 PDT
Mats: how scary would it be to backport to ESR-10, or uplift to Beta (Fx15)?
Comment 141 Mats Palmgren (:mats) 2012-07-20 07:37:13 PDT
I don't think we should take this on esr-10/beta, the regression risk is quite
high and I would rate this bug as sec-high, not sec-critical.
(I've just found a regression in bug 775958.)
Comment 142 Jordi Chancel 2012-07-20 10:59:10 PDT
Like demonstated in bug691014 and bug726264 this vulnerability is critical.
not high , it's a critical vulnerability.
Comment 143 Alex Keybl [:akeybl] 2012-07-26 16:47:04 PDT
Given the risk here, we're going to land in 16 first for mainline, and 17 first for ESR.
Comment 144 Jordi Chancel 2012-09-11 07:10:46 PDT
Firefox 15.0.1 don't fixe this vulnerability. 

I have reported a critical way of this vuln last year ...

PLEASE UPDATE to Fix this vulnerability ... it's too long...
Comment 145 Joe Drew (not getting mail) 2012-09-11 12:32:22 PDT
This is fixed in Firefox 16, which will be released at the beginning of October.
Comment 146 Jordi Chancel 2015-08-12 20:54:55 PDT
*** Bug 691014 has been marked as a duplicate of this bug. ***

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