From 29d16c0ed61e46b00b4becfd9e1d093d1551b11b Mon Sep 17 00:00:00 2001 From: Philipp Hancke Date: Thu, 3 Feb 2022 12:24:52 +0100 Subject: [PATCH] stun/turn: use hostname when reconstructing the url in the case of an ip address the hostname() call will return that. This may also avoid leaking IP addresses from DNS resolutions and is more similar to the url originally passed into the peerconnection (but will for example produce a fully formed url and resolves the port if none was given). BUG=webrtc:13652 Change-Id: I000c66f7988b4b205e38c4dde5b888e48d8f6a0f Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/250202 Reviewed-by: Harald Alvestrand Commit-Queue: Philipp Hancke Cr-Commit-Position: refs/heads/main@{#35898} --- p2p/base/stun_port.cc | 2 +- p2p/base/turn_port.cc | 18 +++++++----------- p2p/base/turn_port.h | 2 +- p2p/base/turn_port_unittest.cc | 17 ++++++++++++++++- 4 files changed, 25 insertions(+), 14 deletions(-) diff --git a/p2p/base/stun_port.cc b/p2p/base/stun_port.cc index 3ab3b5bf74..1ba6486049 100644 --- a/p2p/base/stun_port.cc +++ b/p2p/base/stun_port.cc @@ -512,7 +512,7 @@ void UDPPort::OnStunBindingRequestSucceeded( } rtc::StringBuilder url; - url << "stun:" << stun_server_addr.ipaddr().ToString() << ":" + url << "stun:" << stun_server_addr.hostname() << ":" << stun_server_addr.port(); AddAddress(stun_reflected_addr, socket_->GetLocalAddress(), related_address, UDP_PROTOCOL_NAME, "", "", STUN_PORT_TYPE, diff --git a/p2p/base/turn_port.cc b/p2p/base/turn_port.cc index e622b62982..bde5bf603c 100644 --- a/p2p/base/turn_port.cc +++ b/p2p/base/turn_port.cc @@ -862,10 +862,9 @@ void TurnPort::OnAllocateSuccess(const rtc::SocketAddress& address, related_address, // Related address. UDP_PROTOCOL_NAME, ProtoToString(server_address_.proto), // The first hop protocol. - "", // TCP canddiate type, empty for turn candidates. + "", // TCP candidate type, empty for turn candidates. RELAY_PORT_TYPE, GetRelayPreference(server_address_.proto), - server_priority_, ReconstructedServerUrl(false /* use_hostname */), - true); + server_priority_, ReconstructedServerUrl(), true); } void TurnPort::OnAllocateError(int error_code, const std::string& reason) { @@ -881,9 +880,8 @@ void TurnPort::OnAllocateError(int error_code, const std::string& reason) { port = 0; } SignalCandidateError( - this, IceCandidateErrorEvent( - address, port, ReconstructedServerUrl(true /* use_hostname */), - error_code, reason)); + this, IceCandidateErrorEvent(address, port, ReconstructedServerUrl(), + error_code, reason)); } void TurnPort::OnRefreshError() { @@ -1291,7 +1289,7 @@ bool TurnPort::SetEntryChannelId(const rtc::SocketAddress& address, return true; } -std::string TurnPort::ReconstructedServerUrl(bool use_hostname) { +std::string TurnPort::ReconstructedServerUrl() { // draft-petithuguenin-behave-turn-uris-01 // turnURI = scheme ":" turn-host [ ":" turn-port ] // [ "?transport=" transport ] @@ -1314,10 +1312,8 @@ std::string TurnPort::ReconstructedServerUrl(bool use_hostname) { break; } rtc::StringBuilder url; - url << scheme << ":" - << (use_hostname ? server_address_.address.hostname() - : server_address_.address.ipaddr().ToString()) - << ":" << server_address_.address.port() << "?transport=" << transport; + url << scheme << ":" << server_address_.address.hostname() << ":" + << server_address_.address.port() << "?transport=" << transport; return url.Release(); } diff --git a/p2p/base/turn_port.h b/p2p/base/turn_port.h index 891af7605a..172dcef5ad 100644 --- a/p2p/base/turn_port.h +++ b/p2p/base/turn_port.h @@ -367,7 +367,7 @@ class TurnPort : public Port { bool FailAndPruneConnection(const rtc::SocketAddress& address); // Reconstruct the URL of the server which the candidate is gathered from. - std::string ReconstructedServerUrl(bool use_hostname); + std::string ReconstructedServerUrl(); void MaybeAddTurnLoggingId(StunMessage* message); diff --git a/p2p/base/turn_port_unittest.cc b/p2p/base/turn_port_unittest.cc index 48d901f2ba..44d258363e 100644 --- a/p2p/base/turn_port_unittest.cc +++ b/p2p/base/turn_port_unittest.cc @@ -112,6 +112,9 @@ static const cricket::ProtocolAddress kTurnPort80ProtoAddr(kTurnPort80Addr, cricket::PROTO_TCP); static const cricket::ProtocolAddress kTurnPort443ProtoAddr(kTurnPort443Addr, cricket::PROTO_TCP); +static const cricket::ProtocolAddress kTurnPortHostnameProtoAddr( + kTurnInvalidAddr, + cricket::PROTO_UDP); static const unsigned int MSG_TESTFINISH = 0; @@ -818,6 +821,18 @@ TEST_F(TurnPortTest, TestReconstructedServerUrlForTls) { TestReconstructedServerUrl(PROTO_TLS, "turns:99.99.99.4:3478?transport=tcp"); } +TEST_F(TurnPortTest, TestReconstructedServerUrlForHostname) { + CreateTurnPort(kTurnUsername, kTurnPassword, kTurnPortHostnameProtoAddr); + // This test follows the pattern from TestTurnTcpOnAddressResolveFailure. + // As VSS doesn't provide DNS resolution, name resolve will fail, + // the error will be set and contain the url. + turn_port_->PrepareAddress(); + EXPECT_TRUE_WAIT(turn_error_, kResolverTimeout); + std::string server_url = + "turn:" + kTurnInvalidAddr.ToString() + "?transport=udp"; + ASSERT_EQ(error_event_.url, server_url); +} + // Do a normal TURN allocation. TEST_F(TurnPortTest, TestTurnAllocate) { CreateTurnPort(kTurnUsername, kTurnPassword, kTurnUdpProtoAddr); @@ -1020,7 +1035,7 @@ TEST_F(TurnPortTest, TestTurnTcpOnAddressResolveFailure) { ProtocolAddress(kTurnInvalidAddr, PROTO_TCP)); turn_port_->PrepareAddress(); EXPECT_TRUE_WAIT(turn_error_, kResolverTimeout); - // As VSS doesn't provide a DNS resolution, name resolve will fail. TurnPort + // As VSS doesn't provide DNS resolution, name resolve will fail. TurnPort // will proceed in creating a TCP socket which will fail as there is no // server on the above domain and error will be set to SOCKET_ERROR. EXPECT_EQ(SOCKET_ERROR, turn_port_->error());