Closed Bug 1356387 Opened 3 years ago Closed 2 years ago

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

Categories

(Core :: WebVR, defect, critical)

Unspecified
Windows 10
defect
Not set
critical

Tracking

()

RESOLVED FIXED
Tracking Status
firefox-esr45 --- wontfix
firefox52 --- disabled
firefox-esr52 --- disabled
firefox53 --- disabled
firefox54 --- disabled
firefox55 --- fixed

People

(Reporter: mccr8, Assigned: kip)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [post-critsmash-triage])

Crash Data

Attachments

(1 file)

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.
The URLs are various WebVR sites, as you'd expect, including https://chromatic.funktroniclabs.com/ and https://webvrexperiments.com/
Kip, can you look at this or know somebody who could? Thanks.
Flags: needinfo?(kgilbert)
I can take this one
Assignee: nobody → kgilbert
Flags: needinfo?(kgilbert)
I filed bug 1356392 about making the Write() checks into a release assert.
Julian, do you know what kind of security ratings we give information leaks like this? Is it worth leaving it hidden?
Flags: needinfo?(julian.r.hector)
: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.
Flags: needinfo?(julian.r.hector)
Thanks for the explanation. I'll mark it sec-moderate. sec-low is usually a little less severe than that.
Keywords: sec-moderate
Attachment #8858147 - Flags: review?(continuation)
Duplicate of this bug: 1356230
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().
Attachment #8858147 - Flags: review?(continuation) → review+
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?
(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.
Flags: needinfo?(kgilbert)
Aurora 54 goes to Beta tomorrow, so I think we're good to just let it ride the 55 train then. Thanks!
https://hg.mozilla.org/mozilla-central/rev/208be3422479
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Duplicate of this bug: 1357404
Group: gfx-core-security → core-security-release
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.