The attached crash information was submitted via the ASan Nightly Reporter on mozilla-central-asan-nightly revision 63.0a1-20180724100052-https://hg.mozilla.org/mozilla-central/rev/1e5fa52a612e8985e12212d1950a732954e00e45. For detailed crash information, see attachment.

:gcp, could this be something for you to look at? Thanks!

This looks like the runnable in CamerasChild::GetCaptureDevice: nsCOMPtr<nsIRunnable> runnable = mozilla::NewRunnableMethod<CaptureEngine, unsigned int>( "camera::PCamerasChild::SendGetCaptureDevice", this, &CamerasChild::SendGetCaptureDevice, aCapEngine, list_number); |this| is presumably the dead pointer.

Bug 1426129 is a previous similar looking crash.

We probably need to hold a strong reference there for the length of life of the runnable.

I am noticing that this bug is marked as sec-high but has no assignee set nor any activity since it was filed 2 weeks ago, David could you help us find somebody to work on it please? Thanks!

Andreas, Paul, you guys fixed the previous case of this, could you take this one? I could take it if no-one else can but at this point the review (checking all the threading issues noted in bug 1426129) is going to take as long as the actual fix.

Tentatively taking, but I could do with someone stealing this from me :-)

(In reply to Alex Gaynor [:Alex_Gaynor] from comment #5) > We probably need to hold a strong reference there for the length of life of > the runnable. NewRunnableMethod already keeps a strong ref to the instance. CamerasChild::GetCaptureDevice must have been called on an already destroyed CamerasChild. All calls to GetCaptureDevice go through the static CamerasChild::GetChildAndCall. GetChildAndCall does operate on a rawptr, though behind a locked mutex: https://searchfox.org/mozilla-central/rev/83562422ecb0f5683da7a9a26ce05ce62bc0c882/dom/media/systemservices/CamerasChild.h#142-145 DeallocPCamerasChild which releases the CamerasChild in this case (thanks asan) doesn't grab this mutex: https://searchfox.org/mozilla-central/rev/83562422ecb0f5683da7a9a26ce05ce62bc0c882/ipc/glue/BackgroundChildImpl.cpp#401-403 This appears to be the hole we're looking for.

Comment on attachment 9002412 [details] [diff] [review] Unify CamerasChild shutdown paths [Security approval request comment] How easily could an exploit be constructed based on the patch? Very hard. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? Well, shutdown is bulls-eye but the specific shutdown mode needed here (see stack in comment 0) is unclear. We're not sure how to trigger this for repro either. Which older supported branches are affected by this flaw? All If not all supported branches, which bug introduced the flaw? - Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? I expect them to go cleanly. Relevant code appears to have been mostly stable (a few sec fixes) since way before 52. How likely is this patch to cause regressions; how much testing does it need? Unlikely, the added path is thread safe.

I'd love to take this but we only have a single beta left, building tomorrow. I need to ni? release management and get their approval.

We can take it if it hits m-c today still.

Comment on attachment 9002412 [details] [diff] [review] Unify CamerasChild shutdown paths Sec-approval+ for trunk. We'll need a beta patch and an ESR60 patch nominated ASAP to try to get tomorrow's final beta for 62.

https://hg.mozilla.org/integration/mozilla-inbound/rev/366a2aa802b5a7bd06328a7162f10292cbde3411 This grafts cleanly to Beta and ESR60 as-is, so it just needs the approval requests made after it's merged to m-c.

Comment on attachment 9002412 [details] [diff] [review] Unify CamerasChild shutdown paths [Approval Request Comment] If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-high User impact if declined: Possible crash/UAF during shutdown or after an ipc error. Errors are likely connected to shutdown. Fix Landed on Version: 63 Risk to taking this patch (and alternatives if risky): Low risk as it makes a rare shutdown path go through normal (and well-tested) shutdown. I haven't seen any big numbers for this crash in the wild. String or UUID changes made by this patch: None

When you find some time, it would be very helpful if you could answer the following questions so I can work on improving ASan Nightly further. In particular, many of the reports we saw about use-after-free were sent only once, not multiple times. So I'm asking myself what changes to ASan Nightly I can make to improve overall uaf detection rate: 1) Do you think more frequent GC/CC would have helped detecting this problem more frequently? 2) Do you think GC/CC on specific DOM events (e.g. beforeunload, pagehide, DOMContentLoaded) would have helped? 3) Do you think triggering the event loop in the specific child processes would have helped here? (e.g. dummy XMLHttpRequest onbeforeunload, useful in fuzzing). 4) Do you have any other ideas to reproduce this particular problem better or do you think any of the above points (or other approaches) might help to find other use-after-free issues in areas that you work in? Any feedback would be greatly appreciated. Thanks!

Comment on attachment 9002412 [details] [diff] [review] Unify CamerasChild shutdown paths Fixes a WebRTC sec-high. Approved for 62.0b20 and ESR 60.2.

decoder: I doubt this sort of problem is strongly influenced by GC/CC timing; this is in IPC and pure refcounted objects. This is influenced by thread timing, ordering, load, etc. some form of thread 'chaos' mode may expose more (see rr's chaos mode, etc). Especially timing around IPC; typically on multicore an IPC may not force an immediate yield by the caller; conversely a receiver may run immediately and finish before the caller has done "a lot". Ditto Dispatch() (perhaps even more in Dispatch). Having the sender or receiver occasionally (randomly) yield for a short time might increase the odds of an IPC/Dispatch-related UAF.