[Backport] CVE-2020-6467v5.12.9

Onstate handler is allowed to close a PeerConnection.

Bug: chromium:1068084
Change-Id: Icd3f70b6784ac22ef4e3bc1c99233f51145a917f
Reviewed-by: Michael Brüning <michael.bruning@qt.io>

4 files changed, 35 insertions, 6 deletions

diff --git a/chromium/content/renderer/media/webrtc/rtc_peer_connection_handler.cc b/chromium/content/renderer/media/webrtc/rtc_peer_connection_handler.cc
index 8a9329b9a81..1332ba89e93 100644
--- a/chromium/content/renderer/media/webrtc/rtc_peer_connection_handler.cc
+++ b/chromium/content/renderer/media/webrtc/rtc_peer_connection_handler.cc
@@ -1041,6 +1041,7 @@ RTCPeerConnectionHandler::RTCPeerConnectionHandler(
       initialize_called_(false),
       client_(client),
       is_closed_(false),
+      is_unregistered_(false),
       dependency_factory_(dependency_factory),
       track_adapter_map_(
           new WebRtcMediaStreamTrackAdapterMap(dependency_factory_,
@@ -1055,6 +1056,12 @@ RTCPeerConnectionHandler::RTCPeerConnectionHandler(
 }
 
 RTCPeerConnectionHandler::~RTCPeerConnectionHandler() {
+  if (!is_unregistered_) {
+    StopAndUnregister();
+  }
+}
+
+void RTCPeerConnectionHandler::StopAndUnregister() {
   DCHECK(task_runner_->RunsTasksInCurrentSequence());
 
   Stop();
@@ -1065,6 +1072,10 @@ RTCPeerConnectionHandler::~RTCPeerConnectionHandler() {
 
   UMA_HISTOGRAM_COUNTS_10000(
       "WebRTC.NumDataChannelsPerPeerConnection", num_data_channels_created_);
+  // Clear the pointer to client_ so that it does not interfere with
+  // garbage collection.
+  client_ = nullptr;
+  is_unregistered_ = true;
 }
 
 void RTCPeerConnectionHandler::associateWithFrame(blink::WebLocalFrame* frame) {
@@ -1998,6 +2009,10 @@ void RTCPeerConnectionHandler::OnSignalingChange(
     peer_connection_tracker_->TrackSignalingStateChange(this, new_state);
   if (!is_closed_)
     client_->DidChangeSignalingState(new_state);
+     // The callback may have closed the PC. If so, do not continue.
+  if (is_closed_ || !client_) {
+     return;
+  }
 }
 
 // Called any time the IceConnectionState changes
diff --git a/chromium/content/renderer/media/webrtc/rtc_peer_connection_handler.h b/chromium/content/renderer/media/webrtc/rtc_peer_connection_handler.h
index 74cb1b77c1a..c6700cb154d 100644
--- a/chromium/content/renderer/media/webrtc/rtc_peer_connection_handler.h
+++ b/chromium/content/renderer/media/webrtc/rtc_peer_connection_handler.h
@@ -162,6 +162,8 @@ class CONTENT_EXPORT RTCPeerConnectionHandler
       const blink::WebString& label,
       const blink::WebRTCDataChannelInit& init) override;
   void Stop() override;
+  void StopAndUnregister() override;
+
   blink::WebString Id() const override;
 
   // Delegate functions to allow for mocking of WebKit interfaces.
@@ -302,14 +304,18 @@ class CONTENT_EXPORT RTCPeerConnectionHandler
   // first call fails.
   bool initialize_called_;
 
-  // |client_| is a weak pointer to the blink object (blink::RTCPeerConnection)
+  // |client_| is a raw pointer to the blink object (blink::RTCPeerConnection)
   // that owns this object.
-  // It is valid for the lifetime of this object.
-  blink::WebRTCPeerConnectionHandlerClient* const client_;
+  // It is valid for the lifetime of this object, but is cleared when
+  // StopAndUnregister() is called, in order to make sure it doesn't
+  // interfere with garbage collection of the owner object.
+  blink::WebRTCPeerConnectionHandlerClient* client_;
   // True if this PeerConnection has been closed.
   // After the PeerConnection has been closed, this object may no longer
   // forward callbacks to blink.
   bool is_closed_;
+  // True if StopAndUnregister has been called.
+  bool is_unregistered_;
 
   // |dependency_factory_| is a raw pointer, and is valid for the lifetime of
   // RenderThreadImpl.
diff --git a/chromium/third_party/blink/public/platform/web_rtc_peer_connection_handler.h b/chromium/third_party/blink/public/platform/web_rtc_peer_connection_handler.h
index 9c0e27aa3d7..c147fd3a39b 100644
--- a/chromium/third_party/blink/public/platform/web_rtc_peer_connection_handler.h
+++ b/chromium/third_party/blink/public/platform/web_rtc_peer_connection_handler.h
@@ -121,6 +121,12 @@ class WebRTCPeerConnectionHandler {
   virtual webrtc::RTCErrorOr<std::unique_ptr<WebRTCRtpTransceiver>> RemoveTrack(
       WebRTCRtpSender*) = 0;
   virtual void Stop() = 0;
+  // This function should be called when the object is taken out of service.
+  // There might be functions that need to return through the object, so it
+  // cannot be deleted yet, but no new operations should be allowed.
+  // All references to the object except the owning reference are deleted
+  // by this function.
+  virtual void StopAndUnregister() = 0;
 
   // Origin Trial - RtcPeerConnectionId
   virtual WebString Id() const = 0;
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 1506e6e6346..c1b374ed62d 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
@@ -629,9 +629,11 @@ RTCPeerConnection::~RTCPeerConnection() {
 }
 
 void RTCPeerConnection::Dispose() {
-  // Promptly clears a raw reference from content/ to an on-heap object
+  // Promptly clears the handler's pointer to |this|
   // so that content/ doesn't access it in a lazy sweeping phase.
-  peer_handler_.reset();
+  if (peer_handler_) {
+    peer_handler_->StopAndUnregister();
+  }
 }
 
 ScriptPromise RTCPeerConnection::createOffer(ScriptState* script_state,
@@ -2201,7 +2203,7 @@ void RTCPeerConnection::ReleasePeerConnectionHandler() {
 
   dispatch_scheduled_event_runner_->Stop();
 
-  peer_handler_.reset();
+  peer_handler_->StopAndUnregister();
 
   connection_handle_for_scheduler_.reset();
 }