1735071 - (CVE-2022-22746) reportValidity allow focus stealing can be abused to overlap fullscreen security notification

Status: VERIFIED FIXED

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

Description

[Irvan Kurniawan [:sourc7]]

Attachment: spoof-fullscreen-reportvalidity-focus.html

After creating popup with window.open then call requestValidity on parent window or child window, on Windows OS the focus will be switched from popup window to the parent window or vice versa, allowing pop-under popup window and focus stealing.

Normally after request fullscreen then subsequently call childWindow.focus() to overlap fullscreen notification, it will be disallowed, the console will show warning message Opening multiple popups was blocked due to lack of user activation. or Exited fullscreen because a window was focused.

Interestingly when using requestValidity to switch focus to child window after requesting fullscreen, it will be allowed, the fullscreen notification will be overlapped with child window.

Steps to reproduce:

1. Visit attached spoof-fullscreen-requestvalidity-focus.html
2. Click "New Window" button
3. After child window launched, the parent window will regain focus with validation message "Spoof window is ready!"
4. Click "Spoof" button
5. Fullscreen notification will be overlapped with child window

Comment 1

[Irvan Kurniawan [:sourc7]]

Attachment: Firefox - requestValidity focus steal abused to overlap fullscreen security notification.mp4

Comment 2

[Irvan Kurniawan [:sourc7]]

Oh sorry typo in description, it should be reportValidity not requestValidity.

Comment 3

[Daniel Veditz [:dveditz]]

Could reproduce on Windows, but only when the window is maximized. Couldn't reproduce on Mac or Linux

On Mac I get the new window covering the spoof in both nightly and release without the spoof regaining focus. In Nightly the form validation message bleeds through the top window which seems like a regression of a spoofing bug we recently fixed (quite probably one of Irvan's).

Calling sec-moderate for now, but we'll have to study how much of our user population uses maximized windows. Could be a full sec-high

Comment 4

[Daniel Veditz [:dveditz]]

We have a lot of windows users, and maximizing is an easier click than dragging to fill the screen. Likely used a bunch by laptop folks (just checked -- my wife's computer was maximized).

Comment 5

[Edgar Chen [:edgar]]

It seems like a focus bug. The reportValidity() brings the popup window to the foreground which happens only on Window.

Comment 6

[Irvan Kurniawan [:sourc7]]

Attachment: spoof-fullscreen-reportvalidity-focus-maximized.html

(In reply to Daniel Veditz [:dveditz] from comment #4)

We have a lot of windows users, and maximizing is an easier click than dragging to fill the screen. Likely used a bunch by laptop folks (just checked -- my wife's computer was maximized).

Thanks! Yes it is also possible to maximize the popup window by adding width=99999 and height=99999 to the windowFeatures. The popup window will launch maximized even the main window is resized.

Comment 7

[Edgar Chen [:edgar]]

There are two things need to check,

1. Why would calling reportValidity() on background popup bring the popup foreground? This happens only on Windows, not on Mac and Linux. Probably an issue on Widget layer?
2. Why doesn't the focus changes exit the fullscreen? When a fullscreen document goes to background, it should exit fullscreen, e.g. if you replace the spoofWindow.document.getElementById("input1").reportValidity(); with spoofWindow.focus(); in test script, fullscreen exits expectedly. We might also have some issue around that.

Comment 8

[Edgar Chen [:edgar]]

(In reply to Edgar Chen [:edgar] from comment #7)

1. Why would calling reportValidity() on background popup bring the popup foreground? This happens only on Windows, not on Mac and Linux. Probably an issue on Widget layer?

Yeah, seem like a Windows Widget specific behavior, see https://searchfox.org/mozilla-central/rev/88cd13997fb0747cdcd78638fc762ff2d75e1fc5/widget/windows/nsWindow.cpp#1638-1642, if I remove flags |= SWP_NOACTIVATE then the validation popup won't bring the window to the foreground like what we do in Mac and Linux. Hi Chris, do you know why we make the popup always on the top in Windows?

Comment 9

[Edgar Chen [:edgar]]

(In reply to Edgar Chen [:edgar] from comment #7)

2. Why doesn't the focus changes exit the fullscreen? When a fullscreen document goes to background, it should exit fullscreen, e.g. if you replace the spoofWindow.document.getElementById("input1").reportValidity(); with spoofWindow.focus(); in test script, fullscreen exits expectedly. We might also have some issue around that.

Okay, not all focus changes would exit the fullscreen, like, Alt + Tab to switch window. We exit the fullscreen when the focus changes which is initiated from the script. And other browsers behave the same. I think we don't expect the validation popup would change the focus as we don't behave like that in Mac or Linux, nor other browsers.

Comment 10

[Gian-Carlo Pascutto [:gcp]]

Hi Chris, do you know why we make the popup always on the top in Windows?

The code (and comment) was added 12 years ago by Jim Mathies, so I doubt Chris would know the reasoning. Hey Jim, how good is your memory?

Comment 11

[Gian-Carlo Pascutto [:gcp]]

Whoops, blamed the wrong person here. The code was added by [e-mail] in 1999.
https://searchfox.org/mozilla-central/commit/98f934025938d0937fc3cc612ce9c00045b86c86

So I don't think you're going to find a WHY answer to your question without removing it and testing the scenarios.

Comment 12

[Gian-Carlo Pascutto [:gcp]]

David, do you have any particular insight here? (See also my questions in Matrix about the apparent reversed logic, which may be due to my unfamiliarity with Windows widgets)

Comment 13

[David Parks [:handyman]]

I basically agree with Edgar, although removing SWP_NOACTIVATE for popups is confusing. I can't explain why activating the popup changes anything about z-order, much less fixes this, but it does seem that activating the popup is not right. I've instead changed that line from

    ::SetWindowPos(mWnd, owner ? 0 : HWND_TOPMOST, 0, 0, 0, 0, flags);	

to

    if (owner) {
      // ePopupLevelTop popups should be above all else.  All other
      // types should be placed in front of their owner, without
      // changing the owner's z-level relative to other windows.
      if (PopupLevel() != ePopupLevelTop) {
        ::SetWindowPos(mWnd, owner, 0, 0, 0, 0, flags);
        ::SetWindowPos(owner, mWnd, 0, 0, 0, 0,
                       SWP_NOMOVE | SWP_NOSIZE | SWP_NOACTIVATE);
      } else {
        ::SetWindowPos(mWnd, HWND_TOP, 0, 0, 0, 0, flags);
      }
    } else {
      ::SetWindowPos(mWnd, HWND_TOPMOST, 0, 0, 0, 0, flags);
    }

which, since the popups are not ePopupLevelTop, moves them into place without moving the owner. (The first SWP call moves the popup behind the owner but handles the SWP_SHOWWINDOW -- the second SWP moves the owner behind the popup, which was immediately behind the owner, so the owner's relative position wrt all other windows is unchanged.)

I've left the ePopupLevelTop case as HWND_TOP, although I think it probably should be HWND_TOPMOST, but that's not what we had.

Does this seem like a good approach?

Comment 14

[David Parks [:handyman]]

Attachment: Bug 1735071: Make Windows non-ePopupLevelTop popups respect owner z-order r=edgar!,cmartin!

Popups that aren't ePopupLevelTop should only appear above their owner and should do so without changing their owner's Z-order relative to other windows.

Comment 15

[David Parks [:handyman]]

Looks good after experiments so I'm switching it to review.

Comment 16

[Edgar Chen [:edgar]]

Change the assignee to David, as he wrote the patch. Thanks! :)

Comment 17

[Daniel Veditz [:dveditz]]

The reportValidity() brings the popup window to the foreground which happens only on Window.

Isn't that how you get the dreaded "pop-unders"?

Is the problem the SWP_NOACTIVATE flag, or the fact that we're calling nsWindow::Show() at all as part of the reportValidity code? Do we call it on the other platforms and it doesn't have the window-raising effect for some reason? In the testcase, the original window.open puts the popup on top, and on mac and linux it stays there until the user manually switches to the original testcase window. On windows the first reportValidity() call brings that testcase window back up on top which is already a problem (the pop-unders mentioned above). Focus stealing can be abused, and we don't allow web content to otherwise set focus to another window nor grab focus back to itself. web content could move the focus between elements in another same-origin tab/window, but that won't make the tab/window itself the active tab. It's only apparent when the user themselves switches windows. (as an exception, you're allowed to close a script-opened window. This can be abused to unexpectedly shift focus, such as in so-called "tab-napping" spoofs, but we're kind of stuck with that for now.)

Comment 18

[Daniel Veditz [:dveditz]]

See also bug 1432856 where Smaug added code to exit fullscreen on windowRaise to stop a previous attempt to hide the fullscreen notification with a popup.
https://searchfox.org/mozilla-central/rev/4aae583beac98b934e2457dbd1eb6c8d7b9e3215/dom/base/nsFocusManager.cpp#1623

Comment 19

[Edgar Chen [:edgar]]

(In reply to Daniel Veditz [:dveditz] from comment #17)

In the testcase, the original window.open puts the popup on top, and on mac and linux it stays there until the user manually switches to the original testcase window. On windows the first reportValidity() call brings that testcase window back up on top which is already a problem (the pop-unders mentioned above).

Yes, exactly. Showing a popup widget should not bring the owner window to the top.
nsWindow::Show() is called on the popup widget to show the validity popup (we behave the same on all platforms), but the z-order of the owner window is also changed on Windows, and move the owner window of the popup widget to the top. Note that, in such case, the focus state isn't actually changed at all, i.e. the focus is still on the original window, but we show the unfocused one to the top, and when the user clicks the top one (which doesn't have the focus), then we really move the focus to that window.

Comment 20

[Hsin-Yi Tsai (she/her) [:hsinyi]]

The patch is r+. Shall we request sec-approval? so we can land the patch?

Comment 21

[David Parks [:handyman]]

Comment on attachment 9252184 [details]
Bug 1735071: Make Windows non-ePopupLevelTop popups respect owner z-order r=edgar!,cmartin!

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Not easily. The issue is only known to be exploitable by reportValidity, and someone would have to make the connection between the popup level and validity reporting to think to use that as an exploit.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?: all (mozregression shows issue is at least 8 years old)
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: Not difficult. This is not volatile code.
• How likely is this patch to cause regressions; how much testing does it need?: Unclear but my sense in it's unlikely. Patch seems fundamentally correct but popups are used in many unexpected places so thorough testing is hard.

Comment 22

[David Parks [:handyman]]

I think we can land this without further widget involvement. I wanted to confirm that the patch deals with things according to dveditz and edgar's comments above (it does).

Comment 23

[Tom Ritter [:tjr]]

Comment on attachment 9252184 [details]
Bug 1735071: Make Windows non-ePopupLevelTop popups respect owner z-order r=edgar!,cmartin!

Approved to land and uplift

Comment 24

[Tom Ritter [:tjr]]

[Tracking Requested - why for this release]:

Comment 25

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Make Windows non-ePopupLevelTop popups respect owner z-order r=edgar
https://hg.mozilla.org/integration/autoland/rev/ddd7ab8b7a2749f93c2ec056f4c2469fc798d80f
https://hg.mozilla.org/mozilla-central/rev/ddd7ab8b7a27

Comment 26

[BugBot [:suhaib / :marco/ :calixte]]

As part of a security bug pattern analysis, we are requesting your help with a high level analysis of this bug. It is our hope to develop static analysis (or potentially runtime/dynamic analysis) in the future to identify classes of bugs.

Please visit this google form to reply.

Comment 27

[Dianna Smith [:diannaS]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f365176fa3eb

Approved for 96.0b4

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr91/rev/e99a3304b8f4

Comment 29

[David Parks [:handyman]]

(In reply to Release mgmt bot [:sylvestre / :calixte / :marco for bugbug] from comment #26)

As part of a security bug pattern analysis, we are requesting your help with a high level analysis of this bug. It is our hope to develop static analysis (or potentially runtime/dynamic analysis) in the future to identify classes of bugs.

Please visit this google form to reply.

Done.

Comment 30

[Anca Soncutean, Desktop QA]

Reproduced with Fx 95.0a1 (2021-10-10) on Windows 10.
Verified fixed with Fx 97.0a1 (2021-12-12) and Fx 96.0b4 on Windows 10.

Comment 31

[Anca Soncutean, Desktop QA]

Verified fixed also with 91.5.0esr (treeherder build) on Windows 10.

Comment 32

[Frederik Braun [:freddy]]

Attachment: advisory.txt