Closed Bug 1739091 (CVE-2021-43538) Opened 3 years ago Closed 2 years ago

Force Collision Between requestFullscreen() and requestPointerLock() Notification will Hide Both Notification

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Platform:
Desktop
All
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
96 Branch
Tracking Flags:
Tracking Status
firefox-esr91 95+ verified
firefox94 --- wontfix
firefox95 + verified
firefox96 + verified

People

(Reporter: sourc7, Assigned: pbz)

References

Details

(Keywords: csectype-spoof, sec-high, Whiteboard: Windows only? [reporter-external] [client-bounty-form][sec-survey][adv-main95+][adv-ESR91.4.0+])

Attachments

(5 files, 1 obsolete file)

testcase.html
477 bytes, text/html
Details
Firefox - Force Collison Between Fullscreen and Pointerlock will Hide Both Notification on Windows 11.mp4
162.14 KB, video/mp4
Details
testcase-timeout1333.html
478 bytes, text/html
Details
Bug 1739091, r=gijs!
48 bytes, text/x-phabricator-request
pascalc
: approval-mozilla-beta+
pascalc
: approval-mozilla-esr91+
tjr
: sec-approval+
Details | Review
advisory.txt
300 bytes, text/plain
Details
Attached file testcase.html —

After requesting fullscreen then request pointer lock and append fullscreenElement to pointerLock element multiple times, interestingly when the notification collide, the both notification will immediately hidden and will not show on the screen.

Steps to reproduce

  1. Visit attached testcase.html
  2. Click anywhere on the page
  3. Browser goes into fullscreen without any fullscreen notification
Flags: sec-bounty?
Group: firefox-core-security → dom-core-security
Type: task → defect
Component: Security → DOM: Core & HTML
Product: Firefox → Core
Summary: Force Collison Between requestFullscreen() and requestPointerLock() Notification will Hide Both Notification → Force Collision Between requestFullscreen() and requestPointerLock() Notification will Hide Both Notification

On a Mac nightly I see the fullscreen notification.
Tyson can reproduce on Windows.

Keywords: csectype-spoof, sec-high
OS: Unspecified → Windows
Hardware: Unspecified → Desktop
Whiteboard: [reporter-external] [client-bounty-form] [verif?] → Windows only? [reporter-external] [client-bounty-form] [verif?]

I also see the notification on Linux.

I can also reproduce this on Arch Linux with KDE X11 and Ubuntu 21.04 Wayland on VirtualBox resized to half.

I also able to see the notification after maximizing the Ubuntu 21.04 on VirtualBox due to emulated rendering using more CPU on high resolution (2560x1440). I certain it caused by bottleneck so startCollison interval will be cleared before fullscreen is requested.

Fortunately after changing the setTimeout to clearInterval from 333 to 1333 I also able to reproduce this on Ubuntu 21.04 on VirtualBox with maximized screen.

If the fullscreen notification still appear, try to changing setTimeout value from 1333 to higher value.

Severity: -- → S2

I could also reproduce this on Mac.

It looks like this is because fullscreen and pointerlock share the same warning UI.
What the test does is to periodically request and release pointerlock, and the fullscreen warning would be closed by releasing pointerlock as they share the same warning UI. Hi Paul, is this something you could take a look at? Thanks!

Flags: needinfo?(pbz)
OS: Windows → All

I can take a look. Thanks!

Here is a simpler PoC:

<html onclick=main()>
<script>
    async function main() {
        await document.body.requestFullscreen();
        await document.body.requestPointerLock();
        setTimeout(() => document.exitPointerLock(), 200);
    }
</script>
</html>

The timeout can be decreased but then it gets less reliable.

The issue is that exiting the pointer lock sends a MozDOMPointerLock:Exited message, which means we'll call the UI here: https://searchfox.org/mozilla-central/rev/fb8d77331582639ea6848a61dd8ee812fac31b77/browser/actors/PointerLockParent.jsm#19-20 which then hides the security UI: https://searchfox.org/mozilla-central/rev/fb8d77331582639ea6848a61dd8ee812fac31b77/browser/base/content/browser-fullScreenAndPointerLock.js#125.
The UI does not check which message is currently shown and whether the document is in full screen. Thus it hides the full screen warning message.

For a simple fix, I'm wondering if we can add a check for document.fullscreenElement and return early if we're still in full screen.
i.e. change this condition:
https://searchfox.org/mozilla-central/rev/fb8d77331582639ea6848a61dd8ee812fac31b77/browser/base/content/browser-fullScreenAndPointerLock.js#126
to if (!this._element || document.fullscreenElement) {.

Edgar, do you think such a check in the frontend is fine, or are we looking for a more involved solution on the DOM side?

Generally, I'm wondering why we didn't see this issue earlier, given how easy it is to produce. I ran mozregression from FF80, it's at least not a recent regression.

Assignee: nobody → pbz
Status: NEW → ASSIGNED
Flags: needinfo?(pbz) → needinfo?(echen)

We can also be more explicit about it and only close the warning if its a pointerlock one, for that specific close call: https://searchfox.org/mozilla-central/rev/6deb8b6af57a8b5b6b1bcb143ea498e566475d8d/browser/base/content/browser-fullScreenAndPointerLock.js#248
This may be more reliable / less susceptible to race conditions than asking the document about the fullscreen state.

(In reply to Paul Zühlcke [:pbz] from comment #6)

For a simple fix, I'm wondering if we can add a check for document.fullscreenElement and return early if we're still in full screen.
i.e. change this condition:
https://searchfox.org/mozilla-central/rev/fb8d77331582639ea6848a61dd8ee812fac31b77/browser/base/content/browser-fullScreenAndPointerLock.js#126
to if (!this._element || document.fullscreenElement) {.

Edgar, do you think such a check in the frontend is fine, or are we looking for a more involved solution on the DOM side?

It should work for this case, but I worry about it might cause bugs in another way around, e.g. user request pointer lock then fullscreen and we would try to close the pointer lock warning in https://searchfox.org/mozilla-central/rev/6deb8b6af57a8b5b6b1bcb143ea498e566475d8d/browser/base/content/browser-fullScreenAndPointerLock.js#445 before performing the fullscreen transition. Thus if we can have a more explicit way to control the warning, that may be more reliable. Thanks!

Flags: needinfo?(echen)

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

(In reply to Paul Zühlcke [:pbz] from comment #6)

For a simple fix, I'm wondering if we can add a check for document.fullscreenElement and return early if we're still in full screen.
i.e. change this condition:
https://searchfox.org/mozilla-central/rev/fb8d77331582639ea6848a61dd8ee812fac31b77/browser/base/content/browser-fullScreenAndPointerLock.js#126
to if (!this._element || document.fullscreenElement) {.

Edgar, do you think such a check in the frontend is fine, or are we looking for a more involved solution on the DOM side?

It should work for this case, but I worry about it might cause bugs in another way around, e.g. user request pointer lock then fullscreen and we would try to close the pointer lock warning in https://searchfox.org/mozilla-central/rev/6deb8b6af57a8b5b6b1bcb143ea498e566475d8d/browser/base/content/browser-fullScreenAndPointerLock.js#445 before performing the fullscreen transition. Thus if we can have a more explicit way to control the warning, that may be more reliable. Thanks!

I don't think this case is problematic, since the full screen warning will always take priority over pointer lock. Also, exiting full screen will also exit pointer lock.
I'll submit a patch for the approach described in comment 7 and will do some more testing.

Attached file Bug 1739091, r=gijs! —
Attached file Bug 1742433 - Test, r=gijs! (obsolete) —

Depends on D131268

Comment on attachment 9250987 [details]
Bug 1739091, r=gijs!

Security Approval Request

  • How easily could an exploit be constructed based on the patch?: Constructing an exploit based on the patch is not trivial. However, looking at the patch it's easy to recognize that we were closing the wrong warning UI before. This means some exploratory testing with full screen and pointer lock could lead to the exploit. The exploit isn't very complex.
  • 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, this is not a regression
  • 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?: Small frontend code changes which should be easy to apply to all branches.
  • How likely is this patch to cause regressions; how much testing does it need?: Since the patch makes it more specific which warning UI is closed at which state, there is a risk of lingering warning UI. However, this should be covered by existing automated tests.
    Beyond some manual testing, a variant of the original PoC is covered by a new automated test in a separate patch.
Attachment #9250987 - Flags: sec-approval?
Attachment #9251222 - Flags: sec-approval?

Comment on attachment 9250987 [details]
Bug 1739091, r=gijs!

Approved to land and request uplift

Attachment #9250987 - Flags: sec-approval? → sec-approval+

Comment on attachment 9251222 [details]
Bug 1742433 - Test, r=gijs!

We can land the test after Jan 15

Attachment #9251222 - Flags: sec-approval? → sec-approval+
Flags: in-testsuite?
Blocks: 1742433
Attachment #9251222 - Attachment description: Bug 1739091 - Test, r=gijs! → Bug 1742433 - Test, r=gijs!

Comment on attachment 9251222 [details]
Bug 1742433 - Test, r=gijs!

Revision D131426 was moved to bug 1742433. Setting attachment 9251222 [details] to obsolete.

Attachment #9251222 - Attachment is obsolete: true

r=Gijs
https://hg.mozilla.org/integration/autoland/rev/4a67b761db2beda9805ab6f358c77e02740dd178
https://hg.mozilla.org/mozilla-central/rev/4a67b761db2b

Group: dom-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
status-firefox96: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 96 Branch

NI for uplift requests once I've verified this in Nightly.

Flags: needinfo?(pbz)

Comment on attachment 9250987 [details]
Bug 1739091, r=gijs!

Beta/Release Uplift Approval Request

  • User impact if declined: This is a high severity spoofing issue. Though a specific combination of entering / leaving full screen and pointer lock sites can enter full screen mode and pointer lock without Firefox showing the full screen warning UI. Since the user may not be aware that the site is in full screen this could allow sites to spoof trusted Firefox or operating system UI. It can also lead to users being trapped in full screen.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce: See comment 0
  • List of other uplifts needed: None
  • Risk to taking this patch: Medium
  • Why is the change risky/not risky? (and alternatives if risky): Since the patch makes it more specific which warning UI is closed at which state, there is a risk of lingering warning UI. However, this should be covered by existing automated tests.
    Beyond some manual testing, a variant of the original PoC is covered by a new automated test in a separate patch.
  • String changes made/needed:

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration:
  • User impact if declined: This is a high severity spoofing issue. Though a specific combination of entering / leaving full screen and pointer lock sites can enter full screen mode and pointer lock without Firefox showing the full screen warning UI. Since the user may not be aware that the site is in full screen this could allow sites to spoof trusted Firefox or operating system UI. It can also lead to users being trapped in full screen.
  • Fix Landed on Version: 96
  • Risk to taking this patch: Medium
  • Why is the change risky/not risky? (and alternatives if risky): Since the patch makes it more specific which warning UI is closed at which state, there is a risk of lingering warning UI. However, this should be covered by existing automated tests.
    Beyond some manual testing, a variant of the original PoC is covered by a new automated test in a separate patch.
  • String or UUID changes made by this patch:
Flags: needinfo?(pbz)
Attachment #9250987 - Flags: approval-mozilla-esr91?
Attachment #9250987 - Flags: approval-mozilla-beta?
Flags: qe-verify+
Flags: sec-bounty? → sec-bounty+
QA Whiteboard: [qa-triaged]

Comment on attachment 9250987 [details]
Bug 1739091, r=gijs!

Approved for 95 beta 12 (last beta), thanks.

Attachment #9250987 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

I've verified the fix using Firefox Nightly 96.0a1 (20211123215113) on MacOS 11.6, Ubuntu 18 and Windows 10.

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.

Flags: needinfo?(pbz)
Whiteboard: Windows only? [reporter-external] [client-bounty-form] [verif?] → Windows only? [reporter-external] [client-bounty-form] [verif?][sec-survey]
Flags: needinfo?(pbz)
Attachment #9250987 - Flags: approval-mozilla-esr91? → approval-mozilla-esr91+

I've also verified the fix on Beta 95.0b12 (20211124102546)

status-firefox95: fixed → verified

Verified fixed on 91.4.0 ESR (20211125105843)

Status: RESOLVED → VERIFIED
QA Whiteboard: [qa-triaged]
status-firefox-esr91: fixed → verified
Flags: qe-verify+
Whiteboard: Windows only? [reporter-external] [client-bounty-form] [verif?][sec-survey] → Windows only? [reporter-external] [client-bounty-form][sec-survey][adv-main95+][adv-ESR91.4.0+]
Attached file advisory.txt —
Alias: CVE-2021-43538
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: