Closed Bug 1254876 Opened 4 years ago Closed 4 years ago

Intermittent 1113005.html | application crashed [@ webrtc::AudioDeviceWindowsCore::DoCaptureThread()]

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox45 --- wontfix
firefox46 + fixed
firefox47 + fixed
firefox48 + fixed
firefox-esr38 46+ fixed
firefox-esr45 46+ fixed

People

(Reporter: philor, Assigned: jesup)

Details

(Keywords: csectype-uaf, intermittent-failure, sec-high, Whiteboard: [post-critsmash-triage][adv-main46+][adv-esr45.1+][adv-esr38.8+])

Attachments

(1 file)

Crash address is e5's.... likely UAF
Group: media-core-security
Rank: 10
Keywords: csectype-uaf
Priority: -- → P1
Note: media/webrtc/trunk/webrtc/modules/audio_device/win/audio_device_core_win.cc
Upstream code.  Crashed in the thread at Lock()
Component: WebRTC → WebRTC: Audio/Video
This will be moot once we land full-duplex for wasapi and turn it on, note
Assignee: nobody → rjesup
Keywords: sec-high
Ok, this crash is in shutdown - MainThread is in directory-service shutdown, doing profile-before-change or profile-before-change2.  And this is in shutdown (or error) for the WASAPI capture thread; it appears the AudioDeviceWindowsCore object has gone poof (~AudioDeviceWindowsCore()) without shutting down the thread; I don't see how else we could end up here with a UAF.

I don't see any checks that StopRecording() has been called; it may make sense to add a safety assert here (to convert anything bad into a safe crash).  This code will be moot in 48 as we'll be enabling full-duplex cubeb WASAPI on windows, and not using the webrtc.org upstream capture code.
MozReview-Commit-ID: 67qG1L63Tgn
Attachment #8732655 - Flags: review?(pkerr)
Attachment #8732655 - Flags: review?(pkerr) → review+
Comment on attachment 8732655 [details] [diff] [review]
assert windows recording is shut down

[Security approval request comment]
How easily could an exploit be constructed based on the patch?  
Hard to very hard to impossible.  Adds an assert; one might therefor infer that this might happen, but with no easy way to find how it occurred (we don't know).  Inferring how StopRecording might not be called would be rough; the code that does that is far away from the object.  And then turning the UAF into something actionable is a whole additional layer.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Not really, but an assert is obviously a check for something.  Removing the comment doesn't really hide what we're checking.


Which older supported branches are affected by this flaw?
all of them I believe

If not all supported branches, which bug introduced the flaw?
unknown, old import likely.

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?
Extremely unlikely.
Attachment #8732655 - Flags: sec-approval?
Attachment #8732655 - Flags: approval-mozilla-esr45?
Attachment #8732655 - Flags: approval-mozilla-beta?
Attachment #8732655 - Flags: approval-mozilla-aurora?
sec-approval+ for trunk.

Is ERS38 unaffected?
Attachment #8732655 - Flags: sec-approval? → sec-approval+
ESR 38 is likely affected; can't tell for sure as we can't repro.  Safe change though
https://hg.mozilla.org/mozilla-central/rev/6e705888d2a8
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Group: media-core-security → core-security-release
Comment on attachment 8732655 [details] [diff] [review]
assert windows recording is shut down

Sec-high issue, Aurora47+, Beta46+, ESR45+
Attachment #8732655 - Flags: approval-mozilla-esr45?
Attachment #8732655 - Flags: approval-mozilla-esr45+
Attachment #8732655 - Flags: approval-mozilla-beta?
Attachment #8732655 - Flags: approval-mozilla-beta+
Attachment #8732655 - Flags: approval-mozilla-aurora?
Attachment #8732655 - Flags: approval-mozilla-aurora+
(In reply to Randell Jesup [:jesup] from comment #8)
> ESR 38 is likely affected; can't tell for sure as we can't repro.  Safe
> change though

Hi Jesup, in that case, could you also nominate for uplift to esr38? You can approve and land it with a=ritu. Thanks!
Flags: needinfo?(rjesup)
Comment on attachment 8732655 [details] [diff] [review]
assert windows recording is shut down

Per ritu's request
Flags: needinfo?(rjesup)
Attachment #8732655 - Flags: approval-mozilla-esr38?
Comment on attachment 8732655 [details] [diff] [review]
assert windows recording is shut down

Fix a sec-high, taking it.
Should be in 38.8.0
Attachment #8732655 - Flags: approval-mozilla-esr38? → approval-mozilla-esr38+
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main46+][adv-esr45.1+][adv-esr38.8+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.