1261933 - (CVE-2016-2831) mozRequestFullScreen + mozRequestPointerLock: bypassing pointer lock permission

Status: VERIFIED FIXED

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

Description

[me]

Attachment: Show case.html

JavaScript: Calling mozRequestFullScreen followed by mozRequestPointerLock or vice versa causes some interesting bugs, especially when paired with window.close().
In the attached file there are three buttons that showcase some of these bugs:

Crash firefox!:
- Crashes Firefox. Crash report: https://crash-stats.mozilla.com/report/index/ce29f731-9d98-40ee-9391-d6a832160404 

Cursor Lock without permission and apply to all firefox windows!:
- Causes the permission popup for mozRequestPointerLock to not show up, permission is immediately granted instead. 
If the window then is closed, mouse cursor will be locked in the middle of ANY active firefox window until the firefox process is terminated (can't be cancelled with Escape key)

Extra Cursor lock demonstration (getUserMedia Video): 
-Makes use of the aforementioned cursor lock to grant Video camera permission by opening another window that asks for permission with the confirm button positioned where the mouse cursor would be centered and locked.

Comment 1

[:Gijs (he/him)]

(In reply to me from comment #0)
> Created attachment 8737914 [details]
> Show case.html
> 
> JavaScript: Calling mozRequestFullScreen followed by mozRequestPointerLock
> or vice versa causes some interesting bugs, especially when paired with
> window.close().
> In the attached file there are three buttons that showcase some of these
> bugs:
> 
> Crash firefox!:
> - Crashes Firefox. Crash report:
> https://crash-stats.mozilla.com/report/index/ce29f731-9d98-40ee-9391-
> d6a832160404 

This at least is fixed on 46. It was fixed by bug 1236607. AFAICT it's a nullpointer crash, so I don't think we'll spin a 45.0.2 dot-release for that - 46 is going to be released within the next few weeks and will have the fix. Olli, the testcase on this bug seems 100% deterministic on my machine, don't know if you want to use it to create a crash test?

I'll look at the other issues in a second.

Comment 2

[:Gijs (he/him)]

I can reproduce the pointer lock being permanently applied to all tabs on Nightly. Moving to Core::DOM as it seems like an issue there. Hopefully Olli can help? :-)

Comment 3

[Matt Wobensmith [:mwobensmith][:matt:]]

Good clickjacking attack, can successfully get camera/microphone.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

xidorn has been looking at fullscreen stuff lately.

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch

This should fix the issue here.

But I'm not actually very sure why we need to GetWindow()->GetDocShell()->GetPresContext() to get the pres context in the first place. Wouldn't it be easier to just GetShell()->GetPresContext()? Is there any difference I'm missing?

I guess code there could be simplified as well.

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

FWIW, bypassing pointer lock permission prompt for fullscreen is intended behavior. The security issue here is actually, the pointer lock continues to be effective even when the window has been closed. And even worse, we can never unlock the pointer lock after the document closes.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8738350 [details] [diff] [review]
patch

># HG changeset patch
># User Xidorn Quan <[e-mail]>
># Date 1459906996 -36000
>#      Wed Apr 06 11:43:16 2016 +1000
># Node ID 3624a7261e9c4617e1a49c07734ea2ffd37c0a2c
># Parent  0ae85e2b5a1470e61724a050d44492e62f4e396a
>Bug 1261933 - Continue unlocking pointer even if the widget has gone. r?smaug
>
>MozReview-Commit-ID: 1siQhemFf9O
>
>diff --git a/dom/base/nsDocument.cpp b/dom/base/nsDocument.cpp
>--- a/dom/base/nsDocument.cpp
>+++ b/dom/base/nsDocument.cpp
>@@ -12589,40 +12589,29 @@ nsDocument::SetPointerLock(Element* aEle
>   RefPtr<nsPresContext> presContext;
>   docShell->GetPresContext(getter_AddRefs(presContext));
>   if (!presContext) {
>     NS_WARNING("SetPointerLock(): Unable to get presContext in \
>                 domWindow->GetDocShell()->GetPresContext()");
>     return false;
>   }
> 
>-  nsCOMPtr<nsIPresShell> shell = presContext->PresShell();
>-  if (!shell) {
>-    NS_WARNING("SetPointerLock(): Unable to find presContext->PresShell()");
>-    return false;
>-  }
>-
>-  nsIFrame* rootFrame = shell->GetRootFrame();
>-  if (!rootFrame) {
>-    NS_WARNING("SetPointerLock(): Unable to get root frame");
>-    return false;
>-  }
>-
>-  nsCOMPtr<nsIWidget> widget = rootFrame->GetNearestWidget();
>-  if (!widget) {
>-    NS_WARNING("SetPointerLock(): Unable to find widget in \
>-                shell->GetRootFrame()->GetNearestWidget();");
>-    return false;
>-  }
>-
>   if (aElement && (aElement->OwnerDoc() != this)) {
>     NS_WARNING("SetPointerLock(): Element not in this document.");
>     return false;
>   }
> 
>+  nsCOMPtr<nsIWidget> widget;
>+  nsIFrame* rootFrame = presContext->PresShell()->GetRootFrame();
Please null check PresShell(). For some reason layout code isn't consistent with Get* prefix usage the same was as DOM code where
Get* methods may return null and without Get* null isn't possible, ever. nsPresContext::PresShell() may return null.

>+  if (!NS_WARN_IF(!rootFrame)) {
>+    widget = rootFrame->GetNearestWidget();
>+    NS_WARN_IF_FALSE(widget, "SetPointerLock(): Unable to find widget "
>+                     "in shell->GetRootFrame()->GetNearestWidget();");
>+  }
>+
>   // Hide the cursor and set pointer lock for future mouse events
>   RefPtr<EventStateManager> esm = presContext->EventStateManager();
>   esm->SetCursor(aCursorStyle, nullptr, false,
>                  0.0f, 0.0f, widget, true);
>   esm->SetPointerLock(widget, aElement);

Ok, so the idea here is that we end up executing this code path even with null widget and null aElement

>   if (sIsPointerLocked) {
>     // Store the last known ref point so we can reposition the pointer after unlock.
>     mPreLockPoint = sLastRefPoint;
> 
>     // Fire a synthetic mouse move to ensure event state is updated. We first
>     // set the mouse to the center of the window, so that the mouse event
>     // doesn't report any movement.
>-    sLastRefPoint = GetWindowClientRectCenter(aWidget);
>-    aWidget->SynthesizeNativeMouseMove(sLastRefPoint + aWidget->WidgetToScreenOffset(),
>-                                       nullptr);
>+    if (aWidget) {
>+      sLastRefPoint = GetWindowClientRectCenter(aWidget);
>+      aWidget->SynthesizeNativeMouseMove(
>+        sLastRefPoint + aWidget->WidgetToScreenOffset(), nullptr);
>+    }
> 
>     // Retarget all events to this element via capture.
>     nsIPresShell::SetCapturingContent(aElement, CAPTURE_POINTERLOCK);
> 
>     // Suppress DnD
>     if (dragService) {
>       dragService->Suppress();
>     }
Why would we let any of this to be executed if aWidget is null? I would prefer if null aWidget would mean releasing possible pointerlock

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #5) 
> But I'm not actually very sure why we need to
> GetWindow()->GetDocShell()->GetPresContext() to get the pres context in the
> first place. Wouldn't it be easier to just GetShell()->GetPresContext()? Is
> there any difference I'm missing?
The difference if that GetWindow()/GetDocShell() represent browsing context so a new page might have been loaded there.
PresShell and PresContext are per page. So I think *not* using GetWindow()/GetDocShell() here would be more correct, or at least error prone.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

should have been ...at least less...

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Olli Pettay [:smaug] from comment #7)
> Comment on attachment 8738350 [details] [diff] [review]
> patch
> >   if (aElement && (aElement->OwnerDoc() != this)) {
> >     NS_WARNING("SetPointerLock(): Element not in this document.");
> >     return false;
> >   }
> > 
> >+  nsCOMPtr<nsIWidget> widget;
> >+  nsIFrame* rootFrame = presContext->PresShell()->GetRootFrame();
> Please null check PresShell(). For some reason layout code isn't consistent
> with Get* prefix usage the same was as DOM code where
> Get* methods may return null and without Get* null isn't possible, ever.
> nsPresContext::PresShell() may return null.

Well, it seems to me nsPresContext::PresShell() will assert if it is going to return null, so it is not supposed to return that... Anyway, in the new patch we get pres shell from the document with a null check.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

nsPresContext::PresShell() is odd beast. it is supposed to be called only when it doesn't return null, yet it may return null.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8738820 [details] [diff] [review]
patch

Curious, do we have code to automatically call
ESM::SetPointerLock(nullptr, nullptr) when 
pointerlock element's ownerDoc's presshell is being deleted?

Comment 14

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Olli Pettay [:smaug] from comment #13)
> Comment on attachment 8738820 [details] [diff] [review]
> patch
> 
> Curious, do we have code to automatically call
> ESM::SetPointerLock(nullptr, nullptr) when 
> pointerlock element's ownerDoc's presshell is being deleted?

I don't think so. The only callsite of ESM::SetPointerLock is nsDocument::SetPointerLock, which is only called by UnlockPointer with null element. UnlockPointer is called in nsFocusManager::SetFocusedWindowInternal, Element::UnbindFromTree, and nsDocument::OnPageHide.

But I guess we can put ESM::SetPointerLock static at least partially, so that it doesn't require an ESM instance for unlocking pointer, and consequently we can unlock it any time even when the document has been destroyed.

Comment 15

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8738820 [details] [diff] [review]
patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Not very hard I suppose. Given the changeset (and the commit message) indicates this bug could be fixed if we do unlock pointer even if the widget has gone, it is not hard to relate the condition to closing the window, as far as one knows that widget is a window.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
It seems to me if this is not a security bug, people are likely just ignore it. But given it is a security bug, people may find something from the patch and the commit message.

Which older supported branches are affected by this flaw?
All branches, given we support pointer lock since very long ago.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
I suppose this would not be hard to backport this patch, as code related to pointer lock seems to have been stable for quite a while.

How likely is this patch to cause regressions; how much testing does it need?
I think it is unlikely to cause regression. In-tree mochitest of pointer lock should be enough, but they have been disabled on Windows and Linux in bug 931445... Ahhhh...

Comment 16

[Xidorn Quan [:xidorn] UTC+10]

I can probably create a larger patch which refactor some part of the code there and request review from a separate non-security bug, so that it would not be that noticeable.

Comment 17

[Al Billings [:abillings - ex-MoCo]]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #16)
> I can probably create a larger patch which refactor some part of the code
> there and request review from a separate non-security bug, so that it would
> not be that noticeable.

Well, if you did that, I'd give this sec-approval now and it could go in through the other bug. Otherwise, this is going to wait until we're a couple of weeks into the new cycle as we're getting fairly close to shipping now. We'd normally backport a sec-high to Aurora, Beta, etc.

That said, if this is sec-high and affects ESR branches, we're going to want it there, which usually screams "security bug" for backporting.

Dan, what do you think?

Comment 18

[Xidorn Quan [:xidorn] UTC+10]

Hmmm, but uplifting an "imaginary improvement" for a feature which has been stable for a while does look weird, so I guess doing this through a non-security bug may not be that useful.

Comment 19

[Daniel Veditz [:dveditz]]

Is there a reason you don't want this for Firefox 46? Worries about the disabled tests? If you want to skip 46 I'm not overly worried about the patch giving things away if we land now or in a few weeks. We can't land on the ESR branch until we're in the right cycle anyway.

Comment 20

[Al Billings [:abillings - ex-MoCo]]

I don't want this for 46 because it is too late for it to go in there.

I'll give sec-approval+ for this then.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1b485082e803

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1b485082e803

Comment 23

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8738820 [details] [diff] [review]
patch

Approval Request Comment
[Feature/regressing bug #]: pointer lock
[User impact if declined]: pointer may be locked by malicious website until the browser is killed somehow
[Describe test coverage new/current, TreeHerder]: existing tests should be enough for the original functionality, but they are currently only enabled on Mac. The behavior fixed here is probably not testable itself.
[Risks and why]: This isn't very risky since it doesn't change the code path a lot for normal cases.
[String/UUID change made/needed]: n/a

Comment 24

[Liz Henry (:lizzard) (relman/hg->git project)]

Wontfix for 46, we are very late in beta and I worry about the tests being disabled/no clear way to test.

Comment 25

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8738820 [details] [diff] [review]
patch

Sec-high issue, Aurora47+

Comment 26

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Sylvestre, fyi, this (was wontfix'd for Fx46) affects ESR45. I don't see an esr45 nom but not sure if you will take it or not.

Comment 27

[Carsten Book [:Tomcat]]

has problems to apply to aurora - 
grafting 339247:1b485082e803 "Bug 1261933 - Continue unlocking pointer even if the widget has gone. r=smaug"
merging dom/base/nsDocument.cpp
merging dom/events/EventStateManager.cpp
warning: conflicts while merging dom/events/EventStateManager.cpp! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use hg resolve and hg graft --continue)

Comment 28

[Xidorn Quan [:xidorn] UTC+10]

https://hg.mozilla.org/releases/mozilla-aurora/rev/419d8bfa6a79

Comment 29

[Carsten Book [:Tomcat]]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #28)
> https://hg.mozilla.org/releases/mozilla-aurora/rev/419d8bfa6a79

thanks! setting flag

Comment 30

[me]

I understand it may take a while but I'm just wondering how long it usually takes to consider a bounty reward? :-)

Comment 31

[Sylvestre Ledru [:Sylvestre]]

I agree with the 47+ tracking.

Comment 32

[Al Billings [:abillings - ex-MoCo]]

Bounty questions should be sent to [e-mail], not in the bugs.

That said, we evaluate bounties once a week but we often wait for the bug to be fixed before making a decision. We awarded it today.

Comment 33

[Paul Silaghi, QA [:pauly]]

Reproduced the issue using the testcase on FX 46.0.1, Win 7.
Verified fixed FX 47b9, 48.0a2 (2016-05-30).

Comment 34

[Al Billings [:abillings - ex-MoCo]]

Is there a reason we didn't take this on ESR45.

Comment 35

[Liz Henry (:lizzard) (relman/hg->git project)]

Hi Xidorn, can you request approval for esr45 uplift? Thanks.

Comment 36

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8738820 [details] [diff] [review]
patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: it is a sec-high
User impact if declined: pointer may be locked by malicious website until the browser is killed somehow
Fix Landed on Version: 48, and uplifted to 47
Risk to taking this patch (and alternatives if risky): This isn't very risky since it doesn't change the code path a lot for normal cases.
String or UUID changes made by this patch:  n/a

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

The test was only disabled on Firefox 48, so it should be fine for 45.

Comment 37

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8738820 [details] [diff] [review]
patch

Effect should be limited so this is relatively safe, fixes sec-high bug, taking it for esr 45.2.0. We are looking to go to build tomorrow morning, so if this doesn't merge it may not make the cutoff.

Comment 38

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Uplifting this hit a minor conflict in nsDocument.cpp because Bug 1241764 wasn't uplifted, but it looks like the new changes from this patch replaced that part of the file anyway. Would probably be worth doublechecking that my assumption is correct.

https://hg.mozilla.org/releases/mozilla-esr45/rev/a3fff31b8b70

Comment 39

[Xidorn Quan [:xidorn] UTC+10]

Yeah, that looks right. Thanks.