Prevent pointer from being sent in the clear over SCTP.

We were using the address of the SctpTransport object as
the sconn_addr field in usrsctp, which is used to get access to
the SctpTransport object in various callbacks.

However, this address is sent in the clear in the SCTP cookie,
which is undesirable.

This change uses a monotonically increasing id instead, which
is mapped back to a SctpTransport using a SctpTransportMap helper
class.

Bug: chromium:1076703
Change-Id: Iffb23fdbfa13625e921a9fd5500fe772b4d4015f
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/176422
Reviewed-by: Harald Alvestrand <[email protected]>
Reviewed-by: Tommi <[email protected]>
Commit-Queue: Taylor <[email protected]>
Cr-Commit-Position: refs/heads/master@{#31449}

Author: Taylor Brandstetter

media/sctp/sctp_transport.cc

@@ -22,6 +22,7 @@ enum PreservedErrno {
 #include <stdio.h>
 
 #include <memory>
+#include <unordered_map>
 
 #include "absl/algorithm/container.h"
 #include "absl/base/attributes.h"
@@ -39,6 +40,7 @@ enum PreservedErrno {
 #include "rtc_base/logging.h"
 #include "rtc_base/numerics/safe_conversions.h"
 #include "rtc_base/string_utils.h"
+#include "rtc_base/thread_annotations.h"
 #include "rtc_base/thread_checker.h"
 #include "rtc_base/trace_event.h"
 #include "usrsctplib/usrsctp.h"
@@ -72,6 +74,59 @@ enum PayloadProtocolIdentifier {
   PPID_TEXT_LAST = 51
 };
 
+// Maps SCTP transport ID to SctpTransport object, necessary in send threshold
+// callback and outgoing packet callback.
+// TODO(crbug.com/1076703): Remove once the underlying problem is fixed or
+// workaround is provided in usrsctp.
+class SctpTransportMap {
+ public:
+  SctpTransportMap() = default;
+
+  // Assigns a new unused ID to the following transport.
+  uintptr_t Register(cricket::SctpTransport* transport) {
+    rtc::CritScope cs(&lock_);
+    // usrsctp_connect fails with a value of 0...
+    if (next_id_ == 0) {
+      ++next_id_;
+    }
+    // In case we've wrapped around and need to find an empty spot from a
+    // removed transport. Assumes we'll never be full.
+    while (map_.find(next_id_) != map_.end()) {
+      ++next_id_;
+      if (next_id_ == 0) {
+        ++next_id_;
+      }
+    };
+    map_[next_id_] = transport;
+    return next_id_++;
+  }
+
+  // Returns true if found.
+  bool Deregister(uintptr_t id) {
+    rtc::CritScope cs(&lock_);
+    return map_.erase(id) > 0;
+  }
+
+  cricket::SctpTransport* Retrieve(uintptr_t id) const {
+    rtc::CritScope cs(&lock_);
+    auto it = map_.find(id);
+    if (it == map_.end()) {
+      return nullptr;
+    }
+    return it->second;
+  }
+
+ private:
+  rtc::CriticalSection lock_;
+
+  uintptr_t next_id_ RTC_GUARDED_BY(lock_) = 0;
+  std::unordered_map<uintptr_t, cricket::SctpTransport*> map_
+      RTC_GUARDED_BY(lock_);
+};
+
+// Should only be modified by UsrSctpWrapper.
+ABSL_CONST_INIT SctpTransportMap* g_transport_map_ = nullptr;
+
 // Helper for logging SCTP messages.
 #if defined(__GNUC__)
 __attribute__((__format__(__printf__, 1, 2)))
@@ -242,9 +297,12 @@ class SctpTransport::UsrSctpWrapper {
     // Set the number of default outgoing streams. This is the number we'll
     // send in the SCTP INIT message.
     usrsctp_sysctl_set_sctp_nr_outgoing_streams_default(kMaxSctpStreams);
+
+    g_transport_map_ = new SctpTransportMap();
   }
 
   static void UninitializeUsrSctp() {
+    delete g_transport_map_;
     RTC_LOG(LS_INFO) << __FUNCTION__;
     // usrsctp_finish() may fail if it's called too soon after the transports
     // are
@@ -282,7 +340,14 @@ class SctpTransport::UsrSctpWrapper {
                                   size_t length,
                                   uint8_t tos,
                                   uint8_t set_df) {
-    SctpTransport* transport = static_cast<SctpTransport*>(addr);
+    SctpTransport* transport =
+        g_transport_map_->Retrieve(reinterpret_cast<uintptr_t>(addr));
+    if (!transport) {
+      RTC_LOG(LS_ERROR)
+          << "OnSctpOutboundPacket: Failed to get transport for socket ID "
+          << addr;
+      return EINVAL;
+    }
     RTC_LOG(LS_VERBOSE) << "global OnSctpOutboundPacket():"
                            "addr: "
                         << addr << "; length: " << length
@@ -392,14 +457,14 @@ class SctpTransport::UsrSctpWrapper {
       return nullptr;
     }
     // usrsctp_getladdrs() returns the addresses bound to this socket, which
-    // contains the SctpTransport* as sconn_addr.  Read the pointer,
+    // contains the SctpTransport id as sconn_addr.  Read the id,
     // then free the list of addresses once we have the pointer.  We only open
     // AF_CONN sockets, and they should all have the sconn_addr set to the
-    // pointer that created them, so [0] is as good as any other.
+    // id of the transport that created them, so [0] is as good as any other.
     struct sockaddr_conn* sconn =
         reinterpret_cast<struct sockaddr_conn*>(&addrs[0]);
-    SctpTransport* transport =
-        reinterpret_cast<SctpTransport*>(sconn->sconn_addr);
+    SctpTransport* transport = g_transport_map_->Retrieve(
+        reinterpret_cast<uintptr_t>(sconn->sconn_addr));
     usrsctp_freeladdrs(addrs);
 
     return transport;
@@ -779,9 +844,10 @@ bool SctpTransport::OpenSctpSocket() {
     UsrSctpWrapper::DecrementUsrSctpUsageCount();
     return false;
   }
-  // Register this class as an address for usrsctp. This is used by SCTP to
+  id_ = g_transport_map_->Register(this);
+  // Register our id as an address for usrsctp. This is used by SCTP to
   // direct the packets received (by the created socket) to this class.
-  usrsctp_register_address(this);
+  usrsctp_register_address(reinterpret_cast<void*>(id_));
   return true;
 }
 
@@ -872,7 +938,8 @@ void SctpTransport::CloseSctpSocket() {
     // discarded instead of being sent.
     usrsctp_close(sock_);
     sock_ = nullptr;
-    usrsctp_deregister_address(this);
+    usrsctp_deregister_address(reinterpret_cast<void*>(id_));
+    RTC_CHECK(g_transport_map_->Deregister(id_));
     UsrSctpWrapper::DecrementUsrSctpUsageCount();
     ready_to_send_data_ = false;
   }
@@ -1003,7 +1070,7 @@ void SctpTransport::OnPacketRead(rtc::PacketTransportInternal* transport,
     // will be will be given to the global OnSctpInboundData, and then,
     // marshalled by the AsyncInvoker.
     VerboseLogPacket(data, len, SCTP_DUMP_INBOUND);
-    usrsctp_conninput(this, data, len, 0);
+    usrsctp_conninput(reinterpret_cast<void*>(id_), data, len, 0);
   } else {
     // TODO(ldixon): Consider caching the packet for very slightly better
     // reliability.
@@ -1033,7 +1100,7 @@ sockaddr_conn SctpTransport::GetSctpSockAddr(int port) {
 #endif
   // Note: conversion from int to uint16_t happens here.
   sconn.sconn_port = rtc::HostToNetwork16(port);
-  sconn.sconn_addr = this;
+  sconn.sconn_addr = reinterpret_cast<void*>(id_);
   return sconn;
 }
 

media/sctp/sctp_transport.h

@@ -13,6 +13,7 @@
 
 #include <errno.h>
 
+#include <cstdint>
 #include <map>
 #include <memory>
 #include <set>
@@ -267,6 +268,10 @@ class SctpTransport : public SctpTransportInternal,
   absl::optional<int> max_outbound_streams_;
   absl::optional<int> max_inbound_streams_;
 
+  // Used for associating this transport with the underlying sctp socket in
+  // various callbacks.
+  uintptr_t id_ = 0;
+
   RTC_DISALLOW_COPY_AND_ASSIGN(SctpTransport);
 };