[Backport] CVE-2025-7657: Use after free in WebRTC

Manual cherry-pick of patch originally reviewed on
https://chromium-review.googlesource.com/c/chromium/src/+/6694889:
Harden P2PSocketTcpBase::SendBatch against errors

This change ensures that even if errors occur that cause the socket
to be deleted during packet processing, SendBatch will terminate
correctly.

Bug: 427681143
Change-Id: I41da6789c8aab44b31c00c6a7844cd86b918561c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6694889
Reviewed-by: Sergey Ulanov <[email protected]>
Commit-Queue: Harald Alvestrand <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1481925}
Reviewed-on: https://codereview.qt-project.org/c/qt/qtwebengine-chromium/+/665025
Reviewed-by: Allan Sandfeld Jensen <[email protected]>

Author: alvestrand

chromium/services/network/p2p/socket_tcp.cc

@@ -68,13 +68,13 @@ P2PSocketTcpBase::P2PSocketTcpBase(
 
 P2PSocketTcpBase::~P2PSocketTcpBase() = default;
 
-void P2PSocketTcpBase::InitAccepted(const net::IPEndPoint& remote_address,
+bool P2PSocketTcpBase::InitAccepted(const net::IPEndPoint& remote_address,
                                     std::unique_ptr<net::StreamSocket> socket) {
   DCHECK(socket);
   remote_address_.ip_address = remote_address;
   // TODO(ronghuawu): Add FakeSSLServerSocket.
   socket_ = std::move(socket);
-  DoRead();
+  return DoRead();
 }
 
 void P2PSocketTcpBase::Init(
@@ -123,15 +123,18 @@ void P2PSocketTcpBase::OnConnected(int result) {
   DCHECK_NE(result, net::ERR_IO_PENDING);
 
   if (result != net::OK) {
-    LOG(WARNING) << "Error from connecting socket, result=" << result;
+    LOG(WARNING) << "Error from connecting socket, result=" << result
+                 << ", destroying socket";
     OnError();
     return;
   }
 
-  OnOpen();
+  if (!OnOpen()) {
+    LOG(ERROR) << "Socket destroyed in OnConnected/OnOpen";
+  }
 }
 
-void P2PSocketTcpBase::OnOpen() {
+bool P2PSocketTcpBase::OnOpen() {
   // Setting socket send and receive buffer size.
   if (net::OK != socket_->SetReceiveBufferSize(kTcpRecvSocketBufferSize)) {
     LOG(WARNING) << "Failed to set socket receive buffer size to "
@@ -144,9 +147,9 @@ void P2PSocketTcpBase::OnOpen() {
   }
 
   if (!DoSendSocketCreateMsg())
-    return;
+    return false;
 
-  DoRead();
+  return DoRead();
 }
 
 bool P2PSocketTcpBase::DoSendSocketCreateMsg() {
@@ -193,7 +196,7 @@ bool P2PSocketTcpBase::DoSendSocketCreateMsg() {
   return true;
 }
 
-void P2PSocketTcpBase::DoRead() {
+bool P2PSocketTcpBase::DoRead() {
   while (true) {
     if (!read_buffer_.get()) {
       read_buffer_ = base::MakeRefCounted<net::GrowableIOBuffer>();
@@ -209,14 +212,23 @@ void P2PSocketTcpBase::DoRead() {
     const int result = socket_->Read(
         read_buffer_.get(), read_buffer_->RemainingCapacity(),
         base::BindOnce(&P2PSocketTcp::OnRead, base::Unretained(this)));
-    if (result == net::ERR_IO_PENDING || !HandleReadResult(result))
-      return;
+    if (result == net::ERR_IO_PENDING) {
+      return true;  // not finished, but blocked
+    }
+    if (!HandleReadResult(result)) {
+      return false;  // error, socket deleted
+    }
   }
 }
 
 void P2PSocketTcpBase::OnRead(int result) {
-  if (HandleReadResult(result))
-    DoRead();
+  if (!HandleReadResult(result)) {
+    LOG(ERROR) << "OnRead/HandleReadResult reports socket destroyed";
+    return;
+  }
+  if (!DoRead()) {
+    LOG(ERROR) << "OnRead/DoRead reports socket destroyed";
+  }
 }
 
 bool P2PSocketTcpBase::OnPacket(base::span<const uint8_t> data) {
@@ -256,17 +268,17 @@ bool P2PSocketTcpBase::OnPacket(base::span<const uint8_t> data) {
   return true;
 }
 
-void P2PSocketTcpBase::WriteOrQueue(SendBuffer& send_buffer) {
+bool P2PSocketTcpBase::WriteOrQueue(SendBuffer& send_buffer) {
   if (write_buffer_.buffer.get()) {
     write_queue_.push(send_buffer);
-    return;
+    return true;
   }
 
   write_buffer_ = send_buffer;
-  DoWrite();
+  return DoWrite();
 }
 
-void P2PSocketTcpBase::DoWrite() {
+bool P2PSocketTcpBase::DoWrite() {
   while (!write_pending_ && write_buffer_.buffer.get()) {
     int result = socket_->Write(
         write_buffer_.buffer.get(), write_buffer_.buffer->BytesRemaining(),
@@ -276,9 +288,10 @@ void P2PSocketTcpBase::DoWrite() {
     if (result == net::ERR_IO_PENDING) {
       write_pending_ = true;
     } else if (!HandleWriteResult(result)) {
-      break;
+      return false;  // Error, socket is destroyed.
     }
   }
+  return true;
 }
 
 void P2PSocketTcpBase::OnWritten(int result) {
@@ -287,8 +300,13 @@ void P2PSocketTcpBase::OnWritten(int result) {
 
   write_pending_ = false;
 
-  if (HandleWriteResult(result))
-    DoWrite();
+  if (!HandleWriteResult(result)) {
+    LOG(ERROR) << "Socket destroyed in OnWritten/HandleWriteResult";
+    return;
+  }
+  if (!DoWrite()) {
+    LOG(ERROR) << "Socket destroyed in OnWritten/DoWrite";
+  }
 }
 
 bool P2PSocketTcpBase::HandleWriteResult(int result) {
@@ -376,13 +394,14 @@ bool P2PSocketTcpBase::SendPacket(base::span<const uint8_t> data,
     }
   }
 
-  DoSend(packet_info.destination, data, packet_info.packet_options);
-  return true;
+  return DoSend(packet_info.destination, data, packet_info.packet_options);
 }
 
 void P2PSocketTcpBase::Send(base::span<const uint8_t> data,
                             const P2PPacketInfo& packet_info) {
-  SendPacket(data, packet_info);
+  if (!SendPacket(data, packet_info)) {
+    LOG(ERROR) << "Socket destroyed while sending";
+  }
 }
 
 void P2PSocketTcpBase::SendBatch(
@@ -448,7 +467,7 @@ bool P2PSocketTcp::ProcessInput(base::span<const uint8_t> input,
   return OnPacket(input.subspan(kPacketHeaderSize, packet_size));
 }
 
-void P2PSocketTcp::DoSend(const net::IPEndPoint& to,
+bool P2PSocketTcp::DoSend(const net::IPEndPoint& to,
                           base::span<const uint8_t> data,
                           const rtc::PacketOptions& options) {
   int buffer_size = kPacketHeaderSize + data.size();
@@ -467,7 +486,7 @@ void P2PSocketTcp::DoSend(const net::IPEndPoint& to,
       send_buffer.buffer->BytesRemaining() - kPacketHeaderSize,
       options.packet_time_params, rtc::TimeMicros());
 
-  WriteOrQueue(send_buffer);
+  return WriteOrQueue(send_buffer);
 }
 
 // P2PSocketStunTcp
@@ -508,15 +527,15 @@ bool P2PSocketStunTcp::ProcessInput(base::span<const uint8_t> input,
   return OnPacket(input.subspan(0, packet_size));
 }
 
-void P2PSocketStunTcp::DoSend(const net::IPEndPoint& to,
+bool P2PSocketStunTcp::DoSend(const net::IPEndPoint& to,
                               base::span<const uint8_t> data,
                               const rtc::PacketOptions& options) {
   // Each packet is expected to have header (STUN/TURN ChannelData), where
   // header contains message type and and length of message.
   if (data.size() < kPacketHeaderSize + kPacketLengthOffset) {
     NOTREACHED();
     OnError();
-    return;
+    return false;
   }
 
   int pad_bytes;
@@ -526,7 +545,7 @@ void P2PSocketStunTcp::DoSend(const net::IPEndPoint& to,
   if (data.size() != expected_len) {
     NOTREACHED();
     OnError();
-    return;
+    return false;
   }
 
   // Add any pad bytes to the total size.
@@ -554,7 +573,7 @@ void P2PSocketStunTcp::DoSend(const net::IPEndPoint& to,
                                         data.size()),
                         false);
 
-  WriteOrQueue(send_buffer);
+  return WriteOrQueue(send_buffer);
 }
 
 int P2PSocketStunTcp::GetExpectedPacketSize(base::span<const uint8_t> data,

chromium/services/network/p2p/socket_tcp.h

@@ -45,8 +45,9 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) P2PSocketTcpBase : public P2PSocket {
 
   ~P2PSocketTcpBase() override;
 
-  void InitAccepted(const net::IPEndPoint& remote_address,
-                    std::unique_ptr<net::StreamSocket> socket);
+  // The TCP socket is deleted on errors; in this case, false is returned.
+  [[nodiscard]] bool InitAccepted(const net::IPEndPoint& remote_address,
+                                  std::unique_ptr<net::StreamSocket> socket);
 
   // P2PSocket overrides.
   void Init(
@@ -76,36 +77,40 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) P2PSocketTcpBase : public P2PSocket {
   // Derived classes will provide the implementation.
   virtual bool ProcessInput(base::span<const uint8_t> input,
                             size_t* bytes_consumed) = 0;
-  virtual void DoSend(const net::IPEndPoint& to,
-                      base::span<const uint8_t> data,
-                      const rtc::PacketOptions& options) = 0;
+  [[nodiscard]] virtual bool DoSend(
+      const net::IPEndPoint& to,
+      base::span<const uint8_t> data,
+      const rtc::PacketOptions& options) = 0;
 
-  void WriteOrQueue(SendBuffer& send_buffer);
+  [[nodiscard]] bool WriteOrQueue(SendBuffer& send_buffer);
   [[nodiscard]] bool OnPacket(base::span<const uint8_t> data);
 
-  bool SendPacket(base::span<const uint8_t> data,
-                  const P2PPacketInfo& packet_info);
+  [[nodiscard]] bool SendPacket(base::span<const uint8_t> data,
+                                const P2PPacketInfo& packet_info);
 
  private:
   friend class P2PSocketTcpTestBase;
   friend class P2PSocketTcpServerTest;
 
-  void DoRead();
-  void DoWrite();
+  // These functions return |false| in case of an error.
+  // The socket is destroyed in that case, so the caller should not use |this|.
+  [[nodiscard]] bool DoRead();
+  [[nodiscard]] bool DoWrite();
 
-  // Return |false| in case of an error. The socket is destroyed in that case,
-  // so the caller should not use |this|.
   [[nodiscard]] bool HandleReadResult(int result);
   [[nodiscard]] bool HandleWriteResult(int result);
 
   // Callbacks for Connect(), Read() and Write().
+  // Socket destruction may happen in these functions, but they are
+  // invoked asynchronously, on the same thread, so we assume that
+  // socket destruction doesn't happen while a function is active.
   void OnConnected(int result);
   void OnRead(int result);
   void OnWritten(int result);
 
   // Helper method to send socket create message and start read.
-  void OnOpen();
-  bool DoSendSocketCreateMsg();
+  [[nodiscard]] bool OnOpen();
+  [[nodiscard]] bool DoSendSocketCreateMsg();
 
   P2PHostAndIPEndPoint remote_address_;
 
@@ -140,9 +145,10 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) P2PSocketTcp : public P2PSocketTcpBase {
  protected:
   bool ProcessInput(base::span<const uint8_t> input,
                     size_t* bytes_consumed) override;
-  void DoSend(const net::IPEndPoint& to,
-              base::span<const uint8_t> data,
-              const rtc::PacketOptions& options) override;
+  [[nodiscard]] bool DoSend(
+      const net::IPEndPoint& to,
+      base::span<const uint8_t> data,
+      const rtc::PacketOptions& options) override;
 };
 
 // P2PSocketStunTcp class provides the framing of STUN messages when used
@@ -168,9 +174,10 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) P2PSocketStunTcp
  protected:
   bool ProcessInput(base::span<const uint8_t> input,
                     size_t* bytes_consumed) override;
-  void DoSend(const net::IPEndPoint& to,
-              base::span<const uint8_t> data,
-              const rtc::PacketOptions& options) override;
+  [[nodiscard]] bool DoSend(
+      const net::IPEndPoint& to,
+      base::span<const uint8_t> data,
+      const rtc::PacketOptions& options) override;
 
  private:
   int GetExpectedPacketSize(base::span<const uint8_t> data, int* pad_bytes);