From 301eb5370b150ee4c25ffc380a9064e48373bf27 Mon Sep 17 00:00:00 2001 From: Taylor Brandstetter Date: Wed, 3 Jun 2020 15:45:40 -0700 Subject: [PATCH] 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 Reviewed-by: Tommi Commit-Queue: Taylor Cr-Commit-Position: refs/heads/master@{#31449} --- media/sctp/sctp_transport.cc | 87 +++++++++++++++++++++++++++++++----- media/sctp/sctp_transport.h | 5 +++ 2 files changed, 82 insertions(+), 10 deletions(-) diff --git a/media/sctp/sctp_transport.cc b/media/sctp/sctp_transport.cc index effb2211ae..6be9461e91 100644 --- a/media/sctp/sctp_transport.cc +++ b/media/sctp/sctp_transport.cc @@ -22,6 +22,7 @@ enum PreservedErrno { #include #include +#include #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 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(addr); + SctpTransport* transport = + g_transport_map_->Retrieve(reinterpret_cast(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(&addrs[0]); - SctpTransport* transport = - reinterpret_cast(sconn->sconn_addr); + SctpTransport* transport = g_transport_map_->Retrieve( + reinterpret_cast(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(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(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(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(id_); return sconn; } diff --git a/media/sctp/sctp_transport.h b/media/sctp/sctp_transport.h index d346cfc71f..758503b509 100644 --- a/media/sctp/sctp_transport.h +++ b/media/sctp/sctp_transport.h @@ -13,6 +13,7 @@ #include +#include #include #include #include @@ -267,6 +268,10 @@ class SctpTransport : public SctpTransportInternal, absl::optional max_outbound_streams_; absl::optional 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); };