575294 - (CVE-2012-3984) Navigation away from a page with an active <select> dropdown menu can be used for URL spoofing, other evil

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[David Bloom]

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]

Attachment: simple proof of concept of the vulnerability

Comment 2

[Daniel Veditz [:dveditz]]

The select can be moved to cover chrome, didn't see that the first time I ran the testcase.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Attachment: wip

...but I'm investigating still few things

Comment 4

[David Bloom]

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]

Any updates?

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

(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][[e-mail]]]

Ah, nsComboboxControlFrame::AbsolutelyPositionDropDown might need some
tweaking.

Comment 8

[David Bloom]

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][[e-mail]]]

select.innerHTML isn't handled in any special way.

IMO allowing html content <option> elements is quite handy.

Comment 10

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

Maybe we should just clamp the select position to the viewport when determining the location of the dropdown?

Comment 11

[David Bloom]

(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) (email my personal email if necessary)]

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]

Would that take care of generated content and other CSS styling as well?

Comment 14

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

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]

(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) (email my personal email if necessary)]

> 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)]

Would totally take a patch, but we have enough sg:crits that already block.

Comment 19

[Johnny Stenback (:jst)]

Olli, could you get back to this sg:high bug? It's been sitting for quite a
while now...

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

My blocker list is now quite short, so yes.
Sorry about the delay!

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

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]

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][[e-mail]]]

No, but able to spoof location bar. It is similar, though probably not
as serious as this one.

Comment 24

[Boris Zbarsky [:bzbarsky]]

> 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]

(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][[e-mail]]]

Thanks. I'll inform Opera.
Haven't tested Safari nor IE.

Comment 27

[Daniel Veditz [:dveditz]]

> 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][[e-mail]]]

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]

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][[e-mail]]]

(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]

Ah, I forgot to test that in Opera!

Yes, you definitely should file an Opera bug then :-)

Comment 32

[Olli Pettay [:smaug][[e-mail]]]

Attachment: 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) (email my personal email if necessary)]

+  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][[e-mail]]]

(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][[e-mail]]]

(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) (email my personal email if necessary)]

(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]

CCing Jordi, who found the somewhat similar bug 613935.

Comment 38

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Simple part

This is the simple part of the patch - just close the popup when unloading a page.

Comment 39

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

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][[e-mail]]]

Attachment: even safer "simple part"

Comment 41

[Olli Pettay [:smaug][[e-mail]]]

Attachment: close popup when unloading

Comment 42

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Ensure that popup is under chrome

Comment 43

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

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 45

[Olli Pettay [:smaug][[e-mail]]]

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][[e-mail]]]

...so I decided to implement an easier (IMO) approach.

Comment 47

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

Doesn't that mean that event handling/hit testing is broken when this method returns false?

Comment 48

[Olli Pettay [:smaug][[e-mail]]]

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][[e-mail]]]

Attachment: 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)]

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) (email my personal email if necessary)]

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][[e-mail]]]

Attachment: 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) (email my personal email if necessary)]

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][[e-mail]]]

(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) (email my personal email if necessary)]

So why not just do whatever AbsolutelyPositionDropDown would do in that situation?

Comment 56

[Olli Pettay [:smaug][[e-mail]]]

Well, it doesn't do anything useful in that situation.
I could ofc change AbsolutelyPositionDropDown.

Comment 57

[Olli Pettay [:smaug][[e-mail]]]

...so that it closes the popup.

Comment 58

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

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][[e-mail]]]

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) (email my personal email if necessary)]

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][[e-mail]]]

we shouldn't open the popup at all.

I could add a check for it to AbsolutelyPositionDropDown

Comment 62

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

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) (email my personal email if necessary)]

... so we should just change the callback to a "notify widget moved" callback.

Comment 64

[Olli Pettay [:smaug][[e-mail]]]

Attachment: 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][[e-mail]]]

Apparently the redrawing problems might be because of some other regression.
Not yet sure though.

Comment 66

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

(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][[e-mail]]]

The async close was causing painting problems in some cases.
But I need to retest.

Comment 68

[Olli Pettay [:smaug][[e-mail]]]

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][[e-mail]]]

Apparently Bug 627399 could be related. Will test.

Comment 70

[Mike Beltzner [:beltzner, not reading bugmail]]

** 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][[e-mail]]]

Comment on attachment 505477 [details] [diff] [review]
v7

Will need to update this.

Comment 72

[David Bloom]

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]

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]

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]

So this vulnerability is more high than critical for you or not?

Comment 76

[David Bloom]

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]

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]

Yes I can! Can i report a new issue or post my PoC here?

Comment 79

[Jesse Ruderman]

Please report a new issue -- it's likely to require a different fix (maybe even an API change between Firefox and Java).

Comment 80

[Jordi Chancel]

https://bugzilla.mozilla.org/show_bug.cgi?id=691014

Comment 82

[Jordi Chancel]

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]

smaug, are you still working on this?

Comment 84

[Olli Pettay [:smaug][[e-mail]]]

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]

@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][[e-mail]]]

This bug has nothing to do with XUL.

Comment 87

[Jordi Chancel]

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]]

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 (inactive)]

Mats: Please see if the diagnosis for 726264 applies here. I'd like to knock both bugs out with the same fix.

Comment 91

[Mats Palmgren (inactive)]

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 (inactive)]

Attachment: 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 (inactive)]

Attachment: 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 (inactive)]

Attachment: part 3/3, minor cleanup

s/mFocused/sFocused/ and some indentation fixes

Comment 95

[Mats Palmgren (inactive)]

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/[e-mail]-748972c39205/

Comment 96

[Olli Pettay [:smaug][[e-mail]]]

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 (inactive)]

> 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 (inactive)]

s/status bar/add-on bar/

Comment 99

[Olli Pettay [:smaug][[e-mail]]]

(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 (inactive)]

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][[e-mail]]]

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 (inactive)]

Attachment: 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]

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]

Mozilla Firefox : CRITICAL
Opera : HIGH
Google Chrome : Low/Moderate

Comment 105

[Neil Deakin]

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][[e-mail]]]

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 (inactive)]

Attachment: 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 (inactive)]

Attachment: 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 (inactive)]

Attachment: part 3/3, minor cleanup

Comment 110

[Mats Palmgren (inactive)]

Try builds for testing:
https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-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][[e-mail]]]

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 - ex-MoCo]]

> 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 - ex-MoCo]]

(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 (inactive)]

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 (inactive)]

Attachment: 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 (inactive)]

Attachment: 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 (inactive)]

Attachment: part 5/5, minor cleanup

Comment 118

[Mats Palmgren (inactive)]

Try builds for testing:
https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-052391e8f60d/

Comment 119

[Mats Palmgren (inactive)]

> I'll ask roc for a review of parts 3 and 4 too.

I meant parts 2 and 3.

Comment 120

[Olli Pettay [:smaug][[e-mail]]]

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][[e-mail]]]

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][[e-mail]]]

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]]

Mats let us know this is likely too risky to get into FF13. Marking 14+ for the ESR.

Comment 124

[Mats Palmgren (inactive)]

Attachment: 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 125

[Mats Palmgren (inactive)]

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

Comment 126

[Phil Ringnalda (:philor)]

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][[e-mail]]]

(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 (inactive)]

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 (inactive)]

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 130

[Mats Palmgren (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1727de7d26ec
https://hg.mozilla.org/integration/mozilla-inbound/rev/011c1c8e9cc7
https://hg.mozilla.org/integration/mozilla-inbound/rev/0be261a5042b
https://hg.mozilla.org/integration/mozilla-inbound/rev/d51338c5cd0c

Comment 131

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/1727de7d26ec
https://hg.mozilla.org/mozilla-central/rev/011c1c8e9cc7
https://hg.mozilla.org/mozilla-central/rev/0be261a5042b
https://hg.mozilla.org/mozilla-central/rev/d51338c5cd0c

Comment 132

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

This appears to have caused the topcrash in bug 760946.

Comment 133

[Mats Palmgren (inactive)]

Backed out bug 575294 and bug 726264:
https://tbpl.mozilla.org/?usebuildbot=1&rev=e7ca047b71b2

Comment 134

[Daniel Veditz [:dveditz]]

so now what? any new patch in the works?

Comment 135

[Mats Palmgren (inactive)]

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 (inactive)]

Attachment: 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]]

This bug does not have a patch landed on m-c, so clearing the ESR tracking flag for now.

Comment 138

[Mats Palmgren (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cc89df1b08ae
https://hg.mozilla.org/integration/mozilla-inbound/rev/e3676fde39a8
https://hg.mozilla.org/integration/mozilla-inbound/rev/23f5c88adb8f
https://hg.mozilla.org/integration/mozilla-inbound/rev/612ee5c6300d
https://hg.mozilla.org/integration/mozilla-inbound/rev/b47364c051d2

Comment 139

[Mats Palmgren (inactive)]

https://hg.mozilla.org/mozilla-central/rev/e3676fde39a8
https://hg.mozilla.org/mozilla-central/rev/23f5c88adb8f
https://hg.mozilla.org/mozilla-central/rev/612ee5c6300d
https://hg.mozilla.org/mozilla-central/rev/b47364c051d2
https://hg.mozilla.org/mozilla-central/rev/b2e5b06b0c24

Comment 140

[Daniel Veditz [:dveditz]]

Mats: how scary would it be to backport to ESR-10, or uplift to Beta (Fx15)?

Comment 141

[Mats Palmgren (inactive)]

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]

Like demonstated in bug691014 and bug726264 this vulnerability is critical.
not high , it's a critical vulnerability.

Comment 143

[Alex Keybl [:akeybl]]

Given the risk here, we're going to land in 16 first for mainline, and 17 first for ESR.

Comment 144

[Jordi Chancel]

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)]

This is fixed in Firefox 16, which will be released at the beginning of October.