diff --git a/p2p/base/turn_port.cc b/p2p/base/turn_port.cc index f04313f1b2..c78a94642d 100644 --- a/p2p/base/turn_port.cc +++ b/p2p/base/turn_port.cc @@ -346,6 +346,15 @@ void TurnPort::PrepareAddress() { server_address_.address.SetPort(TURN_DEFAULT_PORT); } + if (!AllowedTurnPort(server_address_.address.port())) { + // This can only happen after a 300 ALTERNATE SERVER, since the port can't + // be created with a disallowed port number. + RTC_LOG(LS_ERROR) << "Attempt to start allocation with disallowed port# " + << server_address_.address.port(); + OnAllocateError(STUN_ERROR_SERVER_ERROR, + "Attempt to start allocation to a disallowed port"); + return; + } if (server_address_.address.IsUnresolvedIP()) { ResolveTurnAddress(server_address_.address); } else { @@ -943,6 +952,21 @@ rtc::DiffServCodePoint TurnPort::StunDscpValue() const { return stun_dscp_value_; } +// static +bool TurnPort::AllowedTurnPort(int port) { + // Port 443 is used for existing deployments. Ports above 1024 are assumed to + // be OK to use. + if (port == 443 || port >= 1024) { + return true; + } + // Allow any port if relevant field trial is set. This allows disabling the + // check. + if (webrtc::field_trial::IsEnabled("WebRTC-Turn-AllowSystemPorts")) { + return true; + } + return false; +} + void TurnPort::OnMessage(rtc::Message* message) { switch (message->message_id) { case MSG_ALLOCATE_ERROR: diff --git a/p2p/base/turn_port.h b/p2p/base/turn_port.h index 6f9caaf795..3a7915274a 100644 --- a/p2p/base/turn_port.h +++ b/p2p/base/turn_port.h @@ -68,6 +68,12 @@ class TurnPort : public Port { if (credentials.username.size() > kMaxTurnUsernameLength) { return nullptr; } + // Do not connect to low-numbered ports. The default STUN port is 3478. + if (!AllowedTurnPort(server_address.address.port())) { + RTC_LOG(LS_ERROR) << "Attempt to use TURN to connect to port " + << server_address.address.port(); + return nullptr; + } // Using `new` to access a non-public constructor. return absl::WrapUnique(new TurnPort( thread, factory, network, socket, username, password, server_address, @@ -113,6 +119,12 @@ class TurnPort : public Port { if (credentials.username.size() > kMaxTurnUsernameLength) { return nullptr; } + // Do not connect to low-numbered ports. The default STUN port is 3478. + if (!AllowedTurnPort(server_address.address.port())) { + RTC_LOG(LS_ERROR) << "Attempt to use TURN to connect to port " + << server_address.address.port(); + return nullptr; + } // Using `new` to access a non-public constructor. return absl::WrapUnique( new TurnPort(thread, factory, network, min_port, max_port, username, @@ -296,6 +308,7 @@ class TurnPort : public Port { typedef std::map SocketOptionsMap; typedef std::set AttemptedServerSet; + static bool AllowedTurnPort(int port); void OnMessage(rtc::Message* pmsg) override; bool CreateTurnClientSocket(); diff --git a/p2p/base/turn_port_unittest.cc b/p2p/base/turn_port_unittest.cc index 6ed090ab07..b1e359337a 100644 --- a/p2p/base/turn_port_unittest.cc +++ b/p2p/base/turn_port_unittest.cc @@ -41,6 +41,7 @@ #include "rtc_base/thread.h" #include "rtc_base/time_utils.h" #include "rtc_base/virtual_socket_server.h" +#include "test/field_trial.h" #include "test/gtest.h" using rtc::SocketAddress; @@ -58,6 +59,11 @@ static const SocketAddress kTurnTcpIntAddr("99.99.99.4", static const SocketAddress kTurnUdpExtAddr("99.99.99.5", 0); static const SocketAddress kTurnAlternateIntAddr("99.99.99.6", cricket::TURN_SERVER_PORT); +// Port for redirecting to a TCP Web server. Should not work. +static const SocketAddress kTurnDangerousAddr("99.99.99.7", 80); +// Port 443 (the HTTPS port); should work. +static const SocketAddress kTurnPort443Addr("99.99.99.7", 443); +// The default TURN server port. static const SocketAddress kTurnIntAddr("99.99.99.7", cricket::TURN_SERVER_PORT); static const SocketAddress kTurnIPv6IntAddr( @@ -94,6 +100,11 @@ static const cricket::ProtocolAddress kTurnTlsProtoAddr(kTurnTcpIntAddr, cricket::PROTO_TLS); static const cricket::ProtocolAddress kTurnUdpIPv6ProtoAddr(kTurnUdpIPv6IntAddr, cricket::PROTO_UDP); +static const cricket::ProtocolAddress kTurnDangerousProtoAddr( + kTurnDangerousAddr, + cricket::PROTO_TCP); +static const cricket::ProtocolAddress kTurnPort443ProtoAddr(kTurnPort443Addr, + cricket::PROTO_TCP); static const unsigned int MSG_TESTFINISH = 0; @@ -1785,4 +1796,48 @@ TEST_F(TurnPortTest, TestOverlongUsername) { CreateTurnPort(overlong_username, kTurnPassword, kTurnTlsProtoAddr)); } +TEST_F(TurnPortTest, TestTurnDangerousServer) { + CreateTurnPort(kTurnUsername, kTurnPassword, kTurnDangerousProtoAddr); + ASSERT_FALSE(turn_port_); +} + +TEST_F(TurnPortTest, TestTurnDangerousServerPermits443) { + CreateTurnPort(kTurnUsername, kTurnPassword, kTurnPort443ProtoAddr); + ASSERT_TRUE(turn_port_); +} + +TEST_F(TurnPortTest, TestTurnDangerousAlternateServer) { + const ProtocolType protocol_type = PROTO_TCP; + std::vector redirect_addresses; + redirect_addresses.push_back(kTurnDangerousAddr); + + TestTurnRedirector redirector(redirect_addresses); + + turn_server_.AddInternalSocket(kTurnIntAddr, protocol_type); + turn_server_.AddInternalSocket(kTurnDangerousAddr, protocol_type); + turn_server_.set_redirect_hook(&redirector); + CreateTurnPort(kTurnUsername, kTurnPassword, + ProtocolAddress(kTurnIntAddr, protocol_type)); + + // Retrieve the address before we run the state machine. + const SocketAddress old_addr = turn_port_->server_address().address; + + turn_port_->PrepareAddress(); + // This should result in an error event. + EXPECT_TRUE_SIMULATED_WAIT(error_event_.error_code != 0, + TimeToGetAlternateTurnCandidate(protocol_type), + fake_clock_); + // but should NOT result in the port turning ready, and no candidates + // should be gathered. + EXPECT_FALSE(turn_ready_); + ASSERT_EQ(0U, turn_port_->Candidates().size()); +} + +TEST_F(TurnPortTest, TestTurnDangerousServerAllowedWithFieldTrial) { + webrtc::test::ScopedFieldTrials override_field_trials( + "WebRTC-Turn-AllowSystemPorts/Enabled/"); + CreateTurnPort(kTurnUsername, kTurnPassword, kTurnDangerousProtoAddr); + ASSERT_TRUE(turn_port_); +} + } // namespace cricket