374570 - Limit content-generated popups to content area

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Daniel Veditz [:dveditz]]

From bug 326877 comment 72, user content can generate menu popups that float over all windows and can be positioned at arbitrary window positions. This allows spoofing things such as CST's BSOD testcase
  https://bugzilla.mozilla.org/attachment.cgi?id=253104

and the phisher's favorite, covering up the URL-bar, http://ctho.ath.cx/tmp/moz/326877evil2.xul from bug 326877 comment 76

We need to restrict non-privileged popups to the area of the content window.

Comment 1

[Neil Deakin]

Do you mean just adjust the size of popups to the rectangle of the parent document (by resizing the popup), or make the popups work more like absolute positioned items that are just overlaid into the page (such that one would need to scroll if the popup is off the border)?

Comment 2

[Daniel Veditz [:dveditz]]

I mean adjust the size and position of the popup to the parent document, it can still be a popup (I saw your "in page" Mac patch in bug 326877 and I think people who use popups would rebel). We'd also have to add position checks if popups can be moved--can they? I had a partially working patch from bug 326877, I'll try to find and attach it.

Comment 3

[Neil Deakin]

OK, that's actualy easier to implement.

Comment 4

[Neil Deakin]

Attachment: This seems to work

Not sure if GetScreenRect is right here for xul documents, but it seems to work. Want to test it a bit more before it gets reviewed. It constrains popups from content to the window area.

Comment 5

[Neil Deakin]

Attachment: testcase

This testcase shows some menus and moves and resizes them when the mouse is moved over them. The patch currently just prevents popups from being moved, but resizing should be constrained to the window.

Comment 6

[Neil Deakin]

Comment on attachment 259238 [details] [diff] [review]
This seems to work

I tested this some more and it seems to work

Comment 7

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

+    !GetStyleDisplay()->mAppearance && !mInContentShell;

Do we need this check anymore? Seems to me that with this patch transparent popups are OK, right?

What if someone displays a popup and then moves the window away? The popup stays where it is, right? That seems like a problem.

Comment 8

[Neil Deakin]

I suppose what should happen if the window is moved is that the popups should be closed, perhaps by calling DocumentViewerImpl::HideViewIfPopup

Do we need to be concerned only when a script moves the window, or when the user does?

Note also that the popup will shuffle position back to be constrained within the frame area anyway when it gets reflowed.

Comment 9

[Daniel Veditz [:dveditz]]

I'm not at all concerned if the user manually moves the window, but wouldn't popups normally close when the user clicks on the window to drag it anyway?

Comment 10

[Neil Deakin]

Yes, it should do, although only the one that the widget system is keeping track of.

Comment 11

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

I'm primarily concerned about the script moving the window, right.

Comment 12

[Neil Deakin]

Attachment: this version closes popups when the parent window is moved

Comment 13

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

+    !GetStyleDisplay()->mAppearance && !mInContentShell;

We can allow transparent popups for content now, right?

On trunk, instead of HideViewIfPopup, how about using nsPresContext::GetActivePopups, or whatever the equivalent is after the popups rework? I guess we can land this first if you promise to fix that later.

Comment 14

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

Comment on attachment 260587 [details] [diff] [review]
this version closes popups when the parent window is moved

OK, so this patch is acceptable as-is.

Comment 15

[Neil Deakin]

(In reply to comment #13)
> +    !GetStyleDisplay()->mAppearance && !mInContentShell;
> 
> We can allow transparent popups for content now, right?
> 

Don't know. I didn't follow the implementing of transparent popups.

> On trunk, instead of HideViewIfPopup, how about using
> nsPresContext::GetActivePopups, or whatever the equivalent is after the popups
> rework? I guess we can land this first if you promise to fix that later.

The list in GetActivePopups doesn't contain all the popups. Once the popup rework is done, HideViewIfPopup won't be needed either so it won't need to mess about with views.

Comment 16

[Neil Deakin]

dveditz, does this patch fix what's needed?

Comment 17

[Daniel Veditz [:dveditz]]

Finally got a chance to test and it addresses the cases I was worried about.

The testcase attachment 259240 [details] doesn't open menus "up" in a small window as advertised by the text in the page, dunno if that's anything to worry about from a regression POV.

Comment 18

[Neil Deakin]

> The testcase attachment 259240 [details] doesn't open menus "up" in a small window as
> advertised by the text in the page, dunno if that's anything to worry about
> from a regression POV.
> 

Which text says this? Maybe you linked to the wrong attachment?

Comment 19

[Daniel Veditz [:dveditz]]

Attachment 259240 [details], which is the second attachment in this bug ("testcase"), says 

  "Try shrinking the window down and opening either of the three menus.
   They should move to open upwards instead of down if there is room,
   otherwise shrink to the frame size."

I'm happy with what they're doing post-patch, just noting that nothing goes "up" no matter how small I make the window. Maybe the testcase description is wrong.

Comment 20

[Neil Deakin]

What kind of interface changes are allowed for the branch now? There's a change in nsIPresShell in this patch.

Comment 21

[Daniel Veditz [:dveditz]]

I *think* changing nsIPresShell is OK because it's not an .idl or otherwise available externally. We haven't made promises about internal layout interfaces as far as I know. But we should double-check that with one of the layout owners like roc or dbaron.

Comment 22

[Daniel Veditz [:dveditz]]

Absent approval to change nsIPresShell on the branch (and dbaron was not sure) you could add the new function to a nsIPresShell_MOZILLA_1_8_BRANCH interface.

Comment 23

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

I think we have avoided changing nsIPresShell on the branch because extensions can (and I think do) use it.

Comment 24

[Neil Deakin]

There actually already is a nsIPresShell_MOZILLA_1_8_BRANCH interface, should this just be modified or should yet another interface be added?

Why are extensions using nsIPresShell? It's an internal non-scriptable interface.
 

Comment 25

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

I think we can modify nsIPresShell_MOZILLA_1_8_BRANCH.

Maybe it's embedders that use it, not extensions. I'm pretty sure gnome-web-photo and Epiphany use it.

Comment 26

[Neil Deakin]

Attachment: branch patch

Comment 27

[Boris Zbarsky [:bzbarsky]]

The branch patch has the same missing return as the trunk one: see bug 378827.  Fix that when you land?

Comment 28

[Daniel Veditz [:dveditz]]

Comment on attachment 262652 [details] [diff] [review]
branch patch

approved for 1.8.0.12 and 1.8.1.4, a=dveditz for release-drivers

Comment 29

[Hannes Verschore [:h4writer]]

I don't know if I get it right. But if I go to http://ctho.ath.cx/tmp/moz/326877evil2.xul I get a bar that tries to cover the url-bar. Now this has been patched and it just comes on the screen.

But is it normal that if I go to another tab, it still shows the bar? Isn't that a possible way to fake a login screen?
1) load an xul showing a login screen (like the one of gmail)
2) open gmail in a new tab
3) The login screen of the XUL is on top of the gmail one and has the same look. Nobody expects a fake one?!!!!

Comment 30

[gml4410]

It's also the case that:

1) goto http://ctho.ath.cx/tmp/moz/326877evil2.xul   - pop-up appears in window
2) click back button -  back to previous page, with no pop-up
3) forward button (ie: return to pop-up page) - there is now no pop-up window

The pop-up returns if you click on the window (well, in the main part of the window - it doesn't return if you clock in the entry box at the bottom).

I can't see any security issue in this at the moment, just reporting and oddity.