1135878 - owned_critical_section UAF in wasapi_stream_init()

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Karl Tomlinson (:karlt)]

At https://hg.mozilla.org/mozilla-central/annotate/056c556b41b2/media/libcubeb/src/cubeb_wasapi.cpp#l1137
an auto_lock is created with a copy of the pointer in stm->stream_reset_lock.

When setup_wasapi_stream fails, it calls wasapi_stream_destroy(stm).
e.g. https://hg.mozilla.org/mozilla-central/annotate/056c556b41b2/media/libcubeb/src/cubeb_wasapi.cpp#l971

That deletes the object referenced by stm->stream_reset_lock
https://hg.mozilla.org/mozilla-central/annotate/056c556b41b2/media/libcubeb/src/cubeb_wasapi.cpp#l1193

On return to wasapi_stream_init(), the auto_lock destructor tries to leave the
CRITICAL_SECTION that has been Delete()d using the pointer to the
owned_critical_section that has been deleted.

Comment 1

[Matthew Gregan [:kinetik]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e9ce5c9f8f72

Comment 2

[Matthew Gregan [:kinetik]]

Attachment: bug1135878_v0.patch

Move wasapi_stream_destroy outside of setup_wasapi_stream, since it should only be called when the stream is being allocated.  In all other error cases, the library caller is expected to destroy the stream after an error.

Also change the render loop's callback from STOPPED to ERROR.

Comment 3

[Matthew Gregan [:kinetik]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/65a968102d6d

Comment 4

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

Was this trunk only?

Comment 5

[Matthew Gregan [:kinetik]]

(In reply to Al Billings [:abillings] from comment #4)
> Was this trunk only?

Yes.

Comment 6

[Andrew McCreight [:mccr8]]

Based on the blocking bugs, it looks like it also affects 38.  Let me know if that's not the case, or just set the 38 flag to disabled.

Comment 7

[Matthew Gregan [:kinetik]]

Comment on attachment 8568269 [details] [diff] [review]
bug1135878_v0.patch

Sorry, I'm confused and wrong, since we've just branched this *does* affect aurora. :(

Approval Request Comment
[Feature/regressing bug #]: bug 1127213
[User impact if declined]: crash/UAF
[Describe test coverage new/current, TreeHerder]: n/a
[Risks and why]: low
[String/UUID change made/needed]: none

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/65a968102d6d

Comment 9

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

Not to be a PITA but can you set sec-approval? and answer the template questions?

Comment 10

[Matthew Gregan [:kinetik]]

Comment on attachment 8568269 [details] [diff] [review]
bug1135878_v0.patch

I'm not sure I would consider this a security bug... I think Karl was being overly cautious filing it as one.  I'd be interesting in hearing how this could lead to an exploit.

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

Seems difficult to me, requires finding a way to invoke error handling path, which depends on state of OS audio stack... and then building a controllable exploit from it.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

It's not obvious how one would construct an exploit from the changes made in the patch.

Which older supported branches are affected by this flaw?

Only aurora/38.

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

Bug 1127213.

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

Exact same patch applies to trunk and aurora.

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

Low risk, fairly simple changes to error handling.

Comment 11

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

Seems straightforward enough to just land, especially since we just branched.

Comment 12

[Matthew Gregan [:kinetik]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/330952212a28