Closed Bug 1135878 Opened 5 years ago Closed 5 years ago
_critical _section UAF in wasapi _stream _init()
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.
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.
Assignee: nobody → kinetik
Status: NEW → ASSIGNED
Attachment #8568269 - Flags: review?(padenot)
Attachment #8568269 - Flags: review?(padenot) → review+
Was this trunk only?
(In reply to Al Billings [:abillings] from comment #4) > Was this trunk only? Yes.
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 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
Not to be a PITA but can you set sec-approval? and answer the template questions?
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.
Seems straightforward enough to just land, especially since we just branched.
You need to log in before you can comment on or make changes to this bug.