Closed Bug 1656741 (CVE-2020-26953) Opened 1 year ago Closed 1 year ago

Entering fullscreen without UI message showing

Categories

(Firefox :: Security, defect, P3)

defect

Tracking

()

VERIFIED FIXED
83 Branch
Tracking Status
firefox-esr78 83+ verified
firefox82 --- wontfix
firefox83 --- verified
firefox84 --- verified

People

(Reporter: qab, Assigned: smacleod)

Details

(Keywords: csectype-spoof, sec-moderate, Whiteboard: [reporter-external] [client-bounty-form] [verif?][post-critsmash-triage][adv-main83+][adv-esr78.5+])

Attachments

(8 files, 1 obsolete file)

Attached file letsrepro.html

OS/Firefox version:
Works on Firefox 79.0 (64-bit) using Windows 10 Pro
Works on Nightly 81.0a1 (2020-08-01) (64-bit) using Windows 10 Pro
Works on Firefox 79.0 (64-bit) using macOS Catalina Version 10.15.5

Does not work on Firefox 79.0 (64-bit) using Ubuntu 18.04.4 desktop

Repro:

  1. Host attached PoC HTML in an HTTP server (localhost will do, if other then make sure its HTTPS)
  2. Open the hosted 'letsrepro.html' and click the anchor tag

Result:
Firefox enters full screen mode without the security UI saying you are entering fullscreen appearing.

Expected:
When entering full screen mode, the UI indicating so should always appear to prevent security concerns.
See Bug 1432856

Other info:
For some reason a WebSocket connection attempt has to happen first for this to work
If I remove 'new WebSocket("ws://nonexistent");' from the PoC, it does not work.

Flags: sec-bounty?

Al, can you take a look at what's going on here? There's some timing involved, I think (I can repro on Windows but not on mac, where we just immediately leave fullscreen again), but not showing the warning is bad.

Type: task → defect
Flags: needinfo?(alchen)

Sure, I will update what I find.

Flags: needinfo?(alchen)

(In reply to :Gijs (he/him) from comment #1)

Al, can you take a look at what's going on here? There's some timing involved, I think (I can repro on Windows but not on mac, where we just immediately leave fullscreen again), but not showing the warning is bad.

I also see the symptom of leaving fullscreen immediately on mac and linux.
I can enter the fullscreen and see the warning by clicking the page again.

If I modified the timeout from 260 to 1260, it works fine.
Besides, I don't see any difference between the file with or without 'new WebSocket("ws://nonexistent");'.

-			setTimeout(x=>{location='?go#go';}, 260);
+			setTimeout(x=>{location='?go#go';},1260);

Hi reporter,
could you try to extend the second timeout and update the result?

Flags: needinfo?(qab)

Hello triager,

For me the websocket is needed AFAICT. Every time I remove it I stop reproducing. Applying your changes did not result in repro for MacOS.

On MacOS the original repro stopped working but then I modified two things:

			
			
			setTimeout(()=>{document.body.requestFullscreen({})},106);
			setTimeout(x=>{location='?gfo#go';},200);//<---- lowered the timeout and changed the URL a bit to prevent cache

With the above modifications it started working on MacOS and Windows 10 just fine.

My guess is that once you run it, subsequent runs will ruin the timing due to cache. Will work on a more reliable PoC and update if I get one with 100% on all OS. The WebSocket request is probably just adding to the timing aspect, not sure why it is required for me.

Flags: needinfo?(qab)

I misused the markdown and part of the code above did not show.



		new WebSocket("ws://facebook.com");//<----- changed the websocket to an existing domain
			
			
			setTimeout(()=>{document.body.requestFullscreen({})},106);
			setTimeout(x=>{location='?gfo#go';},200);//<---- lowered the timeout and changed the URL a bit to prevent cache


Al, can you try again based on qab's comments?

Flags: needinfo?(alchen)

(In reply to Abdulrahman Alqabandi from comment #0)

Repro:

  1. Host attached PoC HTML in an HTTP server (localhost will do, if other then make sure its HTTPS)

In the first trial, I test it on a localhost. (http://localhost:8000)
The result is in comment 3.

If I modified the file based on comment 4 and comment 5, I still saw the same symptom.
The symptom is leaving fullscreen immediately on mac and linux.

  • changed the websocket to an existing domain
  • lowered the timeout (setTimeout(x=>{location='?gfo#go';},200);)

This time I also tried to use "https://localhost:4443" to do the test on mac and linux.
In this setup, after clicking the anchor tag, FF won't enter fullscreen with "new WebSocket("ws://facebook.com")" or "new WebSocket("ws://nonexistent");".
If I removed "new WebSocket(...)", the behavior is leaving fullscreen immediately.
I also tried it on Chome. Chome has the same behavior as FF.

:qab, could you also update the behavior of Chome in your setup?
If possible, could you provide the video which can show the problem on your side?

For me, the problem is still unclear.

  1. Is the problem related to "new WebSocket()"?
  2. Is the problem related to "change the URL right after entering fullscreen"?

:Gijs, is it possible to ask QA to reproduce the symptom?
We need a solid STR to investigate the problem.

Flags: needinfo?(alchen)

(In reply to Alphan Chen [:alchen] from comment #7)

:Gijs, is it possible to ask QA to reproduce the symptom?

I don't think QA have the skills to adapt the testcase to make it 100% reproducible. If they reproduce and you cannot, what's the point?

We need a solid STR to investigate the problem.

The issue reproduced for me on Windows. (see comment #1)

Do you not have access to a Windows machine at all?

Flags: needinfo?(alchen)

(In reply to :Gijs (he/him) from comment #8)

The issue reproduced for me on Windows. (see comment #1)

Do you not have access to a Windows machine at all?

I will try to find a windows.
If you can reproduce the problem on windows, could you help to classify these two questions?

Is the problem related to "new WebSocket()"?
Is the problem related to "change the URL right after entering fullscreen"?

Flags: needinfo?(alchen) → needinfo?(gijskruitbosch+bugs)

(In reply to Alphan Chen [:alchen] from comment #7)

:qab, could you also update the behavior of Chome in your setup?
If possible, could you provide the video which can show the problem on your side?

For me, the problem is still unclear.

  1. Is the problem related to "new WebSocket()"?
  2. Is the problem related to "change the URL right after entering fullscreen"?
Flags: needinfo?(qab)

In case it makes a difference, I reproduced on Windows by saving the testcase to disk and opening from the file using file:///. Of course this means that subsequent requests (on a fast SSD) to the relative ?go#go link are almost instantaneous.

(In reply to Alphan Chen [:alchen] from comment #9)

Is the problem related to "new WebSocket()"?

Without the websocket connection it doesn't reproduce.

Is the problem related to "change the URL right after entering fullscreen"?

Without changing the URL, the warning comes up fine. Also, if you change the relative link to only point to #go, the warning comes up fine.

Do we check for the URL/document that requested fullscreen and that actually enters fullscreen somewhere?

I wonder if the websocket connection breaks bfcache, and that causes us to create a different windowglobal, and that probably affects the fullscreen actor. Nika, do you know whether websockets break bfcache and whether this is plausible?

Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(nika)

Off the top of my head, I think that an active WebSocket connection would disable bfcache, yeah. You can check if this is caused by that change in behaviour by removing the websocket connection, and instead adding an "unload" event listener on the document, which should also suppress bfcache.

Flags: needinfo?(nika)

Using window.addEventListener("unload", console.log) instead of the web socket seems to work, too (ie causes the warning not to come up while the window still goes into fullscreen).

Attached file 1.html

(In reply to Alphan Chen [:alchen] from comment #10)

:qab, could you also update the behavior of Chome in your setup?
Does not repro on Chrome AFAICT.

If possible, could you provide the video which can show the problem on your side?
Certainly, attached in the next comment.

For me, the problem is still unclear.

  1. Is the problem related to "new WebSocket()"?
    From the subsequent comments, looks like its an issue with bfcache more than WebSocket itself.
  1. Is the problem related to "change the URL right after entering fullscreen"?
    Yes.

In the console it says "TypeError: Fullscreen request aborted" when the PoC reproduces successfully. Firefox is definitely getting confused due to the navigation.

I was able to come up with a new PoC which seems to be reliable on all platforms (Windows, Linux and MacOS), at least for me.

Flags: needinfo?(qab)
Attached video poc.mp4

Oops the previous comment mixed my answers with the quotes (keep in mind)

This is the video PoC showcasing it working in three different set ups.

I can reproduce on macOS where I couldn't before using the new PoC. Alphan, can you take a look again? Thank you.

Flags: needinfo?(alchen)

By the new Poc, I can reproduce the problem.
Actually, the symptom is FF don't leave the fullscreen normally.
At that time, "aActor.requestOrigin" is undefined.
https://searchfox.org/mozilla-central/rev/ab81b8552f4aa9696a2524f97fdfeb59d4dc31c1/browser/base/content/browser-fullScreenAndPointerLock.js#573
https://searchfox.org/mozilla-central/rev/ab81b8552f4aa9696a2524f97fdfeb59d4dc31c1/browser/actors/DOMFullscreenParent.jsm#163

From my experiments, the timing is a big factor.
I can reproduce the problem easily when the timeout is less than 15.

setTimeout(x=>{location=window.URL.createObjectURL(new Blob([blobContent], { type: "text/html" }));},1);

Flags: needinfo?(alchen)

The fix works on my Ubuntu.
I will verify the result on other platforms when the build is ready.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f5775c0faf0a7e2c0384250e0881dd8d1dec8a8a

Flags: needinfo?(gijskruitbosch+bugs)

It looks like Alphen was affected by the layoffs. :-(

I looked at this, but the patch that Alphen pushed to try didn't fix the issue for me.

From local logging, it appears the parent process actor gets a DOMFullscreen:Request message, and then we unload the document, and the next thing the parent actor hears is that the actor is being destroyed. Then the parent gets a MozDOMFullscreen:Entered event.

The warning is normally generated by the MozDOMFullscreen:NewOrigin event (forwarded to the parent by the child), which is never seen by either of the bits of frontend code involved (again, based on logging). It looks like it is dispatched asynchronously at https://searchfox.org/mozilla-central/rev/19c23d725f27d0989e4a60f36d64004cebb39736/dom/base/Document.cpp#13444-13449 . I suspect that it never fires or is never seen by frontend because presumably the original document is destroyed while the event is queued, and/or we decide we don't need to re-emit it when the document to which we navigate is same-origin.

Olli, does this seem like an accurate diagnosis (if you're not the right person, please forward the needinfo to whoever still knows about our fullscreen code...)? I'm pretty confused, because the code in the PoC invokes document.body.requestFullscreen and so I'd have expected that the document being unloaded would mean we would interrupt our entering into fullscreen and abort it using the relevant APIs from the child to the parent, given that the element in question is no longer going to be around... :-\

Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(bugs)

What I don't understand is that why frontend doesn't show the normal warning (nothing to do with MozDOMFullscreen:NewOrigin).

But anyhow, can fix this.

Assignee: nobody → bugs
Flags: needinfo?(bugs)

(In reply to Olli Pettay [:smaug] from comment #20)

What I don't understand is that why frontend doesn't show the normal warning (nothing to do with MozDOMFullscreen:NewOrigin).

Can you elaborate on what you mean? The only warning I'm aware of is keyed off this event, it seems.

We could change what we use to show the warning, if that would help?

The wip patch isn't necessary quite right for nested iframes.

Comment on attachment 9175268 [details]
Bug 1656741, inform parent process about canceled fullscreen request

Or perhaps this should be fixed in a bit different way, since I think, this was working while framescripts were used.

Attachment #9175268 - Attachment is obsolete: true

Thanks, Olli! Let me know if I can help in any way.

Severity: -- → S3
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Priority: -- → P3

This should be fixed in a difference way. JS actor stuff gets confused here.

Attached file Bug 1656741

Steven has been looking into other fullscreen related issues recently and said he could help here. (thanks!)

https://bugzilla.mozilla.org/attachment.cgi?id=9177090 helps with the issue here (testing with https://bugzilla.mozilla.org/attachment.cgi?id=9169053)
but I think there is another problematic case if parent gets "DOMFullscreen:Request" and then didDestroy is called, since at that point
this._fullscreenWindow is null, I think, and also window.document.documentElement.hasAttribute("inDOMFullscreen") returns false.

Assignee: bugs → smacleod
Attached file Bug 1656741, r=Gijs!

Attachment https://bugzilla.mozilla.org/attachment.cgi?id=9178482 should fix things as far as I can tell. Alphan's original patch that worked on linux went part of the way, but based on timing (which I found changed drastically between mac and linux) it wouldn't catch all cases. This patch does a lot more checking for actor's being destroyed before attempting to send a msg to transition the child to Fullscreen. I also needed to keep track in the parent when the msg had already been sent so that if the child died before it could reply we would still cleanup.

I have yet to test this on Windows, but I'm decently confident it will fix things based on my tracing/logging/testing of the pieces involved here.

Attachment #9178482 - Attachment description: Bug 1656741 - abort entering fullscreen if it will not complete. r=Gijs → Bug 1656741 - abort entering fullscreen if it will not complete. r=Gijs!
Attachment #9178482 - Attachment description: Bug 1656741 - abort entering fullscreen if it will not complete. r=Gijs! → Bug 1656741, r=Gijs!
Group: firefox-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → 83 Branch
Flags: sec-bounty? → sec-bounty+
Flags: qe-verify+
Whiteboard: [reporter-external] [client-bounty-form] [verif?] → [reporter-external] [client-bounty-form] [verif?][post-critsmash-triage]

Attaching the results of testing on Firefox Nightly 81.0a1 (2020-08-01) (on the left) and on the latest Firefox Nighty (On the right).
I'm not sure if exiting the full screen mode is the expected behavior here or should I look for something else here?
Thanks.

Flags: needinfo?(smacleod)

(In reply to Hani Yacoub from comment #33)

Attaching the results of testing on Firefox Nightly 81.0a1 (2020-08-01) (on the left) and on the latest Firefox Nighty (On the right).
I'm not sure if exiting the full screen mode is the expected behavior here or should I look for something else here?
Thanks.

Yup, exiting full screen mode is the expected behaviour with the fix. Looks like it is working as intended.

Flags: needinfo?(smacleod)

Verified as fixed on Firefox Nightly 84.0a1 (2020-10-20) on Windows 10 x64, MacOS 10.14 and on Ubuntu 20.04 x64.

Status: RESOLVED → VERIFIED

Is this something we should consider uplifting to ESR78 or is riding mainline to release sufficient?

Flags: needinfo?(smacleod)

[Tracking Requested - why for this release]: sec-moderate bug with a relatively low risk patch.

(In reply to Ryan VanderMeulen [:RyanVM] from comment #36)

Is this something we should consider uplifting to ESR78 or is riding mainline to release sufficient?

Ya, probably worth considering. Patch coming soon, just need to make sure everything is still working properly after that graft.

Flags: needinfo?(smacleod)
Attached file Bug 1656741, r=Gijs

Comment on attachment 9185496 [details]
Bug 1656741, r=Gijs

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is a low risk patch fixing a sec-moderate bug.
  • User impact if declined: A malicious website may full-screen itself without showing a warning that the page has entered full-screen.
  • Fix Landed on Version: 83
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): This fix was landed to central ~1 month ago and does not appear to have caused problems there or in the 83 beta, making it relatively low risk IMO.
  • String or UUID changes made by this patch:
Attachment #9185496 - Flags: approval-mozilla-esr78?
QA Whiteboard: [qa-triaged]

Comment on attachment 9185496 [details]
Bug 1656741, r=Gijs

Approved for ESR, thanks.

Attachment #9185496 - Flags: approval-mozilla-esr78? → approval-mozilla-esr78+

Verified as fixed on Firefox 83.0b8 on Windows 10 x64, MacOS 10.14 and on Ubuntu 20.04 x64.

Whiteboard: [reporter-external] [client-bounty-form] [verif?][post-critsmash-triage] → [reporter-external] [client-bounty-form] [verif?][post-critsmash-triage][adv-main83+]

Verified as fixed on Firefox 78.4.1esr on Windows 10 x64, MacOS 10.14 and on Ubuntu 20.04 x64.

Whiteboard: [reporter-external] [client-bounty-form] [verif?][post-critsmash-triage][adv-main83+] → [reporter-external] [client-bounty-form] [verif?][post-critsmash-triage][adv-main83+][adv-esr78.5+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.