This issue is a security regression. If you are not able to fix this quickly, please revert the change that introduced it.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Guessing https://codereview.chromium.org/2395163002 based on regression range

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/92d59c5c7195bc11f72c6d2a965cbe8ea571e455

commit 92d59c5c7195bc11f72c6d2a965cbe8ea571e455
Author: mcasas <mcasas@chromium.org>
Date: Mon Oct 10 19:53:17 2016

VideoCaptureHost: Remove |controllers_| entry before calling VideoCaptureManager::StopCaptureForClient() to prevent a race

The destruction sequence in case of error implies
several walks around VideoCaptureManager and
MediaStreamManager. I believe the changes introduced
by mojo have made the UAF in the bug more evident.

In any case, this CL prevents that cycle by removing
the entry from |controllers_| before requesting
something from the VideoCaptureManager.

BUG= 654199 

Review-Url: https://codereview.chromium.org/2408853002
Cr-Commit-Position: refs/heads/master@{#424210}

[modify] https://crrev.com/92d59c5c7195bc11f72c6d2a965cbe8ea571e455/content/browser/renderer_host/media/video_capture_host.cc

Fixed, maybe

Issue 654935 has been merged into this issue.

Copy of https://bugs.chromium.org/p/chromium/issues/detail?id=654935#c4 :

"Please rerun the test case by clicking "Redo" 
https://cluster-fuzz.appspot.com/testcase?key=6385800625520640"

Copy of https://bugs.chromium.org/p/chromium/issues/detail?id=654935#c5

FTR: #1 did not solve completely the case, but I managed to repro 
locally by using the diff lines below and running [1] and I'm
whipping up a CL .


[1] https://codepen.io/miguelao/pen/amjEaY




diff --git a/content/browser/renderer_host/media/video_capture_controller.cc b/content/browser/renderer_host/media/video_capture_controller.cc
index 85f0a29..86bc132 100644
--- a/content/browser/renderer_host/media/video_capture_controller.cc
+++ b/content/browser/renderer_host/media/video_capture_controller.cc
@@ -222,8 +222,8 @@ void VideoCaptureController::AddClient(
       params.requested_format.pixel_storage != media::PIXEL_STORAGE_CPU) {
     // Crash in debug builds since the renderer should not have asked for
     // invalid or unsupported parameters.
-    LOG(DFATAL) << "Invalid or unsupported video capture parameters requested: "
-                << media::VideoCaptureFormat::ToString(params.requested_format);
+    //LOG(DFATAL) << "Invalid or unsupported video capture parameters requested: "
+    //            << media::VideoCaptureFormat::ToString(params.requested_format);
     event_handler->OnError(id);
     return;
   }
diff --git a/content/browser/renderer_host/media/video_capture_host.cc b/content/browser/renderer_host/media/video_capture_host.cc
index d37dc46..3127d92 100644
--- a/content/browser/renderer_host/media/video_capture_host.cc
+++ b/content/browser/renderer_host/media/video_capture_host.cc
@@ -160,10 +160,13 @@ void VideoCaptureHost::Start(int32_t device_id,
     return;
   }
 
+  auto params2 = params;
+  params2.requested_format.frame_rate = -1.0;
+
   controllers_[controller_id] = base::WeakPtr<VideoCaptureController>();
   media_stream_manager_->video_capture_manager()->StartCaptureForClient(
       session_id,
-      params,
+      params2,
       PeerHandle(),
       controller_id,
       this,

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/abb68a36070c6f5a183c8db994a3f1f9e250d16f

commit abb68a36070c6f5a183c8db994a3f1f9e250d16f
Author: mcasas <mcasas@chromium.org>
Date: Sat Oct 15 01:07:34 2016

VideoCaptureManager: fixed double-deletion bug in StopCaptureForClient() on error

VideoCaptureManager::StopCaptureForClient() [1], when called in error
condition, will run, in the beginning,
  DeviceEntry* entry = GetDeviceEntryByController(controller);
but then, if |aborted_due_to_error|, it would also call (l.719 [2])
synchronuosly:

 listener_->Aborted(...);
  MediaStreamManager::StopDevice(...)
   MediaStreamManager::CloseDevice(...);
    VideoCaptureManager::Close(...);
     VideoCaptureManager::DestroyDeviceIfNoClient(...);

the last one will invalidate |entry| and destroy the |controller|
needed in l.727 [3], hence the UAF.

This bug was there before the blamed CLs (see bug), but the
migration to mojo has made this path more possible for Cluster
Fuzz to exercise. Note: I have no clue how ClusterFuzz managed
this, I had to patch the code to cause this error condition :)

[1] https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/video_capture_manager.cc?q=VideoCaptureManager::StopCaptureForClient&sq=package:chromium&l=689&dr=CSs
[2] https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/video_capture_manager.cc?q=VideoCaptureManager::StopCaptureForClient&sq=package:chromium&dr=CSs&l=719
[3] https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/video_capture_manager.cc?q=VideoCaptureManager::StopCaptureForClient&sq=package:chromium&dr=CSs&l=727

BUG= 654199 
TEST= see bug for repro; doesn't crash with the patch, capture
device is still correctly closed.

Review-Url: https://codereview.chromium.org/2417383002
Cr-Commit-Position: refs/heads/master@{#425525}

[modify] https://crrev.com/abb68a36070c6f5a183c8db994a3f1f9e250d16f/content/browser/renderer_host/media/video_capture_manager.cc

ClusterFuzz has detected this issue as fixed in range 425442:425531.

Detailed report: https://cluster-fuzz.appspot.com/testcase?key=5650736920920064

Fuzzer: phoglund_webrtc_peerconnection
Job Type: linux_asan_chrome_chromeos
Platform Id: linux

Crash Type: Heap-use-after-free READ 8
Crash Address: 0x6070003dbab0
Crash State:
  content::VideoCaptureController::RemoveClient
  content::VideoCaptureManager::StopCaptureForClient
  content::VideoCaptureHost::DeleteVideoCaptureController
  
Recommended Security Severity: Critical

Regressed: https://cluster-fuzz.appspot.com/revisions?job=linux_asan_chrome_chromeos&range=423512:423881
Fixed: https://cluster-fuzz.appspot.com/revisions?job=linux_asan_chrome_chromeos&range=425442:425531

Minimized Testcase (0.93 Kb): https://cluster-fuzz.appspot.com/download/AMIfv97TRTZC2ZxGdEyZ299fYWj4vy6g8mTiBMRHEoUjMc8VBRjcx08ZXoXLYb_z2ZDD5ZvRNYzD33dI-Jc-sMK-TchaN-fdERw3EgCeeUSvY-EDwJStwu7DNkmmw4P5pZvsQM_O03k1HByjoUI9i9OzmbB0VdDmCQ?testcase_id=5650736920920064

Additional requirements: Requires HTTP

See https://dev.chromium.org/Home/chromium-security/bugs/reproducing-clusterfuzz-bugs for more information.

If you suspect that the result above is incorrect, try re-doing that job on the test case report page.

ClusterFuzz testcase is verified as fixed, closing issue.

If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue.

Approving merge to M55 branch 2883 based on comment #14 and per chat with  awhalley@. Please merge ASAP. Thank you.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0b03d060131a750921842d19360fe4c6ce50055a

commit 0b03d060131a750921842d19360fe4c6ce50055a
Author: mcasas <mcasas@chromium.org>
Date: Tue Oct 25 22:17:26 2016

VideoCaptureManager: fixed double-deletion bug in StopCaptureForClient() on error

VideoCaptureManager::StopCaptureForClient() [1], when called in error
condition, will run, in the beginning,
  DeviceEntry* entry = GetDeviceEntryByController(controller);
but then, if |aborted_due_to_error|, it would also call (l.719 [2])
synchronuosly:

 listener_->Aborted(...);
  MediaStreamManager::StopDevice(...)
   MediaStreamManager::CloseDevice(...);
    VideoCaptureManager::Close(...);
     VideoCaptureManager::DestroyDeviceIfNoClient(...);

the last one will invalidate |entry| and destroy the |controller|
needed in l.727 [3], hence the UAF.

This bug was there before the blamed CLs (see bug), but the
migration to mojo has made this path more possible for Cluster
Fuzz to exercise. Note: I have no clue how ClusterFuzz managed
this, I had to patch the code to cause this error condition :)

[1] https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/video_capture_manager.cc?q=VideoCaptureManager::StopCaptureForClient&sq=package:chromium&l=689&dr=CSs
[2] https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/video_capture_manager.cc?q=VideoCaptureManager::StopCaptureForClient&sq=package:chromium&dr=CSs&l=719
[3] https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/video_capture_manager.cc?q=VideoCaptureManager::StopCaptureForClient&sq=package:chromium&dr=CSs&l=727

BUG= 654199 
TEST= see bug for repro; doesn't crash with the patch, capture
device is still correctly closed.

Review-Url: https://codereview.chromium.org/2417383002
Cr-Commit-Position: refs/heads/master@{#425525}
(cherry picked from commit abb68a36070c6f5a183c8db994a3f1f9e250d16f)

NOTRY=true
NOPRESUBMIT=true

Review-Url: https://codereview.chromium.org/2447263002
Cr-Commit-Position: refs/branch-heads/2883@{#301}
Cr-Branched-From: 614d31daee2f61b0180df403a8ad43f20b9f6dd7-refs/heads/master@{#423768}

[modify] https://crrev.com/0b03d060131a750921842d19360fe4c6ce50055a/content/browser/renderer_host/media/video_capture_manager.cc

Is this need a merge to M54? If yes, please request a merge to M54.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0b03d060131a750921842d19360fe4c6ce50055a

commit 0b03d060131a750921842d19360fe4c6ce50055a
Author: mcasas <mcasas@chromium.org>
Date: Tue Oct 25 22:17:26 2016

VideoCaptureManager: fixed double-deletion bug in StopCaptureForClient() on error

VideoCaptureManager::StopCaptureForClient() [1], when called in error
condition, will run, in the beginning,
  DeviceEntry* entry = GetDeviceEntryByController(controller);
but then, if |aborted_due_to_error|, it would also call (l.719 [2])
synchronuosly:

 listener_->Aborted(...);
  MediaStreamManager::StopDevice(...)
   MediaStreamManager::CloseDevice(...);
    VideoCaptureManager::Close(...);
     VideoCaptureManager::DestroyDeviceIfNoClient(...);

the last one will invalidate |entry| and destroy the |controller|
needed in l.727 [3], hence the UAF.

This bug was there before the blamed CLs (see bug), but the
migration to mojo has made this path more possible for Cluster
Fuzz to exercise. Note: I have no clue how ClusterFuzz managed
this, I had to patch the code to cause this error condition :)

[1] https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/video_capture_manager.cc?q=VideoCaptureManager::StopCaptureForClient&sq=package:chromium&l=689&dr=CSs
[2] https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/video_capture_manager.cc?q=VideoCaptureManager::StopCaptureForClient&sq=package:chromium&dr=CSs&l=719
[3] https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/video_capture_manager.cc?q=VideoCaptureManager::StopCaptureForClient&sq=package:chromium&dr=CSs&l=727

BUG= 654199 
TEST= see bug for repro; doesn't crash with the patch, capture
device is still correctly closed.

Review-Url: https://codereview.chromium.org/2417383002
Cr-Commit-Position: refs/heads/master@{#425525}
(cherry picked from commit abb68a36070c6f5a183c8db994a3f1f9e250d16f)

NOTRY=true
NOPRESUBMIT=true

Review-Url: https://codereview.chromium.org/2447263002
Cr-Commit-Position: refs/branch-heads/2883@{#301}
Cr-Branched-From: 614d31daee2f61b0180df403a8ad43f20b9f6dd7-refs/heads/master@{#423768}

[modify] https://crrev.com/0b03d060131a750921842d19360fe4c6ce50055a/content/browser/renderer_host/media/video_capture_manager.cc

govind@ do I need to do something, after #24?
FTR, although the bug was there, it was the IPC->Mojo
migration that made it surface in the clusterfuzz. This
migration was started after M54 branch IIRC.

#24: As per mmoss@, was an accidental branch push, since fixed, but it was noticed by bugdroid and commitsentry, which thought a bunch of new commits landed on 2840 (+mmoss@). Removing "merge-merged-2840" label.

#25: awhalley@ (Security TPM) can comment.

[Automated comment] removing mislabelled merge-merged-2840

[Automated comment] removing mislabelled merge-merged-2840

This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot