1415582 - Cleanup WebRTCGMP decoder initialization to match Encoder side

Status: RESOLVED FIXED

(Core :: WebRTC: Audio/Video, defect, P1)

Description

[Randell Jesup [:jesup] (needinfo me)]

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 ]

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Mirror changes done to Encoder InitDone to decoder

Comment 2

[Byron Campen [:bwc]]

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.

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

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.

Comment 4

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

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.

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/833205c1f498bed137fb2e0a06d14ef31e3c0609

Comment 7

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

https://hg.mozilla.org/mozilla-central/rev/833205c1f498

Comment 8

[Gerry Chang [:gchang]]

Comment on attachment 8926434 [details] [diff] [review]
Mirror changes done to Encoder InitDone to decoder

Fix a sec-high. Beta58+ & ESR52+.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/7bea6415eb77

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/7eba7d14704a