1309469 - Crash in nsXULElement::AddRef from webrtc::ViECaptureImpl::NumberOfCaptureDevices

Status: RESOLVED FIXED

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

Description

[Ting-Yu Chou [:ting] (away)]

This bug was filed from the Socorro interface and is 
report bp-52abff49-d362-41f2-afc3-2e16a2161010.
=============================================================
#44 of Nightly 20161109 on Windows, 3 crashes from 3 installations, low volume.

Comment 1

[Dan Minor [:dminor]]

See also Bug 1309456 which has a different signature but is in related code.

Comment 2

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Judging by the stack

> 6 	xul.dll 	mozilla::WidgetMouseEvent::Duplicate() 	obj-firefox/dist/include/mozilla/MouseEvents.h:268
> 7 	xul.dll 	webrtc::ViECaptureImpl::NumberOfCaptureDevices() 	media/webrtc/trunk/webrtc/video_engine/vie_capture_impl.cc:64

this looks like a UAF here: https://hg.mozilla.org/mozilla-central/annotate/7a7ba250bb2f/media/webrtc/trunk/webrtc/video_engine/vie_capture_impl.cc#l64

Jesup do you concur?

Comment 3

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

Yeah, that's not good... though it might be an artifact of the stack-walker.

Comment 5

[Daniel Veditz [:dveditz]]

Randell showed me how to search on "proto signature". Looking at the following list of crashes
https://crash-stats.mozilla.com/search/?proto_signature=~ViECaptureImpl&product=Firefox&version=52.0a1&version=51.0a2&version=50.0b&version=49.0.1&version=49.0&date=%3E%3D2016-10-07T16%3A14%3A00.000Z&date=%3C2016-10-14T16%3A14%3A00.000Z&_sort=build_id&_sort=-date&_facets=signature&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#crash-reports

The earliest build (in a quick scan, don't hold me to it) where the crashing function seems really out of place is nightly 20161007030207, so we might be looking for things checked in right before then.

bp-664bc31c-13fe-4ef7-907e-1f4d12161009
crashes in [@ NS_ReadOptionalObject ] after webrtc::ViEInputManager::RegisterObserver (frame 3) somehow jumps to nsPrincipal::Read.

In the same build there's also bp-45b70096-2f06-48e1-b67c-df47c2161008, crashing in [@ nsID::Equals ] after a call to webrtc::ViECaptureImpl::NumberOfCaptureDevices() jumps to nsStructuredCloneContainer::QueryInterface

and bp-7b10e091-07d4-47fd-8b12-e24a62161008, where webrtc::ViEInputManager::RegisterObserver jumps to nsNavHistoryResultNode::GetParent and crashes in [@ nsPresContext::AddRef ]

Comment 6

[Gian-Carlo Pascutto [:gcp]]

I looked through the code and I see one potential UAF:

https://hg.mozilla.org/mozilla-central/annotate/7be6b348c431/dom/media/systemservices/CamerasParent.cpp#l47

This issues a lambda-Runnable to run on the IPC thread, where "this" is InputObserver, a regular non-ref-counted class, that holds a ref-counted pointer to mParent (which is CamerasParent).

It's possible for CamerasParent::CloseEngines() to be called without any guarantees as to which runnables are still outstanding in the IPC thread. CloseEngines() will wipe out all InputObservers:
https://hg.mozilla.org/mozilla-central/annotate/7be6b348c431/dom/media/systemservices/CamerasParent.cpp#l486 This will deref CameraParent as the RefPtr inside InputObserver gets destructed. 

At this point CamerasParent is free to be destroyed, but the state of the original runnable is still not guaranteed. If it runs after the CamerasParent destruction, it will try to deref this (pointing to a freed object) and/or this->mParent (also pointing to a freed object, a double-indirect UAF).

The solution can be seen in other parts of the code: the Runnable *itself* should take along a reference (pointer) to CamerasParent in the lambda capture list, i.e. something like:

  RefPtr<CamerasParent> parent(this->mParent);
  RefPtr<nsIRunnable> ipc_runnable =
    media::NewRunnableFrom([parent]() -> nsresult {
      if (parent->IsShuttingDown()) {
etc.	

InputObserver seems to have intended to take a RefPtr to CamerasParent exactly to avoid this kind of bug, but itself not being a reference counted object, there's no way for it to get this right across threads.

Comment 7

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

Munro has a fix for the RegisterObserver bug on another bug that's ready to land.  We should add a fix for what gcp noticed.  However, I'm not sure either of those can cause what we see here.

Comment 8

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

Munro - any analysis/thoughts on the above?

Comment 9

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

(In reply to Daniel Veditz [:dveditz] from comment #5)
> Randell showed me how to search on "proto signature". Looking at the
> following list of crashes
> https://crash-stats.mozilla.com/search/
> ?proto_signature=~ViECaptureImpl&product=Firefox&version=52.0a1&version=51.
> 0a2&version=50.0b&version=49.0.1&version=49.0&date=%3E%3D2016-10-
> 07T16%3A14%3A00.000Z&date=%3C2016-10-14T16%3A14%3A00.
> 000Z&_sort=build_id&_sort=-
> date&_facets=signature&_columns=date&_columns=signature&_columns=product&_col
> umns=version&_columns=build_id&_columns=platform#crash-reports
> 
> The earliest build (in a quick scan, don't hold me to it) where the crashing
> function seems really out of place is nightly 20161007030207, so we might be
> looking for things checked in right before then.
> 
> bp-664bc31c-13fe-4ef7-907e-1f4d12161009
> crashes in [@ NS_ReadOptionalObject ] after
> webrtc::ViEInputManager::RegisterObserver (frame 3) somehow jumps to
> nsPrincipal::Read.
> 
> In the same build there's also bp-45b70096-2f06-48e1-b67c-df47c2161008,
> crashing in [@ nsID::Equals ] after a call to
> webrtc::ViECaptureImpl::NumberOfCaptureDevices() jumps to
> nsStructuredCloneContainer::QueryInterface
> 
> and bp-7b10e091-07d4-47fd-8b12-e24a62161008, where
> webrtc::ViEInputManager::RegisterObserver jumps to
> nsNavHistoryResultNode::GetParent and crashes in [@ nsPresContext::AddRef ]
webrtc::ViEInputManager::RegisterObserver() crash has been addressed in bug 1308792.

webrtc::ViECaptureImpl::NumberOfCaptureDevices() crash may be caused by a UAF here:
https://dxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/webrtc/video_engine/vie_input_manager.cc#65. Probably compiler makes the function body inline due to optimization, so the call stack doesn't show it. If my theory is true, then this is also caused by bug 1308792.

I keep monitoring the crash log of webrtc::ViECaptureImpl::NumberOfCaptureDevices() and webrtc::ViECaptureImpl::GetCaptureDevice().

Both of them don't occur after we landed the fix for bug 1308792 (The earliest nightly build containing the fix is 20161018030211)

I will file another bug for the UAF gcp pointed out.

Comment 10

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

I was wrong. Keep analyzing.

Comment 11

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Attachment: Bug-1309469-Replace-the-singleton-design.patch

Below is the crash list after the last patch of bug 1308792 landed.

https://crash-stats.mozilla.com/search/?proto_signature=~ViECaptureImpl&build_id=%3E20161018030204&product=Firefox&version=52.0a1&date=%3E%3D2016-10-10T13%3A26%3A00.000Z&_sort=-build_id&_sort=-date&_facets=signature&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#crash-reports

Since all these crashes only occur on Windows, it should be caused by some code change in Windows-specific module.

I think the singleton I use to implement ondevicechange for Windows is fragile.
https://hg.mozilla.org/mozilla-central/rev/18722be58f18#l2.35

As long as there is one unexpected flow releasing the device info object, all other objects pointing to it will hit UAF.

So I decide to revert the design back to the original one. multiple deviceInfoDS instances will be created and managed by each client.

Comment 12

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b59b820be957

Comment 13

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8803820 [details] [diff] [review]
Bug-1309469-Replace-the-singleton-design.patch

Review of attachment 8803820 [details] [diff] [review]:
-----------------------------------------------------------------

For a security bug, do we want less info in the commit message pinpointing the problem?

Comment 14

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Attachment: Bug-1309469-Crash-webrtc-ViECaptureImpl-NumberOfCaptureDevices.patch

modify commit message
carry r+ from jesup

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/148057da1d5edd89e404ea5af27a7d5de2ed20f0

Comment 16

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/148057da1d5e

Comment 17

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

Better search for failures to verify this:
https://crash-stats.mozilla.com/search/?proto_signature=~ViECaptureImpl%3A%3ANumberOfCaptureDevices&product=Firefox&date=%3E%3D2016-10-19T20%3A44%3A00.000Z&date=%3C2016-10-26T20%3A44%3A00.000Z&_sort=-date&_facets=signature&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#facet-signature

Comment 18

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

Dropping to sec-high given signature and frequency

Comment 19

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

Actually, this was the wrong bug.  back to crit (but it's fixed in any case)