From 589acd56d0530f6cd6cf2944dc6e5df8430b410d Mon Sep 17 00:00:00 2001 From: Philipp Hancke Date: Thu, 23 Jan 2025 09:02:38 -0800 Subject: [PATCH] dtls-stun piggybacking: make it compatible with DTLS 1.3 DTLS 1.3 encrypts more parts of the handshake so we move from deep packet inspection to looking at the state of DTLS to decide whether to intercept the packet. BUG=webrtc:367395350 Change-Id: Idb1eda0437f24002f48381af5d6a167a4a153381 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/374501 Reviewed-by: Jonas Oreland Commit-Queue: Jonas Oreland Reviewed-by: Harald Alvestrand Cr-Commit-Position: refs/heads/main@{#43794} --- p2p/BUILD.gn | 1 - p2p/base/ice_transport_internal.h | 2 +- p2p/base/p2p_transport_channel.cc | 21 ++++++----- p2p/base/p2p_transport_channel.h | 19 +++++++--- p2p/dtls/dtls_ice_integrationtest.cc | 28 +++++++++------ p2p/dtls/dtls_stun_piggyback_controller.cc | 36 ++++++++++++------- p2p/dtls/dtls_stun_piggyback_controller.h | 8 +++-- ...dtls_stun_piggyback_controller_unittest.cc | 27 ++++++++------ p2p/dtls/dtls_transport.cc | 23 ++++++------ 9 files changed, 100 insertions(+), 65 deletions(-) diff --git a/p2p/BUILD.gn b/p2p/BUILD.gn index 05807b388a..3cd819f76e 100644 --- a/p2p/BUILD.gn +++ b/p2p/BUILD.gn @@ -570,7 +570,6 @@ rtc_library("p2p_transport_channel") { ":connection", ":connection_info", ":dtls_stun_piggyback_controller", - ":dtls_utils", ":ice_agent_interface", ":ice_controller_factory_interface", ":ice_controller_interface", diff --git a/p2p/base/ice_transport_internal.h b/p2p/base/ice_transport_internal.h index 3a52073b76..d2f32de91b 100644 --- a/p2p/base/ice_transport_internal.h +++ b/p2p/base/ice_transport_internal.h @@ -410,7 +410,7 @@ class RTC_EXPORT IceTransportInternal : public rtc::PacketTransportInternal { piggybacked_dtls_callback_ = std::move(callback); } virtual void SetDtlsDataToPiggyback(rtc::ArrayView) {} - virtual void SetDtlsHandshakeComplete(bool is_dtls_client) {} + virtual void SetDtlsHandshakeComplete(bool is_dtls_client, bool is_dtls13) {} virtual bool IsDtlsPiggybackSupportedByPeer() { return false; } protected: diff --git a/p2p/base/p2p_transport_channel.cc b/p2p/base/p2p_transport_channel.cc index c82e3f1a15..5b0d48af2e 100644 --- a/p2p/base/p2p_transport_channel.cc +++ b/p2p/base/p2p_transport_channel.cc @@ -55,7 +55,6 @@ #include "p2p/base/regathering_controller.h" #include "p2p/base/transport_description.h" #include "p2p/base/wrapping_active_ice_controller.h" -#include "p2p/dtls/dtls_utils.h" #include "rtc_base/async_packet_socket.h" #include "rtc_base/checks.h" #include "rtc_base/dscp.h" @@ -1634,22 +1633,22 @@ int P2PTransportChannel::SendPacket(const char* data, error_ = EINVAL; return -1; } + + // If we try to use DTLS-in-STUN, DTLS packets will be sent as part of STUN + // packets and are consumed here. + rtc::ArrayView data_view = + rtc::MakeArrayView(reinterpret_cast(data), len); + if (config_.dtls_handshake_in_stun && + dtls_stun_piggyback_controller_.MaybeConsumePacket(data_view)) { + return len; + } + // If we don't think the connection is working yet, return ENOTCONN // instead of sending a packet that will probably be dropped. if (!ReadyToSend(selected_connection_)) { error_ = ENOTCONN; return -1; } - /* - * When trying DTLS-STUN piggyback we need to drop handshake packets - * as we start fresh if this fails. - */ - if (config_.dtls_handshake_in_stun && IsDtlsPiggybackSupportedByPeer() && - IsDtlsHandshakePacket( - rtc::MakeArrayView(reinterpret_cast(data), len))) { - RTC_LOG(LS_INFO) << "Dropping DTLS handshake while attemping DTLS-in-STUN"; - return len; - } packets_sent_++; last_sent_packet_id_ = options.packet_id; diff --git a/p2p/base/p2p_transport_channel.h b/p2p/base/p2p_transport_channel.h index cef9714f71..7367391234 100644 --- a/p2p/base/p2p_transport_channel.h +++ b/p2p/base/p2p_transport_channel.h @@ -252,12 +252,12 @@ class RTC_EXPORT P2PTransportChannel : public IceTransportInternal, const webrtc::FieldTrialsView* field_trials() const override { return field_trials_; } - void SetDtlsDataToPiggyback(rtc::ArrayView data) override { - dtls_stun_piggyback_controller_.SetDataToPiggyback(data); - } - void SetDtlsHandshakeComplete(bool is_dtls_client) override { - dtls_stun_piggyback_controller_.SetDtlsHandshakeComplete(is_dtls_client); + + void SetDtlsHandshakeComplete(bool is_dtls_client, bool is_dtls13) override { + dtls_stun_piggyback_controller_.SetDtlsHandshakeComplete(is_dtls_client, + is_dtls13); } + bool IsDtlsPiggybackSupportedByPeer() override { RTC_DCHECK_RUN_ON(network_thread_); return config_.dtls_handshake_in_stun && @@ -265,6 +265,15 @@ class RTC_EXPORT P2PTransportChannel : public IceTransportInternal, DtlsStunPiggybackController::State::OFF; } + bool IsDtlsPiggybackHandshaking() { + RTC_DCHECK_RUN_ON(network_thread_); + return config_.dtls_handshake_in_stun && + (dtls_stun_piggyback_controller_.state() == + DtlsStunPiggybackController::State::TENTATIVE || + dtls_stun_piggyback_controller_.state() == + DtlsStunPiggybackController::State::CONFIRMED); + } + private: P2PTransportChannel( absl::string_view transport_name, diff --git a/p2p/dtls/dtls_ice_integrationtest.cc b/p2p/dtls/dtls_ice_integrationtest.cc index 664535b723..21145ee884 100644 --- a/p2p/dtls/dtls_ice_integrationtest.cc +++ b/p2p/dtls/dtls_ice_integrationtest.cc @@ -12,7 +12,6 @@ #include #include #include -#include #include "api/candidate.h" #include "api/crypto/crypto_options.h" @@ -62,7 +61,8 @@ namespace cricket { using ::testing::IsTrue; class DtlsIceIntegrationTest - : public ::testing::TestWithParam>, + : public ::testing::TestWithParam< + std::tuple>, public sigslot::has_slots<> { public: void CandidateC2S(IceTransportInternal*, const Candidate& c) { @@ -95,11 +95,11 @@ class DtlsIceIntegrationTest client_dtls_(client_ice_.get(), webrtc::CryptoOptions(), /*event_log=*/nullptr, - rtc::SSL_PROTOCOL_DTLS_12), + std::get<2>(GetParam())), server_dtls_(server_ice_.get(), webrtc::CryptoOptions(), /*event_log=*/nullptr, - rtc::SSL_PROTOCOL_DTLS_12), + std::get<2>(GetParam())), client_ice_parameters_("c_ufrag", "c_icepwd_something_something", false), @@ -190,11 +190,19 @@ TEST_P(DtlsIceIntegrationTest, SmokeTest) { client_dtls_stun_piggyback_ && server_dtls_stun_piggyback_); } -INSTANTIATE_TEST_SUITE_P(DtlsStunPiggybackingIntegrationTest, - DtlsIceIntegrationTest, - ::testing::Values(std::make_pair(false, false), - std::make_pair(true, false), - std::make_pair(false, true), - std::make_pair(true, true))); +// Test cases are parametrized by +// * client-piggybacking-enabled, +// * server-piggybacking-enabled, +// * maximum DTLS version to use. +INSTANTIATE_TEST_SUITE_P( + DtlsStunPiggybackingIntegrationTest, + DtlsIceIntegrationTest, + ::testing::Values( + std::make_tuple(false, false, rtc::SSL_PROTOCOL_DTLS_12), + std::make_tuple(true, false, rtc::SSL_PROTOCOL_DTLS_12), + std::make_tuple(false, true, rtc::SSL_PROTOCOL_DTLS_12), + std::make_tuple(true, true, rtc::SSL_PROTOCOL_DTLS_12), + // Skip negative cases that are behaving similar for DTLS 1.3 + std::make_tuple(true, true, rtc::SSL_PROTOCOL_DTLS_13))); } // namespace cricket diff --git a/p2p/dtls/dtls_stun_piggyback_controller.cc b/p2p/dtls/dtls_stun_piggyback_controller.cc index e3c5f5414d..602f6235a2 100644 --- a/p2p/dtls/dtls_stun_piggyback_controller.cc +++ b/p2p/dtls/dtls_stun_piggyback_controller.cc @@ -33,34 +33,44 @@ DtlsStunPiggybackController::DtlsStunPiggybackController( DtlsStunPiggybackController::~DtlsStunPiggybackController() {} -void DtlsStunPiggybackController::SetDtlsHandshakeComplete( - bool is_dtls_client) { +void DtlsStunPiggybackController::SetDtlsHandshakeComplete(bool is_dtls_client, + bool is_dtls13) { RTC_DCHECK_RUN_ON(&sequence_checker_); // Peer does not support this so fallback to a normal DTLS handshake // happened. if (state_ == State::OFF) { return; } - // As DTLS server we need to keep the last flight around until + state_ = State::PENDING; + // As DTLS 1.2 server we need to keep the last flight around until // we receive the post-handshake acknowledgment. - // As DTLS client we have nothing more to send at this point + // As DTLS 1.2 client we have nothing more to send at this point // but will continue to send ACK attributes until receiving // the last flight from the server. - state_ = State::PENDING; - if (is_dtls_client) { + // For DTLS 1.3 this is reversed since the handshake has one round trip less. + if ((is_dtls_client && !is_dtls13) || (!is_dtls_client && is_dtls13)) { pending_packet_.Clear(); } } -void DtlsStunPiggybackController::SetDataToPiggyback( +bool DtlsStunPiggybackController::MaybeConsumePacket( rtc::ArrayView data) { RTC_DCHECK_RUN_ON(&sequence_checker_); - if (state_ == State::OFF) { - return; + bool should_consume = + (state_ == State::TENTATIVE || state_ == State::CONFIRMED) && + IsDtlsPacket(data); + if (should_consume) { + // Note: this overwrites the existing packets which is an issue + // if this gets called with fragmented DTLS flights. + pending_packet_.SetData(data); + return true; } - // Note: this overwrites the existing packets which is an issue - // if this gets called with fragmented DTLS flights. - pending_packet_.SetData(data); + return false; +} + +void DtlsStunPiggybackController::ClearCachedPacketForTesting() { + RTC_DCHECK_RUN_ON(&sequence_checker_); + pending_packet_.Clear(); } std::optional @@ -124,7 +134,7 @@ void DtlsStunPiggybackController::ReportDataPiggybacked( state_ = State::CONFIRMED; } - if (ack != nullptr) { + if (ack != nullptr && !ack->string_view().empty()) { RTC_LOG(LS_VERBOSE) << "DTLS-STUN piggybacking ACK: " << rtc::hex_encode(ack->string_view()); } diff --git a/p2p/dtls/dtls_stun_piggyback_controller.h b/p2p/dtls/dtls_stun_piggyback_controller.h index e505eab646..c25d93f3f1 100644 --- a/p2p/dtls/dtls_stun_piggyback_controller.h +++ b/p2p/dtls/dtls_stun_piggyback_controller.h @@ -60,10 +60,12 @@ class DtlsStunPiggybackController { } // Called by DtlsTransport when handshake is complete. - void SetDtlsHandshakeComplete(bool is_dtls_client); + void SetDtlsHandshakeComplete(bool is_dtls_client, bool is_dtls13); - // Called by DtlsTransport transport when there is data to piggyback. - void SetDataToPiggyback(rtc::ArrayView data); + // Intercepts DTLS packets which should go into the STUN packets during the + // handshake. + bool MaybeConsumePacket(rtc::ArrayView data); + void ClearCachedPacketForTesting(); // Called by Connection, when sending a STUN BINDING { REQUEST / RESPONSE } // to obtain optional DTLS data or ACKs. diff --git a/p2p/dtls/dtls_stun_piggyback_controller_unittest.cc b/p2p/dtls/dtls_stun_piggyback_controller_unittest.cc index 42e6e0f1fa..80b28aaf28 100644 --- a/p2p/dtls/dtls_stun_piggyback_controller_unittest.cc +++ b/p2p/dtls/dtls_stun_piggyback_controller_unittest.cc @@ -65,7 +65,11 @@ class DtlsStunPiggybackControllerTest : public ::testing::Test { void SendClientToServer(const std::vector data, StunMessageType type) { - client_.SetDataToPiggyback(data); + if (!data.empty()) { + EXPECT_TRUE(client_.MaybeConsumePacket(data)); + } else { + client_.ClearCachedPacketForTesting(); + } std::unique_ptr attr_data; if (client_.GetDataToPiggyback(type)) { attr_data = std::make_unique( @@ -77,14 +81,14 @@ class DtlsStunPiggybackControllerTest : public ::testing::Test { STUN_ATTR_META_DTLS_IN_STUN_ACK, *client_.GetAckToPiggyback(type)); } server_.ReportDataPiggybacked(attr_data.get(), attr_ack.get()); - if (data == dtls_flight3) { - // When receiving flight 3, server handshake is complete. - server_.SetDtlsHandshakeComplete(/*is_client=*/false); - } } void SendServerToClient(const std::vector data, StunMessageType type) { - server_.SetDataToPiggyback(data); + if (!data.empty()) { + EXPECT_TRUE(server_.MaybeConsumePacket(data)); + } else { + server_.ClearCachedPacketForTesting(); + } std::unique_ptr attr_data; if (server_.GetDataToPiggyback(type)) { attr_data = std::make_unique( @@ -97,8 +101,11 @@ class DtlsStunPiggybackControllerTest : public ::testing::Test { } client_.ReportDataPiggybacked(attr_data.get(), attr_ack.get()); if (data == dtls_flight4) { + // After sending flight 4, the server handshake is complete. + server_.SetDtlsHandshakeComplete(/*is_client=*/false, + /*is_dtls13=*/false); // When receiving flight 4, client handshake is complete. - client_.SetDtlsHandshakeComplete(/*is_client=*/true); + client_.SetDtlsHandshakeComplete(/*is_client=*/true, /*is_dtls13=*/false); } } @@ -143,7 +150,7 @@ TEST_F(DtlsStunPiggybackControllerTest, FirstClientPacketLost) { // Flight 2+3 SendServerToClient(dtls_flight2, STUN_BINDING_REQUEST); SendClientToServer(dtls_flight3, STUN_BINDING_RESPONSE); - EXPECT_EQ(server_.state(), State::PENDING); + EXPECT_EQ(server_.state(), State::CONFIRMED); EXPECT_EQ(client_.state(), State::CONFIRMED); // Flight 4 @@ -235,9 +242,9 @@ TEST_F(DtlsStunPiggybackControllerTest, DisableSupport(server_); // Set DTLS complete after normal handshake. - client_.SetDtlsHandshakeComplete(true); + client_.SetDtlsHandshakeComplete(/*is_client=*/true, /*is_dtls13=*/false); EXPECT_EQ(client_.state(), State::OFF); - server_.SetDtlsHandshakeComplete(true); + server_.SetDtlsHandshakeComplete(/*is_client=*/false, /*is_dtls13=*/false); EXPECT_EQ(server_.state(), State::OFF); } diff --git a/p2p/dtls/dtls_transport.cc b/p2p/dtls/dtls_transport.cc index 6f73c7b577..41ebe78e58 100644 --- a/p2p/dtls/dtls_transport.cc +++ b/p2p/dtls/dtls_transport.cc @@ -99,12 +99,6 @@ rtc::StreamResult StreamInterfaceChannel::Write( int& /* error */) { RTC_DCHECK_RUN_ON(&callback_sequence_); - if (IsDtlsHandshakePacket(data) && - ice_transport_->IsDtlsPiggybackSupportedByPeer()) { - ice_transport_->SetDtlsDataToPiggyback(data); - // The ICE transport is responsible for dropping these packets. - } - // Always succeeds, since this is an unreliable transport anyway. // TODO(zhihuang): Should this block if ice_transport_'s temporarily // unwritable? @@ -545,12 +539,12 @@ void DtlsTransport::ConnectToIceTransport() { [this](rtc::PacketTransportInternal* transport, const rtc::ReceivedPacket& packet) { RTC_DCHECK(dtls_active_); - RTC_DCHECK(IsDtlsHandshakePacket(packet.payload())); + RTC_DCHECK(IsDtlsPacket(packet.payload())); if (!dtls_active_) { // Not doing DTLS. return; } - if (!IsDtlsHandshakePacket(packet.payload())) { + if (!IsDtlsPacket(packet.payload())) { return; } OnReadPacket(transport, packet); @@ -748,15 +742,22 @@ void DtlsTransport::OnReadyToSend( void DtlsTransport::OnDtlsEvent(int sig, int err) { RTC_DCHECK_RUN_ON(&thread_checker_); + RTC_DCHECK(dtls_); + if (sig & rtc::SE_OPEN) { // This is the first time. RTC_LOG(LS_INFO) << ToString() << ": DTLS handshake complete."; + // The check for OPEN shouldn't be necessary but let's make + // sure we don't accidentally frob the state if it's closed. if (dtls_->GetState() == rtc::SS_OPEN) { - // The check for OPEN shouldn't be necessary but let's make - // sure we don't accidentally frob the state if it's closed. + int ssl_version_bytes; + bool ret = dtls_->GetSslVersionBytes(&ssl_version_bytes); + RTC_DCHECK(ret); + ice_transport_->SetDtlsHandshakeComplete( + dtls_role_ == rtc::SSL_CLIENT, + ssl_version_bytes == rtc::kDtls13VersionBytes); set_dtls_state(webrtc::DtlsTransportState::kConnected); set_writable(true); - ice_transport_->SetDtlsHandshakeComplete(dtls_role_ == rtc::SSL_CLIENT); } } if (sig & rtc::SE_READ) {