[Backport] CVE-2023-0472: Use after free in WebRTC

Manual cherry-pick of patch originally reviewed on
https://chromium-review.googlesource.com/c/chromium/src/+/4176412:
Delete PeerConnectionHandler in PeerConnection finalizer

Also guard against removal of PC during PeerConnectionHandler
call that may cause garbage collection.

(cherry picked from commit 5066dd66309d884762e5fb9be04b59582893d09a)

Bug: chromium:1405256
Change-Id: I9adf7b219e2026e07ccc0868c1a85f3b35cd9d26
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4154578
Commit-Queue: Harald Alvestrand <hta@chromium.org>
Reviewed-by: Guido Urdaneta <guidou@chromium.org>
Commit-Queue: Guido Urdaneta <guidou@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1091801}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4176412
Auto-Submit: Harald Alvestrand <hta@chromium.org>
Cr-Commit-Position: refs/branch-heads/5359@{#1347}
Cr-Branched-From: 27d3765d341b09369006d030f83f582a29eb57ae-refs/heads/main@{#1058933}
Reviewed-on: https://codereview.qt-project.org/c/qt/qtwebengine-chromium/+/456883
Reviewed-by: Michal Klocek <michal.klocek@qt.io>
Reviewed-on: https://codereview.qt-project.org/c/qt/qtwebengine-chromium/+/461409
Reviewed-by: Allan Sandfeld Jensen <allan.jensen@qt.io>

2 files changed, 17 insertions, 6 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 7ede07459f1..cd7181d348f 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
@@ -839,10 +839,11 @@ RTCPeerConnection::~RTCPeerConnection() {
 }
 
 void RTCPeerConnection::Dispose() {
-  // Promptly clears the handler's pointer to |this|
+  // 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_->StopAndUnregister();
+    peer_handler_.reset();
   }
 }
 
diff --git a/chromium/third_party/blink/renderer/modules/peerconnection/rtc_peer_connection_handler.cc b/chromium/third_party/blink/renderer/modules/peerconnection/rtc_peer_connection_handler.cc
index 70b2997d55e..7781c4083dc 100644
--- a/chromium/third_party/blink/renderer/modules/peerconnection/rtc_peer_connection_handler.cc
+++ b/chromium/third_party/blink/renderer/modules/peerconnection/rtc_peer_connection_handler.cc
@@ -802,6 +802,8 @@ class RTCPeerConnectionHandler::WebRtcSetDescriptionObserverImpl
     if (handler_) {
       handler_->OnModifySctpTransport(std::move(states.sctp_transport_state));
     }
+    // Since OnSessionDescriptionsUpdated can fire events, it may cause
+    // garbage collection. Ensure that handler_ is still valid.
     if (handler_) {
       handler_->OnModifyTransceivers(
           states.signaling_state, std::move(states.transceiver_states),
@@ -1114,6 +1116,8 @@ bool RTCPeerConnectionHandler::Initialize(
   CHECK(!initialize_called_);
   initialize_called_ = true;
 
+  // Prevent garbage collection of client_ during processing.
+  auto* client_on_stack = client_;
   // TODO(crbug.com/787254): Evaluate the need for passing weak ptr since
   // PeerConnectionTracker is now leaky with base::LazyInstance.
   peer_connection_tracker_ = PeerConnectionTracker::GetInstance()->AsWeakPtr();
@@ -1152,7 +1156,8 @@ bool RTCPeerConnectionHandler::Initialize(
                                                      options, frame_);
   }
 
-  return true;
+  // Gratuitous usage of client_on_stack to prevent compiler errors.
+  return !!client_on_stack;
 }
 
 bool RTCPeerConnectionHandler::InitializeForTest(
@@ -2200,9 +2205,11 @@ void RTCPeerConnectionHandler::OnSessionDescriptionsUpdated(
         pending_remote_description,
     std::unique_ptr<webrtc::SessionDescriptionInterface>
         current_remote_description) {
+  // Prevent garbage collection of client_ during processing.
+  auto* client_on_stack = client_;
   if (!client_ || is_closed_)
     return;
-  client_->DidChangeSessionDescriptions(
+  client_on_stack->DidChangeSessionDescriptions(
       pending_local_description
           ? CreateWebKitSessionDescription(pending_local_description.get())
           : nullptr,
@@ -2516,8 +2523,12 @@ void RTCPeerConnectionHandler::OnIceCandidate(const String& sdp,
                                               int sdp_mline_index,
                                               int component,
                                               int address_family) {
+  // In order to ensure that the RTCPeerConnection is not garbage collected
+  // from under the function, we keep a pointer to it on the stack.
+  auto* client_on_stack = client_;
   DCHECK(task_runner_->RunsTasksInCurrentSequence());
   TRACE_EVENT0("webrtc", "RTCPeerConnectionHandler::OnIceCandidateImpl");
+  // This line can cause garbage collection.
   auto* platform_candidate = MakeGarbageCollected<RTCIceCandidatePlatform>(
       sdp, sdp_mid, sdp_mline_index);
   if (peer_connection_tracker_) {
@@ -2537,7 +2548,7 @@ void RTCPeerConnectionHandler::OnIceCandidate(const String& sdp,
     }
   }
   if (!is_closed_)
-    client_->DidGenerateICECandidate(platform_candidate);
+    client_on_stack->DidGenerateICECandidate(platform_candidate);
 }
 
 void RTCPeerConnectionHandler::OnIceCandidateError(
@@ -2549,7 +2560,6 @@ void RTCPeerConnectionHandler::OnIceCandidateError(
     const String& error_text) {
   DCHECK(task_runner_->RunsTasksInCurrentSequence());
   TRACE_EVENT0("webrtc", "RTCPeerConnectionHandler::OnIceCandidateError");
-
   if (peer_connection_tracker_) {
     peer_connection_tracker_->TrackIceCandidateError(
         this, address, port, host_candidate, url, error_code, error_text);