1512429 - AddressSanitizer: heap-use-after-free [@ operator bool] with READ of size 8 through VRProcessManager

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Christian Holler (:decoder)]

The attached crash information was submitted via the ASan Nightly Reporter on mozilla-central-asan-nightly revision 65.0a1-20181206092619-https://hg.mozilla.org/mozilla-central/rev/f7895b06b74299a13bba51e3de50c8eaa9729e10.

For detailed crash information, see attachment.

Comment 1

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 2

[Darkspirit]

I think this has been bug 1512405 comment 1, but tried with ASan Nightly.

Comment 3

[Andrew McCreight [:mccr8]]

It looks like the problem is that VRProcessManager::Observer has only a weak pointer to VRProcessManager, so it is outliving the manager. I'm not sure why VRProcessManager::Initialize() would free a VRProcessManager, but my guess would be that there's already something in sSingleton. So, maybe initialize should check if it is initialized already?

Also the dtor in the VRProcessManager should clear the weak pointer from the Observer to the Manager. I did something like this for other shutdown observers in: https://hg.mozilla.org/mozilla-central/rev/f5c1dbff0c9a

Comment 4

[Andrew McCreight [:mccr8]]

I'll mark this sec-high, but maybe it requires turning VR off and on manually in some kind of way that would be impossible to trigger from content.

Comment 5

[:kip (Kearwood Gilbert)]

I'll take this one and investigate.

Comment 6

[:kip (Kearwood Gilbert)]

I have been catching up on a number of these sec-high bugs, and am now moving on to this one.

Comment 7

[:kip (Kearwood Gilbert)]

Perhaps there is a race between the VR subsystem timing out due to inactivity and trying to stop its VR process and the browser shutting down.  This could perhaps result in multiple shutdown messages in the IPC queue.  It could also be possible that a not-yet processed message in the queue to (re-)start the VR process was present while the browser shutdown was occurring.

Either way, more checks as suggested in Comment 3 could avoid the UAF in these scenarios by ensuring that we don't have duplicate VRProcessManager's or have an Observer with a pointer to the destroyed VRProcessManager.

Comment 8

[:kip (Kearwood Gilbert)]

Attachment: Bug 1512429 - Ensure that VRProcessManager::Observer releases its pointer from VRProcessManager in case it outlives VRProcessManager and that only one VRProcessManager is allocated

Corrections as per Comment 3.  I am currently testing this patch, then will assign for review.

Comment 9

[:kip (Kearwood Gilbert)]

Attachment: Bug 1512429 - Ensure that VRProcessManager::Observer releases its pointer from VRProcessManager in case it outlives VRProcessManager and that only one VRProcessManager is allocated

Comment 10

[:kip (Kearwood Gilbert)]

Local testing using https://sketchfab.com and https://webvr.info/samples did not show any problems with the attached patch.

Running through try for a sanity check:

./mach try -b do -p all -u all[x64,10.10,Windows\ 10,Android\ 4.3] -t none

remote: View your changes here:
remote:   https://hg.mozilla.org/try/rev/77f3e8bee919ecc8275ded435a17c7c42a8827e8
remote:   https://hg.mozilla.org/try/rev/9bbe7ae4b64a6dc636715f0089dcd97a95e3e0b5
remote:
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=9bbe7ae4b64a6dc636715f0089dcd97a95e3e0b5

Comment 11

[:kip (Kearwood Gilbert)]

(In reply to :kip (Kearwood Gilbert) from comment #10)

Local testing using https://sketchfab.com and https://webvr.info/samples did
not show any problems with the attached patch.

Running through try for a sanity check:

./mach try -b do -p all -u all[x64,10.10,Windows\ 10,Android\ 4.3] -t none

remote: View your changes here:
remote:
https://hg.mozilla.org/try/rev/77f3e8bee919ecc8275ded435a17c7c42a8827e8
remote:
https://hg.mozilla.org/try/rev/9bbe7ae4b64a6dc636715f0089dcd97a95e3e0b5
remote:
remote: Follow the progress of your build on Treeherder:
remote:
https://treeherder.mozilla.org/#/
jobs?repo=try&revision=9bbe7ae4b64a6dc636715f0089dcd97a95e3e0b5

Try push looks good

Comment 12

[Daosheng Mu[:daoshengmu]]

Comment on attachment 9034069 [details] [diff] [review]
Bug 1512429 - Ensure that VRProcessManager::Observer releases its pointer from VRProcessManager in case it outlives VRProcessManager and that only one VRProcessManager is allocated

LGTM. Thanks for helping this.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Hi Kip, is this ready for a sec-approval request? We've only got one Beta left before 65 goes into RC and it would be good to get this patch into it.

Comment 14

[:kip (Kearwood Gilbert)]

Comment on attachment 9034069 [details] [diff] [review]
Bug 1512429 - Ensure that VRProcessManager::Observer releases its pointer from VRProcessManager in case it outlives VRProcessManager and that only one VRProcessManager is allocated

[Security Approval Request]

How easily could an exploit be constructed based on the patch?: The dom.vr.process.enabled pref is not yet enabled by default, and would need to be activated manually for this code to be hit.

The patch does not illustrate the means to create an exploit. This would have to be combined with another exploit that allows writing memory in the region that is accessed in the UAF. This crash occurs due to specific timing of events, but does not involve untrusted web content to overflow buffers or otherwise write to memory.

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?:

If not all supported branches, which bug introduced the flaw?: Bug 1430038

Do you have backports for the affected branches?: No

If not, how different, hard to create, and risky will they be?: Non risky, but little benefit as this feature is not enabled by default.

How likely is this patch to cause regressions; how much testing does it need?: Not likely to cause regressions, as it only affects code behind a pref (dom.vr.process.enabled), which is not yet enabled by default.

Comment 15

[:kip (Kearwood Gilbert)]

I expect the scope of this vulnerability to be limited.

Upon closer inspection, it appears that the crash stack is not possible with the default pref setting. It is possible that someone manually enabled dom.vr.process.enabled mistakenly or that this was submitted from a Firefox developer's workstation.

This pref will soon be turned on by default; however, so this fix will still be needed.

Comment 16

[:kip (Kearwood Gilbert)]

As this crash is occurring in code that is not enabled by default, perhaps it would not need to be sec-high

Comment 17

[Al Billings [:abillings - ex-MoCo]]

This can stay sec-high but if it is not currently on in beta or release, I'll mark it disabled there and give sec-approval. Please confirm the setting.

Comment 18

[:kip (Kearwood Gilbert)]

(In reply to Al Billings [:abillings] from comment #17)

This can stay sec-high but if it is not currently on in beta or release, I'll mark it disabled there and give sec-approval. Please confirm the setting.

This sounds good to me, thanks! Please advise if I've missed any steps.

Comment 19

[Al Billings [:abillings - ex-MoCo]]

sec-approval+ for trunk. I don't think we need to backport this to Beta since it is disabled by default there.

Comment 20

[Liz Henry (:lizzard) (privacy/fingerprinting team)]

Are you still planning to land this in nightly? And, I'm marking this as disabled in beta for 66 but please correct that if it's wrong.

Comment 21

[:kip (Kearwood Gilbert)]

(In reply to Liz Henry (:lizzard) (use needinfo) from comment #20)

Are you still planning to land this in nightly? And, I'm marking this as disabled in beta for 66 but please correct that if it's wrong.

Indeed, tree was closed earlier. I will land this right away.

Comment 22

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/489bd0cf5e1c262b64f2f1c2a4d2d20dd237c6ff
https://hg.mozilla.org/mozilla-central/rev/489bd0cf5e1c