1478575 - AddressSanitizer: heap-use-after-free [@ Id] with READ of size 4 through [@ mozilla::camera::PCamerasChild::SendGetCaptureDevice]

Status: RESOLVED FIXED

(Core :: WebRTC: Audio/Video, defect, P1)

Description

[Christian Holler (:decoder)]

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.

Comment 1

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 2

[Christian Holler (:decoder)]

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

Comment 3

[Andrew McCreight [:mccr8]]

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.

Comment 4

[Andrew McCreight [:mccr8]]

Bug 1426129 is a previous similar looking crash.

Comment 5

[Alex Gaynor [:Alex_Gaynor]]

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

Comment 6

[Pascal Chevrel:pascalc (PTO until April 26)]

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!

Comment 7

[Gian-Carlo Pascutto [:gcp]]

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.

Comment 8

[Paul Adenot (:padenot)]

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

Comment 9

[Andreas Pehrson [:pehrsons]]

(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 10

[Andreas Pehrson [:pehrsons]]

Attachment: Unify CamerasChild shutdown paths

Comment 11

[Andreas Pehrson [:pehrsons]]

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.

Comment 12

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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

Comment 14

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/366a2aa802b5

Comment 17

[Andreas Pehrson [:pehrsons]]

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

Comment 18

[Christian Holler (:decoder)]

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 19

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9002412 [details] [diff] [review]
Unify CamerasChild shutdown paths

Fixes a WebRTC sec-high. Approved for 62.0b20 and ESR 60.2.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0d676b757477

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/12213cfa93d9

Comment 22

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

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.