From 7627fdd68a31f4873d32e2af4c78e0f16eab3bd8 Mon Sep 17 00:00:00 2001 From: Qingsi Wang Date: Mon, 19 Aug 2019 16:07:40 -0700 Subject: [PATCH] Sanitize the address field of peer-reflexive remote candidates. Per the latest WebRTC stats spec (https://w3c.github.io/webrtc-stats/#dom-rtcicecandidatestats) the address field of a peer-reflexive remote candidate should be concealed until the same address is learnt via addIceCandidate. This CL also refactors the sanitization-related code paths. Bug: chromium:968161 Change-Id: I74c5da78232b2f604689867bda2937b8af827c4f Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/149381 Reviewed-by: Steve Anton Commit-Queue: Qingsi Wang Cr-Commit-Position: refs/heads/master@{#28909} --- p2p/base/connection.cc | 41 +------------- p2p/base/connection.h | 6 --- p2p/base/p2p_transport_channel.cc | 35 ++++++++++-- p2p/base/p2p_transport_channel.h | 9 ++++ p2p/base/p2p_transport_channel_unittest.cc | 62 ++++++++++++++++++++++ p2p/base/port_allocator.cc | 42 +++++++-------- p2p/base/port_allocator.h | 12 ++++- p2p/client/basic_port_allocator.cc | 45 +++++++--------- p2p/client/basic_port_allocator.h | 12 ++--- 9 files changed, 158 insertions(+), 106 deletions(-) diff --git a/p2p/base/connection.cc b/p2p/base/connection.cc index 7c6ae5cb4a..8b2c8d96db 100644 --- a/p2p/base/connection.cc +++ b/p2p/base/connection.cc @@ -720,38 +720,6 @@ void Connection::HandlePiggybackCheckAcknowledgementIfAny(StunMessage* msg) { } } -CandidatePair Connection::ToCandidatePairAndSanitizeIfNecessary() const { - auto get_sanitized_copy = [](const Candidate& c) { - bool use_hostname_address = c.type() == LOCAL_PORT_TYPE; - bool filter_related_address = c.type() == STUN_PORT_TYPE; - return c.ToSanitizedCopy(use_hostname_address, filter_related_address); - }; - - CandidatePair pair; - if (port_->Network()->GetMdnsResponder() != nullptr) { - // When the mDNS obfuscation of local IPs is enabled, we sanitize local - // candidates. - pair.local = get_sanitized_copy(local_candidate()); - } else { - pair.local = local_candidate(); - } - - if (!remote_candidate().address().hostname().empty()) { - // If the remote endpoint signaled us a hostname candidate, we assume it is - // supposed to be sanitized in the stats. - // - // A prflx remote candidate should not have a hostname set. - RTC_DCHECK(remote_candidate().type() != PRFLX_PORT_TYPE); - // A remote hostname candidate should have a resolved IP before we can form - // a candidate pair. - RTC_DCHECK(!remote_candidate().address().IsUnresolvedIP()); - pair.remote = get_sanitized_copy(remote_candidate()); - } else { - pair.remote = remote_candidate(); - } - return pair; -} - void Connection::ReceivedPingResponse( int rtt, const std::string& request_id, @@ -1061,7 +1029,8 @@ ConnectionInfo Connection::stats() { stats_.nominated = nominated(); stats_.total_round_trip_time_ms = total_round_trip_time_ms_; stats_.current_round_trip_time_ms = current_round_trip_time_ms_; - CopyCandidatesToStatsAndSanitizeIfNecessary(); + stats_.local_candidate = local_candidate(); + stats_.remote_candidate = remote_candidate(); return stats_; } @@ -1138,12 +1107,6 @@ void Connection::MaybeUpdateLocalCandidate(ConnectionRequest* request, SignalStateChange(this); } -void Connection::CopyCandidatesToStatsAndSanitizeIfNecessary() { - auto pair = ToCandidatePairAndSanitizeIfNecessary(); - stats_.local_candidate = pair.local_candidate(); - stats_.remote_candidate = pair.remote_candidate(); -} - bool Connection::rtt_converged() const { return rtt_samples_ > (RTT_RATIO + 1); } diff --git a/p2p/base/connection.h b/p2p/base/connection.h index 92fc2ed9cb..b872dbfd70 100644 --- a/p2p/base/connection.h +++ b/p2p/base/connection.h @@ -238,10 +238,6 @@ class Connection : public CandidatePairInterface, void HandlePiggybackCheckAcknowledgementIfAny(StunMessage* msg); int64_t last_data_received() const { return last_data_received_; } - // Returns the equivalent candidate pair and sanitizes the local and the - // remote candidates if necessary. - CandidatePair ToCandidatePairAndSanitizeIfNecessary() const; - // Debugging description of this connection std::string ToDebugId() const; std::string ToString() const; @@ -346,8 +342,6 @@ class Connection : public CandidatePairInterface, void MaybeUpdateLocalCandidate(ConnectionRequest* request, StunMessage* response); - void CopyCandidatesToStatsAndSanitizeIfNecessary(); - void LogCandidatePairConfig(webrtc::IceCandidatePairConfigType type); void LogCandidatePairEvent(webrtc::IceCandidatePairEventType type, uint32_t transaction_id); diff --git a/p2p/base/p2p_transport_channel.cc b/p2p/base/p2p_transport_channel.cc index 01caaa9b10..f6a8bbc8d7 100644 --- a/p2p/base/p2p_transport_channel.cc +++ b/p2p/base/p2p_transport_channel.cc @@ -387,7 +387,11 @@ P2PTransportChannel::GetSelectedCandidatePair() const { return absl::nullopt; } - return selected_connection_->ToCandidatePairAndSanitizeIfNecessary(); + CandidatePair pair; + pair.local = SanitizeLocalCandidate(selected_connection_->local_candidate()); + pair.remote = + SanitizeRemoteCandidate(selected_connection_->remote_candidate()); + return pair; } // A channel is considered ICE completed once there is at most one active @@ -1502,9 +1506,11 @@ bool P2PTransportChannel::GetStats(ConnectionInfos* candidate_pair_stats_list, // TODO(qingsi): Remove naming inconsistency for candidate pair/connection. for (Connection* connection : connections_) { - ConnectionInfo candidate_pair_stats = connection->stats(); - candidate_pair_stats.best_connection = (selected_connection_ == connection); - candidate_pair_stats_list->push_back(std::move(candidate_pair_stats)); + ConnectionInfo stats = connection->stats(); + stats.local_candidate = SanitizeLocalCandidate(stats.local_candidate); + stats.remote_candidate = SanitizeRemoteCandidate(stats.remote_candidate); + stats.best_connection = (selected_connection_ == connection); + candidate_pair_stats_list->push_back(std::move(stats)); connection->set_reported(true); } @@ -2645,6 +2651,27 @@ void P2PTransportChannel::SetReceiving(bool receiving) { SignalReceivingState(this); } +Candidate P2PTransportChannel::SanitizeLocalCandidate( + const Candidate& c) const { + RTC_DCHECK_RUN_ON(network_thread_); + // Delegates to the port allocator. + return allocator_->SanitizeCandidate(c); +} + +Candidate P2PTransportChannel::SanitizeRemoteCandidate( + const Candidate& c) const { + RTC_DCHECK_RUN_ON(network_thread_); + // If the remote endpoint signaled us a hostname host candidate, we assume it + // is supposed to be sanitized. + bool use_hostname_address = + c.type() == LOCAL_PORT_TYPE && !c.address().hostname().empty(); + // Remove the address for prflx remote candidates. See + // https://w3c.github.io/webrtc-stats/#dom-rtcicecandidatestats. + use_hostname_address |= c.type() == PRFLX_PORT_TYPE; + return c.ToSanitizedCopy(use_hostname_address, + false /* filter_related_address */); +} + void P2PTransportChannel::LogCandidatePairConfig( Connection* conn, webrtc::IceCandidatePairConfigType type) { diff --git a/p2p/base/p2p_transport_channel.h b/p2p/base/p2p_transport_channel.h index f9ea59d12e..6fa64e0055 100644 --- a/p2p/base/p2p_transport_channel.h +++ b/p2p/base/p2p_transport_channel.h @@ -404,6 +404,15 @@ class RTC_EXPORT P2PTransportChannel : public IceTransportInternal { void SetWritable(bool writable); // Sets the receiving state, signaling if necessary. void SetReceiving(bool receiving); + // Clears the address and the related address fields of a local candidate to + // avoid IP leakage. This is applicable in several scenarios as commented in + // |PortAllocator::SanitizeCandidate|. + Candidate SanitizeLocalCandidate(const Candidate& c) const; + // Clears the address field of a remote candidate to avoid IP leakage. This is + // applicable in the following scenarios: + // 1. mDNS candidates are received. + // 2. Peer-reflexive remote candidates. + Candidate SanitizeRemoteCandidate(const Candidate& c) const; std::string transport_name_ RTC_GUARDED_BY(network_thread_); int component_ RTC_GUARDED_BY(network_thread_); diff --git a/p2p/base/p2p_transport_channel_unittest.cc b/p2p/base/p2p_transport_channel_unittest.cc index 1b9347565f..65f7d20eba 100644 --- a/p2p/base/p2p_transport_channel_unittest.cc +++ b/p2p/base/p2p_transport_channel_unittest.cc @@ -1556,6 +1556,68 @@ TEST_F(P2PTransportChannelTest, PeerReflexiveCandidateBeforeSignaling) { DestroyChannels(); } +// Test that if we learn a prflx remote candidate, its address is concealed in +// 1. the selected candidate pair accessed via the public API, and +// 2. the candidate pair stats +// until we learn the same address from signaling. +TEST_F(P2PTransportChannelTest, PeerReflexiveRemoteCandidateIsSanitized) { + ConfigureEndpoints(OPEN, OPEN, kOnlyLocalPorts, kOnlyLocalPorts); + // Emulate no remote parameters coming in. + set_remote_ice_parameter_source(FROM_CANDIDATE); + CreateChannels(); + // Only have remote parameters come in for ep2, not ep1. + ep2_ch1()->SetRemoteIceParameters(kIceParams[0]); + + // Pause sending ep2's candidates to ep1 until ep1 receives the peer reflexive + // candidate. + PauseCandidates(1); + + ASSERT_TRUE_WAIT(ep2_ch1()->selected_connection() != nullptr, kMediumTimeout); + ep1_ch1()->SetRemoteIceParameters(kIceParams[1]); + ASSERT_TRUE_WAIT(ep1_ch1()->selected_connection() != nullptr, kMediumTimeout); + + // Check the selected candidate pair. + auto pair_ep1 = ep1_ch1()->GetSelectedCandidatePair(); + ASSERT_TRUE(pair_ep1.has_value()); + EXPECT_EQ(PRFLX_PORT_TYPE, pair_ep1->remote_candidate().type()); + EXPECT_TRUE(pair_ep1->remote_candidate().address().ipaddr().IsNil()); + + ConnectionInfos pair_stats; + CandidateStatsList candidate_stats; + ep1_ch1()->GetStats(&pair_stats, &candidate_stats); + // Check the candidate pair stats. + ASSERT_EQ(1u, pair_stats.size()); + EXPECT_EQ(PRFLX_PORT_TYPE, pair_stats[0].remote_candidate.type()); + EXPECT_TRUE(pair_stats[0].remote_candidate.address().ipaddr().IsNil()); + + // Let ep1 receive the remote candidate to update its type from prflx to host. + ResumeCandidates(1); + ASSERT_TRUE_WAIT( + ep1_ch1()->selected_connection() != nullptr && + ep1_ch1()->selected_connection()->remote_candidate().type() == + LOCAL_PORT_TYPE, + kMediumTimeout); + + // We should be able to reveal the address after it is learnt via + // AddIceCandidate. + // + // Check the selected candidate pair. + auto updated_pair_ep1 = ep1_ch1()->GetSelectedCandidatePair(); + ASSERT_TRUE(updated_pair_ep1.has_value()); + EXPECT_EQ(LOCAL_PORT_TYPE, updated_pair_ep1->remote_candidate().type()); + EXPECT_TRUE( + updated_pair_ep1->remote_candidate().address().EqualIPs(kPublicAddrs[1])); + + ep1_ch1()->GetStats(&pair_stats, &candidate_stats); + // Check the candidate pair stats. + ASSERT_EQ(1u, pair_stats.size()); + EXPECT_EQ(LOCAL_PORT_TYPE, pair_stats[0].remote_candidate.type()); + EXPECT_TRUE( + pair_stats[0].remote_candidate.address().EqualIPs(kPublicAddrs[1])); + + DestroyChannels(); +} + // Test that we properly create a connection on a STUN ping from unknown address // when the signaling is slow and the end points are behind NAT. TEST_F(P2PTransportChannelTest, PeerReflexiveCandidateBeforeSignalingWithNAT) { diff --git a/p2p/base/port_allocator.cc b/p2p/base/port_allocator.cc index 62287912ed..a9d7cb6493 100644 --- a/p2p/base/port_allocator.cc +++ b/p2p/base/port_allocator.cc @@ -83,27 +83,6 @@ bool PortAllocatorSession::IsStopped() const { return false; } -void PortAllocatorSession::GetCandidateStatsFromReadyPorts( - CandidateStatsList* candidate_stats_list) const { - auto ports = ReadyPorts(); - for (auto* port : ports) { - auto candidates = port->Candidates(); - for (const auto& candidate : candidates) { - CandidateStats candidate_stats(candidate); - port->GetStunStats(&candidate_stats.stun_stats); - bool mdns_obfuscation_enabled = - port->Network()->GetMdnsResponder() != nullptr; - if (mdns_obfuscation_enabled) { - bool use_hostname_address = candidate.type() == LOCAL_PORT_TYPE; - bool filter_related_address = candidate.type() == STUN_PORT_TYPE; - candidate_stats.candidate = candidate_stats.candidate.ToSanitizedCopy( - use_hostname_address, filter_related_address); - } - candidate_stats_list->push_back(std::move(candidate_stats)); - } - } -} - uint32_t PortAllocatorSession::generation() { return generation_; } @@ -318,4 +297,25 @@ std::vector PortAllocator::GetPooledIceCredentials() { return list; } +Candidate PortAllocator::SanitizeCandidate(const Candidate& c) const { + CheckRunOnValidThreadAndInitialized(); + // For a local host candidate, we need to conceal its IP address candidate if + // the mDNS obfuscation is enabled. + bool use_hostname_address = + c.type() == LOCAL_PORT_TYPE && MdnsObfuscationEnabled(); + // If adapter enumeration is disabled or host candidates are disabled, + // clear the raddr of STUN candidates to avoid local address leakage. + bool filter_stun_related_address = + ((flags() & PORTALLOCATOR_DISABLE_ADAPTER_ENUMERATION) && + (flags() & PORTALLOCATOR_DISABLE_DEFAULT_LOCAL_CANDIDATE)) || + !(candidate_filter_ & CF_HOST) || MdnsObfuscationEnabled(); + // If the candidate filter doesn't allow reflexive addresses, empty TURN raddr + // to avoid reflexive address leakage. + bool filter_turn_related_address = !(candidate_filter_ & CF_REFLEXIVE); + bool filter_related_address = + ((c.type() == STUN_PORT_TYPE && filter_stun_related_address) || + (c.type() == RELAY_PORT_TYPE && filter_turn_related_address)); + return c.ToSanitizedCopy(use_hostname_address, filter_related_address); +} + } // namespace cricket diff --git a/p2p/base/port_allocator.h b/p2p/base/port_allocator.h index d78b6cbc65..c0b0e605dc 100644 --- a/p2p/base/port_allocator.h +++ b/p2p/base/port_allocator.h @@ -244,7 +244,7 @@ class RTC_EXPORT PortAllocatorSession : public sigslot::has_slots<> { // Get candidate-level stats from all candidates on the ready ports and return // the stats to the given list. virtual void GetCandidateStatsFromReadyPorts( - CandidateStatsList* candidate_stats_list) const; + CandidateStatsList* candidate_stats_list) const {} // Set the interval at which STUN candidates will resend STUN binding requests // on the underlying ports to keep NAT bindings open. // The default value of the interval in implementation is restored if a null @@ -430,6 +430,13 @@ class RTC_EXPORT PortAllocator : public sigslot::has_slots<> { // Discard any remaining pooled sessions. void DiscardCandidatePool(); + // Clears the address and the related address fields of a local candidate to + // avoid IP leakage. This is applicable in several scenarios: + // 1. Sanitization is configured via the candidate filter. + // 2. Sanitization is configured via the port allocator flags. + // 3. mDNS concealment of private IPs is enabled. + Candidate SanitizeCandidate(const Candidate& c) const; + uint32_t flags() const { CheckRunOnValidThreadIfInitialized(); return flags_; @@ -594,6 +601,9 @@ class RTC_EXPORT PortAllocator : public sigslot::has_slots<> { return pooled_sessions_; } + // Returns true if there is an mDNS responder attached to the network manager. + virtual bool MdnsObfuscationEnabled() const { return false; } + // The following thread checks are only done in DCHECK for the consistency // with the exsiting thread checks. void CheckRunOnValidThreadIfInitialized() const { diff --git a/p2p/client/basic_port_allocator.cc b/p2p/client/basic_port_allocator.cc index b1f147dcdc..316bc879dd 100644 --- a/p2p/client/basic_port_allocator.cc +++ b/p2p/client/basic_port_allocator.cc @@ -14,6 +14,7 @@ #include #include #include +#include #include #include "absl/algorithm/container.h" @@ -530,6 +531,19 @@ void BasicPortAllocatorSession::Regather( } } +void BasicPortAllocatorSession::GetCandidateStatsFromReadyPorts( + CandidateStatsList* candidate_stats_list) const { + auto ports = ReadyPorts(); + for (auto* port : ports) { + auto candidates = port->Candidates(); + for (const auto& candidate : candidates) { + CandidateStats candidate_stats(allocator_->SanitizeCandidate(candidate)); + port->GetStunStats(&candidate_stats.stun_stats); + candidate_stats_list->push_back(std::move(candidate_stats)); + } + } +} + void BasicPortAllocatorSession::SetStunKeepaliveIntervalForReadyPorts( const absl::optional& stun_keepalive_interval) { RTC_DCHECK_RUN_ON(network_thread_); @@ -578,35 +592,12 @@ void BasicPortAllocatorSession::GetCandidatesFromPort( if (!CheckCandidateFilter(candidate)) { continue; } - auto sanitized_candidate = SanitizeCandidate(candidate); - candidates->push_back(sanitized_candidate); + candidates->push_back(allocator_->SanitizeCandidate(candidate)); } } -bool BasicPortAllocatorSession::MdnsObfuscationEnabled() const { - return allocator_->network_manager()->GetMdnsResponder() != nullptr; -} - -Candidate BasicPortAllocatorSession::SanitizeCandidate( - const Candidate& c) const { - RTC_DCHECK_RUN_ON(network_thread_); - // If the candidate has a generated hostname, we need to obfuscate its IP - // address when signaling this candidate. - bool use_hostname_address = - !c.address().hostname().empty() && !c.address().IsUnresolvedIP(); - // If adapter enumeration is disabled or host candidates are disabled, - // clear the raddr of STUN candidates to avoid local address leakage. - bool filter_stun_related_address = - ((flags() & PORTALLOCATOR_DISABLE_ADAPTER_ENUMERATION) && - (flags() & PORTALLOCATOR_DISABLE_DEFAULT_LOCAL_CANDIDATE)) || - !(candidate_filter_ & CF_HOST) || MdnsObfuscationEnabled(); - // If the candidate filter doesn't allow reflexive addresses, empty TURN raddr - // to avoid reflexive address leakage. - bool filter_turn_related_address = !(candidate_filter_ & CF_REFLEXIVE); - bool filter_related_address = - ((c.type() == STUN_PORT_TYPE && filter_stun_related_address) || - (c.type() == RELAY_PORT_TYPE && filter_turn_related_address)); - return c.ToSanitizedCopy(use_hostname_address, filter_related_address); +bool BasicPortAllocator::MdnsObfuscationEnabled() const { + return network_manager()->GetMdnsResponder() != nullptr; } bool BasicPortAllocatorSession::CandidatesAllocationDone() const { @@ -1014,7 +1005,7 @@ void BasicPortAllocatorSession::OnCandidateReady(Port* port, if (data->ready() && CheckCandidateFilter(c)) { std::vector candidates; - candidates.push_back(SanitizeCandidate(c)); + candidates.push_back(allocator_->SanitizeCandidate(c)); SignalCandidatesReady(this, candidates); } else { RTC_LOG(LS_INFO) << "Discarding candidate because it doesn't match filter."; diff --git a/p2p/client/basic_port_allocator.h b/p2p/client/basic_port_allocator.h index 13611e724f..50cb83d442 100644 --- a/p2p/client/basic_port_allocator.h +++ b/p2p/client/basic_port_allocator.h @@ -88,6 +88,8 @@ class RTC_EXPORT BasicPortAllocator : public PortAllocator { // This function makes sure that relay_port_factory_ is set properly. void InitRelayPortFactory(RelayPortFactoryInterface* relay_port_factory); + bool MdnsObfuscationEnabled() const override; + rtc::NetworkManager* network_manager_; rtc::PacketSocketFactory* socket_factory_; bool allow_tcp_listen_; @@ -147,6 +149,8 @@ class RTC_EXPORT BasicPortAllocatorSession : public PortAllocatorSession, bool CandidatesAllocationDone() const override; void RegatherOnFailedNetworks() override; void RegatherOnAllNetworks() override; + void GetCandidateStatsFromReadyPorts( + CandidateStatsList* candidate_stats_list) const override; void SetStunKeepaliveIntervalForReadyPorts( const absl::optional& stun_keepalive_interval) override; void PruneAllPorts() override; @@ -248,14 +252,6 @@ class RTC_EXPORT BasicPortAllocatorSession : public PortAllocatorSession, bool CheckCandidateFilter(const Candidate& c) const; bool CandidatePairable(const Candidate& c, const Port* port) const; - // Returns true if there is an mDNS responder attached to the network manager - bool MdnsObfuscationEnabled() const; - - // Clears 1) the address if the candidate is supposedly a hostname candidate; - // 2) the related address according to the flags and candidate filter in order - // to avoid leaking any information. - Candidate SanitizeCandidate(const Candidate& c) const; - std::vector GetUnprunedPorts( const std::vector& networks); // Prunes ports and signal the remote side to remove the candidates that