Closed Bug 1234576 Opened 5 years ago Closed 4 years ago
UAF in Return
SSRC() (in Mutex Auto Lock::Mutex Auto Lock)
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.
backlog: --- → webrtc/webaudio+
Priority: -- → P1
Randell, have you had a chance to look at this? Is there somebody else who might have time to work on it? Thanks.
[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
Closed: 4 years ago
Resolution: --- → FIXED
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?
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.
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
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!
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
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.
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.
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
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. :-)
You need to log in before you can comment on or make changes to this bug.