From 29464b06c50316d8f16c8f5687117cb93a1f376b Mon Sep 17 00:00:00 2001
From: Tommi <tommi@webrtc.org>
Date: Sun, 11 Dec 2022 13:04:40 +0100
Subject: [PATCH] [TurnPort] Check if a matching connection exists before
 replying.

Add an override to TurnPort for SendBindingErrorResponse to check
if a matching connection object exists before continuing. This is
needed to match with the check in `TurnPort::DispatchPacket` whereby
we forward calls to `Port` when no matching connections exist.

Bug: chromium:1395625
Change-Id: Idf1f898c2a93de6bc2832268db1cadd52cae23a9
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/287223
Reviewed-by: Sameer Vijaykar <samvi@google.com>
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#38871}
---
 p2p/base/connection.h          | 12 ++++++++++++
 p2p/base/turn_port.cc          | 10 ++++++++++
 p2p/base/turn_port.h           |  8 ++++++++
 p2p/base/turn_port_unittest.cc | 24 ++++++++++++++++++++++++
 4 files changed, 54 insertions(+)

diff --git a/p2p/base/connection.h b/p2p/base/connection.h
index 4be8483f9a..c0a19b510f 100644
--- a/p2p/base/connection.h
+++ b/p2p/base/connection.h
@@ -316,10 +316,22 @@ class Connection : public CandidatePairInterface {
   Port* PortForTest() { return port_.get(); }
   const Port* PortForTest() const { return port_.get(); }
 
+  std::unique_ptr<IceMessage> BuildPingRequestForTest() {
+    RTC_DCHECK_RUN_ON(network_thread_);
+    return BuildPingRequest();
+  }
+
   // Public for unit tests.
   uint32_t acked_nomination() const;
   void set_remote_nomination(uint32_t remote_nomination);
 
+  const std::string& remote_password_for_test() const {
+    return remote_candidate().password();
+  }
+  void set_remote_password_for_test(absl::string_view pwd) {
+    remote_candidate_.set_password(pwd);
+  }
+
  protected:
   // A ConnectionRequest is a simple STUN ping used to determine writability.
   class ConnectionRequest;
diff --git a/p2p/base/turn_port.cc b/p2p/base/turn_port.cc
index 3879ceb63a..33662a95ba 100644
--- a/p2p/base/turn_port.cc
+++ b/p2p/base/turn_port.cc
@@ -687,6 +687,16 @@ bool TurnPort::CanHandleIncomingPacketsFrom(
   return server_address_.address == addr;
 }
 
+void TurnPort::SendBindingErrorResponse(StunMessage* message,
+                                        const rtc::SocketAddress& addr,
+                                        int error_code,
+                                        absl::string_view reason) {
+  if (!GetConnection(addr))
+    return;
+
+  Port::SendBindingErrorResponse(message, addr, error_code, reason);
+}
+
 bool TurnPort::HandleIncomingPacket(rtc::AsyncPacketSocket* socket,
                                     const char* data,
                                     size_t size,
diff --git a/p2p/base/turn_port.h b/p2p/base/turn_port.h
index 0672931d1c..ac660d6599 100644
--- a/p2p/base/turn_port.h
+++ b/p2p/base/turn_port.h
@@ -150,6 +150,14 @@ class TurnPort : public Port {
                             int64_t packet_time_us) override;
   bool CanHandleIncomingPacketsFrom(
       const rtc::SocketAddress& addr) const override;
+
+  // Checks if a connection exists for `addr` before forwarding the call to
+  // the base class.
+  void SendBindingErrorResponse(StunMessage* message,
+                                const rtc::SocketAddress& addr,
+                                int error_code,
+                                absl::string_view reason) override;
+
   virtual void OnReadPacket(rtc::AsyncPacketSocket* socket,
                             const char* data,
                             size_t size,
diff --git a/p2p/base/turn_port_unittest.cc b/p2p/base/turn_port_unittest.cc
index 3f01fbef1b..d63dd4a75c 100644
--- a/p2p/base/turn_port_unittest.cc
+++ b/p2p/base/turn_port_unittest.cc
@@ -683,6 +683,30 @@ class TurnPortTest : public ::testing::Test,
     // unknown address.
     turn_unknown_address_ = false;
     fake_clock_.AdvanceTime(webrtc::TimeDelta::Seconds(5 * 60));
+
+    // TODO(chromium:1395625): When `TurnPort` doesn't find connection objects
+    // for incoming packets, it forwards calls to the parent class, `Port`. This
+    // happens inside `TurnPort::DispatchPacket`. The `Port` implementation may
+    // need to send a binding error back over a connection which, unless the
+    // `TurnPort` implementation handles it, could result in a null deref.
+    // This special check tests if dispatching messages via `TurnPort` for which
+    // there's no connection, results in a no-op rather than crashing.
+    // See `TurnPort::SendBindingErrorResponse` for the check.
+    // This should probably be done in a neater way both from a testing pov and
+    // how incoming messages are handled in the `Port` class, when an assumption
+    // is made about connection objects existing and when those assumptions
+    // may not hold.
+    std::string pwd = conn1->remote_password_for_test();
+    conn1->set_remote_password_for_test("bad");
+    auto msg = conn1->BuildPingRequestForTest();
+
+    rtc::ByteBufferWriter buf;
+    msg->Write(&buf);
+    conn1->Send(buf.Data(), buf.Length(), options);
+
+    // Now restore the password before continuing.
+    conn1->set_remote_password_for_test(pwd);
+
     conn1->Ping(0);
     EXPECT_TRUE_SIMULATED_WAIT(turn_unknown_address_, kSimulatedRtt,
                                fake_clock_);