Closed Bug 1135878 Opened 5 years ago Closed 5 years ago

owned_critical_section UAF in wasapi_stream_init()


(Core :: Audio/Video, defect)

38 Branch
Not set



Tracking Status
firefox37 --- unaffected
firefox38 --- fixed
firefox39 --- fixed
firefox-esr31 --- unaffected
b2g-v1.4 --- unaffected
b2g-v2.0 --- unaffected
b2g-v2.1 --- unaffected
b2g-v2.2 --- unaffected
b2g-master --- fixed


(Reporter: karlt, Assigned: kinetik)



(Keywords: csectype-uaf, sec-critical)


(1 file)

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).

That deletes the object referenced by stm->stream_reset_lock

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.
Blocks: 1127213
No longer blocks: 127213
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
Attachment #8568269 - Flags: review?(padenot)
Attachment #8568269 - Flags: review?(padenot) → review+
Was this trunk only?
Flags: needinfo?(kinetik)
(In reply to Al Billings [:abillings] from comment #4)
> Was this trunk only?

Flags: needinfo?(kinetik)
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]

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
Attachment #8568269 - Flags: approval-mozilla-aurora?
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Not to be a PITA but can you set sec-approval? and answer the template questions?
Comment on attachment 8568269 [details] [diff] [review]

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.
Attachment #8568269 - Flags: sec-approval?
Seems straightforward enough to just land, especially since we just branched.
Attachment #8568269 - Flags: sec-approval?
Attachment #8568269 - Flags: sec-approval+
Attachment #8568269 - Flags: approval-mozilla-aurora?
Attachment #8568269 - Flags: approval-mozilla-aurora+
Group: core-security
You need to log in before you can comment on or make changes to this bug.