UAF in ReturnSSRC() (in MutexAutoLock::MutexAutoLock)

RESOLVED WORKSFORME

Status

()

P1
critical
Rank:
10
RESOLVED WORKSFORME
3 years ago
2 years ago

People

(Reporter: jesup, Assigned: jesup)

Tracking

({crash, csectype-uaf, sec-high})

Trunk
x86
All
crash, csectype-uaf, sec-high
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox42 wontfix, firefox43 wontfix, firefox44 wontfix, firefox45 fixed, firefox46 fixed, firefox-esr38- wontfix, firefox-esr45 fixed)

Details

(Whiteboard: [adv-main44+][post-critsmash-triage], crash signature)

(Assignee)

Description

3 years ago
This bug was filed from the Socorro interface and is 
report bp-669bef59-2d03-45b8-9137-015f42151218.
=============================================================

UAF, seen a few times in the last month ending a call.

May be fixed in 45 by the webrtc.org 43 update, which changes the threads things are accessed on.
(Assignee)

Updated

3 years ago
backlog: --- → webrtc/webaudio+
Rank: 15
Priority: -- → P1
(Assignee)

Updated

3 years ago
status-firefox42: --- → wontfix
status-firefox43: --- → affected
status-firefox44: --- → ?
status-firefox45: --- → ?
status-firefox46: affected → ?
(Assignee)

Updated

3 years ago
Group: media-core-security
(Assignee)

Updated

3 years ago
Assignee: nobody → rjesup
Group: core-security
(Assignee)

Updated

3 years ago
Rank: 15 → 10
Randell, have you had a chance to look at this? Is there somebody else who might have time to work on it? Thanks.
Flags: needinfo?(rjesup)
(Assignee)

Comment 2

3 years ago
[Tracking Requested - why for this release]:

So after pulling my hair out for a while, I noticed there are two signatures for this crash: one from MainThread in shutdown (via MediaManager's Observe()), and one from the MediaManager thread.  These engines/etc shouldn't be shut down from MainThread, and some of the interfaces certainly have single-thread-use assumptions (i.e. no locks).  This would allow one thread to delete the ssrc_db, while the other doesn't know this and walks down into there and goes Poof.

I strongly suspect (given these are just stack backtraces) that this is fixed by bug 1218799, which moved MediaManager backend shutdown from MainThread to a dispatch to MediaManager thread.  Bug 1218799 was uplifted to 44, so if we see this signature (with ReturnSSRC() and a UAF-ish address in 44 or later, we should re-open/investigate more.

Note this may mean we should consider bug 1218799 for ESR 38 if another point release is to be done.  Note that an uplift would require a brand-new patch, as bug1218799 one landed on top of a big refactor of that code in 44.  It likely wouldn't be a very hard patch, but not a 1-liner.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox43: affected → wontfix
status-firefox44: ? → fixed
status-firefox45: ? → fixed
status-firefox46: ? → fixed
status-firefox-esr38: --- → affected
status-firefox-esr45: --- → fixed
tracking-firefox-esr38: --- → ?
Flags: needinfo?(rjesup)
Resolution: --- → FIXED
Whiteboard: [adv-main44+]
Group: media-core-security → core-security-release
I missed this for the esr build this week. We should either wontfix this for 38 completely or plan better and build earlier for 38.7.0.  Dan, what do you think?
Flags: needinfo?(dveditz)
After talking with Dan, it seems best to wontfix this for esr38, too risky.
status-firefox-esr38: affected → wontfix
tracking-firefox-esr38: ? → -
[Tracking Requested - why for this release]:
I meant 'no fix' specifically for the impending ESR 38.6 release. I'm not ready to write off ESR 38 forever although we only have two more planned releases.

Maybe we /should/ wontfix entirely but I'd like feedback from Randell
 * is this a WebRTC shutdown under control of attacker, or global browser shutdown?
 * I assume victims have to grant at least microphone access given the stack in
   comment 0 and if so that would limit the spread of an attack. Is that right?
 * How risky would the code change be, especially given we have no ESR nightly testers?

If it's easy and safe and something that an attacker could easily trigger then we should consider fixing in esr 38.7. If it's risky and/or a hard to exploit global shutdown crash then we shouldn't bother.
status-firefox-esr38: wontfix → affected
tracking-firefox-esr38: - → ?
Flags: needinfo?(rjesup)
Flags: needinfo?(dveditz)
(Assignee)

Comment 6

3 years ago
So far no crashes in 44, but it'll take a while to be sure.

This is Browser shutdown (which shouldn;t be shutting this down on MainThread) which limits the risk considerably.
This likely needs some sort of access granted, yes.
The change would be low-but-not-no risk, and wouldn't get lots of testing.

I think we should hold off on this for ESR
Flags: needinfo?(rjesup)

Comment 7

3 years ago
Randell, it's been a few weeks now and I was reviewing possible fixes for esr 38.7 and wondering whether you still think we need to wait some more. If this has had enough bake time, please nominate for uplift to esr38.7. Thanks!
Flags: needinfo?(rjesup)
(Assignee)

Comment 8

3 years ago
Ok, giving it some time showed this was *not* fixed by bug 1218799, due to this 44.0.2 crash:
https://crash-stats.mozilla.com/report/index/7234c046-0e35-4543-8309-7e6862160216
Status: RESOLVED → REOPENED
status-firefox44: fixed → affected
status-firefox45: fixed → affected
status-firefox46: fixed → affected
Flags: needinfo?(rjesup)
Resolution: FIXED → ---
This is some kind of shutdown issue, so I'm going to assume that it would be hard to trigger from content in a controllable way, and lower it to sec-high. Feel free to switch it back if you think that's better.
Keywords: sec-critical → sec-high
(Assignee)

Comment 10

3 years ago
Andrew: the stack in question is in shutdown of a webrtc call, not browser shutdown.  Bug 1218799 should have resolved any browser-shutdown issue; apparently there's an additional race/UAF within the webrtc.org code.
(Assignee)

Comment 11

3 years ago
No reports after 44/44.0.2... and 45 has a major update to the webrtc.org code, including critical section and threading (see initial report).  Tentatively resolving as fixed; if we see any more mutexautolock errors in 45+ reopen
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
status-firefox44: affected → wontfix
status-firefox45: affected → fixed
status-firefox46: affected → fixed
status-firefox-esr38: affected → wontfix
Resolution: --- → FIXED
Flags: qe-verify-
Whiteboard: [adv-main44+] → [adv-main44+][post-critsmash-triage]
tracking-firefox-esr38: ? → -
Resolution: FIXED → WORKSFORME
(Assignee)

Comment 12

2 years ago
Note: still nothing after 44.0.2; the only report in the last month for this stack was from 44.0.2.  :-)
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.