From 3cb9c6afe749f20f8c50aa1fe7f39a2913f0a4f3 Mon Sep 17 00:00:00 2001 From: Harald Alvestrand Date: Mon, 2 Nov 2020 10:16:32 +0000 Subject: [PATCH] Check for oversized TURN usernames Bug: chromium:1144646 Change-Id: I8e71a025246708f05e38ba6f397f9655251da788 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/191222 Reviewed-by: Philipp Hancke Reviewed-by: Jonas Oreland Commit-Queue: Harald Alvestrand Cr-Commit-Position: refs/heads/master@{#32536} --- p2p/base/turn_port.h | 10 +++++++++ p2p/base/turn_port_unittest.cc | 37 +++++++++++++++++++++------------ p2p/client/turn_port_factory.cc | 4 ++++ 3 files changed, 38 insertions(+), 13 deletions(-) diff --git a/p2p/base/turn_port.h b/p2p/base/turn_port.h index 8247dbc777..a9ec434194 100644 --- a/p2p/base/turn_port.h +++ b/p2p/base/turn_port.h @@ -33,6 +33,8 @@ class TurnCustomizer; namespace cricket { +const int kMaxTurnUsernameLength = 509; // RFC 8489 section 14.3 + extern const int STUN_ATTR_TURN_LOGGING_ID; extern const char TURN_PORT_TYPE[]; class TurnAllocateRequest; @@ -61,6 +63,10 @@ class TurnPort : public Port { int server_priority, const std::string& origin, webrtc::TurnCustomizer* customizer) { + // Do basic parameter validation. + if (credentials.username.size() > kMaxTurnUsernameLength) { + return nullptr; + } // Using `new` to access a non-public constructor. return absl::WrapUnique(new TurnPort( thread, factory, network, socket, username, password, server_address, @@ -102,6 +108,10 @@ class TurnPort : public Port { const std::vector& tls_elliptic_curves, webrtc::TurnCustomizer* customizer, rtc::SSLCertificateVerifier* tls_cert_verifier = nullptr) { + // Do basic parameter validation. + if (credentials.username.size() > kMaxTurnUsernameLength) { + return nullptr; + } // Using `new` to access a non-public constructor. return absl::WrapUnique( new TurnPort(thread, factory, network, min_port, max_port, username, diff --git a/p2p/base/turn_port_unittest.cc b/p2p/base/turn_port_unittest.cc index ce86fe4a3a..e8c9b5c8ad 100644 --- a/p2p/base/turn_port_unittest.cc +++ b/p2p/base/turn_port_unittest.cc @@ -236,43 +236,43 @@ class TurnPortTest : public ::testing::Test, return &networks_.back(); } - void CreateTurnPort(const std::string& username, + bool CreateTurnPort(const std::string& username, const std::string& password, const ProtocolAddress& server_address) { - CreateTurnPortWithAllParams(MakeNetwork(kLocalAddr1), username, password, - server_address, std::string()); + return CreateTurnPortWithAllParams(MakeNetwork(kLocalAddr1), username, + password, server_address, std::string()); } - void CreateTurnPort(const rtc::SocketAddress& local_address, + bool CreateTurnPort(const rtc::SocketAddress& local_address, const std::string& username, const std::string& password, const ProtocolAddress& server_address) { - CreateTurnPortWithAllParams(MakeNetwork(local_address), username, password, - server_address, std::string()); + return CreateTurnPortWithAllParams(MakeNetwork(local_address), username, + password, server_address, std::string()); } // Should be identical to CreateTurnPort but specifies an origin value // when creating the instance of TurnPort. - void CreateTurnPortWithOrigin(const rtc::SocketAddress& local_address, + bool CreateTurnPortWithOrigin(const rtc::SocketAddress& local_address, const std::string& username, const std::string& password, const ProtocolAddress& server_address, const std::string& origin) { - CreateTurnPortWithAllParams(MakeNetwork(local_address), username, password, - server_address, origin); + return CreateTurnPortWithAllParams(MakeNetwork(local_address), username, + password, server_address, origin); } - void CreateTurnPortWithNetwork(rtc::Network* network, + bool CreateTurnPortWithNetwork(rtc::Network* network, const std::string& username, const std::string& password, const ProtocolAddress& server_address) { - CreateTurnPortWithAllParams(network, username, password, server_address, - std::string()); + return CreateTurnPortWithAllParams(network, username, password, + server_address, std::string()); } // Version of CreateTurnPort that takes all possible parameters; all other // helper methods call this, such that "SetIceRole" and "ConnectSignals" (and // possibly other things in the future) only happen in one place. - void CreateTurnPortWithAllParams(rtc::Network* network, + bool CreateTurnPortWithAllParams(rtc::Network* network, const std::string& username, const std::string& password, const ProtocolAddress& server_address, @@ -281,6 +281,9 @@ class TurnPortTest : public ::testing::Test, turn_port_ = TurnPort::Create( &main_, &socket_factory_, network, 0, 0, kIceUfrag1, kIcePwd1, server_address, credentials, 0, origin, {}, {}, turn_customizer_.get()); + if (!turn_port_) { + return false; + } // This TURN port will be the controlling. turn_port_->SetIceRole(ICEROLE_CONTROLLING); ConnectSignals(); @@ -292,6 +295,7 @@ class TurnPortTest : public ::testing::Test, turn_port_->SetTlsCertPolicy( TlsCertPolicy::TLS_CERT_POLICY_INSECURE_NO_CHECK); } + return true; } void CreateSharedTurnPort(const std::string& username, @@ -1774,4 +1778,11 @@ TEST_F(TurnPortTest, TestTurnCustomizerAddAttribute) { turn_port_.reset(nullptr); } +TEST_F(TurnPortTest, TestOverlongUsername) { + std::string overlong_username(513, 'x'); + RelayCredentials credentials(overlong_username, kTurnPassword); + EXPECT_FALSE( + CreateTurnPort(overlong_username, kTurnPassword, kTurnTlsProtoAddr)); +} + } // namespace cricket diff --git a/p2p/client/turn_port_factory.cc b/p2p/client/turn_port_factory.cc index de4b9e6a09..fd3420c016 100644 --- a/p2p/client/turn_port_factory.cc +++ b/p2p/client/turn_port_factory.cc @@ -28,6 +28,8 @@ std::unique_ptr TurnPortFactory::Create( args.username, args.password, *args.server_address, args.config->credentials, args.config->priority, args.origin, args.turn_customizer); + if (!port) + return nullptr; port->SetTlsCertPolicy(args.config->tls_cert_policy); port->SetTurnLoggingId(args.config->turn_logging_id); return std::move(port); @@ -42,6 +44,8 @@ std::unique_ptr TurnPortFactory::Create(const CreateRelayPortArgs& args, args.config->credentials, args.config->priority, args.origin, args.config->tls_alpn_protocols, args.config->tls_elliptic_curves, args.turn_customizer, args.config->tls_cert_verifier); + if (!port) + return nullptr; port->SetTlsCertPolicy(args.config->tls_cert_policy); port->SetTurnLoggingId(args.config->turn_logging_id); return std::move(port);