Closed
Bug 1254876
Opened 8 years ago
Closed 8 years ago
Intermittent 1113005.html | application crashed [@ webrtc::AudioDeviceWindowsCore::DoCaptureThread()]
Categories
(Core :: WebRTC: Audio/Video, defect, P1)
Core
WebRTC: Audio/Video
Tracking
()
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)
1.29 KB,
patch
|
pkerr
:
review+
ritu
:
approval-mozilla-aurora+
ritu
:
approval-mozilla-beta+
Sylvestre
:
approval-mozilla-esr38+
ritu
:
approval-mozilla-esr45+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
Assignee | ||
Comment 1•8 years ago
|
||
Crash address is e5's.... likely UAF
Assignee | ||
Comment 2•8 years ago
|
||
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
Assignee | ||
Comment 3•8 years ago
|
||
This will be moot once we land full-duplex for wasapi and turn it on, note
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → rjesup
Assignee | ||
Comment 4•8 years ago
|
||
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.
Rank: 10 → 15
status-firefox45:
--- → wontfix
status-firefox46:
--- → affected
status-firefox47:
--- → affected
status-firefox-esr45:
--- → affected
Assignee | ||
Comment 5•8 years ago
|
||
MozReview-Commit-ID: 67qG1L63Tgn
Attachment #8732655 -
Flags: review?(pkerr)
Updated•8 years ago
|
Attachment #8732655 -
Flags: review?(pkerr) → review+
Assignee | ||
Comment 6•8 years ago
|
||
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?
Comment 7•8 years ago
|
||
sec-approval+ for trunk. Is ERS38 unaffected?
status-firefox-esr38:
--- → ?
tracking-firefox46:
--- → +
tracking-firefox47:
--- → +
tracking-firefox48:
--- → +
tracking-firefox-esr45:
--- → ?
Updated•8 years ago
|
Attachment #8732655 -
Flags: sec-approval? → sec-approval+
Assignee | ||
Comment 8•8 years ago
|
||
ESR 38 is likely affected; can't tell for sure as we can't repro. Safe change though
Assignee | ||
Comment 9•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e705888d2a8
Comment 10•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/6e705888d2a8
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Updated•8 years ago
|
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)
Assignee | ||
Comment 16•8 years ago
|
||
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?
Assignee | ||
Comment 17•8 years ago
|
||
https://hg.mozilla.org/releases/mozilla-esr38/rev/e7c23c08bf84 Landed with a=ritu per request
Comment 18•8 years ago
|
||
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+
Updated•8 years ago
|
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Updated•8 years ago
|
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main46+][adv-esr45.1+][adv-esr38.8+]
Updated•8 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•