Closed Bug 1432856 (CVE-2020-6810) Opened 3 years ago Closed 9 months ago

Popup window can cover fullscreen security UI

Categories

(Core :: DOM: Core & HTML, defect, P3)

57 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla74
Tracking Status
firefox-esr68 --- wontfix
firefox72 --- wontfix
firefox73 --- wontfix
firefox74 --- verified

People

(Reporter: avi, Assigned: pbz)

References

(Blocks 1 open bug)

Details

(Keywords: csectype-spoof, sec-moderate, Whiteboard: [parity Chrome and Edge][post-critsmash-triage][adv-main74+])

Attachments

(9 files, 2 obsolete files)

933 bytes, text/html
Details
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
459 bytes, text/plain
Details
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/65.0.3315.3 Safari/537.36

Steps to reproduce:

Hello. I am Avi Drissman on the Chrome security team. While investigating a security issue in Chrome, I found that Firefox is also vulnerable. The issue is https://crbug.com/776418 . While I'm happy to allow any Firefox security people access to that bug, I will explain the issue entirely in this bug.

When you take a page in Chrome into fullscreen, a bubble is shown at the top of the screen, saying "hey, you're in fullscreen, tap the escape key to get out". Chrome considers this to be security UI. Firefox has a similar bubble, and I imagine you consider this bubble to be security UI as well.

Obscuring that bubble would be bad. Therefore, if a page tries to open a popup while it is fullscreen, Chrome drops fullscreen. Firefox has the same behavior, and I again imagine it is for the same reason.

However, suppose a page opens a popup while not in fullscreen. If the page is subsequently taken to fullscreen, and then the page uses window.focus() to focus the popup, the popup is shown on top of the fullscreened page, and it obscures the fullscreen security UI bubble.

I attach a sample HTML page that will allow you to reproduce this behavior.

The original Chrome bug report has a very elaborate reproduction that spoofs Chrome UI to induce the user to click on the original page. Please be assured that limits on user gestures will not help here.

The solution that Chrome will be taking will be to kick a page out of fullscreen if it attempts to do any focusing of popups. Edge already has this behavior, and so I recommend that Firefox adopt this behavior.

In addition, I'm proposing this behavior to be standardized; see https://github.com/whatwg/fullscreen/issues/116 .

Please feel free to contact me (avi@google.com/avi@chromium.org) with any questions you may have.
We have a bug on making window.focus() kick us out of fullscreen. Was used as an abuse technique--this fullscreen thing is new to us. When I find that again I'll make this bug depend on it.
Flags: needinfo?(dveditz)
Actually we have conflicting bugs about window.focus() and fullscreen, but the one I was vaguely remembering is not this bug.
Group: firefox-core-security → dom-core-security
Component: Untriaged → DOM: Core & HTML
Flags: needinfo?(dveditz)
Keywords: csectype-spoof
Product: Firefox → Core
Whiteboard: [parity Chrome and Edge]
Flags: needinfo?(xidorn+moz)
(In reply to Daniel Veditz [:dveditz] from comment #2)
> Actually we have conflicting bugs about window.focus() and fullscreen, but
> the one I was vaguely remembering is not this bug.

I guess you were thinking about bug 1196588 and probably bug 724554.

(In reply to Avi Drissman from comment #0)
> Created attachment 8945145 [details]
> fullscreenpopuptest.html

This test page is great. Thanks!

> Obscuring that bubble would be bad. Therefore, if a page tries to open a
> popup while it is fullscreen, Chrome drops fullscreen. Firefox has the same
> behavior, and I again imagine it is for the same reason.

It doesn't seem to me that we do, at least I cannot reproduce this behavior on Windows. There may be some other behavior on other platforms due to certain code path, but I believe it's not intended.

It's not clear to me how the mechanism you proposed would work, or specifically, where should we put the check in when someone invokes window.focus(). I guess there are several options:
(1) kick out any fullscreen document when anyone invokes an effective window.focus()
(2) kick out any fullscreen document of an origin when another window of that origin is raised by an effective window.focus()
(3) kick out fullscreen document when anything in the stack calling into window.focus() is related to that document

So, 3 is complicated, and not enough, because one can always ask other document to do this in an async way, although that may be rejected by the popup abuse protection we have for window.focus(), but nothing in the spec really stops it from happening.

(1) and (2) seem to be too strict to me. I think there are valid usecase for a fullscreen document to open a popup or something like that, e.g. when you are doing presentation, and you want to show another page in a new window.

IIUC, the problem we want to solve is that, the security UI should not be blocked by anything when we show it. In that sense, I think we can solve it in an easier way:
* when we about to show the security UI, we check that we do have the focus, if not, kick the document out from fullscreen, and
* when the notification is on screen, if we lose the focus, kick the document out from fullscreen.

That should solve the security concern here without adding much complexity to the platform logic, I believe. And the spec just need to make developers aware in the section of "Security and Privacy Considerations".
Flags: needinfo?(xidorn+moz)
Daniel, what do you think about the solution I proposed at the end of comment 3?
Flags: needinfo?(dveditz)
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #3)
> > the one I was vaguely remembering is not this bug.
> 
> I guess you were thinking about bug 1196588 and probably bug 724554.

I think I was thinking of conversations the Security Engineering team has had about Chrome's Intent to Implement
https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/8Z36QLPkCd0/xrGkjvLcAAAJ

But also see bug 1437219

> (2) kick out any fullscreen document of an origin when another window of
> that origin is raised by an effective window.focus()

Restricting it to the same origin wouldn't stop abuse -- easy to make sure the other window is on a different but confederate domain.

> IIUC, the problem we want to solve is that, the security UI should not be
> blocked by anything when we show it. In that sense, I think we can solve it
> in an easier way:
> * when we about to show the security UI, we check that we do have the focus,
> if not, kick the document out from fullscreen, and
> * when the notification is on screen, if we lose the focus, kick the
> document out from fullscreen.

That sounds great.
Flags: needinfo?(dveditz)
ni? myself for implementing this.
Flags: needinfo?(xidorn+moz)

I am not sure if Xidorn has bandwidth to implement this since he's not full-time employee.
NI Ethan to put this into the security team's radar.

Flags: needinfo?(xidorn+moz) → needinfo?(ettseng)
Priority: -- → P3

Johann, this is an issue that our team can handle?

Flags: needinfo?(ettseng) → needinfo?(jhofmann)
Duplicate of this bug: 1437219

Paul might be able to give it a shot, but to set expectations this is more of a backlog item for us. I'll add it to the list :)

Flags: needinfo?(jhofmann)

(In reply to Johann Hofmann [:johannh] from comment #10)

Paul might be able to give it a shot, but to set expectations this is more of a backlog item for us. I'll add it to the list :)

Sounds good. Thanks!

Thanks! I indeed may not have bandwidth to work on this myself at the moment, but I'm always happy to answer any question and possibly review the code, so feel free to flag me if there's anything I can help!

Assignee: nobody → pbz
Status: NEW → ASSIGNED

IIUC, the problem we want to solve is that, the security UI should not be
blocked by anything when we show it. In that sense, I think we can solve it
in an easier way:

  • when we about to show the security UI, we check that we do have the focus,
    if not, kick the document out from fullscreen, and
  • when the notification is on screen, if we lose the focus, kick the
    document out from fullscreen.

I've done some testing and also implemented this behaviour by listening for the blur event. However this can interfere with normal usage. For example, opening a Video in fullscreen and then doing something on a secondary monitor would cause the browser to leave fullscreen.

I think the approach Chromium uses, to leave fullscreen on window.open() and also on window.focus() might be a good solution. Alternatively we could only behave this way while the fullscreen warning is shown.

I think the approach Chromium uses, to leave fullscreen on window.open() and also on window.focus() might be a good solution.

It might be a good solution but I also think there can be reasonable usecase triggering this. A imaginary case, for example, if user is watching a sample movie in fullscreen, and when it comes to part requiring payment, the website may pop up a payment window, and when the payment finishes, user can continue the movie in fullscreen.

Alternatively we could only behave this way while the fullscreen warning is shown.

That's exactly what I meant.

Apologies for the delayed update on this. I've looked into various options and also submitted a patch, but I'm not happy with it yet. Only leaving fullscreen when the fs warning is on screen requires a lot of refactoring, since we only know about the fs warning state in the frontend. The event has to be passed all the way through the focus manager to the frontend code. This includes extending the webidl interfaces to pass caller type (if the open/focus was triggered by the website or the system).
Going for Chromiums approach and exiting fullscreen on window.open and window.focus, would make for a much cleaner patch. We could handle everything in the backend. It would also help with other 'evil traps' where websites abuse popups in fullscreen to trick users into installing addons (Bug 1596189).

While there are some valid use cases for popups in fullscreen this wouldn't fully break workflows. Websites are aware of the fullscreen state and can avoid this behavior; given the market share of Chromium based browsers they already do. An example use case is Google Slides, where the user can open speaker notes in a popup. In Chromium, if the notes window is opened while in (DOM) fullscreen, the fullscreen is cancelled and the user has to enter it again. That's an extra click.

Depends on D55809

Attachment #9080659 - Attachment is obsolete: true

That seems reasonable to me. Thank you for all the work, Paul.

Attachment #9113477 - Attachment description: Bug 1432856 - Added console logging for DOM fullscreen leave on window open or raise. → Bug 1432856 - Added console logging for DOM fullscreen leave on window open or raise. r=smaug

Backed out for build bustages failures in nsWindow.h:

https://hg.mozilla.org/integration/autoland/rev/d94afce0c18b72e8febfaa68a21fcb7790d1118c

Build failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=281441289&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel%2Crunnable&revision=3d08c3cce53325d6623178e60d1ca5d38b4721c5
/builds/worker/workspace/build/src/widget/cocoa/nsChildView.h:310:16: error: 'SetFocus' marked 'override' but does not override any member functions
/builds/worker/workspace/build/src/widget/cocoa/nsChildView.h:282:7: error: abstract class is marked 'final' [-Werror,-Wabstract-final-class]
/builds/worker/workspace/build/src/widget/cocoa/nsChildView.h:310:16: error: 'nsChildView::SetFocus' hides overloaded virtual function [-Werror,-Woverloaded-virtual]
/builds/worker/workspace/build/src/widget/cocoa/nsCocoaWindow.h:235:16: error: 'SetFocus' marked 'override' but does not override any member functions
/builds/worker/workspace/build/src/widget/cocoa/nsCocoaWindow.h:207:7: error: abstract class is marked 'final' [-Werror,-Wabstract-final-class]
/builds/worker/workspace/build/src/widget/cocoa/nsCocoaWindow.h:235:16: error: 'nsCocoaWindow::SetFocus' hides overloaded virtual function [-Werror,-Woverloaded-virtual]

Eslint failure: https://treeherder.mozilla.org/logviewer.html#?job_id=281436170&repo=autoland
browser/base/content/test/fullscreen/open_and_focus_helper.html:32:37 | Prefer boolean length check (mozilla/prefer-boolean-length-check)

Had landed before as
https://hg.mozilla.org/integration/autoland/rev/a303b775a51b3df084ccd88d457ed2c0f8c7a4a3
https://hg.mozilla.org/integration/autoland/rev/8351a8b1d96ad8ff5b501d94b3b2c76c79db4293
https://hg.mozilla.org/integration/autoland/rev/c81f3d5b9bf33e648b9d7b4e427d1c9a8a10c38e
https://hg.mozilla.org/integration/autoland/rev/a8a4fa63f5b20f2dcc0b11e0496076e435c7e83c
https://hg.mozilla.org/integration/autoland/rev/62fc84c8ce9978f4df6d65ac6e8d8e4939b0bb65
https://hg.mozilla.org/integration/autoland/rev/49d03dd89b1759bdf28f50ad5330d5a2f9651803
https://hg.mozilla.org/integration/autoland/rev/3d08c3cce53325d6623178e60d1ca5d38b4721c5

Flags: needinfo?(pbz)
Attachment #9113477 - Attachment description: Bug 1432856 - Added console logging for DOM fullscreen leave on window open or raise. r=smaug → Bug 1432856 - Added console logging for DOM fullscreen leave on window raise. r=smaug
Flags: needinfo?(pbz)
Blocks: 1609896
Flags: qe-verify+
Whiteboard: [parity Chrome and Edge] → [parity Chrome and Edge][post-critsmash-triage]

I see no reason to rush this fix to release. LMK if you feel strongly otherwise.

Dan, Tom suggests in bug 1596189 comment 17 that we should open up this bug and I think it makes sense. The patch is public and I think since the projected attack is mostly about spoofing or irritating the user, there's no risk of an "active exploit" appearing in the small window of time until the fix hits release. It made sense to keep it sensitive while the time to fix was unknown but now I don't think we're at risk.

If you agree, can you open this up, please?

Flags: needinfo?(dveditz)

I have attempted verification with the "fullscreenpopuptest.html" attachment using the following steps:

  1. Open browser.
  2. Load the attachment (login to Bugzilla required).
  3. Make sure that the browser window is on the primary screen (if more than one is being used).
  4. Click on the "Make pop-up" button.
  5. Click on the "Toggle fullscreen" button.
  6. Click on the "Focus pop-up" button.
    Affected: The pop-up window is displayed and overlaps the full-screen related notification.
    Fixed: The pop-up window DOES NOT overlap the full-screen related notification. However, the user is forced to leave full-screen mode.

These are my results on Nightly v74.0a1 from 2020-01-31:

  • Windows 10: The pop-up window DOES NOT overlap the full-screen related notification. However, the user is forced to leave full-screen mode. Also, in this case, the pop-up does not stay in focus because the normal window gets focused after leaving full-screen mode.
  • Ubuntu 18.04: The pop-up window DOES NOT overlap the full-screen related notification. However, the user is forced to leave full-screen mode. Also, in this case, the pop-up will stay in focus (as seen in the Chrome browser).
  • Mac OS 10.14.6: The pop-up window DOES NOT overlap the full-screen related notification. However, the user is forced to leave full-screen mode. Also, in this case, the pop-up does not stay in focus because the normal window gets focused after leaving full-screen mode.

Should a new bug be logged for the secondary issue seen on Windows 10 and Mac OS?

Status: RESOLVED → VERIFIED
Flags: qe-verify+ → needinfo?(pbz)

Hsin-Tsai, I am requesting information from you in hope that you could answer some questions regarding the fix. Which are:
One issue that I observed is that the browser will instantly leave the full-screen mode when the pop-up is focused, but I am assuming that this is the compromise we are making in favor of not covering the notification about exiting full-screen. Can you confirm this is a valid approach?
A second issue is that the pop-up that gets focused will lose its focus because the "normal" browser window will be focused when leaving full-screen mode. This issue occurs only on Windows and Mac OS. Is this a valid issue? Should it be logged?

Flags: needinfo?(htsai)

Thanks for verifying the fix!

(In reply to Bodea Daniel [:danibodea] from comment #30)

Hsin-Tsai, I am requesting information from you in hope that you could answer some questions regarding the fix. Which are:
One issue that I observed is that the browser will instantly leave the full-screen mode when the pop-up is focused, but I am assuming that this is the compromise we are making in favor of not covering the notification about exiting full-screen. Can you confirm this is a valid approach?

Yes, this is the approach we opted for. See discussion above.

A second issue is that the pop-up that gets focused will lose its focus because the "normal" browser window will be focused when leaving full-screen mode. This issue occurs only on Windows and Mac OS. Is this a valid issue? Should it be logged?

Yes, that's a bit inconsistent. The behavior on Linux is the correct one, we should honor the focus request, so the window should be in focus after leaving fullscreen. I've filed Bug 1613065.

Flags: needinfo?(pbz)
Flags: needinfo?(htsai)
Group: core-security-release
Flags: needinfo?(dveditz)
Duplicate of this bug: 1596189
Whiteboard: [parity Chrome and Edge][post-critsmash-triage] → [parity Chrome and Edge][post-critsmash-triage][adv-main74+]
You need to log in before you can comment on or make changes to this bug.