Cleanup WebRTCGMP decoder initialization to match Encoder side

RESOLVED FIXED in Firefox -esr52



2 years ago
Last year


(Reporter: jesup, Assigned: jesup)


({csectype-uaf, sec-high})

55 Branch
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr5258+ fixed, firefox56 wontfix, firefox57 wontfix, firefox58+ fixed, firefox59+ fixed)


(Whiteboard: [adv-main58+][adv-esr52.6+][post-critsmash-triage], crash signature)


(1 attachment)

This was missed when mirroring encoder-side fixes in bug 1182289 to the decoder side.  Important since Init is async.

Possible UAF.  Might cause nullptr crashes at [@ mozilla::WebrtcGmpVideoDecoder::GmpInitDone ]
Crash Signature: [@ mozilla::WebrtcGmpVideoDecoder::GmpInitDone ]
Rank: 5
Priority: -- → P1
Comment on attachment 8926434 [details] [diff] [review]
Mirror changes done to Encoder InitDone to decoder

Review of attachment 8926434 [details] [diff] [review]:

Hopefully this won't leak stuff, but I guess we'll see from the tests.
Attachment #8926434 - Flags: review?(docfaraday) → review+
Comment on attachment 8926434 [details] [diff] [review]
Mirror changes done to Encoder InitDone to decoder

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Quite tough.  We move something to a RefPtr, but it would be tricky to get any control over how fast it inits; you could try to close off the device fast enough to recycle the pipeline the GmpVideoDecoder, but that would not be easy.  And then you'd have to re-fill it with an attack.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?  Other than it makes a bare ptr a refptr in a callback, no.

Which older supported branches are affected by this flaw? All

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

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

How likely is this patch to cause regressions; how much testing does it need?  Little possiblity for regressions other than leaks, and I don't see any leaks running mochitests.
Attachment #8926434 - Flags: sec-approval?
Sec-approval+ for checkin on November 28, two weeks into the new cycle. Please do not check in before that date.

We'll want this on other branches so branch patches should be made and nominated at that time as well.
Whiteboard: [checkin on 11/28]
Attachment #8926434 - Flags: sec-approval? → sec-approval+
Comment on attachment 8926434 [details] [diff] [review]
Mirror changes done to Encoder InitDone to decoder

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: UAFs

Fix Landed on Version: will be 59

Risk to taking this patch (and alternatives if risky): No risk

String or UUID changes made by this patch: none

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1182289

[User impact if declined]: UAFs

[Is this code covered by automated tests?]: yes though they don't hit it

[Has the fix been verified in Nightly?]: No

[Needs manual test from QE? If yes, steps to reproduce]:  no (no STR)

[List of other uplifts needed for the feature/fix]: none

[Is the change risky?]: no

[Why is the change risky/not risky?]: Adds refptrs

[String changes made/needed]: none
Attachment #8926434 - Flags: approval-mozilla-esr52?
Attachment #8926434 - Flags: approval-mozilla-beta?
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Group: media-core-security → core-security-release
Comment on attachment 8926434 [details] [diff] [review]
Mirror changes done to Encoder InitDone to decoder

Fix a sec-high. Beta58+ & ESR52+.
Attachment #8926434 - Flags: approval-mozilla-esr52?
Attachment #8926434 - Flags: approval-mozilla-esr52+
Attachment #8926434 - Flags: approval-mozilla-beta?
Attachment #8926434 - Flags: approval-mozilla-beta+
Whiteboard: [adv-main58+][adv-esr52.6+]
Flags: qe-verify-
Whiteboard: [adv-main58+][adv-esr52.6+] → [adv-main58+][adv-esr52.6+][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.