Closed Bug 1261933 (CVE-2016-2831) Opened 8 years ago Closed 8 years ago

mozRequestFullScreen + mozRequestPointerLock: bypassing pointer lock permission

Categories

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

45 Branch
x86_64
Windows 8.1
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla48
Tracking Status
firefox45 --- wontfix
firefox46 + wontfix
firefox47 + verified
firefox48 + verified
firefox-esr38 --- unaffected
firefox-esr45 47+ fixed

People

(Reporter: me, Assigned: xidorn)

References

Details

(Keywords: regression, sec-high, Whiteboard: [adv-main47+][adv-esr45.2+])

Attachments

(2 files, 1 obsolete file)

Attached file 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.
(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.
Component: General → Untriaged
Depends on: 1236607
Flags: needinfo?(bugs)
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? :-)
Group: firefox-core-security → core-security
Status: UNCONFIRMED → NEW
Component: Untriaged → DOM
Ever confirmed: true
Product: Firefox → Core
Summary: mozRequestFullScreen + mozRequestPointerLock: bypassing permission grant / crash → mozRequestFullScreen + mozRequestPointerLock: bypassing pointer lock permission
Good clickjacking attack, can successfully get camera/microphone.
Keywords: sec-high
xidorn has been looking at fullscreen stuff lately.
Flags: needinfo?(bugs) → needinfo?(quanxunzhen)
Attached patch patch (obsolete) — — Splinter Review
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.
Flags: needinfo?(quanxunzhen)
Attachment #8738350 - Flags: review?(bugs)
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.
Assignee: nobody → quanxunzhen
Comment on attachment 8738350 [details] [diff] [review]
patch

># HG changeset patch
># User Xidorn Quan <quanxunzhen@gmail.com>
># 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
Attachment #8738350 - Flags: review?(bugs) → review-
(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.
should have been ...at least less...
Attached patch patch — — Splinter Review
Attachment #8738350 - Attachment is obsolete: true
Attachment #8738820 - Flags: review?(bugs)
(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.
nsPresContext::PresShell() is odd beast. it is supposed to be called only when it doesn't return null, yet it may return null.
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?
Attachment #8738820 - Flags: review?(bugs) → review+
(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 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...
Attachment #8738820 - Flags: sec-approval?
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.
(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?
Flags: needinfo?(dveditz)
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.
Flags: sec-bounty?
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.
Group: core-security → dom-core-security
Flags: needinfo?(dveditz) → needinfo?(abillings)
Keywords: regression
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.
Flags: needinfo?(abillings)
Attachment #8738820 - Flags: sec-approval? → sec-approval+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1b485082e803
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
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
Attachment #8738820 - Flags: approval-mozilla-beta?
Attachment #8738820 - Flags: approval-mozilla-aurora?
Wontfix for 46, we are very late in beta and I worry about the tests being disabled/no clear way to test.
Attachment #8738820 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Comment on attachment 8738820 [details] [diff] [review]
patch

Sec-high issue, Aurora47+
Attachment #8738820 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Flags: needinfo?(sledru)
Group: dom-core-security → core-security-release
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)
Flags: needinfo?(bugzilla)
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #28)
> https://hg.mozilla.org/releases/mozilla-aurora/rev/419d8bfa6a79

thanks! setting flag
I agree with the 47+ tracking.
Flags: needinfo?(sledru)
Flags: sec-bounty? → sec-bounty+
Reproduced the issue using the testcase on FX 46.0.1, Win 7.
Verified fixed FX 47b9, 48.0a2 (2016-05-30).
Status: RESOLVED → VERIFIED
Is there a reason we didn't take this on ESR45.
Flags: needinfo?(rkothari)
Flags: needinfo?(lhenry)
Whiteboard: [adv-main47+]
Alias: CVE-2016-2831
Hi Xidorn, can you request approval for esr45 uplift? Thanks.
Flags: needinfo?(lhenry) → needinfo?(bugzilla)
Flags: needinfo?(rkothari)
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.
Flags: needinfo?(bugzilla)
Attachment #8738820 - Flags: approval-mozilla-esr45?
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.
Attachment #8738820 - Flags: approval-mozilla-esr45? → approval-mozilla-esr45+
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
Flags: needinfo?(bugzilla)
Yeah, that looks right. Thanks.
Flags: needinfo?(bugzilla)
Whiteboard: [adv-main47+] → [adv-main47+][adv-esr45.2+]
Group: core-security-release
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.