From 1cd97e2602e0d4a501dd3765fceb331b5d6bd0ce Mon Sep 17 00:00:00 2001 From: Philipp Hancke Date: Thu, 27 Oct 2022 09:56:08 +0200 Subject: [PATCH] [M108] ice: include tiebreaker in computation of foundation attribute the foundation attribute is currently calculated as CRC32(baseaddress, protocol, relayprotocol) which is a way to satisfy the requirements from https://www.rfc-editor.org/rfc/rfc5245#section-4.1.1.3 However, this leaks the base address which defeats the MDNS obfuscation described in https://datatracker.ietf.org/doc/draft-ietf-mmusic-mdns-ice-candidates/ since the CRC32 can be reversed using a table lookup as shown in https://github.com/niespodd/webrtc-local-ip-leak/ To defeat that lookup, "seed" the CRC32 with the ICE tie-breaker which is a randomly picked unsigned 64 bit integer described in https://www.rfc-editor.org/rfc/rfc5245#section-5.2 The tie-breaker is not known to Javascript and adding it scopes the foundation within the peer connection as described in section 4.1.1.3 To manually test (preferably with a DCHECK for IceTiebreaker() in ComputeFoundation) - gather candidates twice on https://webrtc.github.io/samples/src/content/peerconnection/trickle-ice/ and observe that the foundations are not the same after this change - create two RTCPeerConnections with {iceCandidatePoolSize: 1}, create a datachannel, call setLocalDescription, inspect the candidates and observe that the foundations are not the same after this change. Unit test changes have been split into a separate CL for easier integration. BUG=webrtc:14605 (cherry picked from commit 08b882d762edadb9797334859d915c5c1e34896b) Change-Id: I6bbad1635b48997b00ae74d251ae357bf8afd12f Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/280621 Reviewed-by: Harald Alvestrand Reviewed-by: Jonas Oreland Commit-Queue: Harald Alvestrand Cr-Original-Commit-Position: refs/heads/main@{#38485} Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/281220 Commit-Queue: Philipp Hancke Cr-Commit-Position: refs/branch-heads/5359@{#4} Cr-Branched-From: fb3bd4a01d7c840dfe7b3efa144c0fbcb6a97fef-refs/heads/main@{#38387} --- p2p/base/connection.cc | 2 +- p2p/base/p2p_transport_channel.cc | 3 +++ p2p/base/port.cc | 4 +++- p2p/base/port.h | 8 ++++---- p2p/base/port_allocator.cc | 15 +++++++++++++-- p2p/base/port_allocator.h | 13 +++++++++++++ p2p/client/basic_port_allocator.cc | 4 ++++ pc/jsep_transport_controller.cc | 3 +++ 8 files changed, 44 insertions(+), 8 deletions(-) diff --git a/p2p/base/connection.cc b/p2p/base/connection.cc index 44dc982c84..1606850924 100644 --- a/p2p/base/connection.cc +++ b/p2p/base/connection.cc @@ -1584,7 +1584,7 @@ void Connection::MaybeUpdateLocalCandidate(StunRequest* request, // Set the related address and foundation attributes before changing the // address. local_candidate_.set_related_address(local_candidate_.address()); - local_candidate_.set_foundation(Port::ComputeFoundation( + local_candidate_.set_foundation(port()->ComputeFoundation( PRFLX_PORT_TYPE, local_candidate_.protocol(), local_candidate_.relay_protocol(), local_candidate_.address())); local_candidate_.set_priority(priority); diff --git a/p2p/base/p2p_transport_channel.cc b/p2p/base/p2p_transport_channel.cc index 0a0d8c8b1d..c255e2402b 100644 --- a/p2p/base/p2p_transport_channel.cc +++ b/p2p/base/p2p_transport_channel.cc @@ -897,6 +897,7 @@ int P2PTransportChannel::check_receiving_interval() const { void P2PTransportChannel::MaybeStartGathering() { RTC_DCHECK_RUN_ON(network_thread_); + // TODO(bugs.webrtc.org/14605): ensure tie_breaker_ is set. if (ice_parameters_.ufrag.empty() || ice_parameters_.pwd.empty()) { RTC_LOG(LS_ERROR) << "Cannot gather candidates because ICE parameters are empty" @@ -941,6 +942,7 @@ void P2PTransportChannel::MaybeStartGathering() { ice_parameters_.ufrag, ice_parameters_.pwd); if (pooled_session) { + pooled_session->set_ice_tiebreaker(tiebreaker_); AddAllocatorSession(std::move(pooled_session)); PortAllocatorSession* raw_pooled_session = allocator_sessions_.back().get(); @@ -957,6 +959,7 @@ void P2PTransportChannel::MaybeStartGathering() { AddAllocatorSession(allocator_->CreateSession( transport_name(), component(), ice_parameters_.ufrag, ice_parameters_.pwd)); + allocator_sessions_.back()->set_ice_tiebreaker(tiebreaker_); allocator_sessions_.back()->StartGettingPorts(); } } diff --git a/p2p/base/port.cc b/p2p/base/port.cc index 6792939009..dea18a4c2a 100644 --- a/p2p/base/port.cc +++ b/p2p/base/port.cc @@ -101,8 +101,10 @@ std::string Port::ComputeFoundation(absl::string_view type, absl::string_view protocol, absl::string_view relay_protocol, const rtc::SocketAddress& base_address) { + // TODO(bugs.webrtc.org/14605): ensure IceTiebreaker() is set. rtc::StringBuilder sb; - sb << type << base_address.ipaddr().ToString() << protocol << relay_protocol; + sb << type << base_address.ipaddr().ToString() << protocol << relay_protocol + << rtc::ToString(IceTiebreaker()); return rtc::ToString(rtc::ComputeCrc32(sb.Release())); } diff --git a/p2p/base/port.h b/p2p/base/port.h index a1377a827b..d746f3de02 100644 --- a/p2p/base/port.h +++ b/p2p/base/port.h @@ -386,10 +386,10 @@ class Port : public PortInterface, public sigslot::has_slots<> { // then the foundation will be different. Two candidate pairs with // the same foundation pairs are likely to have similar network // characteristics. Foundations are used in the frozen algorithm. - static std::string ComputeFoundation(absl::string_view type, - absl::string_view protocol, - absl::string_view relay_protocol, - const rtc::SocketAddress& base_address); + std::string ComputeFoundation(absl::string_view type, + absl::string_view protocol, + absl::string_view relay_protocol, + const rtc::SocketAddress& base_address); protected: virtual void UpdateNetworkCost(); diff --git a/p2p/base/port_allocator.cc b/p2p/base/port_allocator.cc index d22b4288ce..522f0beb98 100644 --- a/p2p/base/port_allocator.cc +++ b/p2p/base/port_allocator.cc @@ -68,7 +68,8 @@ PortAllocatorSession::PortAllocatorSession(absl::string_view content_name, content_name_(content_name), component_(component), ice_ufrag_(ice_ufrag), - ice_pwd_(ice_pwd) { + ice_pwd_(ice_pwd), + tiebreaker_(0) { // Pooled sessions are allowed to be created with empty content name, // component, ufrag and password. RTC_DCHECK(ice_ufrag.empty() == ice_pwd.empty()); @@ -99,7 +100,8 @@ PortAllocator::PortAllocator() max_ipv6_networks_(kDefaultMaxIPv6Networks), step_delay_(kDefaultStepDelay), allow_tcp_listen_(true), - candidate_filter_(CF_ALL) { + candidate_filter_(CF_ALL), + tiebreaker_(0) { // The allocator will be attached to a thread in Initialize. thread_checker_.Detach(); } @@ -199,6 +201,7 @@ bool PortAllocator::SetConfiguration( PortAllocatorSession* pooled_session = CreateSessionInternal("", 0, iceCredentials.ufrag, iceCredentials.pwd); pooled_session->set_pooled(true); + pooled_session->set_ice_tiebreaker(tiebreaker_); pooled_session->StartGettingPorts(); pooled_sessions_.push_back( std::unique_ptr(pooled_session)); @@ -206,6 +209,13 @@ bool PortAllocator::SetConfiguration( return true; } +void PortAllocator::SetIceTiebreaker(uint64_t tiebreaker) { + tiebreaker_ = tiebreaker; + for (auto& pooled_session : pooled_sessions_) { + pooled_session->set_ice_tiebreaker(tiebreaker_); + } +} + std::unique_ptr PortAllocator::CreateSession( absl::string_view content_name, int component, @@ -215,6 +225,7 @@ std::unique_ptr PortAllocator::CreateSession( auto session = std::unique_ptr( CreateSessionInternal(content_name, component, ice_ufrag, ice_pwd)); session->SetCandidateFilter(candidate_filter()); + session->set_ice_tiebreaker(tiebreaker_); return session; } diff --git a/p2p/base/port_allocator.h b/p2p/base/port_allocator.h index 420560ccbb..46358439ac 100644 --- a/p2p/base/port_allocator.h +++ b/p2p/base/port_allocator.h @@ -206,6 +206,10 @@ class RTC_EXPORT PortAllocatorSession : public sigslot::has_slots<> { const std::string& ice_pwd() const { return ice_pwd_; } bool pooled() const { return pooled_; } + // TODO(bugs.webrtc.org/14605): move this to the constructor + void set_ice_tiebreaker(uint64_t tiebreaker) { tiebreaker_ = tiebreaker; } + uint64_t ice_tiebreaker() const { return tiebreaker_; } + // Setting this filter should affect not only candidates gathered in the // future, but candidates already gathered and ports already "ready", // which would be returned by ReadyCandidates() and ReadyPorts(). @@ -322,6 +326,9 @@ class RTC_EXPORT PortAllocatorSession : public sigslot::has_slots<> { bool pooled_ = false; + // TODO(bugs.webrtc.org/14605): move this to the constructor + uint64_t tiebreaker_; + // SetIceParameters is an implementation detail which only PortAllocator // should be able to call. friend class PortAllocator; @@ -374,6 +381,9 @@ class RTC_EXPORT PortAllocator : public sigslot::has_slots<> { const absl::optional& stun_candidate_keepalive_interval = absl::nullopt); + void SetIceTiebreaker(uint64_t tiebreaker); + uint64_t IceTiebreaker() const { return tiebreaker_; } + const ServerAddresses& stun_servers() const { CheckRunOnValidThreadIfInitialized(); return stun_servers_; @@ -665,6 +675,9 @@ class RTC_EXPORT PortAllocator : public sigslot::has_slots<> { // if ice_credentials is nullptr. std::vector>::const_iterator FindPooledSession(const IceParameters* ice_credentials = nullptr) const; + + // ICE tie breaker. + uint64_t tiebreaker_; }; } // namespace cricket diff --git a/p2p/client/basic_port_allocator.cc b/p2p/client/basic_port_allocator.cc index 819bc514b5..5611403bb2 100644 --- a/p2p/client/basic_port_allocator.cc +++ b/p2p/client/basic_port_allocator.cc @@ -1557,6 +1557,7 @@ void AllocationSequence::CreateUDPPorts() { } if (port) { + port->SetIceTiebreaker(session_->ice_tiebreaker()); // If shared socket is enabled, STUN candidate will be allocated by the // UDPPort. if (IsFlagSet(PORTALLOCATOR_ENABLE_SHARED_SOCKET)) { @@ -1592,6 +1593,7 @@ void AllocationSequence::CreateTCPPorts() { session_->allocator()->allow_tcp_listen(), session_->allocator()->field_trials()); if (port) { + port->SetIceTiebreaker(session_->ice_tiebreaker()); session_->AddAllocatedPort(port.release(), this); // Since TCPPort is not created using shared socket, `port` will not be // added to the dequeue. @@ -1621,6 +1623,7 @@ void AllocationSequence::CreateStunPorts() { session_->allocator()->stun_candidate_keepalive_interval(), session_->allocator()->field_trials()); if (port) { + port->SetIceTiebreaker(session_->ice_tiebreaker()); session_->AddAllocatedPort(port.release(), this); // Since StunPort is not created using shared socket, `port` will not be // added to the dequeue. @@ -1717,6 +1720,7 @@ void AllocationSequence::CreateTurnPort(const RelayServerConfig& config) { } } RTC_DCHECK(port != NULL); + port->SetIceTiebreaker(session_->ice_tiebreaker()); session_->AddAllocatedPort(port.release(), this); } } diff --git a/pc/jsep_transport_controller.cc b/pc/jsep_transport_controller.cc index 95ab7991c7..c3fc89ac57 100644 --- a/pc/jsep_transport_controller.cc +++ b/pc/jsep_transport_controller.cc @@ -62,6 +62,9 @@ JsepTransportController::JsepTransportController( RTC_DCHECK(config_.ice_transport_factory); RTC_DCHECK(config_.on_dtls_handshake_error_); RTC_DCHECK(config_.field_trials); + if (port_allocator_) { + port_allocator_->SetIceTiebreaker(ice_tiebreaker_); + } } JsepTransportController::~JsepTransportController() {