Closed Bug 1234576 Opened 8 years ago Closed 8 years ago

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


(Core :: WebRTC, defect, P1)




Tracking Status
firefox42 --- wontfix
firefox43 --- wontfix
firefox44 --- wontfix
firefox45 --- fixed
firefox46 --- fixed
firefox-esr38 - wontfix
firefox-esr45 --- fixed


(Reporter: jesup, Assigned: jesup)


(Keywords: crash, csectype-uaf, sec-high, Whiteboard: [adv-main44+][post-critsmash-triage])

Crash Data

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 43 update, which changes the threads things are accessed on.
backlog: --- → webrtc/webaudio+
Rank: 15
Priority: -- → P1
Group: media-core-security
Assignee: nobody → rjesup
Group: core-security
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)
[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.
Closed: 8 years ago
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.
[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.
Flags: needinfo?(rjesup)
Flags: needinfo?(dveditz)
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)
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)
Ok, giving it some time showed this was *not* fixed by bug 1218799, due to this 44.0.2 crash:
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-criticalsec-high
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 code.
No reports after 44/44.0.2... and 45 has a major update to the code, including critical section and threading (see initial report).  Tentatively resolving as fixed; if we see any more mutexautolock errors in 45+ reopen
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Flags: qe-verify-
Whiteboard: [adv-main44+] → [adv-main44+][post-critsmash-triage]
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.