1758549 - Prevent the creation of WebVR IPDL actors when WebVR is disabled

Status: RESOLVED FIXED

(Core :: WebVR, task)

Description

[Andrew McCreight [:mccr8]]

In light of bug 1758156, we should figure out if WebVR can be used by a compromised content process even if it is preffed off. That's the only other protocol I can think of that is guarded by a pref. I poked around a little bit in the code the other day, but I wasn't able to figure out where such checks might need to happen.

Comment 1

[Andrew McCreight [:mccr8]]

VRManagerParent::AllocPVRLayerParent()
VRManagerChild::AllocPVRLayerChild()

VRProcessChild::Init() allocates a new VRParent.
VRProcessParent::InitAfterConnect() allocates a new VRChild.

I'm not sure which processes any of these run in.

Comment 2

[Andrew McCreight [:mccr8]]

Another IPDL protocol in the gfx/vr/ipc/ directory is PVRGPU (implemented by VRGPUChild and VRGPUParent).

There's also PVRManager, implemented by VRManagerChild and VRManagerParent.

Comment 3

[Andrew McCreight [:mccr8]]

In some sense there's less risk here than with WebGPU, because we were knowingly shipping WebVR for a long time. But it would still be good to be sure that this is locked down.

Comment 4

[Christian Holler (:decoder)]

(In reply to Andrew McCreight [:mccr8] from comment #3)

In some sense there's less risk here than with WebGPU, because we were knowingly shipping WebVR for a long time. But it would still be good to be sure that this is locked down.

I don't the risk is significantly smaller. Indeed, we had been shipping WebVR but we stopped testing it in fuzzing a long time ago because of unresolved issues.

Comment 5

[Andrew McCreight [:mccr8]]

It looks like the PVRManager actor exists even without WebVR enabled. VRManagerParent is in the main process, and the Child is in content processes. Looks like a child process could use that actor to send a PVRLayerConstructor message to create a VRLayerParent in the parent. I'll see what happens when you do that.

Comment 6

[Andrew McCreight [:mccr8]]

AllocPVRLayerParent calls into VRManager::AddLayer() which adds it to mLayers, then calls StartPresentation(). I think StartPresentation can't do anything too interesting, because I think mState will always be Disabled, because the code that would change the state is in fact guarded on the VR prefs. (It does call DispatchVRDisplayInfoUpdate() which does SendUpdateDisplayInfo to every content process, which might do something weird, but is probably okay.)

This doesn't sound too terrible, except that as seen in bug 1758776 I figured out a way to get a UAF in the parent process just from being able to create and destroy VRLayerParents.

Comment 7

[Andrew McCreight [:mccr8]]

I think the summary of the IPDL protocols for VR are

• PVRManager - Between parent and content processes. This is currently runs by default in Firefox even if WebVR is disabled.
• PVRLayer - Between parent and content processes.
• PVR - Between the VR process and the parent process.
• PVRGPU - Between the VR process and the GPU process.

I think PVR and PVRGPU don't present any risk when VR is disabled. PVR and PVRGPU involve processes that have a lot of privilege already. Also, as far as I can tell, the steps to create a VR process are protected by a pref check in VRManager::ProcessManagerState_Disabled(), which is the first step in a series of states the VR manager goes through to initialize the VR process. We might as well add some checks at the point of creation, to be extra sure.

Adding checks when we create VRLayerParent and VRLayerChild looks easy enough.

Dealing with PVRManager might be tricky, because as I said, we are currently running that protocol even when VR is disabled. I'll have to see what depends on it. It also might require some fiddling with tests, because I'm not sure if you can enable the VR manager after things have been set up.

Comment 8

[Andrew McCreight [:mccr8]]

It looks like the tests in dom/vr/test/mochitest and dom/vr/test/reftest are disabled, leaving dom/vr/test/crashtests/enumerate_vr_on_dying_window.html and some web platform tests as our tests.

Comment 9

[Andrew McCreight [:mccr8]]

VRManager::mState is Disabled

Comment 10

[Andrew McCreight [:mccr8]]

I'm not exactly sure how to stop PVRManager from starting up. Maybe I can file a separate bug about that and a graphics person who is more familiar with the compositor can look at it.

Instead, I audited all of the Recv methods in VRManagerParent. They mostly get the current VRManager, then call a method on it. I assumed that VRManager::mState is always Disabled when WebVR is disabled. Under that assumption, most of the VRManager methods (like say, VRManager::SetGroupMask()) will return immediately.

One exception is VRManager::StopAllHaptics(), which is called by VRManagerParent::RecvControllerListenerAdded and ::RecvControllerListenerRemoved. As far as I can tell, that won't do anything scary. Though it does call into VRShMem::PushBrowserState. I'm not sure under which conditions we have an mShmem. In my local testing on OSX, mShmem was null.

Another exception is VRManager::ResetPuppet(), which is called by VRManagerParent::RecvResetPuppet(). I was a little concerned about this one because it is related to the testing infrastructure, but it seems okay to me. It adds the VRManagerParent to a set, but with a strong ref. It does something if mManagerParentRunningPuppet is non-null, but that's only set in VRManager::RunPuppet(), and only if dom.vr.puppet.enabled is set. Finally, it calls CheckForPuppetCompletion() which will send a message (with no data) to any process in the hash set we were added to in the first step. I think this set can't contain anything else, so the content process would basically just be sending a message back to itself which doesn't seem dangerous.

For these two messages, I'll add a relevant check so they won't do anything unless WebVR is going already or the testing pref is set, to match the other messages.

Comment 11

[Andrew McCreight [:mccr8]]

(In reply to Andrew McCreight [:mccr8] from comment #10)

I assumed that VRManager::mState is always Disabled when WebVR is disabled.

This assumption unfortunately does not appear to be true, at least on Android. The VRManager observes the Android-only topics "application-background" and "application-foreground". (I checked on my phone, and the VRManager is running on Android release.)

If VRManager observes "application-background", then mAppPaused gets set to true. Then, if it observes "application-foreground", it will set mAppPaused to false and call StartTasks(), which will start up a timer with the callback TaskTimerCallback. That timer will fire off repeatedly. Then, when "application-foreground" gets observed again, mAppPaused becomes true again. When the timer fires the next time, mState will become Idle. I assume this sequence doesn't require anything more than Firefox going into the background and then into the foreground a few times. On desktop, I manually simulated it by calling NotifyObservers().

A hostile content process can then send a PVRManager::DetectRuntimes message to the parent. Because the state is Idle and not Disabled, the VRManager will actually do something, and try to initialize the various VR stuff. Eventually we end up in VRService::ServiceInitialize(). Fortunately, from my reading of the code, it appears that Android isn't actually supported in there so I don't think we'll do anything further.

Anyways, this can be fixed by not doing anything in Observe if WebVR is not enabled, but it does mean that at least on Android that a lot of this code in VRManager could potentially be run by a hostile content process. (Also it seems like this timer must just be firing off all of the time on Android which doesn't seem great from a performance perspective.)

Comment 12

[Andrew McCreight [:mccr8]]

Attachment: Bug 1758549, part 1 - Check that WebVR is enabled before creating PVR, PVRGPU and PVRLayer actors.

Comment 13

[Andrew McCreight [:mccr8]]

Attachment: Bug 1758549, part 2 - Guard a few more entry points into VRManager.

Comment 14

[Andrew McCreight [:mccr8]]

Attachment: Bug 1758549, part 3 - Only change VRManager::mState away from Disabled when WebVR is enabled.

Comment 15

[Andrew McCreight [:mccr8]]

I filed bug 1759076 about disabling PVRManager when WebVR is not enabled, as this bug is already spiraling out of control. It is probably not a terrible idea to harden VRManager even if we also end up disabling it entirely when WebVR is not enabled.

The idea of my part 3 patch is that it crashes if the state changes away from Disabled if the WebVR pref isn't set. I audited every place that changes the state (which is led to my discovery that Observe can cause the state to go from Disabled to Idle), but it is possible I missed something. This could make the browser crash if you disable WebVR while it is running, but maybe that's okay. Maybe it isn't, and I can skip part 3.

Comment 16

[Andrew McCreight [:mccr8]]

On further reflection, I'm worried about part 3 causing weird crashes if you flip the WebVR pref on and off, so I think I'll skip that one. That's mostly just like a triple check protection, and I think it isn't necessary.

Comment 17

[Andrew McCreight [:mccr8]]

Comment on attachment 9267291 [details]
Bug 1758549, part 1 - Check that WebVR is enabled before creating PVR, PVRGPU and PVRLayer actors.

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Not easily. This would require arbitrary code execution in a content process, and then you'd have to find some further issue with these IPDL interfaces like bug 1758776 to make something bad happen. The way that I think WebVR becomes active on Android without these patches doesn't make me feel good, but it looks stubbed out enough that hopefully it can't really do anything too bad.
• 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
• 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?: WebVR code hasn't changed much lately, so backports should be easy.
• How likely is this patch to cause regressions; how much testing does it need?: Unlikely. The code in the patch in part 1 shouldn't run unless WebVR is enabled, along with most of part 2. The Observe change probably will cause us to run less code on Android after the application goes between background and foreground a few times, but I really hope that the code we do run doesn't do anything of consequence.

Comment 18

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Comment on attachment 9267291 [details]
Bug 1758549, part 1 - Check that WebVR is enabled before creating PVR, PVRGPU and PVRLayer actors.

Approved to land; let's uplift it late next week or the 21st.

Comment 19

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Landed:
https://hg.mozilla.org/integration/autoland/rev/13be61e5ccd3453f189cd9ec1cfd66db9fdcdb32
https://hg.mozilla.org/integration/autoland/rev/bf1b56a58efff97575768745243d31394e9d82db

Backed out for causing wpt leakcheck failures:

https://hg.mozilla.org/integration/autoland/rev/b73d96306cc30c6caf65dc86c5aa6df8a55142e6

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=bf1b56a58efff97575768745243d31394e9d82db&selectedTaskRun=Nn-wzgx5R-OxVutmZORPMg.0
Failure log: https://treeherder.mozilla.org/logviewer?job_id=370717200&repo=autoland

TEST-UNEXPECTED-FAIL | leakcheck | vr 128 bytes leaked (Mutex, ProfilerParentTracker) after the webvr wpt folder ran.

Comment 20

[Andrew McCreight [:mccr8]]

I'm trying to look into this leak but W-fis-bk doesn't seem to exist on try. Maybe I'll just ignore the leak, as that looks like a singleton leak and surely we don't care about leaks in WebVR very much at this point.

Comment 21

[Andrew McCreight [:mccr8]]

Never mind, regular W-fis fails, too.

Comment 22

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

part 1 - Check that WebVR is enabled before creating PVR, PVRGPU and PVRLayer actors. r=jgilbert
https://hg.mozilla.org/integration/autoland/rev/9a30fd6463adad2e3d4bbbb4763bdb369cacc373
https://hg.mozilla.org/mozilla-central/rev/9a30fd6463ad

part 2 - Guard a few more entry points into VRManager. r=jgilbert
https://hg.mozilla.org/integration/autoland/rev/0db4289797430d01a77bd2f34b8b31518d4e3fbd
https://hg.mozilla.org/mozilla-central/rev/0db428979743

Comment 23

[Andrew McCreight [:mccr8]]

The leak was apparently caused by part 1. I added a small leak threshold in VR processes while running the WebVR WPTs to ignore it. It looks like some kind of singleton leak, and the WebVR stuff is not likely to be actively worked on, so I think that's reasonable.

Comment 25

[BugBot [:suhaib / :marco/ :calixte]]

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.

Comment 26

[Andrew McCreight [:mccr8]]

Comment on attachment 9267291 [details]
Bug 1758549, part 1 - Check that WebVR is enabled before creating PVR, PVRGPU and PVRLayer actors.

Beta/Release Uplift Approval Request

• User impact if declined: Possible sec-high issues due to exploitation of WebVR even if it is disabled. My concern here is that the recent zero day involving WebGPU even if it is disabled will get people poking around for similar vulnerabilities in WebVR, so this locks things down a bit.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: Bug 1759432
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The primary risk is that it'll break people who have enabled WebVR. That's what the regression bug 1759432 was about. Nobody's really tested what happens to WebVR with this patch, but our current WebVR implementation is based on an out-of-date spec so it probably isn't used much.
• String changes made/needed: none

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration:
• User impact if declined:
• Fix Landed on Version: 100
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky):

Comment 27

[Donal Meehan [:dmeehan]]

Comment on attachment 9267291 [details]
Bug 1758549, part 1 - Check that WebVR is enabled before creating PVR, PVRGPU and PVRLayer actors.

Approved for 99.0b5. Thanks.

Comment 28

[Donal Meehan [:dmeehan]]

Comment on attachment 9267292 [details]
Bug 1758549, part 2 - Guard a few more entry points into VRManager.

Approved for 99.0b5. Thanks.

Comment 29

[Donal Meehan [:dmeehan]]

https://hg.mozilla.org/releases/mozilla-beta/rev/66d64b2bb54f
https://hg.mozilla.org/releases/mozilla-beta/rev/c118950041c0

Comment 30

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9267291 [details]
Bug 1758549, part 1 - Check that WebVR is enabled before creating PVR, PVRGPU and PVRLayer actors.

Approved for 91.8esr.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr91/rev/cbb04ded5d2a
https://hg.mozilla.org/releases/mozilla-esr91/rev/2bb14c5df785