Closed Bug 1309469 Opened 4 years ago Closed 4 years ago

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

Categories

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

Unspecified
Windows 7
defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox-esr45 --- unaffected
firefox51 --- unaffected
firefox52 + fixed

People

(Reporter: ting, Assigned: mchiang)

References

Details

(Keywords: crash, csectype-uaf, sec-critical, Whiteboard: [post-critsmash-triage])

Crash Data

Attachments

(1 file, 1 obsolete file)

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.
See also Bug 1309456 which has a different signature but is in related code.
Rank: 15
Component: WebRTC → WebRTC: Audio/Video
Priority: -- → P1
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?
Group: core-security
Yeah, that's not good... though it might be an artifact of the stack-walker.
Rank: 15 → 10
Duplicate of this bug: 1309456
Assignee: nobody → mchiang
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 ]
Group: core-security → media-core-security
Crash Signature: [@ nsXULElement::AddRef] → [@ nsXULElement::AddRef] [@ nsPresContext::AddRef ] [@ NS_ReadOptionalObject ] [@ nsID::Equals ]
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.
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.
Munro - any analysis/thoughts on the above?
Flags: needinfo?(mchiang)
(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.
Flags: needinfo?(mchiang)
I was wrong. Keep analyzing.
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.
Attachment #8803820 - Flags: review?(rjesup)
Attachment #8803820 - Flags: review?(jib)
Attachment #8803820 - Flags: review?(rjesup) → review+
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?
Attachment #8803820 - Flags: review?(jib)
modify commit message
carry r+ from jesup
Attachment #8803820 - Attachment is obsolete: true
Attachment #8804088 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/148057da1d5e
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Dropping to sec-high given signature and frequency
Keywords: sec-criticalsec-high
Actually, this was the wrong bug.  back to crit (but it's fixed in any case)
Keywords: sec-highsec-critical
Group: media-core-security → core-security-release
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.