From 72d2ddd36fec38e6aea31a0558232df7e2815f4b Mon Sep 17 00:00:00 2001 From: Jeroen de Borst Date: Tue, 27 Nov 2018 13:20:39 -0800 Subject: [PATCH] Fix raddr on srflx and relay candidates Bug: chromium:905690 Change-Id: Ic16d21672db5d456d7a9727ea5194ec26338c9d0 Reviewed-on: https://webrtc-review.googlesource.com/c/111441 Commit-Queue: Jeroen de Borst Reviewed-by: Justin Uberti Reviewed-by: Qingsi Wang Reviewed-by: Steve Anton Cr-Commit-Position: refs/heads/master@{#25810} --- p2p/base/port.cc | 74 +++++++++++++---------- p2p/base/port.h | 4 ++ p2p/client/basicportallocator.cc | 6 +- p2p/client/basicportallocator.h | 4 ++ p2p/client/basicportallocator_unittest.cc | 30 +++++---- 5 files changed, 72 insertions(+), 46 deletions(-) diff --git a/p2p/base/port.cc b/p2p/base/port.cc index 4954744cc8..5b8e02d28f 100644 --- a/p2p/base/port.cc +++ b/p2p/base/port.cc @@ -430,42 +430,50 @@ void Port::AddAddress(const rtc::SocketAddress& address, c.set_network_name(network_->name()); c.set_network_type(network_->type()); c.set_url(url); + c.set_related_address(related_address); + + bool pending = MaybeObfuscateAddress(&c, type, is_final); + + if (!pending) { + FinishAddingAddress(c, is_final); + } +} + +bool Port::MaybeObfuscateAddress(Candidate* c, + const std::string& type, + bool is_final) { // TODO(bugs.webrtc.org/9723): Use a config to control the feature of IP // handling with mDNS. - if (network_->GetMdnsResponder() != nullptr) { - // Obfuscate the IP address of a host candidates by an mDNS hostname. - if (type == LOCAL_PORT_TYPE) { - auto weak_ptr = weak_factory_.GetWeakPtr(); - auto callback = [weak_ptr, c, is_final](const rtc::IPAddress& addr, - const std::string& name) mutable { - RTC_DCHECK(c.address().ipaddr() == addr); - rtc::SocketAddress hostname_address(name, c.address().port()); - // In Port and Connection, we need the IP address information to - // correctly handle the update of candidate type to prflx. The removal - // of IP address when signaling this candidate will take place in - // BasicPortAllocatorSession::OnCandidateReady, via SanitizeCandidate. - hostname_address.SetResolvedIP(addr); - c.set_address(hostname_address); - RTC_DCHECK(c.related_address() == rtc::SocketAddress()); - if (weak_ptr != nullptr) { - weak_ptr->set_mdns_name_registration_status( - MdnsNameRegistrationStatus::kCompleted); - weak_ptr->FinishAddingAddress(c, is_final); - } - }; - set_mdns_name_registration_status( - MdnsNameRegistrationStatus::kInProgress); - network_->GetMdnsResponder()->CreateNameForAddress(c.address().ipaddr(), - callback); - return; - } - // For other types of candidates, the related address should be set to - // 0.0.0.0 or ::0. - c.set_related_address(rtc::SocketAddress()); - } else { - c.set_related_address(related_address); + if (network_->GetMdnsResponder() == nullptr) { + return false; } - FinishAddingAddress(c, is_final); + if (type != LOCAL_PORT_TYPE) { + return false; + } + + auto copy = *c; + auto weak_ptr = weak_factory_.GetWeakPtr(); + auto callback = [weak_ptr, copy, is_final](const rtc::IPAddress& addr, + const std::string& name) mutable { + RTC_DCHECK(copy.address().ipaddr() == addr); + rtc::SocketAddress hostname_address(name, copy.address().port()); + // In Port and Connection, we need the IP address information to + // correctly handle the update of candidate type to prflx. The removal + // of IP address when signaling this candidate will take place in + // BasicPortAllocatorSession::OnCandidateReady, via SanitizeCandidate. + hostname_address.SetResolvedIP(addr); + copy.set_address(hostname_address); + copy.set_related_address(rtc::SocketAddress()); + if (weak_ptr != nullptr) { + weak_ptr->set_mdns_name_registration_status( + MdnsNameRegistrationStatus::kCompleted); + weak_ptr->FinishAddingAddress(copy, is_final); + } + }; + set_mdns_name_registration_status(MdnsNameRegistrationStatus::kInProgress); + network_->GetMdnsResponder()->CreateNameForAddress(copy.address().ipaddr(), + callback); + return true; } void Port::FinishAddingAddress(const Candidate& c, bool is_final) { diff --git a/p2p/base/port.h b/p2p/base/port.h index e0b3f07860..9a8f92a96e 100644 --- a/p2p/base/port.h +++ b/p2p/base/port.h @@ -514,6 +514,10 @@ class Port : public PortInterface, rtc::WeakPtrFactory weak_factory_; + bool MaybeObfuscateAddress(Candidate* c, + const std::string& type, + bool is_final); + friend class Connection; }; diff --git a/p2p/client/basicportallocator.cc b/p2p/client/basicportallocator.cc index 39895dfa5a..0c2fef3112 100644 --- a/p2p/client/basicportallocator.cc +++ b/p2p/client/basicportallocator.cc @@ -518,6 +518,10 @@ void BasicPortAllocatorSession::GetCandidatesFromPort( } } +bool BasicPortAllocatorSession::MdnsObfuscationEnabled() const { + return allocator_->network_manager()->GetMdnsResponder() != nullptr; +} + Candidate BasicPortAllocatorSession::SanitizeCandidate( const Candidate& c) const { RTC_DCHECK_RUN_ON(network_thread_); @@ -534,7 +538,7 @@ Candidate BasicPortAllocatorSession::SanitizeCandidate( bool filter_stun_related_address = ((flags() & PORTALLOCATOR_DISABLE_ADAPTER_ENUMERATION) && (flags() & PORTALLOCATOR_DISABLE_DEFAULT_LOCAL_CANDIDATE)) || - !(candidate_filter_ & CF_HOST); + !(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); diff --git a/p2p/client/basicportallocator.h b/p2p/client/basicportallocator.h index 1012d92f96..672f3ddb7c 100644 --- a/p2p/client/basicportallocator.h +++ b/p2p/client/basicportallocator.h @@ -236,6 +236,10 @@ 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. diff --git a/p2p/client/basicportallocator_unittest.cc b/p2p/client/basicportallocator_unittest.cc index a07569e94b..8943ebc73d 100644 --- a/p2p/client/basicportallocator_unittest.cc +++ b/p2p/client/basicportallocator_unittest.cc @@ -2246,8 +2246,8 @@ TEST_F(BasicPortAllocatorTest, IceRegatheringMetricsLoggedWhenNetworkChanges) { } // Test that when an mDNS responder is present, the local address of a host -// candidate is masked by an mDNS hostname and the related address of any other -// type of candidates is set to 0.0.0.0 or ::0. +// candidate is concealed by an mDNS hostname and the related address of a srflx +// candidate is set to 0.0.0.0 or ::0. TEST_F(BasicPortAllocatorTest, HostCandidateAddressIsReplacedByHostname) { // Default config uses GTURN and no NAT, so replace that with the // desired setup (NAT, STUN server, TURN server, UDP/TCP). @@ -2269,23 +2269,29 @@ TEST_F(BasicPortAllocatorTest, HostCandidateAddressIsReplacedByHostname) { int num_srflx_candidates = 0; int num_relay_candidates = 0; for (const auto& candidate : candidates_) { + const auto& raddr = candidate.related_address(); + if (candidate.type() == LOCAL_PORT_TYPE) { - EXPECT_TRUE(candidate.address().IsUnresolvedIP()); + EXPECT_FALSE(candidate.address().hostname().empty()); + EXPECT_TRUE(raddr.IsNil()); if (candidate.protocol() == UDP_PROTOCOL_NAME) { ++num_host_udp_candidates; } else { ++num_host_tcp_candidates; } + } else if (candidate.type() == STUN_PORT_TYPE) { + // For a srflx candidate, the related address should be set to 0.0.0.0 or + // ::0 + EXPECT_TRUE(IPIsAny(raddr.ipaddr())); + EXPECT_EQ(raddr.port(), 0); + ++num_srflx_candidates; + } else if (candidate.type() == RELAY_PORT_TYPE) { + EXPECT_EQ(kNatUdpAddr.ipaddr(), raddr.ipaddr()); + EXPECT_EQ(kNatUdpAddr.family(), raddr.family()); + ++num_relay_candidates; } else { - EXPECT_NE(PRFLX_PORT_TYPE, candidate.type()); - // The related address should be set to 0.0.0.0 or ::0 for srflx and - // relay candidates. - EXPECT_EQ(rtc::SocketAddress(), candidate.related_address()); - if (candidate.type() == STUN_PORT_TYPE) { - ++num_srflx_candidates; - } else { - ++num_relay_candidates; - } + // prflx candidates are not expected + FAIL(); } } EXPECT_EQ(1, num_host_udp_candidates);