Use-after-free in GMP process if GMPDecryptorChild actor destroyed before GMPVideoDecoderChild actor

RESOLVED FIXED in Firefox 51

Status

()

defect
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: cpearce, Assigned: cpearce)

Tracking

({csectype-uaf, sec-moderate})

unspecified
mozilla53
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr45 unaffected, firefox50 wontfix, firefox51 fixed, firefox52 fixed, firefox53 fixed)

Details

(Whiteboard: (sandboxed)[post-critsmash-triage][adv-main51+])

Attachments

(1 attachment)

The Widevine CDM gets a reference to the cdm::Host8 instance, and our implementation of this lives on the GMPDecryptorChild instance. We pass a pointer to this to the Widevine CDM.

However we also have a connection into the CDM via the GMPVideoDecoderChild instance. So if the GMPDecrytor IPC connection closes down before the GMPVideoDecoder IPC connection, the CDM can end up trying to talk to the Host stored on the free'd GMPDecryptorChild instance.

So the Widevine CDM ends up doing a use-after-free on the GMPDecryptorChild's vtable.

I'm pretty sure it's not possible for an attacker to write into that memory.
(Assignee)

Updated

2 years ago
Attachment #8817071 - Flags: review?(gsquelart)
Attachment #8817071 - Flags: review?(gsquelart) → review+
(Assignee)

Comment 2

2 years ago
Comment on attachment 8817071 [details] [diff] [review]
Ensure the CDMWrapper maintains a refcount on the GMPDecryptor, so it can't be freed before the CDM is destroyed

[Security approval request comment]
* How easily could an exploit be constructed based on the patch?

I think constructing an exploit would be hard in general, as it requires understanding our IPC system and how its actors correspond to other parts of our media stack.

I also don't see how an attacker could write over the memory stored at the vtable that we try to call. And this is a UAF in the GMP process, which is already sandboxed. So a sandbox escape would be needed to make this exploitable.


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

In theory, Firefox 47 and later are affected. So release, beta, and aurora.


* If not all supported branches, which bug introduced the flaw?

N/A


* Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

The patch applies cleanly to beta, so I assume uplifting will be fine.


* How likely is this patch to cause regressions; how much testing does it need?

Low chance of regressions; this just ensures we keep an object alive for longer; until all clients of the object are done with it.
Attachment #8817071 - Flags: sec-approval?
Keywords: sec-moderate
Whiteboard: (sandboxed)
Comment on attachment 8817071 [details] [diff] [review]
Ensure the CDMWrapper maintains a refcount on the GMPDecryptor, so it can't be freed before the CDM is destroyed

sec-approval=dveditz
Attachment #8817071 - Flags: sec-approval? → sec-approval+
(Assignee)

Updated

2 years ago
Flags: needinfo?(cpearce)
https://hg.mozilla.org/mozilla-central/rev/ff8e8c79f029
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
(Assignee)

Comment 6

2 years ago
Comment on attachment 8817071 [details] [diff] [review]
Ensure the CDMWrapper maintains a refcount on the GMPDecryptor, so it can't be freed before the CDM is destroyed

Approval Request Comment
[Feature/Bug causing the regression]:
[User impact if declined]: Potential sec-moderate exploit in sandboxed GMP process.
[Is this code covered by automated tests?]: Yes; external-media-tests Netflix tests.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No; it's hard to repro.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: It just makes an object in the GMP process live longer.
[String changes made/needed]: None.
Flags: needinfo?(cpearce)
Attachment #8817071 - Flags: approval-mozilla-beta?
Attachment #8817071 - Flags: approval-mozilla-aurora?
Comment on attachment 8817071 [details] [diff] [review]
Ensure the CDMWrapper maintains a refcount on the GMPDecryptor, so it can't be freed before the CDM is destroyed

Fix a sec-moderate. Beta51+ & Aurora52+. Should be in 51 beta 10.
Attachment #8817071 - Flags: approval-mozilla-beta?
Attachment #8817071 - Flags: approval-mozilla-beta+
Attachment #8817071 - Flags: approval-mozilla-aurora?
Attachment #8817071 - Flags: approval-mozilla-aurora+
Group: media-core-security → core-security-release
Flags: qe-verify-
Whiteboard: (sandboxed) → (sandboxed)[post-critsmash-triage]
Whiteboard: (sandboxed)[post-critsmash-triage] → (sandboxed)[post-critsmash-triage][adv-main51+
Whiteboard: (sandboxed)[post-critsmash-triage][adv-main51+ → (sandboxed)[post-critsmash-triage][adv-main51+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.