Closed Bug 1478575 Opened 3 years ago Closed 3 years ago

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


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




Tracking Status
firefox-esr52 --- wontfix
firefox-esr60 62+ fixed
firefox61 --- wontfix
firefox62 + fixed
firefox63 + fixed


(Reporter: decoder, Assigned: pehrsons)


(Blocks 1 open bug)


(4 keywords, Whiteboard: [adv-main62+][adv-esr60.2+][post-cristsmash-triage])


(2 files)

The attached crash information was submitted via the ASan Nightly Reporter on mozilla-central-asan-nightly revision 63.0a1-20180724100052-

For detailed crash information, see attachment.
Group: core-security
:gcp, could this be something for you to look at? Thanks!
Flags: needinfo?(gpascutto)
This looks like the runnable in CamerasChild::GetCaptureDevice:
  nsCOMPtr<nsIRunnable> runnable =
    mozilla::NewRunnableMethod<CaptureEngine, unsigned int>(

|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.
Group: core-security → media-core-security
No longer blocks: asan-nightly-project
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!
Flags: needinfo?(ddurst)
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.
Flags: needinfo?(padenot)
Flags: needinfo?(gpascutto)
Flags: needinfo?(apehrson)
Tentatively taking, but I could do with someone stealing this from me :-)
Assignee: nobody → padenot
Flags: needinfo?(padenot)
(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:

DeallocPCamerasChild which releases the CamerasChild in this case (thanks asan) doesn't grab this mutex:

This appears to be the hole we're looking for.
Assignee: padenot → apehrson
Component: Audio/Video → WebRTC: Audio/Video
Flags: needinfo?(apehrson)
Priority: -- → P1
Attachment #9002412 - Flags: review?(gpascutto)
Flags: needinfo?(ddurst)
Attachment #9002412 - Flags: review?(gpascutto) → review+
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?

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.
Attachment #9002412 - Flags: sec-approval?
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.
Flags: needinfo?(rkothari)
Flags: needinfo?(lhenry)
Flags: needinfo?(jcristau)
We can take it if it hits m-c today still.
Flags: needinfo?(rkothari)
Flags: needinfo?(lhenry)
Flags: needinfo?(jcristau)
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.
Flags: needinfo?(apehrson)
Attachment #9002412 - Flags: sec-approval? → sec-approval+

This grafts cleanly to Beta and ESR60 as-is, so it just needs the approval requests made after it's merged to m-c.
Group: media-core-security → core-security-release
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
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
Flags: needinfo?(apehrson)
Attachment #9002412 - Flags: approval-mozilla-esr60?
Attachment #9002412 - Flags: approval-mozilla-beta?
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!
Flags: needinfo?(apehrson)
Comment on attachment 9002412 [details] [diff] [review]
Unify CamerasChild shutdown paths

Fixes a WebRTC sec-high. Approved for 62.0b20 and ESR 60.2.
Attachment #9002412 - Flags: approval-mozilla-esr60?
Attachment #9002412 - Flags: approval-mozilla-esr60+
Attachment #9002412 - Flags: approval-mozilla-beta?
Attachment #9002412 - Flags: approval-mozilla-beta+
Whiteboard: [adv-main62+][adv-esr60.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.
Flags: needinfo?(choller)
Flags: qe-verify-
Whiteboard: [adv-main62+][adv-esr60.2+] → [adv-main62+][adv-esr60.2+][post-cristsmash-triage]
Flags: needinfo?(choller)
Flags: needinfo?(apehrson)
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.