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 <steveanton@webrtc.org>
Commit-Queue: Qingsi Wang <qingsi@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#28909}
diff --git a/p2p/base/connection.cc b/p2p/base/connection.cc
index 7c6ae5c..8b2c8d9 100644
--- a/p2p/base/connection.cc
+++ b/p2p/base/connection.cc
@@ -720,38 +720,6 @@
}
}
-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 @@
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 @@
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 92fc2ed..b872dbf 100644
--- a/p2p/base/connection.h
+++ b/p2p/base/connection.h
@@ -238,10 +238,6 @@
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 @@
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 01caaa9..f6a8bbc 100644
--- a/p2p/base/p2p_transport_channel.cc
+++ b/p2p/base/p2p_transport_channel.cc
@@ -387,7 +387,11 @@
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 @@
// 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 @@
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 f9ea59d..6fa64e0 100644
--- a/p2p/base/p2p_transport_channel.h
+++ b/p2p/base/p2p_transport_channel.h
@@ -404,6 +404,15 @@
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 1b93475..65f7d20 100644
--- a/p2p/base/p2p_transport_channel_unittest.cc
+++ b/p2p/base/p2p_transport_channel_unittest.cc
@@ -1556,6 +1556,68 @@
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 6228791..a9d7cb6 100644
--- a/p2p/base/port_allocator.cc
+++ b/p2p/base/port_allocator.cc
@@ -83,27 +83,6 @@
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 @@
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 d78b6cb..c0b0e60 100644
--- a/p2p/base/port_allocator.h
+++ b/p2p/base/port_allocator.h
@@ -244,7 +244,7 @@
// 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 @@
// 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 @@
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 b1f147d..316bc87 100644
--- a/p2p/client/basic_port_allocator.cc
+++ b/p2p/client/basic_port_allocator.cc
@@ -14,6 +14,7 @@
#include <functional>
#include <set>
#include <string>
+#include <utility>
#include <vector>
#include "absl/algorithm/container.h"
@@ -530,6 +531,19 @@
}
}
+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<int>& stun_keepalive_interval) {
RTC_DCHECK_RUN_ON(network_thread_);
@@ -578,35 +592,12 @@
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 @@
if (data->ready() && CheckCandidateFilter(c)) {
std::vector<Candidate> 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 13611e7..50cb83d 100644
--- a/p2p/client/basic_port_allocator.h
+++ b/p2p/client/basic_port_allocator.h
@@ -88,6 +88,8 @@
// 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 @@
bool CandidatesAllocationDone() const override;
void RegatherOnFailedNetworks() override;
void RegatherOnAllNetworks() override;
+ void GetCandidateStatsFromReadyPorts(
+ CandidateStatsList* candidate_stats_list) const override;
void SetStunKeepaliveIntervalForReadyPorts(
const absl::optional<int>& stun_keepalive_interval) override;
void PruneAllPorts() override;
@@ -248,14 +252,6 @@
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<PortData*> GetUnprunedPorts(
const std::vector<rtc::Network*>& networks);
// Prunes ports and signal the remote side to remove the candidates that
