1356387 - Crash in Abort | IPDL error [PVRManagerChild]: "Error deserializing 'VRHMDSensorState'". abort... | mozalloc_abort | NS_DebugBreak | mozilla::ipc::FatalError | mozilla::dom::ContentChild::FatalErrorIfNotUsingGPUProcess | mozilla::gfx::PVRManagerChild::...

Status: RESOLVED FIXED

(Core :: WebVR, defect)

Description

[Andrew McCreight [:mccr8]]

This bug was filed from the Socorro interface and is 
report bp-e460b5ee-45c5-4d76-95c4-b29062170412.
=============================================================

I have a theory about what this crash is.

VRManagerParent::RecvGetSensorState contains code like this:
  if (display != nullptr) {
    *aState = display->GetSensorState();
  }

If display is null, nothing is done to aState, which has type VRHMDSensorState. That type has no ctor, so aState will contain random junk. One of the fields of VRHMDSensorState is VRDisplayCapabilityFlags, which is serialized using EnumSerializer which in release builds validates values it has received but not sent, so we could end up sending invalid values to the child, and then the child crashes.

Also, it is bad to leak the contents of random heap values from parent to child, so maybe VRHMDSensorState could have a ctor that does Clear()? That seems like a possible security issue.

Comment 1

[Andrew McCreight [:mccr8]]

The URLs are various WebVR sites, as you'd expect, including https://chromatic.funktroniclabs.com/ and https://webvrexperiments.com/

Comment 2

[Andrew McCreight [:mccr8]]

Kip, can you look at this or know somebody who could? Thanks.

Comment 3

[:kip (Kearwood Gilbert)]

I can take this one

Comment 4

[Andrew McCreight [:mccr8]]

I filed bug 1356392 about making the Write() checks into a release assert.

Comment 5

[Andrew McCreight [:mccr8]]

Julian, do you know what kind of security ratings we give information leaks like this? Is it worth leaving it hidden?

Comment 6

[Julian Hector [:tedd] [:jhector]]

:mccr8, leaking heap content could be used to leak heap addresses (depending on the leak amount and what address, but just assuming the worst case) which then could be used to bypass ASLR in combination with another bug.

So, I don't know what security ratings we give leaks like that, my gut tells me to go with sec-low because it can be combined with another bug for a successful chain.

Comment 7

[Andrew McCreight [:mccr8]]

Thanks for the explanation. I'll mark it sec-moderate. sec-low is usually a little less severe than that.

Comment 8

[:kip (Kearwood Gilbert)]

Attachment: Bug 1356387 - Add constructor to VRHMDSensorState

Comment 10

[Andrew McCreight [:mccr8]]

Comment on attachment 8858147 [details] [diff] [review]
Bug 1356387 - Add constructor to VRHMDSensorState

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

It looks like you could also get rid of the Clear calls in VRPose::VRPose(), VRFrameInfo::VRFrameInfo(), VRMockDisplay::VRMockDisplay(), VRDisplayHost::VRDisplayHost(), and VRDisplayPuppet::VRDisplayPuppet().

Comment 11

[Ryan VanderMeulen (PTO, back July 27)]

AFAICT, this bug is ancient? That said, I'm not sure how much we care about VR support on older releases? Do we need to consider this for backporting to 54 (going to Beta tomorrow) or ESR52, Kip?

Comment 12

[:kip (Kearwood Gilbert)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #11)
> AFAICT, this bug is ancient? That said, I'm not sure how much we care about
> VR support on older releases? Do we need to consider this for backporting to
> 54 (going to Beta tomorrow) or ESR52, Kip?

WebVR is not enabled by default in Beta until Firefox 55, so this should not be exposed.

If Aurora is sticking around for one more cycle, with 54, it may be worth uplifting there.

Comment 13

[Ryan VanderMeulen (PTO, back July 27)]

Aurora 54 goes to Beta tomorrow, so I think we're good to just let it ride the 55 train then. Thanks!

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/208be3422479