1515461 - Crash in webrtc::videocapturemodule::VideoCaptureImpl::IncomingFrame

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P2)

Description

[[:philipp]]

This bug was filed from the Socorro interface and is
report bp-1f845ed2-fabd-4c01-8c85-9d0740181219.
=============================================================

Top 10 frames of crashing thread:

0 XUL webrtc::videocapturemodule::VideoCaptureImpl::IncomingFrame media/webrtc/trunk/webrtc/modules/video_capture/video_capture_impl.cc:129
1 CoreMedia bufQRemoveElementAtIndex 
2 libmozglue.dylib arena_t::DallocSmall memory/build/rb.h:135
3 CoreVideo _getCVPixelBuffer 
4 XUL -[RTCVideoCaptureIosObjC captureOutput:didOutputSampleBuffer:fromConnection:] media/webrtc/trunk/webrtc/modules/video_capture/objc/rtc_video_capture_objc.mm:338
5 AVFoundation GCC_except_table3 
6 libobjc.A.dylib lookUpImpOrNil 
7 AVFoundation GCC_except_table3 
8 AppKit AppKit@0xc69333 
9 AppKit AppKit@0xc655ac 

=============================================================

this crash signature is (re)appearing on macos since firefox 65 - proably related to the changes in bug 1376873

Comment 1

[Dan Minor [:dminor]]

Looks like with the update I once again accidentally switched to the newer videocapture implementation for OS X, and this is Bug 1438554 reappearing. I'll prepare a patch to revert. We really need to fix Bug 1439997 prior to doing another update.

Comment 2

[Dan Minor [:dminor]]

Unfortunately it looks the old video capture implementation is now old enough that it is no longer straightforward to switch back to it, and we're probably better off just fixing the crash in the new implementation.

Comment 3

[Dan Minor [:dminor]]

Setting as a sec bug as it looks like this results in a UAF.

Comment 4

[Dan Minor [:dminor]]

I think the situation is this: Currently in stopCapture the call to AVCaptureSession stopRunning is dispatched asynchronously to a separate dispatch queue [1]. The stopRunning call is a blocking call, so this allows stopCapture to return immediately. In CamerasParent, right after the call to StopCaptureIfAllClientsClose we erase the CaptureEntry which holds a reference to the VideoCaptureIos instance that we've passed into the RTCVideoCaptureIosObjC instance at [3] and is stored as a raw pointer at [4]. If we end up freeing the VideoCaptureIos instance prior to stopRunning executing, we end up with a UAF at [5].

[1] https://searchfox.org/mozilla-central/rev/9528360768d81b1fc84258b5fb3601b5d4f40076/media/webrtc/trunk/webrtc/modules/video_capture/objc/rtc_video_capture_objc.mm#249
[2] https://searchfox.org/mozilla-central/rev/232ced2697b8938073fa79b8e6aa3718876c0b69/dom/media/systemservices/CamerasParent.cpp#1001
[3] https://searchfox.org/mozilla-central/rev/232ced2697b8938073fa79b8e6aa3718876c0b69/media/webrtc/trunk/webrtc/modules/video_capture/objc/video_capture.mm#62
[4] https://searchfox.org/mozilla-central/rev/9528360768d81b1fc84258b5fb3601b5d4f40076/media/webrtc/trunk/webrtc/modules/video_capture/objc/rtc_video_capture_objc.mm#33
[5] https://searchfox.org/mozilla-central/rev/9528360768d81b1fc84258b5fb3601b5d4f40076/media/webrtc/trunk/webrtc/modules/video_capture/objc/rtc_video_capture_objc.mm#338

Comment 5

[Dan Minor [:dminor]]

Attachment: Bug 1515461 - Make AVCaptureSession stopRunning synchronous; r=jib

Comment 6

[Dan Minor [:dminor]]

Comment on attachment 9032801 [details]
Bug 1515461 - Make AVCaptureSession stopRunning synchronous; r=jib

[Security Approval Request]

How easily could an exploit be constructed based on the patch?: The patch makes an asynchronous call synchronous which would give an attacker a hint that we had a race condition before, but I don't think the patch makes it obvious where the actual problem was.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No

Which older supported branches are affected by this flaw?: 65

If not all supported branches, which bug introduced the flaw?: Bug 1376873

Do you have backports for the affected branches?: No

If not, how different, hard to create, and risky will they be?: The patch for Nightly will apply cleanly.

How likely is this patch to cause regressions; how much testing does it need?: There is a risk that moving the stopRunning call to the main thread could cause problems when stopping and restarting the camera repeatedly. I've tested for that manually and have not noticed any problems.

Comment 7

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

Comment on attachment 9032801 [details]
Bug 1515461 - Make AVCaptureSession stopRunning synchronous; r=jib

sec-approval+. We'll want this on Beta as well.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/autoland/rev/01d3cd0517a1

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/01d3cd0517a1

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta approval when you get a chance. It grafts cleanly as-landed.

Comment 11

[Dan Minor [:dminor]]

Comment on attachment 9032801 [details]
Bug 1515461 - Make AVCaptureSession stopRunning synchronous; r=jib

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1376873

User impact if declined: Crashes and potential UAF during screen sharing on OS X

Is this code covered by automated tests?: No

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): This makes what was previously a synchronous call asynchronous. There is a chance that could cause problems when stopping and starting screen sharing repeatedly. I've tested for that manually and not seen any problems, so this is low risk.

String changes made/needed: None

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9032801 [details]
Bug 1515461 - Make AVCaptureSession stopRunning synchronous; r=jib

[Triage Comment]
Fixes a WebRTC sec high. Approved for 65.0b8.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d85d79f64753