From b6173abe59e11b749295482aa7f30c8fc3d2f47e Mon Sep 17 00:00:00 2001 From: "guoweis@webrtc.org" Date: Tue, 21 Oct 2014 20:40:21 +0000 Subject: [PATCH] Fix local address leakage when IceTransportsType is relay As part of implementing IceTransportsType constraint, we should hide the raddr which is the mapped address to prevent local address leakage. BUG=1179 R=juberti@webrtc.org Review URL: https://webrtc-codereview.appspot.com/26889004 git-svn-id: http://webrtc.googlecode.com/svn/trunk@7484 4adac7df-926f-26a2-2b94-8c16560cd09d --- talk/p2p/base/port.cc | 7 +++-- talk/p2p/base/port.h | 12 +++++++++ talk/p2p/base/stunport.cc | 12 ++++++++- talk/p2p/base/turnport.cc | 14 +++++++--- talk/p2p/client/basicportallocator.cc | 3 +++ talk/p2p/client/portallocator_unittest.cc | 32 +++++++++++++++++++++-- 6 files changed, 72 insertions(+), 8 deletions(-) diff --git a/talk/p2p/base/port.cc b/talk/p2p/base/port.cc index 0cf46eb39..670e1de1f 100644 --- a/talk/p2p/base/port.cc +++ b/talk/p2p/base/port.cc @@ -31,6 +31,7 @@ #include #include "talk/p2p/base/common.h" +#include "talk/p2p/base/portallocator.h" #include "webrtc/base/base64.h" #include "webrtc/base/crc32.h" #include "webrtc/base/helpers.h" @@ -187,7 +188,8 @@ Port::Port(rtc::Thread* thread, rtc::PacketSocketFactory* factory, ice_protocol_(ICEPROTO_HYBRID), ice_role_(ICEROLE_UNKNOWN), tiebreaker_(0), - shared_socket_(true) { + shared_socket_(true), + candidate_filter_(CF_ALL) { Construct(); } @@ -213,7 +215,8 @@ Port::Port(rtc::Thread* thread, const std::string& type, ice_protocol_(ICEPROTO_HYBRID), ice_role_(ICEROLE_UNKNOWN), tiebreaker_(0), - shared_socket_(false) { + shared_socket_(false), + candidate_filter_(CF_ALL) { ASSERT(factory_ != NULL); Construct(); } diff --git a/talk/p2p/base/port.h b/talk/p2p/base/port.h index 4893586d5..f0fcb2e92 100644 --- a/talk/p2p/base/port.h +++ b/talk/p2p/base/port.h @@ -308,6 +308,10 @@ class Port : public PortInterface, public rtc::MessageHandler, // Returns if Hybrid ICE protocol is used. bool IsHybridIce() const; + void set_candidate_filter(uint32 candidate_filter) { + candidate_filter_ = candidate_filter; + } + protected: enum { MSG_CHECKTIMEOUT = 0, @@ -351,6 +355,8 @@ class Port : public PortInterface, public rtc::MessageHandler, return rtc::DSCP_NO_CHANGE; } + uint32 candidate_filter() { return candidate_filter_; } + private: void Construct(); // Called when one of our connections deletes itself. @@ -392,6 +398,12 @@ class Port : public PortInterface, public rtc::MessageHandler, std::string user_agent_; rtc::ProxyInfo proxy_; + // Candidate filter is pushed down to Port such that each Port could + // make its own decision on how to create candidates. For example, + // when IceTransportsType is set to relay, both RelayPort and + // TurnPort will hide raddr to avoid local address leakage. + uint32 candidate_filter_; + friend class Connection; }; diff --git a/talk/p2p/base/stunport.cc b/talk/p2p/base/stunport.cc index a185a3501..ce2d828a9 100644 --- a/talk/p2p/base/stunport.cc +++ b/talk/p2p/base/stunport.cc @@ -28,6 +28,7 @@ #include "talk/p2p/base/stunport.h" #include "talk/p2p/base/common.h" +#include "talk/p2p/base/portallocator.h" #include "talk/p2p/base/stun.h" #include "webrtc/base/common.h" #include "webrtc/base/helpers.h" @@ -395,8 +396,17 @@ void UDPPort::OnStunBindingRequestSucceeded( // For STUN, related address is the local socket address. if ((!SharedSocket() || stun_reflected_addr != socket_->GetLocalAddress()) && !HasCandidateWithAddress(stun_reflected_addr)) { + + rtc::SocketAddress related_address = socket_->GetLocalAddress(); + if (!(candidate_filter() & CF_HOST)) { + // If candidate filter doesn't have CF_HOST specified, empty raddr to + // avoid local address leakage. + related_address = rtc::EmptySocketAddressWithFamily( + related_address.family()); + } + AddAddress(stun_reflected_addr, socket_->GetLocalAddress(), - socket_->GetLocalAddress(), UDP_PROTOCOL_NAME, "", + related_address, UDP_PROTOCOL_NAME, "", STUN_PORT_TYPE, ICE_TYPE_PREFERENCE_SRFLX, 0, false); } MaybeSetPortCompleteOrError(); diff --git a/talk/p2p/base/turnport.cc b/talk/p2p/base/turnport.cc index 2908d71c7..64bfed457 100644 --- a/talk/p2p/base/turnport.cc +++ b/talk/p2p/base/turnport.cc @@ -588,10 +588,18 @@ void TurnPort::OnStunAddress(const rtc::SocketAddress& address) { void TurnPort::OnAllocateSuccess(const rtc::SocketAddress& address, const rtc::SocketAddress& stun_address) { connected_ = true; + + rtc::SocketAddress related_address = stun_address; + if (!(candidate_filter() & CF_REFLEXIVE)) { + // If candidate filter only allows relay type of address, empty raddr to + // avoid local address leakage. + related_address = rtc::EmptySocketAddressWithFamily(stun_address.family()); + } + // For relayed candidate, Base is the candidate itself. - AddAddress(address, // Candidate address. - address, // Base address. - stun_address, // Related address. + AddAddress(address, // Candidate address. + address, // Base address. + related_address, // Related address. UDP_PROTOCOL_NAME, "", // TCP canddiate type, empty for turn candidates. RELAY_PORT_TYPE, diff --git a/talk/p2p/client/basicportallocator.cc b/talk/p2p/client/basicportallocator.cc index 0ec13e724..0b35bddd4 100644 --- a/talk/p2p/client/basicportallocator.cc +++ b/talk/p2p/client/basicportallocator.cc @@ -497,6 +497,9 @@ void BasicPortAllocatorSession::AddAllocatedPort(Port* port, port->set_send_retransmit_count_attribute((allocator_->flags() & PORTALLOCATOR_ENABLE_STUN_RETRANSMIT_ATTRIBUTE) != 0); + // Push down the candidate_filter to individual port. + port->set_candidate_filter(allocator_->candidate_filter()); + PortData data(port, seq); ports_.push_back(data); diff --git a/talk/p2p/client/portallocator_unittest.cc b/talk/p2p/client/portallocator_unittest.cc index 84b8d0318..3357fe0be 100644 --- a/talk/p2p/client/portallocator_unittest.cc +++ b/talk/p2p/client/portallocator_unittest.cc @@ -113,6 +113,8 @@ class PortAllocatorTest : public testing::Test, public sigslot::has_slots<> { candidate_allocation_done_(false) { cricket::ServerAddresses stun_servers; stun_servers.insert(kStunAddr); + // Passing the addresses of GTURN servers will enable GTURN in + // Basicportallocator. allocator_.reset(new cricket::BasicPortAllocator( &network_manager_, stun_servers, @@ -137,6 +139,14 @@ class PortAllocatorTest : public testing::Test, public sigslot::has_slots<> { allocator().set_step_delay(cricket::kMinimumStepDelay); } + // Create a BasicPortAllocator without GTURN and add the TURN servers. + void ResetWithTurnServers(const rtc::SocketAddress& udp_turn, + const rtc::SocketAddress& tcp_turn) { + allocator_.reset(new cricket::BasicPortAllocator(&network_manager_)); + allocator().set_step_delay(cricket::kMinimumStepDelay); + AddTurnServers(udp_turn, tcp_turn); + } + void AddTurnServers(const rtc::SocketAddress& udp_turn, const rtc::SocketAddress& tcp_turn) { cricket::RelayServerConfig relay_server(cricket::RELAY_TURN); @@ -565,17 +575,32 @@ TEST_F(PortAllocatorTest, TestGetAllPortsRestarts) { } // Test ICE candidate filter mechanism with options Relay/Host/Reflexive. +// This test also verifies that when the allocator is only allowed to use +// relay (i.e. IceTransportsType is relay), the raddr is an empty +// address with the correct family. This is to prevent any local +// reflective address leakage in the sdp line. TEST_F(PortAllocatorTest, TestCandidateFilterWithRelayOnly) { AddInterface(kClientAddr); + // GTURN is not configured here. + ResetWithTurnServers(kTurnUdpIntAddr, rtc::SocketAddress()); allocator().set_candidate_filter(cricket::CF_RELAY); EXPECT_TRUE(CreateSession(cricket::ICE_CANDIDATE_COMPONENT_RTP)); session_->StartGettingPorts(); EXPECT_TRUE_WAIT(candidate_allocation_done_, kDefaultAllocationTimeout); - // Using GTURN, we will have 4 candidates. - EXPECT_EQ(4U, candidates_.size()); + EXPECT_PRED5(CheckCandidate, + candidates_[0], + cricket::ICE_CANDIDATE_COMPONENT_RTP, + "relay", + "udp", + rtc::SocketAddress(kTurnUdpExtAddr.ipaddr(), 0)); + + EXPECT_EQ(1U, candidates_.size()); EXPECT_EQ(1U, ports_.size()); // Only Relay port will be in ready state. for (size_t i = 0; i < candidates_.size(); ++i) { EXPECT_EQ(std::string(cricket::RELAY_PORT_TYPE), candidates_[i].type()); + EXPECT_EQ( + candidates_[0].related_address(), + rtc::EmptySocketAddressWithFamily(candidates_[0].address().family())); } } @@ -611,6 +636,9 @@ TEST_F(PortAllocatorTest, TestCandidateFilterWithReflexiveOnly) { EXPECT_EQ(1U, ports_.size()); // Only UDP port will be in ready state. for (size_t i = 0; i < candidates_.size(); ++i) { EXPECT_EQ(std::string(cricket::STUN_PORT_TYPE), candidates_[i].type()); + EXPECT_EQ( + candidates_[0].related_address(), + rtc::EmptySocketAddressWithFamily(candidates_[0].address().family())); } }