Closed Bug 1315718 Opened 3 years ago Closed 3 years ago

mGamepadManager looks like a security hazard

Categories

(Core :: Graphics, defect)

50 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox52 --- fixed
firefox-esr52 --- fixed
firefox53 --- fixed

People

(Reporter: smaug, Assigned: daoshengmu)

References

Details

(Keywords: sec-other, Whiteboard: [post-critsmash-triage][adv-main52-])

Attachments

(1 file, 7 obsolete files)

http://searchfox.org/mozilla-central/rev/f5c9e9a249637c9abd88754c8963ecb3838475cb/gfx/vr/ipc/VRManagerChild.h#156
is keeping a raw pointer to refcounted object, and nothing seems to clear mGamepadManager.
oh, it is a service. Then probably fine, or at least the crash would be limited to shutdown.
But it should be documented in raw pointer member variables why they are safe.
(In reply to Olli Pettay [:smaug] from comment #1)
> oh, it is a service. Then probably fine, or at least the crash would be
> limited to shutdown.
> But it should be documented in raw pointer member variables why they are
> safe.

I think probably I should replace it with a RefPtr to protect it would be safe for crash.
Group: core-security → gfx-core-security
Keywords: sec-other
Follow the comment 1, replace mGamepadManager with a RefPtr for avoiding the crash at shutdown.
Attachment #8810325 - Flags: review?(bugs)
Comment on attachment 8810325 [details] [diff] [review]
Bug 1315718: Replace mGamepadManager raw pointer.patch

Why do you need the member variable at all?
GamepadManager::GetService() should be fast. That may return null on shutdown so 
VRManagerChild::RecvGamepadUpdate would need to do a null check, but there is a null check already anyhow.
Attachment #8810325 - Flags: review?(bugs)
Makes understanding the lifetime management a tad simpler if the manager is kept alive only in one place.
(In reply to Olli Pettay [:smaug] from comment #4)
> Comment on attachment 8810325 [details] [diff] [review]
> Bug 1315718: Replace mGamepadManager raw pointer.patch
> 
> Why do you need the member variable at all?
> GamepadManager::GetService() should be fast. That may return null on
> shutdown so 
> VRManagerChild::RecvGamepadUpdate would need to do a null check, but there
> is a null check already anyhow.

This is a tricky for VRManagerChild::RecvGamepadUpdate. Because VRManagerChilds are used for multiple IPC channels. One is for the content process to the chrome process and the other is for the chrome process to the compositor process. Because GamepadManager only exists in the content process, we should just handle VRManagerChild::RecvGamepadUpdate for the the content process to the chrome process one. However, we can't determine which channel we are working on effectively. (https://dxr.mozilla.org/mozilla-central/rev/47e0584afe0ab0b867412189c610b302b6ba0ea7/gfx/vr/VRManager.cpp#386).

Appreciated for the feedback.
So simple XRE_IsContentProcess() check in VRManagerChild::RecvGamepadUpdate isn't enough?

Or even better would be on the sender side to check process type, but I don't quite understand who is sending data and where.
V2-
Refer Comment 7 to check if we are updating gamepad in Content process or the same process in non e10s mode.
Attachment #8810325 - Attachment is obsolete: true
Comment on attachment 8813517 [details] [diff] [review]
Bug 1315718: Replace mGamepadManager raw pointer.patch (v2)

># HG changeset patch
># User Daosheng Mu <daoshengmu@gmail.com>
>
>Bug 1315718 - Replace mGamepadManager raw pointer with RefPtr in VRManagerChild; r?smaug
>
>MozReview-Commit-ID: HZY3ZGUFfdv
>---
> dom/gamepad/GamepadManager.cpp |  1 -
> gfx/vr/ipc/VRManagerChild.cpp  | 16 +++++-----------
> gfx/vr/ipc/VRManagerChild.h    |  5 -----
> 3 files changed, 5 insertions(+), 17 deletions(-)
>
>diff --git a/dom/gamepad/GamepadManager.cpp b/dom/gamepad/GamepadManager.cpp
>index 752db77..dde71dd 100644
>--- a/dom/gamepad/GamepadManager.cpp
>+++ b/dom/gamepad/GamepadManager.cpp
>@@ -679,17 +679,16 @@ GamepadManager::ActorCreated(PBackgroundChild *aActor)
>   MOZ_ASSERT(initedChild == child);
>   child->SendGamepadListenerAdded();
>   mChannelChildren.AppendElement(child);
> 
> #if defined(XP_WIN) || defined(XP_MACOSX) || defined(XP_LINUX)
>   // Construct VRManagerChannel and ask adding the connected
>   // VR controllers to GamepadManager
>   mVRChannelChild = gfx::VRManagerChild::Get();
>-  mVRChannelChild->SetGamepadManager(this);
>   mVRChannelChild->SendControllerListenerAdded();
> #endif
> }
> 
> //Override nsIIPCBackgroundChildCreateCallback
> void
> GamepadManager::ActorFailed()
> {
>diff --git a/gfx/vr/ipc/VRManagerChild.cpp b/gfx/vr/ipc/VRManagerChild.cpp
>index f4732b9..35b6133 100644
>--- a/gfx/vr/ipc/VRManagerChild.cpp
>+++ b/gfx/vr/ipc/VRManagerChild.cpp
>@@ -37,17 +37,16 @@ static StaticRefPtr<VRManagerParent> sVRManagerParentSingleton;
> 
> void ReleaseVRManagerParentSingleton() {
>   sVRManagerParentSingleton = nullptr;
> }
> 
> VRManagerChild::VRManagerChild()
>   : TextureForwarder()
>   , mDisplaysInitialized(false)
>-  , mGamepadManager(nullptr)
>   , mInputFrameID(-1)
>   , mMessageLoop(MessageLoop::current())
>   , mFrameRequestCallbackCounter(0)
>   , mBackend(layers::LayersBackend::LAYERS_NONE)
> {
>   MOZ_COUNT_CTOR(VRManagerChild);
>   MOZ_ASSERT(NS_IsMainThread());
> 
>@@ -472,18 +471,21 @@ VRManagerChild::RecvNotifyVRVSync(const uint32_t& aDisplayID)
> 
> mozilla::ipc::IPCResult
> VRManagerChild::RecvGamepadUpdate(const GamepadChangeEvent& aGamepadEvent)
> {
> #ifdef MOZ_GAMEPAD
>   // VRManagerChild could be at other processes, but GamepadManager
>   // only exists at the content process or the parent process
>   // in non-e10s mode.
>-  if (mGamepadManager) {
>-      mGamepadManager->Update(aGamepadEvent);
>+  if (XRE_IsContentProcess() || IsSameProcess()) {

GamepadManager can't be run at Chrome process.
Kip, do you know if we have a better way to know our VRManagerParent's child process is Content process?

>+    RefPtr<GamepadManager> gamepadManager(GamepadManager::GetService());
>+    if (gamepadManager) {
>+      gamepadManager->Update(aGamepadEvent);
>+    }
>   }
> #endif
> 
>   return IPC_OK();
> }
Flags: needinfo?(kgilbert)
(In reply to Daosheng Mu[:daoshengmu] from comment #9)
> > #ifdef MOZ_GAMEPAD
> >   // VRManagerChild could be at other processes, but GamepadManager
> >   // only exists at the content process or the parent process
> >   // in non-e10s mode.
> >-  if (mGamepadManager) {
> >-      mGamepadManager->Update(aGamepadEvent);
> >+  if (XRE_IsContentProcess() || IsSameProcess()) {
> 
> GamepadManager can't be run at Chrome process.
> Kip, do you know if we have a better way to know our VRManagerParent's child
> process is Content process?
> 
> >+    RefPtr<GamepadManager> gamepadManager(GamepadManager::GetService());
> >+    if (gamepadManager) {
> >+      gamepadManager->Update(aGamepadEvent);
> >+    }
> >   }
> > #endif
> > 
> >   return IPC_OK();
> > }

I did a bit of searching, and it appears that XRE_IsContentProcess() is the most common way to test the current process type.  Are you finding that this returning true for the Chrome process?

Also, would this limitation prevent us from using the gamepad within chrome privileged Javascript code?  We don't need to worry about this right away, but when we build the UI for the VR browser, it will need to interact with the gamepad as an input device.
Flags: needinfo?(kgilbert)
(In reply to :kip (Kearwood Gilbert) from comment #10)
> (In reply to Daosheng Mu[:daoshengmu] from comment #9)
> > > #ifdef MOZ_GAMEPAD
> > >   // VRManagerChild could be at other processes, but GamepadManager
> > >   // only exists at the content process or the parent process
> > >   // in non-e10s mode.
> > >-  if (mGamepadManager) {
> > >-      mGamepadManager->Update(aGamepadEvent);
> > >+  if (XRE_IsContentProcess() || IsSameProcess()) {
> > 
> > GamepadManager can't be run at Chrome process.
> > Kip, do you know if we have a better way to know our VRManagerParent's child
> > process is Content process?
> > 
> > >+    RefPtr<GamepadManager> gamepadManager(GamepadManager::GetService());
> > >+    if (gamepadManager) {
> > >+      gamepadManager->Update(aGamepadEvent);
> > >+    }
> > >   }
> > > #endif
> > > 
> > >   return IPC_OK();
> > > }
> 
> I did a bit of searching, and it appears that XRE_IsContentProcess() is the
> most common way to test the current process type.  Are you finding that this
> returning true for the Chrome process?

For the Chrome process, it will return false;
> 
> Also, would this limitation prevent us from using the gamepad within chrome
> privileged Javascript code?  We don't need to worry about this right away,
> but when we build the UI for the VR browser, it will need to interact with
> the gamepad as an input device.

My plan is checking VRManagerParent's child process whether it is the Content process here, https://dxr.mozilla.org/mozilla-central/rev/47e0584afe0ab0b867412189c610b302b6ba0ea7/gfx/vr/VRManager.cpp#386. In that way, I can reduce some bandwidth for sending message to VRManagerChild. Because in the case of VRManagerParent in the GPU process, its child process is the Chrome process, and we don't have a gamepadManager service in the Chrome process. So, it would occur assertion.
Assignee: nobody → dmu
Do you think this is a good idea for keeping a contentProcessId in VRManagerParent for checking the child process is the Content process, and then sending gamepadUpdate() command?
Attachment #8814354 - Flags: feedback?(kgilbert)
Comment on attachment 8814354 [details] [diff] [review]
Bug-1315718-Replace-mGamepadManager-raw-pointer-with.patch (v3?)

>
> /* static */ bool
> VRManagerParent::CreateForContent(Endpoint<PVRManagerParent>&& aEndpoint)
> {
>   MessageLoop* loop = layers::CompositorThreadHolder::Loop();
> 
>   RefPtr<VRManagerParent> vmp = new VRManagerParent(aEndpoint.OtherPid());
>+  mContentProcessId = aEndpoint.OtherPid();

Keeping the Content process id here.

>   loop->PostTask(NewRunnableMethod<Endpoint<PVRManagerParent>&&>(
>     vmp, &VRManagerParent::Bind, Move(aEndpoint)));
> 
>   return true;
> }
> 
>+bool
>+VRManagerParent::SendGamepadUpdate(const GamepadChangeEvent& aGamepadEvent)
>+{
>+  // Due to GamepadManager only exists at the content process
>+  // or the same process in non-e10s mode.

Checking if the child process id is equal to content process id. Then, updating gamepads.
 
>+  if (mContentProcessId == GetChildProcessId() || IsSameProcess())
>+  {
>+    return PVRManagerParent::SendGamepadUpdate(aGamepadEvent);
>+  }
>+  else {
>+    return true;
>+  }
>+}
>+
> } // namespace gfx
> } // namespace mozilla
(In reply to Daosheng Mu[:daoshengmu] from comment #13)
> Comment on attachment 8814354 [details] [diff] [review]
> Bug-1315718-Replace-mGamepadManager-raw-pointer-with.patch (v3?)
> 
> >
> > /* static */ bool
> > VRManagerParent::CreateForContent(Endpoint<PVRManagerParent>&& aEndpoint)
> > {
> >   MessageLoop* loop = layers::CompositorThreadHolder::Loop();
> > 
> >   RefPtr<VRManagerParent> vmp = new VRManagerParent(aEndpoint.OtherPid());
> >+  mContentProcessId = aEndpoint.OtherPid();
> 
> Keeping the Content process id here.

I think you don't need to add mContentProcessId here, as VRManagerParent keeps track of it for you.  You can call VRManagerParent::OtherPid(), inherited from ITopLevelProtocol to get it.

> 
> >   loop->PostTask(NewRunnableMethod<Endpoint<PVRManagerParent>&&>(
> >     vmp, &VRManagerParent::Bind, Move(aEndpoint)));
> > 
> >   return true;
> > }
> > 
> >+bool
> >+VRManagerParent::SendGamepadUpdate(const GamepadChangeEvent& aGamepadEvent)
> >+{
> >+  // Due to GamepadManager only exists at the content process
> >+  // or the same process in non-e10s mode.
> 
> Checking if the child process id is equal to content process id. Then,
> updating gamepads.
>  
> >+  if (mContentProcessId == GetChildProcessId() || IsSameProcess())

Perhaps you could do something like:
> if (OtherPid() == GetChildProcessId() || IsSameProcess())
Comment on attachment 8814354 [details] [diff] [review]
Bug-1315718-Replace-mGamepadManager-raw-pointer-with.patch (v3?)

Looking good; however, could use additional reviews by others that could double-check the usage of the raw pointer.

Also, see my comment on replacing mContentProcessId with ITopLevelProtocol::OtherPid()
Attachment #8814354 - Flags: feedback?(kgilbert) → feedback+
V3
Add a bool for checking if the child process is the Content process for avoiding to send redundant gamepadUpdate() command.
Attachment #8813517 - Attachment is obsolete: true
Attachment #8814354 - Attachment is obsolete: true
Attachment #8814801 - Flags: review?(kgilbert)
Attachment #8814801 - Flags: review?(bugs)
Comment on attachment 8814801 [details] [diff] [review]
Bug 1315718 - Replace mGamepadManager raw pointer with RefPtr in VRManagerChild; r?smaug, kip (v3)

Review of attachment 8814801 [details] [diff] [review]:
-----------------------------------------------------------------

This looks better now using mIsContentChild boolean.
Thanks!
Attachment #8814801 - Flags: review?(kgilbert) → review+
Comment on attachment 8814801 [details] [diff] [review]
Bug 1315718 - Replace mGamepadManager raw pointer with RefPtr in VRManagerChild; r?smaug, kip (v3)


>+VRManagerParent::SendGamepadUpdate(const GamepadChangeEvent& aGamepadEvent)
>+{
>+  // GamepadManager only exists at the content process
>+  // or the same process in non-e10s mode.
>+  if (mIsContentChild || IsSameProcess()) {
>+    return PVRManagerParent::SendGamepadUpdate(aGamepadEvent);
>+  }
>+  else {
>+    return true;
>+  }
Nit, 
} else {
Attachment #8814801 - Flags: review?(bugs) → review+
Carry r+ from Comment 17 and Comment 18
According to Comment 18, moving the brace.
Attachment #8814801 - Attachment is obsolete: true
Attachment #8815646 - Flags: review+
Keywords: checkin-needed
Keywords: checkin-needed
Carry r+ from Comment 17 and Comment 18
According to Comment 18, moving the brace.
Attachment #8815646 - Attachment is obsolete: true
Attachment #8815658 - Flags: review+
Bug 1315718 - Replace mGamepadManager raw pointer with RefPtr in VRManagerChild; r?smaug, kip (v4)

Carry r+ from Comment 17 and Comment 18
According to Comment 18, moving the brace.
Attachment #8815658 - Attachment is obsolete: true
Attachment #8815666 - Flags: review+
Keywords: checkin-needed
needs rebasing for inbound
Flags: needinfo?(dmu)
Keywords: checkin-needed
V5-
Rebase and try again.

Carry r+ from Comment 17 and Comment 18.
Attachment #8815666 - Attachment is obsolete: true
Flags: needinfo?(dmu)
Attachment #8815973 - Flags: review+
Keywords: checkin-needed
This doesn't apply cleanly to inbound at all. Please rebase. Also, what other branches are affected?
Flags: needinfo?(dmu)
Keywords: checkin-needed
(In reply to Ryan VanderMeulen [:RyanVM] from comment #24)
> This doesn't apply cleanly to inbound at all. Please rebase. Also, what
> other branches are affected?

Sorry, I don't know what happened. I have rebased inbound/default(https://hg.mozilla.org/integration/mozilla-inbound), and it didn't show any fail in my attachment 8815973 [details] [diff] [review].
Flags: needinfo?(dmu)
Sorry I didn't paste the check-in information immediately.
Check-in: https://hg.mozilla.org/integration/mozilla-inbound/rev/0dff674b4497
https://hg.mozilla.org/mozilla-central/rev/0dff674b4497
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Group: gfx-core-security → core-security-release
Should we consider this for backport to Beta52 for the next ESR?
Flags: needinfo?(dmu)
(In reply to Ryan VanderMeulen [:RyanVM] from comment #28)
> Should we consider this for backport to Beta52 for the next ESR?

Yep, you are right. We should up-lift it
Flags: needinfo?(dmu)
Please set the appropriate request then :)
Flags: needinfo?(dmu)
Comment on attachment 8815973 [details] [diff] [review]
Bug 1315718 - Replace mGamepadManager raw pointer with RefPtr in VRManagerChild; r?smaug, kip (v5)

Approval Request Comment
[Feature/Bug causing the regression]: Be more safety for the current usage to avoid using raw pointer.
[User impact if declined]: would have some secure issue.
[Is this code covered by automated tests?]: We don't have automated tests on VR module currently, but I have tested it manually, and it didn't happen any issue before.
[Has the fix been verified in Nightly?]: Yes!
[Needs manual test from QE? If yes, steps to reproduce]: Nope. It is just a secure concern, it didn't happen any issue before.
[List of other uplifts needed for the feature/fix]: Nope.
[Is the change risky?]: Nope. it is safe.
[Why is the change risky/not risky?]: It is more correct way to use RefPtr instead of raw pointer. It will make the code be more robust.
[String changes made/needed]: Nope.
Flags: needinfo?(dmu)
Attachment #8815973 - Flags: approval-mozilla-beta?
Comment on attachment 8815973 [details] [diff] [review]
Bug 1315718 - Replace mGamepadManager raw pointer with RefPtr in VRManagerChild; r?smaug, kip (v5)

use refptr instead of raw pointer, beta52+
Attachment #8815973 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main52-]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.