1234576 - UAF in ReturnSSRC() (in MutexAutoLock::MutexAutoLock)

Status: RESOLVED WORKSFORME

(Core :: WebRTC, defect, P1)

Description

[Randell Jesup [:jesup] (needinfo me)]

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.

Comment 1

[Andrew McCreight [:mccr8]]

Randell, have you had a chance to look at this? Is there somebody else who might have time to work on it? Thanks.

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

[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.

Comment 3

[Liz Henry (:lizzard) (relman/hg->git project)]

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?

Comment 4

[Liz Henry (:lizzard) (relman/hg->git project)]

After talking with Dan, it seems best to wontfix this for esr38, too risky.

Comment 5

[Daniel Veditz [:dveditz]]

[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.

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 7

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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!

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 9

[Andrew McCreight [:mccr8]]

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.

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

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.

Comment 11

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

Note: still nothing after 44.0.2; the only report in the last month for this stack was from 44.0.2.  :-)