From 9d25daa80979aad4f7072ec3443ba806516025be Mon Sep 17 00:00:00 2001 From: Taylor Brandstetter Date: Wed, 14 Apr 2021 11:09:16 -0700 Subject: [Backport] Security bug 1184441 Manual backport of patch originally reviewed on https://webrtc-review.googlesource.com/c/src/+/215060: [Merge M86] - Reland "Fix race between destroying SctpTransport and receiving notification on timer thread." This reverts commit 8a38b1cf681cd77f0d59a68fb45d8dedbd7d4cee. Reason for reland: Problem was identified; has something to do with the unique_ptr with the custom deleter. Original change's description: > Revert "Fix race between destroying SctpTransport and receiving notification on timer thread." > > This reverts commit a88fe7be146b9b85575504d4d5193c007f2e3de4. > > Reason for revert: Breaks downstream test, still investigating. > > Original change's description: > > Fix race between destroying SctpTransport and receiving notification on timer thread. > > > > This gets rid of the SctpTransportMap::Retrieve method and forces > > everything to go through PostToTransportThread, which behaves safely > > with relation to the transport's destruction. > > > > Bug: webrtc:12467 > > Change-Id: Id4a723c2c985be2a368d2cc5c5e62deb04c509ab > > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/208800 > > Reviewed-by: Niels Moller > > Commit-Queue: Taylor > > Cr-Commit-Position: refs/heads/master@{#33364} > > TBR=nisse@webrtc.org > > Bug: webrtc:12467 > Change-Id: Ib5d815a2cbca4feb25f360bff7ed62c02d1910a0 > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/209820 > Reviewed-by: Taylor > Commit-Queue: Taylor > Cr-Commit-Position: refs/heads/master@{#33386} TBR=nisse@webrtc.org Bug: webrtc:12467 Change-Id: I5f9fcd6df7a211e6edfa64577fc953833f4d9b79 Reviewed-by: Niels Moller Reviewed-by: Florent Castelli Commit-Queue: Taylor Cr-Original-Commit-Position: refs/heads/master@{#33427} No-Try: True No-Presubmit: True Reviewed-by: Taylor Commit-Queue: Mirko Bonadei Cr-Commit-Position: refs/branch-heads/4240@{#19} Cr-Branched-From: 93a9d19d4eb53b3f4fb4d22e6c54f2e2824437eb-refs/heads/master@{#31969} Reviewed-by: Allan Sandfeld Jensen --- .../webrtc/media/sctp/sctp_transport.cc | 179 ++++++++++++--------- .../third_party/webrtc/media/sctp/sctp_transport.h | 18 +-- 2 files changed, 111 insertions(+), 86 deletions(-) diff --git a/chromium/third_party/webrtc/media/sctp/sctp_transport.cc b/chromium/third_party/webrtc/media/sctp/sctp_transport.cc index caf52dbbade..4efef687cfb 100644 --- a/chromium/third_party/webrtc/media/sctp/sctp_transport.cc +++ b/chromium/third_party/webrtc/media/sctp/sctp_transport.cc @@ -20,6 +20,7 @@ enum PreservedErrno { // Successful return value from usrsctp callbacks. Is not actually used by // usrsctp, but all example programs for usrsctp use 1 as their return value. constexpr int kSctpSuccessReturn = 1; +constexpr int kSctpErrorReturn = 0; } // namespace @@ -83,6 +84,19 @@ enum { // Should only be modified by UsrSctpWrapper. ABSL_CONST_INIT cricket::SctpTransportMap* g_transport_map_ = nullptr; +// Helper that will call C's free automatically. +// TODO(b/181900299): Figure out why unique_ptr with a custom deleter is causing +// issues in a certain build environment. +class AutoFreedPointer { + public: + explicit AutoFreedPointer(void* ptr) : ptr_(ptr) {} + AutoFreedPointer(AutoFreedPointer&& o) : ptr_(o.ptr_) { o.ptr_ = nullptr; } + ~AutoFreedPointer() { free(ptr_); } + void* get() const { return ptr_; } + private: + void* ptr_; +}; + // Helper for logging SCTP messages. #if defined(__GNUC__) __attribute__((__format__(__printf__, 1, 2))) @@ -239,32 +253,20 @@ class SctpTransportMap { return map_.erase(id) > 0; } - // Must be called on the transport's network thread to protect against - // simultaneous deletion/deregistration of the transport; if that's not - // guaranteed, use ExecuteWithLock. - SctpTransport* Retrieve(uintptr_t id) const { - webrtc::MutexLock lock(&lock_); - SctpTransport* transport = RetrieveWhileHoldingLock(id); - if (transport) { - RTC_DCHECK_RUN_ON(transport->network_thread()); - } - return transport; - } - // Posts |action| to the network thread of the transport identified by |id| // and returns true if found, all while holding a lock to protect against the // transport being simultaneously deleted/deregistered, or returns false if // not found. - bool PostToTransportThread(uintptr_t id, - std::function action) const { + template + bool PostToTransportThread(uintptr_t id, F action) const { webrtc::MutexLock lock(&lock_); SctpTransport* transport = RetrieveWhileHoldingLock(id); if (!transport) { return false; } transport->invoker_.AsyncInvoke( - RTC_FROM_HERE, transport->network_thread_, [transport, action]() { - action(transport); }); + RTC_FROM_HERE, transport->network_thread_, + [transport, action{std::move(action)}]() { action(transport); }); return true; } @@ -406,7 +408,7 @@ class SctpTransport::UsrSctpWrapper { if (!found) { RTC_LOG(LS_ERROR) << "OnSctpOutboundPacket: Failed to get transport for socket ID " - << addr; + << addr << "; possibly was already destroyed."; return EINVAL; } return 0; @@ -423,28 +425,45 @@ class SctpTransport::UsrSctpWrapper { struct sctp_rcvinfo rcv, int flags, void* ulp_info) { - SctpTransport* transport = GetTransportFromSocket(sock); - if (!transport) { + AutoFreedPointer owned_data(data); + absl::optional id = GetTransportIdFromSocket(sock); + if (!id) { RTC_LOG(LS_ERROR) - << "OnSctpInboundPacket: Failed to get transport for socket " << sock - << "; possibly was already destroyed."; - free(data); - return 0; + << "OnSctpInboundPacket: Failed to get transport ID from socket " + << sock; + return kSctpErrorReturn; + } + + if (!g_transport_map_) { + RTC_LOG(LS_ERROR) + << "OnSctpInboundPacket called after usrsctp uninitialized?"; + return kSctpErrorReturn; + } + // PostsToTransportThread protects against the transport being + // simultaneously deregistered/deleted, since this callback may come from + // the SCTP timer thread and thus race with the network thread. + bool found = g_transport_map_->PostToTransportThread( + *id, [owned_data{std::move(owned_data)}, length, rcv, + flags](SctpTransport* transport) { + transport->OnDataOrNotificationFromSctp(owned_data.get(), length, rcv, + flags); + }); + if (!found) { + RTC_LOG(LS_ERROR) + << "OnSctpInboundPacket: Failed to get transport for socket ID " + << *id << "; possibly was already destroyed."; + return kSctpErrorReturn; } - // Sanity check that both methods of getting the SctpTransport pointer - // yield the same result. - RTC_CHECK_EQ(transport, static_cast(ulp_info)); - int result = - transport->OnDataOrNotificationFromSctp(data, length, rcv, flags); - free(data); - return result; + return kSctpSuccessReturn; } - static SctpTransport* GetTransportFromSocket(struct socket* sock) { + static absl::optional GetTransportIdFromSocket( + struct socket* sock) { + absl::optional ret; struct sockaddr* addrs = nullptr; int naddrs = usrsctp_getladdrs(sock, 0, &addrs); if (naddrs <= 0 || addrs[0].sa_family != AF_CONN) { - return nullptr; + return ret; } // usrsctp_getladdrs() returns the addresses bound to this socket, which // contains the SctpTransport id as sconn_addr. Read the id, @@ -453,17 +472,10 @@ class SctpTransport::UsrSctpWrapper { // id of the transport that created them, so [0] is as good as any other. struct sockaddr_conn* sconn = reinterpret_cast(&addrs[0]); - if (!g_transport_map_) { - RTC_LOG(LS_ERROR) - << "GetTransportFromSocket called after usrsctp uninitialized?"; - usrsctp_freeladdrs(addrs); - return nullptr; - } - SctpTransport* transport = g_transport_map_->Retrieve( - reinterpret_cast(sconn->sconn_addr)); + ret = reinterpret_cast(sconn->sconn_addr); usrsctp_freeladdrs(addrs); - return transport; + return ret; } // TODO(crbug.com/webrtc/11899): This is a legacy callback signature, remove @@ -472,14 +484,26 @@ class SctpTransport::UsrSctpWrapper { // Fired on our I/O thread. SctpTransport::OnPacketReceived() gets // a packet containing acknowledgments, which goes into usrsctp_conninput, // and then back here. - SctpTransport* transport = GetTransportFromSocket(sock); - if (!transport) { + absl::optional id = GetTransportIdFromSocket(sock); + if (!id) { RTC_LOG(LS_ERROR) - << "SendThresholdCallback: Failed to get transport for socket " - << sock << "; possibly was already destroyed."; + << "SendThresholdCallback: Failed to get transport ID from socket " + << sock; return 0; } - transport->OnSendThresholdCallback(); + if (!g_transport_map_) { + RTC_LOG(LS_ERROR) + << "SendThresholdCallback called after usrsctp uninitialized?"; + return 0; + } + bool found = g_transport_map_->PostToTransportThread( + *id, + [](SctpTransport* transport) { transport->OnSendThresholdCallback(); }); + if (!found) { + RTC_LOG(LS_ERROR) + << "SendThresholdCallback: Failed to get transport for socket ID " + << *id << "; possibly was already destroyed."; + } return 0; } @@ -489,17 +513,26 @@ class SctpTransport::UsrSctpWrapper { // Fired on our I/O thread. SctpTransport::OnPacketReceived() gets // a packet containing acknowledgments, which goes into usrsctp_conninput, // and then back here. - SctpTransport* transport = GetTransportFromSocket(sock); - if (!transport) { + absl::optional id = GetTransportIdFromSocket(sock); + if (!id) { RTC_LOG(LS_ERROR) - << "SendThresholdCallback: Failed to get transport for socket " - << sock << "; possibly was already destroyed."; + << "SendThresholdCallback: Failed to get transport ID from socket " + << sock; return 0; } - // Sanity check that both methods of getting the SctpTransport pointer - // yield the same result. - RTC_CHECK_EQ(transport, static_cast(ulp_info)); - transport->OnSendThresholdCallback(); + if (!g_transport_map_) { + RTC_LOG(LS_ERROR) + << "SendThresholdCallback called after usrsctp uninitialized?"; + return 0; + } + bool found = g_transport_map_->PostToTransportThread( + *id, + [](SctpTransport* transport) { transport->OnSendThresholdCallback(); }); + if (!found) { + RTC_LOG(LS_ERROR) + << "SendThresholdCallback: Failed to get transport for socket ID " + << *id << "; possibly was already destroyed."; + } return 0; } }; @@ -1150,24 +1183,25 @@ void SctpTransport::OnPacketFromSctpToNetwork( rtc::PacketOptions(), PF_NORMAL); } -int SctpTransport::InjectDataOrNotificationFromSctpForTesting( +void SctpTransport::InjectDataOrNotificationFromSctpForTesting( void* data, size_t length, struct sctp_rcvinfo rcv, int flags) { - return OnDataOrNotificationFromSctp(data, length, rcv, flags); + OnDataOrNotificationFromSctp(data, length, rcv, flags); } -int SctpTransport::OnDataOrNotificationFromSctp(void* data, - size_t length, - struct sctp_rcvinfo rcv, - int flags) { +void SctpTransport::OnDataOrNotificationFromSctp(void* data, + size_t length, + struct sctp_rcvinfo rcv, + int flags) { + RTC_DCHECK_RUN_ON(network_thread_); // If data is NULL, the SCTP association has been closed. if (!data) { RTC_LOG(LS_INFO) << debug_name_ << "->OnDataOrNotificationFromSctp(...): " "No data; association closed."; - return kSctpSuccessReturn; + return; } // Handle notifications early. @@ -1180,13 +1214,10 @@ int SctpTransport::OnDataOrNotificationFromSctp(void* data, << "->OnDataOrNotificationFromSctp(...): SCTP notification" << " length=" << length; - // Copy and dispatch asynchronously rtc::CopyOnWriteBuffer notification(reinterpret_cast(data), length); - invoker_.AsyncInvoke( - RTC_FROM_HERE, network_thread_, - rtc::Bind(&SctpTransport::OnNotificationFromSctp, this, notification)); - return kSctpSuccessReturn; + OnNotificationFromSctp(notification); + return; } // Log data chunk @@ -1204,7 +1235,7 @@ int SctpTransport::OnDataOrNotificationFromSctp(void* data, // Unexpected PPID, dropping RTC_LOG(LS_ERROR) << "Received an unknown PPID " << ppid << " on an SCTP packet. Dropping."; - return kSctpSuccessReturn; + return; } // Expect only continuation messages belonging to the same SID. The SCTP @@ -1240,7 +1271,7 @@ int SctpTransport::OnDataOrNotificationFromSctp(void* data, if (partial_incoming_message_.size() < kSctpSendBufferSize) { // We still have space in the buffer. Continue buffering chunks until // the message is complete before handing it out. - return kSctpSuccessReturn; + return; } else { // The sender is exceeding the maximum message size that we announced. // Spit out a warning but still hand out the partial message. Note that @@ -1254,17 +1285,11 @@ int SctpTransport::OnDataOrNotificationFromSctp(void* data, } } - // Dispatch the complete message. - // The ownership of the packet transfers to |invoker_|. Using - // CopyOnWriteBuffer is the most convenient way to do this. - invoker_.AsyncInvoke( - RTC_FROM_HERE, network_thread_, - rtc::Bind(&SctpTransport::OnDataFromSctpToTransport, this, params, - partial_incoming_message_)); + // Dispatch the complete message and reset the message buffer. - // Reset the message buffer + // Dispatch the complete message and reset the message buffer. + OnDataFromSctpToTransport(params, partial_incoming_message_); partial_incoming_message_.Clear(); - return kSctpSuccessReturn; } void SctpTransport::OnDataFromSctpToTransport( diff --git a/chromium/third_party/webrtc/media/sctp/sctp_transport.h b/chromium/third_party/webrtc/media/sctp/sctp_transport.h index 7aeb6e01bd9..c327525bf1a 100644 --- a/chromium/third_party/webrtc/media/sctp/sctp_transport.h +++ b/chromium/third_party/webrtc/media/sctp/sctp_transport.h @@ -96,10 +96,10 @@ class SctpTransport : public SctpTransportInternal, void set_debug_name_for_testing(const char* debug_name) override { debug_name_ = debug_name; } - int InjectDataOrNotificationFromSctpForTesting(void* data, - size_t length, - struct sctp_rcvinfo rcv, - int flags); + void InjectDataOrNotificationFromSctpForTesting(void* data, + size_t length, + struct sctp_rcvinfo rcv, + int flags); // Exposed to allow Post call from c-callbacks. // TODO(deadbeef): Remove this or at least make it return a const pointer. @@ -180,12 +180,12 @@ class SctpTransport : public SctpTransportInternal, // Called using |invoker_| to send packet on the network. void OnPacketFromSctpToNetwork(const rtc::CopyOnWriteBuffer& buffer); - // Called on the SCTP thread. + // Called on the network thread. // Flags are standard socket API flags (RFC 6458). - int OnDataOrNotificationFromSctp(void* data, - size_t length, - struct sctp_rcvinfo rcv, - int flags); + void OnDataOrNotificationFromSctp(void* data, + size_t length, + struct sctp_rcvinfo rcv, + int flags); // Called using |invoker_| to decide what to do with the data. void OnDataFromSctpToTransport(const ReceiveDataParams& params, const rtc::CopyOnWriteBuffer& buffer); -- cgit v1.2.3