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

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

...but I'm investigating still few things

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

Any updates?

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

Ah, nsComboboxControlFrame::AbsolutelyPositionDropDown might need some tweaking.

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

select.innerHTML isn't handled in any special way. IMO allowing html content <option> elements is quite handy.

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

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

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.

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

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.

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

> An attacker could still use a custom font too, right? Yes. It would be easy to lock the font using a !important rule, though.

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

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

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

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?

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?

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

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

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

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

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

I didn't see exactly this bug in Chrome or Opera, but something similar related select's popup over location bar.

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.

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

Ah, I forgot to test that in Opera! Yes, you definitely should file an Opera bug then :-)

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

+ 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.

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

(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

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

CCing Jordi, who found the somewhat similar bug 613935.

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

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?

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.

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.

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

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

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.

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 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()).

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?

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)

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?

(In reply to comment #53) > Why not just call AbsolutelyPositionDropDown unconditionally? Because there is really no place to re-position the popup always.

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

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

...so that it closes the popup.

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.

Roc, I'm not quite sure what all I should do with this to get the patch good enough.

Tell me what should happen if the user tries to open a popup but we can't find a safe place to put it.

we shouldn't open the popup at all. I could add a check for it to AbsolutelyPositionDropDown

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.

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

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.

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

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

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

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.

Apparently Bug 627399 could be related. Will test.

** 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 on attachment 505477 [details] [diff] [review] v7 Will need to update this.

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

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)

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.

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

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.

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?

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

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

I've found a similare vulnerability that works on 10.0.1 without XUL. https://bugzilla.mozilla.org/show_bug.cgi?id=726264

smaug, are you still working on this?

It would be better if some layout dev could take this. And we should decide what kind of UI we want.

@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

This bug has nothing to do with XUL.

Bug 575294 use XUL and works only on 3.6.X ! ! ! My new vulnerability works on 10.0.1 without xuln ! ! !

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.

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

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.

Send a blur event when changing the focus state on the old content when a new window is opened.

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)

s/mFocused/sFocused/ and some indentation fixes

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

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

s/status bar/add-on bar/

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

I don't understand, this method doesn't change the size of the dropdown, it just changes its position. Can you elaborate please?

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.

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)

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

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

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

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

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

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

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

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.

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.

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.

Try builds for testing: https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mpalmgren@mozilla.com-052391e8f60d/

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

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

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

> 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

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

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.

(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 ;)

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

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

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

This appears to have caused the topcrash in bug 760946.

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

so now what? any new patch in the works?

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.

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

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

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

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

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

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

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

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

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

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