Closed Bug 1254876 Opened 5 years ago Closed 5 years ago
.html | application crashed [@ webrtc::Audio Device Windows Core::Do Capture Thread()]
Crash address is e5's.... likely 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
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.
Attachment #8732655 - Flags: review?(pkerr)
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.
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
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!
Comment on attachment 8732655 [details] [diff] [review] assert windows recording is shut down Per ritu's request
Attachment #8732655 - Flags: approval-mozilla-esr38?
https://hg.mozilla.org/releases/mozilla-esr38/rev/e7c23c08bf84 Landed with a=ritu per request
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+
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main46+][adv-esr45.1+][adv-esr38.8+]
You need to log in before you can comment on or make changes to this bug.