From e5defb167a731cb092dd452dfe41465696931985 Mon Sep 17 00:00:00 2001 From: Qingsi Wang Date: Thu, 15 Aug 2019 11:01:53 -0700 Subject: [PATCH] Sanitize the selected candidate pair in the public API. The public API to obtain the selected candidate pair is changed to GetSelectedCandidatePair in the ICE transport, and the returned pair has address-sanitized candidates. Bug: chromium:993878 Change-Id: I44f9d2385a84f9e22447108be2e57ef9e62671eb Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/149080 Reviewed-by: Steve Anton Commit-Queue: Qingsi Wang Cr-Commit-Position: refs/heads/master@{#28869} --- p2p/base/connection.cc | 62 ++++++++++++---------- p2p/base/connection.h | 14 +++++ p2p/base/fake_ice_transport.h | 4 ++ p2p/base/ice_transport_internal.h | 6 +++ p2p/base/mock_ice_transport.h | 4 ++ p2p/base/p2p_transport_channel.cc | 12 ++++- p2p/base/p2p_transport_channel.h | 1 + p2p/base/p2p_transport_channel_unittest.cc | 53 ++++++++++++++++++ 8 files changed, 127 insertions(+), 29 deletions(-) diff --git a/p2p/base/connection.cc b/p2p/base/connection.cc index 6d4c3de723..7c6ae5cb4a 100644 --- a/p2p/base/connection.cc +++ b/p2p/base/connection.cc @@ -720,6 +720,38 @@ 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, @@ -1107,33 +1139,9 @@ void Connection::MaybeUpdateLocalCandidate(ConnectionRequest* request, } void Connection::CopyCandidatesToStatsAndSanitizeIfNecessary() { - 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); - }; - - if (port_->Network()->GetMdnsResponder() != nullptr) { - // When the mDNS obfuscation of local IPs is enabled, we sanitize local - // candidates. - stats_.local_candidate = get_sanitized_copy(local_candidate()); - } else { - stats_.local_candidate = 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()); - stats_.remote_candidate = get_sanitized_copy(remote_candidate()); - } else { - stats_.remote_candidate = remote_candidate(); - } + auto pair = ToCandidatePairAndSanitizeIfNecessary(); + stats_.local_candidate = pair.local_candidate(); + stats_.remote_candidate = pair.remote_candidate(); } bool Connection::rtt_converged() const { diff --git a/p2p/base/connection.h b/p2p/base/connection.h index 82b2c8924e..92fc2ed9cb 100644 --- a/p2p/base/connection.h +++ b/p2p/base/connection.h @@ -36,6 +36,16 @@ class Port; // Forward declaration so that a ConnectionRequest can contain a Connection. class Connection; +struct CandidatePair final : public CandidatePairInterface { + ~CandidatePair() override = default; + + const Candidate& local_candidate() const override { return local; } + const Candidate& remote_candidate() const override { return remote; } + + Candidate local; + Candidate remote; +}; + // A ConnectionRequest is a simple STUN ping used to determine writability. class ConnectionRequest : public StunRequest { public: @@ -228,6 +238,10 @@ 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; diff --git a/p2p/base/fake_ice_transport.h b/p2p/base/fake_ice_transport.h index 1b95a55c42..467ee2ec1c 100644 --- a/p2p/base/fake_ice_transport.h +++ b/p2p/base/fake_ice_transport.h @@ -207,6 +207,10 @@ class FakeIceTransport : public IceTransportInternal { absl::optional GetRttEstimate() override { return absl::nullopt; } const Connection* selected_connection() const override { return nullptr; } + absl::optional GetSelectedCandidatePair() + const override { + return absl::nullopt; + } // Fake PacketTransportInternal implementation. bool writable() const override { return writable_; } diff --git a/p2p/base/ice_transport_internal.h b/p2p/base/ice_transport_internal.h index 630848f6e6..7f1d70bb94 100644 --- a/p2p/base/ice_transport_internal.h +++ b/p2p/base/ice_transport_internal.h @@ -263,8 +263,14 @@ class RTC_EXPORT IceTransportInternal : public rtc::PacketTransportInternal { // absl::optional if there is none. virtual absl::optional GetRttEstimate() = 0; + // TODO(qingsi): Remove this method once Chrome does not depend on it anymore. virtual const Connection* selected_connection() const = 0; + // Returns the selected candidate pair, or an empty absl::optional if there is + // none. + virtual absl::optional GetSelectedCandidatePair() + const = 0; + sigslot::signal1 SignalGatheringState; // Handles sending and receiving of candidates. diff --git a/p2p/base/mock_ice_transport.h b/p2p/base/mock_ice_transport.h index 6aeb95027c..a28c796970 100644 --- a/p2p/base/mock_ice_transport.h +++ b/p2p/base/mock_ice_transport.h @@ -63,6 +63,10 @@ class MockIceTransport : public IceTransportInternal { void SetIceConfig(const IceConfig& config) override {} absl::optional GetRttEstimate() override { return absl::nullopt; } const Connection* selected_connection() const override { return nullptr; } + absl::optional GetSelectedCandidatePair() + const override { + return absl::nullopt; + } void MaybeStartGathering() override {} void AddRemoteCandidate(const Candidate& candidate) override {} void RemoveRemoteCandidate(const Candidate& candidate) override {} diff --git a/p2p/base/p2p_transport_channel.cc b/p2p/base/p2p_transport_channel.cc index 6356859370..01caaa9b10 100644 --- a/p2p/base/p2p_transport_channel.cc +++ b/p2p/base/p2p_transport_channel.cc @@ -380,6 +380,16 @@ absl::optional P2PTransportChannel::GetRttEstimate() { } } +absl::optional +P2PTransportChannel::GetSelectedCandidatePair() const { + RTC_DCHECK_RUN_ON(network_thread_); + if (selected_connection_ == nullptr) { + return absl::nullopt; + } + + return selected_connection_->ToCandidatePairAndSanitizeIfNecessary(); +} + // A channel is considered ICE completed once there is at most one active // connection per network and at least one active connection. IceTransportState P2PTransportChannel::ComputeState() const { @@ -1965,7 +1975,6 @@ void P2PTransportChannel::SwitchSelectedConnection(Connection* conn, } // Create event for candidate pair change. - if (selected_connection_) { CandidatePairChangeEvent pair_change; pair_change.reason = reason; @@ -1975,7 +1984,6 @@ void P2PTransportChannel::SwitchSelectedConnection(Connection* conn, selected_connection_->last_data_received(); SignalCandidatePairChanged(pair_change); } - SignalNetworkRouteChanged(network_route_); } diff --git a/p2p/base/p2p_transport_channel.h b/p2p/base/p2p_transport_channel.h index 0546b36e3d..f9ea59d12e 100644 --- a/p2p/base/p2p_transport_channel.h +++ b/p2p/base/p2p_transport_channel.h @@ -135,6 +135,7 @@ class RTC_EXPORT P2PTransportChannel : public IceTransportInternal { std::vector* candidate_stats_list) override; absl::optional GetRttEstimate() override; const Connection* selected_connection() const override; + absl::optional GetSelectedCandidatePair() const override; // TODO(honghaiz): Remove this method once the reference of it in // Chromoting is removed. diff --git a/p2p/base/p2p_transport_channel_unittest.cc b/p2p/base/p2p_transport_channel_unittest.cc index a8e83bac60..1b9347565f 100644 --- a/p2p/base/p2p_transport_channel_unittest.cc +++ b/p2p/base/p2p_transport_channel_unittest.cc @@ -4991,6 +4991,59 @@ TEST_F(P2PTransportChannelTest, DestroyChannels(); } +// A similar test as above to check the selected candidate pair is sanitized +// when it is queried via GetSelectedCandidatePair. +TEST_F(P2PTransportChannelTest, + SelectedCandidatePairSanitizedWhenMdnsObfuscationEnabled) { + NiceMock mock_async_resolver; + webrtc::MockAsyncResolverFactory mock_async_resolver_factory; + EXPECT_CALL(mock_async_resolver_factory, Create()) + .WillOnce(Return(&mock_async_resolver)); + + // ep1 and ep2 will gather host candidates with addresses + // kPublicAddrs[0] and kPublicAddrs[1], respectively. + ConfigureEndpoints(OPEN, OPEN, kOnlyLocalPorts, kOnlyLocalPorts); + // ICE parameter will be set up when creating the channels. + set_remote_ice_parameter_source(FROM_SETICEPARAMETERS); + GetEndpoint(0)->network_manager_.set_mdns_responder( + absl::make_unique(rtc::Thread::Current())); + GetEndpoint(1)->async_resolver_factory_ = &mock_async_resolver_factory; + CreateChannels(); + // Pause sending candidates from both endpoints until we find out what port + // number is assigned to ep1's host candidate. + PauseCandidates(0); + PauseCandidates(1); + ASSERT_EQ_WAIT(1u, GetEndpoint(0)->saved_candidates_.size(), kMediumTimeout); + const auto& candidates_data = GetEndpoint(0)->saved_candidates_[0]; + ASSERT_EQ(1u, candidates_data->candidates.size()); + const auto& local_candidate_ep1 = candidates_data->candidates[0]; + ASSERT_TRUE(local_candidate_ep1.type() == LOCAL_PORT_TYPE); + // This is the underlying private IP address of the same candidate at ep1, + // and let the mock resolver of ep2 receive the correct resolution. + rtc::SocketAddress resolved_address_ep1(local_candidate_ep1.address()); + resolved_address_ep1.SetResolvedIP(kPublicAddrs[0].ipaddr()); + EXPECT_CALL(mock_async_resolver, GetResolvedAddress(_, _)) + .WillOnce(DoAll(SetArgPointee<1>(resolved_address_ep1), Return(true))); + ResumeCandidates(0); + ResumeCandidates(1); + + ASSERT_TRUE_WAIT(ep1_ch1()->selected_connection() != nullptr && + ep2_ch1()->selected_connection() != nullptr, + kMediumTimeout); + + const auto pair_ep1 = ep1_ch1()->GetSelectedCandidatePair(); + ASSERT_TRUE(pair_ep1.has_value()); + EXPECT_EQ(LOCAL_PORT_TYPE, pair_ep1->local_candidate().type()); + EXPECT_TRUE(pair_ep1->local_candidate().address().IsUnresolvedIP()); + + const auto pair_ep2 = ep2_ch1()->GetSelectedCandidatePair(); + ASSERT_TRUE(pair_ep2.has_value()); + EXPECT_EQ(LOCAL_PORT_TYPE, pair_ep2->remote_candidate().type()); + EXPECT_TRUE(pair_ep2->remote_candidate().address().IsUnresolvedIP()); + + DestroyChannels(); +} + class MockMdnsResponder : public webrtc::MdnsResponderInterface { public: MOCK_METHOD2(CreateNameForAddress,