From 17fe88a2d7da7d6150fdcbcb764d4f029da4f3f7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Bostr=C3=B6m?= Date: Tue, 14 Mar 2023 13:07:19 +0000 Subject: [Backport] CVE-2023-1218: Use after free in WebRTC MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cherry-pick of patch originally reviewed on https://chromium-review.googlesource.com/c/chromium/src/+/4291513: Shutdown RtpContributingSourceCache in Dispose(). The cache is an off-heap object, but it is owned by an on-heap object (RTCPeerConnection). Dispoing the owning object poisons memory owned by it, but the cache may have in-flight tasks (cache doing ClearCache in a delayed microtask). This CL adds a Shutdown() method to ensure the cache isn't doing anything in the next microtask after disposal. No reliable way to repro this has been found but the change should be safe so hoping we can land without tests. (cherry picked from commit 4d450ecd6ec7776c7505dcf7d2f04157ff3ba0eb) Bug: 1413628 Change-Id: I479aace9859f4c10cd75d4aa5a34808b4726299d Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4247023 Commit-Queue: Henrik Boström Cr-Original-Commit-Position: refs/heads/main@{#1105653} Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4291513 Reviewed-by: Achuith Bhandarkar Owners-Override: Achuith Bhandarkar Reviewed-by: Henrik Boström Commit-Queue: Zakhar Voit Cr-Commit-Position: refs/branch-heads/5359@{#1404} Cr-Branched-From: 27d3765d341b09369006d030f83f582a29eb57ae-refs/heads/main@{#1058933} Reviewed-on: https://codereview.qt-project.org/c/qt/qtwebengine-chromium/+/468219 Reviewed-by: Michal Klocek --- .../renderer/modules/peerconnection/rtc_peer_connection.cc | 12 +++++++++--- .../modules/peerconnection/rtp_contributing_source_cache.cc | 4 ++++ .../modules/peerconnection/rtp_contributing_source_cache.h | 4 ++++ 3 files changed, 17 insertions(+), 3 deletions(-) diff --git a/chromium/third_party/blink/renderer/modules/peerconnection/rtc_peer_connection.cc b/chromium/third_party/blink/renderer/modules/peerconnection/rtc_peer_connection.cc index 636aa342529..be9196ee034 100644 --- a/chromium/third_party/blink/renderer/modules/peerconnection/rtc_peer_connection.cc +++ b/chromium/third_party/blink/renderer/modules/peerconnection/rtc_peer_connection.cc @@ -893,12 +893,18 @@ RTCPeerConnection::~RTCPeerConnection() { } void RTCPeerConnection::Dispose() { - // Promptly clears the handler - // so that content/ doesn't access it in a lazy sweeping phase. - // Other references to the handler use a weak pointer, preventing access. + // Promptly clears the handler so that content doesn't access it in a lazy + // sweeping phase. Other references to the handler use a weak pointer, + // preventing access. if (peer_handler_) { peer_handler_.reset(); } + // Memory owned by RTCPeerConnection must not be touched after Dispose(). + // Shut down the cache to cancel any in-flight tasks that may otherwise have + // used the cache. + if (rtp_contributing_source_cache_.has_value()) { + rtp_contributing_source_cache_.value().Shutdown(); + } } ScriptPromise RTCPeerConnection::createOffer(ScriptState* script_state, diff --git a/chromium/third_party/blink/renderer/modules/peerconnection/rtp_contributing_source_cache.cc b/chromium/third_party/blink/renderer/modules/peerconnection/rtp_contributing_source_cache.cc index a52ddd405eb..d4b21570e64 100644 --- a/chromium/third_party/blink/renderer/modules/peerconnection/rtp_contributing_source_cache.cc +++ b/chromium/third_party/blink/renderer/modules/peerconnection/rtp_contributing_source_cache.cc @@ -100,6 +100,10 @@ RtpContributingSourceCache::RtpContributingSourceCache( DCHECK(worker_thread_runner_); } +void RtpContributingSourceCache::Shutdown() { + weak_factory_.InvalidateWeakPtrs(); +} + HeapVector> RtpContributingSourceCache::getSynchronizationSources( ScriptState* script_state, diff --git a/chromium/third_party/blink/renderer/modules/peerconnection/rtp_contributing_source_cache.h b/chromium/third_party/blink/renderer/modules/peerconnection/rtp_contributing_source_cache.h index b8d78d80aee..827f5eb3524 100644 --- a/chromium/third_party/blink/renderer/modules/peerconnection/rtp_contributing_source_cache.h +++ b/chromium/third_party/blink/renderer/modules/peerconnection/rtp_contributing_source_cache.h @@ -43,6 +43,10 @@ class RtpContributingSourceCache { RTCPeerConnection* pc, scoped_refptr worker_thread_runner); + // When the owner of this object is Disposed(), this method must be called to + // cancel any in-flight tasks. + void Shutdown(); + HeapVector> getSynchronizationSources( ScriptState* script_state, ExceptionState& exception_state, -- cgit v1.2.1