From fdf54f22560aa87f250ba2b5e03d497aadfd2cf1 Mon Sep 17 00:00:00 2001 From: Qingsi Wang Date: Fri, 18 Oct 2019 15:51:40 -0700 Subject: [PATCH] Stop pairing local relay candidates with remote mDNS candidates. To avoid IP leak from the CreatePermission request, local relay candidates must not be paired with remote mDNS candidates, per Section 3.3.2 in draft-ietf-rtcweb-mdns-ice-candidates-04. Bug: webrtc:11038 Change-Id: I13aada79c812712b850293c7e17094dc8f77105a Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/157340 Commit-Queue: Qingsi Wang Reviewed-by: Honghai Zhang Reviewed-by: Alex Drake Cr-Commit-Position: refs/heads/master@{#29546} --- p2p/base/p2p_constants.cc | 2 + p2p/base/p2p_constants.h | 3 ++ p2p/base/p2p_transport_channel.cc | 6 +-- p2p/base/p2p_transport_channel_unittest.cc | 53 ++++++++++++++++++++++ p2p/base/turn_port.cc | 15 ++++-- 5 files changed, 73 insertions(+), 6 deletions(-) diff --git a/p2p/base/p2p_constants.cc b/p2p/base/p2p_constants.cc index 619b73922a..3414939a6f 100644 --- a/p2p/base/p2p_constants.cc +++ b/p2p/base/p2p_constants.cc @@ -41,6 +41,8 @@ const char CONNECTIONROLE_PASSIVE_STR[] = "passive"; const char CONNECTIONROLE_ACTPASS_STR[] = "actpass"; const char CONNECTIONROLE_HOLDCONN_STR[] = "holdconn"; +const char LOCAL_TLD[] = ".local"; + const int MIN_CHECK_RECEIVING_INTERVAL = 50; const int RECEIVING_TIMEOUT = MIN_CHECK_RECEIVING_INTERVAL * 50; const int RECEIVING_SWITCHING_DELAY = 1000; diff --git a/p2p/base/p2p_constants.h b/p2p/base/p2p_constants.h index a2be32de6f..07257d5e18 100644 --- a/p2p/base/p2p_constants.h +++ b/p2p/base/p2p_constants.h @@ -48,6 +48,9 @@ extern const char CONNECTIONROLE_PASSIVE_STR[]; extern const char CONNECTIONROLE_ACTPASS_STR[]; extern const char CONNECTIONROLE_HOLDCONN_STR[]; +// RFC 6762, the .local pseudo-top-level domain used for mDNS names. +extern const char LOCAL_TLD[]; + // Constants for time intervals are in milliseconds unless otherwise stated. // // Most of the following constants are the default values of IceConfig diff --git a/p2p/base/p2p_transport_channel.cc b/p2p/base/p2p_transport_channel.cc index a1c14505e5..f95e7abaff 100644 --- a/p2p/base/p2p_transport_channel.cc +++ b/p2p/base/p2p_transport_channel.cc @@ -16,6 +16,7 @@ #include #include "absl/algorithm/container.h" +#include "absl/strings/match.h" #include "api/candidate.h" #include "logging/rtc_event_log/ice_logger.h" #include "p2p/base/candidate_pair_interface.h" @@ -2700,10 +2701,9 @@ Candidate P2PTransportChannel::SanitizeLocalCandidate( 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 + // If the remote endpoint signaled us an mDNS candidate, we assume it // is supposed to be sanitized. - bool use_hostname_address = - c.type() == LOCAL_PORT_TYPE && !c.address().hostname().empty(); + bool use_hostname_address = absl::EndsWith(c.address().hostname(), LOCAL_TLD); // Remove the address for prflx remote candidates. See // https://w3c.github.io/webrtc-stats/#dom-rtcicecandidatestats. use_hostname_address |= c.type() == PRFLX_PORT_TYPE; diff --git a/p2p/base/p2p_transport_channel_unittest.cc b/p2p/base/p2p_transport_channel_unittest.cc index 8f7fd4aa8a..0c3474bb32 100644 --- a/p2p/base/p2p_transport_channel_unittest.cc +++ b/p2p/base/p2p_transport_channel_unittest.cc @@ -5247,6 +5247,59 @@ TEST_F(P2PTransportChannelTest, DestroyChannels(); } +TEST_F(P2PTransportChannelTest, + NoPairOfLocalRelayCandidateWithRemoteMdnsCandidate) { + const int kOnlyRelayPorts = cricket::PORTALLOCATOR_DISABLE_UDP | + cricket::PORTALLOCATOR_DISABLE_STUN | + cricket::PORTALLOCATOR_DISABLE_TCP; + // We use one endpoint to test the behavior of adding remote candidates, and + // this endpoint only gathers relay candidates. + ConfigureEndpoints(OPEN, OPEN, kOnlyRelayPorts, kDefaultPortAllocatorFlags); + GetEndpoint(0)->cd1_.ch_.reset(CreateChannel( + 0, ICE_CANDIDATE_COMPONENT_DEFAULT, kIceParams[0], kIceParams[1])); + IceConfig config; + // Start gathering and we should have only a single relay port. + ep1_ch1()->SetIceConfig(config); + ep1_ch1()->MaybeStartGathering(); + EXPECT_EQ_WAIT(IceGatheringState::kIceGatheringComplete, + ep1_ch1()->gathering_state(), kDefaultTimeout); + EXPECT_EQ(1u, ep1_ch1()->ports().size()); + // Add a plain remote host candidate and three remote mDNS candidates with the + // host, srflx and relay types. Note that the candidates differ in their + // ports. + cricket::Candidate host_candidate = CreateUdpCandidate( + LOCAL_PORT_TYPE, "1.1.1.1", 1 /* port */, 0 /* priority */); + ep1_ch1()->AddRemoteCandidate(host_candidate); + + std::vector mdns_candidates; + mdns_candidates.push_back(CreateUdpCandidate(LOCAL_PORT_TYPE, "example.local", + 2 /* port */, 0 /* priority */)); + mdns_candidates.push_back(CreateUdpCandidate(STUN_PORT_TYPE, "example.local", + 3 /* port */, 0 /* priority */)); + mdns_candidates.push_back(CreateUdpCandidate(RELAY_PORT_TYPE, "example.local", + 4 /* port */, 0 /* priority */)); + // We just resolve the hostname to 1.1.1.1, and add the candidates with this + // address directly to simulate the process of adding remote candidates with + // the name resolution. + for (auto& mdns_candidate : mdns_candidates) { + rtc::SocketAddress resolved_address(mdns_candidate.address()); + resolved_address.SetResolvedIP(0x1111); // 1.1.1.1 + mdns_candidate.set_address(resolved_address); + EXPECT_FALSE(mdns_candidate.address().IsUnresolvedIP()); + ep1_ch1()->AddRemoteCandidate(mdns_candidate); + } + + // All remote candidates should have been successfully added. + EXPECT_EQ(4u, ep1_ch1()->remote_candidates().size()); + + // Expect that there is no connection paired with any mDNS candidate. + ASSERT_EQ(1u, ep1_ch1()->connections().size()); + ASSERT_NE(nullptr, ep1_ch1()->connections()[0]); + EXPECT_EQ( + "1.1.1.1:1", + ep1_ch1()->connections()[0]->remote_candidate().address().ToString()); +} + class MockMdnsResponder : public webrtc::MdnsResponderInterface { public: MOCK_METHOD2(CreateNameForAddress, diff --git a/p2p/base/turn_port.cc b/p2p/base/turn_port.cc index 68535b7373..f0795ee5e5 100644 --- a/p2p/base/turn_port.cc +++ b/p2p/base/turn_port.cc @@ -16,8 +16,10 @@ #include #include "absl/algorithm/container.h" +#include "absl/strings/match.h" #include "absl/types/optional.h" #include "p2p/base/connection.h" +#include "p2p/base/p2p_constants.h" #include "p2p/base/stun.h" #include "rtc_base/async_packet_socket.h" #include "rtc_base/byte_order.h" @@ -540,11 +542,18 @@ Connection* TurnPort::CreateConnection(const Candidate& remote_candidate, CandidateOrigin origin) { // TURN-UDP can only connect to UDP candidates. if (!SupportsProtocol(remote_candidate.protocol())) { - return NULL; + return nullptr; } if (state_ == STATE_DISCONNECTED || state_ == STATE_RECEIVEONLY) { - return NULL; + return nullptr; + } + + // If the remote endpoint signaled us an mDNS candidate, we do not form a pair + // with the relay candidate to avoid IP leakage in the CreatePermission + // request. + if (absl::EndsWith(remote_candidate.address().hostname(), LOCAL_TLD)) { + return nullptr; } // A TURN port will have two candiates, STUN and TURN. STUN may not @@ -568,7 +577,7 @@ Connection* TurnPort::CreateConnection(const Candidate& remote_candidate, return conn; } } - return NULL; + return nullptr; } bool TurnPort::FailAndPruneConnection(const rtc::SocketAddress& address) {