From 098ae23e11e8948640e7003a29fd6a137a3525e4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Bostr=C3=B6m?= Date: Fri, 20 Jan 2023 13:18:53 +0100 Subject: [Backport] CVE-2023-0698: Out of bounds read in WebRTC (2/2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Manual cherry-pick of patch originally reviewed on https://webrtc-review.googlesource.com/c/src/+/291112: Handle the case of missing certificates. Creating a data channel or negotiating it can make the SCTP transport name go from nothing (empty string) to something. Inside the RTCStatsCollector this is relevant because which transports we have affect which certificates we should cache, so this is an instance of having to call ClearStatsCache(). The bug is that we don't. This CL fixes the bug. I tried to create unittests to cover this, but I was unable to reproduce the race in a testing environment (if I did it would have hit an RTC_DCHECK). Not ideal... but I hope we can land it anyway since the fix is trivial and clearing the cache in response to API calls is worst case harmless. Bug: webrtc:14844 Change-Id: Ia7174cde040839e5555237db6de285297120b123 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/291112 Reviewed-by: Mirko Bonadei Commit-Queue: Henrik Boström Cr-Commit-Position: refs/heads/main@{#39160} Reviewed-on: https://codereview.qt-project.org/c/qt/qtwebengine-chromium/+/460494 Reviewed-by: Allan Sandfeld Jensen --- chromium/third_party/webrtc/pc/peer_connection.cc | 12 +++++++++--- chromium/third_party/webrtc/pc/peer_connection.h | 2 ++ 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/chromium/third_party/webrtc/pc/peer_connection.cc b/chromium/third_party/webrtc/pc/peer_connection.cc index 1a17fd4368d..7b5e51d0da2 100644 --- a/chromium/third_party/webrtc/pc/peer_connection.cc +++ b/chromium/third_party/webrtc/pc/peer_connection.cc @@ -2017,7 +2017,7 @@ void PeerConnection::SetSctpDataMid(const std::string& mid) { void PeerConnection::ResetSctpDataMid() { RTC_DCHECK_RUN_ON(signaling_thread()); sctp_mid_s_.reset(); - sctp_transport_name_s_.clear(); + SetSctpTransportName(""); } void PeerConnection::OnSctpDataChannelClosed(DataChannelInterface* channel) { @@ -2246,6 +2246,12 @@ absl::optional PeerConnection::sctp_transport_name() const { return absl::optional(); } +void PeerConnection::SetSctpTransportName(std::string sctp_transport_name) { + RTC_DCHECK_RUN_ON(signaling_thread()); + sctp_transport_name_s_ = std::move(sctp_transport_name); + ClearStatsCache(); +} + absl::optional PeerConnection::sctp_mid() const { RTC_DCHECK_RUN_ON(signaling_thread()); return sctp_mid_s_; @@ -2469,7 +2475,7 @@ bool PeerConnection::SetupDataChannelTransport_n(const std::string& mid) { ToQueuedTask(signaling_thread_safety_.flag(), [this, name = dtls_transport->transport_name()] { RTC_DCHECK_RUN_ON(signaling_thread()); - sctp_transport_name_s_ = std::move(name); + SetSctpTransportName(std::move(name)); })); } @@ -2882,7 +2888,7 @@ bool PeerConnection::OnTransportChanged( [this, name = std::string(dtls_transport->internal()->transport_name())] { RTC_DCHECK_RUN_ON(signaling_thread()); - sctp_transport_name_s_ = std::move(name); + SetSctpTransportName(std::move(name)); })); } } diff --git a/chromium/third_party/webrtc/pc/peer_connection.h b/chromium/third_party/webrtc/pc/peer_connection.h index 6ada0c0683e..7b4997a31e6 100644 --- a/chromium/third_party/webrtc/pc/peer_connection.h +++ b/chromium/third_party/webrtc/pc/peer_connection.h @@ -592,6 +592,8 @@ class PeerConnection : public PeerConnectionInternal, rtc::scoped_refptr dtls_transport, DataChannelTransportInterface* data_channel_transport) override; + void SetSctpTransportName(std::string sctp_transport_name); + std::function InitializeRtcpCallback(); -- cgit v1.2.1