diff --git a/p2p/BUILD.gn b/p2p/BUILD.gn index f3b5dd4ae5..ae49deb264 100644 --- a/p2p/BUILD.gn +++ b/p2p/BUILD.gn @@ -193,6 +193,7 @@ if (rtc_include_tests) { "base/stun_server_unittest.cc", "base/tcp_port_unittest.cc", "base/transport_description_factory_unittest.cc", + "base/transport_description_unittest.cc", "base/turn_port_unittest.cc", "base/turn_server_unittest.cc", "client/basic_port_allocator_unittest.cc", diff --git a/p2p/base/transport_description.cc b/p2p/base/transport_description.cc index b0a21d6d71..dd7e38e5a8 100644 --- a/p2p/base/transport_description.cc +++ b/p2p/base/transport_description.cc @@ -10,10 +10,86 @@ #include "p2p/base/transport_description.h" +#include "absl/strings/ascii.h" #include "absl/strings/match.h" +#include "p2p/base/p2p_constants.h" #include "rtc_base/arraysize.h" +#include "rtc_base/strings/string_builder.h" + +using webrtc::RTCError; +using webrtc::RTCErrorOr; +using webrtc::RTCErrorType; namespace cricket { +namespace { + +bool IsIceChar(char c) { + return absl::ascii_isalnum(c) || c == '+' || c == '/'; +} + +RTCErrorOr ParseIceUfrag(absl::string_view raw_ufrag) { + if (!(ICE_UFRAG_MIN_LENGTH <= raw_ufrag.size() && + raw_ufrag.size() <= ICE_UFRAG_MAX_LENGTH)) { + rtc::StringBuilder sb; + sb << "ICE ufrag must be between " << ICE_UFRAG_MIN_LENGTH << " and " + << ICE_UFRAG_MAX_LENGTH << " characters long."; + return RTCError(RTCErrorType::SYNTAX_ERROR, sb.Release()); + } + + if (!absl::c_all_of(raw_ufrag, IsIceChar)) { + return RTCError( + RTCErrorType::SYNTAX_ERROR, + "ICE ufrag must contain only alphanumeric characters, '+', and '/'."); + } + + return std::string(raw_ufrag); +} + +RTCErrorOr ParseIcePwd(absl::string_view raw_pwd) { + if (!(ICE_PWD_MIN_LENGTH <= raw_pwd.size() && + raw_pwd.size() <= ICE_PWD_MAX_LENGTH)) { + rtc::StringBuilder sb; + sb << "ICE pwd must be between " << ICE_PWD_MIN_LENGTH << " and " + << ICE_PWD_MAX_LENGTH << " characters long."; + return RTCError(RTCErrorType::SYNTAX_ERROR, sb.Release()); + } + + if (!absl::c_all_of(raw_pwd, IsIceChar)) { + return RTCError( + RTCErrorType::SYNTAX_ERROR, + "ICE pwd must contain only alphanumeric characters, '+', and '/'."); + } + + return std::string(raw_pwd); +} + +} // namespace + +// static +RTCErrorOr IceParameters::Parse(absl::string_view raw_ufrag, + absl::string_view raw_pwd) { + // For legacy protocols. + // TODO(zhihuang): Remove this once the legacy protocol is no longer + // supported. + if (raw_ufrag.empty() && raw_pwd.empty()) { + return IceParameters(); + } + + auto ufrag_result = ParseIceUfrag(raw_ufrag); + if (!ufrag_result.ok()) { + return ufrag_result.MoveError(); + } + + auto pwd_result = ParseIcePwd(raw_pwd); + if (!pwd_result.ok()) { + return pwd_result.MoveError(); + } + + IceParameters parameters; + parameters.ufrag = ufrag_result.MoveValue(); + parameters.pwd = pwd_result.MoveValue(); + return parameters; +} bool StringToConnectionRole(const std::string& role_str, ConnectionRole* role) { const char* const roles[] = { diff --git a/p2p/base/transport_description.h b/p2p/base/transport_description.h index 15e2e919f3..e7934bab20 100644 --- a/p2p/base/transport_description.h +++ b/p2p/base/transport_description.h @@ -17,6 +17,7 @@ #include "absl/algorithm/container.h" #include "absl/types/optional.h" +#include "api/rtc_error.h" #include "p2p/base/p2p_constants.h" #include "rtc_base/ssl_fingerprint.h" @@ -56,6 +57,11 @@ enum ConnectionRole { }; struct IceParameters { + // Constructs an IceParameters from a user-provided ufrag/pwd combination. + // Returns a SyntaxError if the ufrag or pwd are malformed. + static webrtc::RTCErrorOr Parse(absl::string_view raw_ufrag, + absl::string_view raw_pwd); + // TODO(honghaiz): Include ICE mode in this structure to match the ORTC // struct: // http://ortc.org/wp-content/uploads/2016/03/ortc.html#idl-def-RTCIceParameters diff --git a/p2p/base/transport_description_unittest.cc b/p2p/base/transport_description_unittest.cc new file mode 100644 index 0000000000..41d7336ff6 --- /dev/null +++ b/p2p/base/transport_description_unittest.cc @@ -0,0 +1,58 @@ +/* + * Copyright 2020 The WebRTC Project Authors. All rights reserved. + * + * Use of this source code is governed by a BSD-style license + * that can be found in the LICENSE file in the root of the source + * tree. An additional intellectual property rights grant can be found + * in the file PATENTS. All contributing project authors may + * be found in the AUTHORS file in the root of the source tree. + */ + +#include "p2p/base/transport_description.h" +#include "test/gtest.h" + +using webrtc::RTCErrorType; + +namespace cricket { + +TEST(IceParameters, SuccessfulParse) { + auto result = IceParameters::Parse("ufrag", "22+characters+long+pwd"); + ASSERT_TRUE(result.ok()); + IceParameters parameters = result.MoveValue(); + EXPECT_EQ("ufrag", parameters.ufrag); + EXPECT_EQ("22+characters+long+pwd", parameters.pwd); +} + +TEST(IceParameters, FailedParseShortUfrag) { + auto result = IceParameters::Parse("3ch", "22+characters+long+pwd"); + EXPECT_EQ(RTCErrorType::SYNTAX_ERROR, result.error().type()); +} + +TEST(IceParameters, FailedParseLongUfrag) { + std::string ufrag(257, '+'); + auto result = IceParameters::Parse(ufrag, "22+characters+long+pwd"); + EXPECT_EQ(RTCErrorType::SYNTAX_ERROR, result.error().type()); +} + +TEST(IceParameters, FailedParseShortPwd) { + auto result = IceParameters::Parse("ufrag", "21+character+long+pwd"); + EXPECT_EQ(RTCErrorType::SYNTAX_ERROR, result.error().type()); +} + +TEST(IceParameters, FailedParseLongPwd) { + std::string pwd(257, '+'); + auto result = IceParameters::Parse("ufrag", pwd); + EXPECT_EQ(RTCErrorType::SYNTAX_ERROR, result.error().type()); +} + +TEST(IceParameters, FailedParseBadUfragChar) { + auto result = IceParameters::Parse("ufrag\r\n", "22+characters+long+pwd"); + EXPECT_EQ(RTCErrorType::SYNTAX_ERROR, result.error().type()); +} + +TEST(IceParameters, FailedParseBadPwdChar) { + auto result = IceParameters::Parse("ufrag", "22+characters+long+pwd\r\n"); + EXPECT_EQ(RTCErrorType::SYNTAX_ERROR, result.error().type()); +} + +} // namespace cricket diff --git a/pc/jsep_transport.cc b/pc/jsep_transport.cc index 37f31628dd..8a555f2c67 100644 --- a/pc/jsep_transport.cc +++ b/pc/jsep_transport.cc @@ -31,28 +31,6 @@ using webrtc::SdpType; namespace cricket { -static bool VerifyIceParams(const JsepTransportDescription& jsep_description) { - // For legacy protocols. - // TODO(zhihuang): Remove this once the legacy protocol is no longer - // supported. - if (jsep_description.transport_desc.ice_ufrag.empty() && - jsep_description.transport_desc.ice_pwd.empty()) { - return true; - } - - if (jsep_description.transport_desc.ice_ufrag.length() < - ICE_UFRAG_MIN_LENGTH || - jsep_description.transport_desc.ice_ufrag.length() > - ICE_UFRAG_MAX_LENGTH) { - return false; - } - if (jsep_description.transport_desc.ice_pwd.length() < ICE_PWD_MIN_LENGTH || - jsep_description.transport_desc.ice_pwd.length() > ICE_PWD_MAX_LENGTH) { - return false; - } - return true; -} - JsepTransportDescription::JsepTransportDescription() {} JsepTransportDescription::JsepTransportDescription( @@ -199,10 +177,17 @@ webrtc::RTCError JsepTransport::SetLocalJsepTransportDescription( webrtc::RTCError error; RTC_DCHECK_RUN_ON(network_thread_); - if (!VerifyIceParams(jsep_description)) { + + webrtc::RTCErrorOr ice_parameters_result = + IceParameters::Parse(jsep_description.transport_desc.ice_ufrag, + jsep_description.transport_desc.ice_pwd); + if (!ice_parameters_result.ok()) { + rtc::StringBuilder sb; + sb << "Invalid ICE parameters: " << ice_parameters_result.error().message(); return webrtc::RTCError(webrtc::RTCErrorType::INVALID_PARAMETER, - "Invalid ice-ufrag or ice-pwd length."); + sb.Release()); } + IceParameters ice_parameters = ice_parameters_result.MoveValue(); if (!SetRtcpMux(jsep_description.rtcp_mux_enabled, type, ContentSource::CS_LOCAL)) { @@ -233,8 +218,7 @@ webrtc::RTCError JsepTransport::SetLocalJsepTransportDescription( local_description_ != nullptr && IceCredentialsChanged(local_description_->transport_desc.ice_ufrag, local_description_->transport_desc.ice_pwd, - jsep_description.transport_desc.ice_ufrag, - jsep_description.transport_desc.ice_pwd); + ice_parameters.ufrag, ice_parameters.pwd); local_description_.reset(new JsepTransportDescription(jsep_description)); rtc::SSLFingerprint* local_fp = @@ -252,11 +236,13 @@ webrtc::RTCError JsepTransport::SetLocalJsepTransportDescription( { rtc::CritScope scope(&accessor_lock_); RTC_DCHECK(rtp_dtls_transport_->internal()); - SetLocalIceParameters(rtp_dtls_transport_->internal()->ice_transport()); + rtp_dtls_transport_->internal()->ice_transport()->SetIceParameters( + ice_parameters); if (rtcp_dtls_transport_) { RTC_DCHECK(rtcp_dtls_transport_->internal()); - SetLocalIceParameters(rtcp_dtls_transport_->internal()->ice_transport()); + rtcp_dtls_transport_->internal()->ice_transport()->SetIceParameters( + ice_parameters); } } // If PRANSWER/ANSWER is set, we should decide transport protocol type. @@ -286,11 +272,18 @@ webrtc::RTCError JsepTransport::SetRemoteJsepTransportDescription( webrtc::RTCError error; RTC_DCHECK_RUN_ON(network_thread_); - if (!VerifyIceParams(jsep_description)) { + + webrtc::RTCErrorOr ice_parameters_result = + IceParameters::Parse(jsep_description.transport_desc.ice_ufrag, + jsep_description.transport_desc.ice_pwd); + if (!ice_parameters_result.ok()) { remote_description_.reset(); + rtc::StringBuilder sb; + sb << "Invalid ICE parameters: " << ice_parameters_result.error().message(); return webrtc::RTCError(webrtc::RTCErrorType::INVALID_PARAMETER, - "Invalid ice-ufrag or ice-pwd length."); + sb.Release()); } + IceParameters ice_parameters = ice_parameters_result.MoveValue(); if (!SetRtcpMux(jsep_description.rtcp_mux_enabled, type, ContentSource::CS_REMOTE)) { @@ -324,10 +317,11 @@ webrtc::RTCError JsepTransport::SetRemoteJsepTransportDescription( remote_description_.reset(new JsepTransportDescription(jsep_description)); RTC_DCHECK(rtp_dtls_transport()); - SetRemoteIceParameters(rtp_dtls_transport()->ice_transport()); + SetRemoteIceParameters(ice_parameters, rtp_dtls_transport()->ice_transport()); if (rtcp_dtls_transport()) { - SetRemoteIceParameters(rtcp_dtls_transport()->ice_transport()); + SetRemoteIceParameters(ice_parameters, + rtcp_dtls_transport()->ice_transport()); } // If PRANSWER/ANSWER is set, we should decide transport protocol type. @@ -456,21 +450,13 @@ void JsepTransport::SetActiveResetSrtpParams(bool active_reset_srtp_params) { } } -void JsepTransport::SetLocalIceParameters(IceTransportInternal* ice_transport) { - RTC_DCHECK_RUN_ON(network_thread_); - RTC_DCHECK(ice_transport); - RTC_DCHECK(local_description_); - ice_transport->SetIceParameters( - local_description_->transport_desc.GetIceParameters()); -} - void JsepTransport::SetRemoteIceParameters( + const IceParameters& ice_parameters, IceTransportInternal* ice_transport) { RTC_DCHECK_RUN_ON(network_thread_); RTC_DCHECK(ice_transport); RTC_DCHECK(remote_description_); - ice_transport->SetRemoteIceParameters( - remote_description_->transport_desc.GetIceParameters()); + ice_transport->SetRemoteIceParameters(ice_parameters); ice_transport->SetRemoteIceMode(remote_description_->transport_desc.ice_mode); } diff --git a/pc/jsep_transport.h b/pc/jsep_transport.h index 6edf0aecee..6d88deff07 100644 --- a/pc/jsep_transport.h +++ b/pc/jsep_transport.h @@ -297,12 +297,9 @@ class JsepTransport : public sigslot::has_slots<> { ConnectionRole remote_connection_role, absl::optional* negotiated_dtls_role); - // Pushes down the ICE parameters from the local description, such - // as the ICE ufrag and pwd. - void SetLocalIceParameters(IceTransportInternal* ice); - // Pushes down the ICE parameters from the remote description. - void SetRemoteIceParameters(IceTransportInternal* ice); + void SetRemoteIceParameters(const IceParameters& ice_parameters, + IceTransportInternal* ice); // Pushes down the DTLS parameters obtained via negotiation. webrtc::RTCError SetNegotiatedDtlsParameters( diff --git a/pc/peer_connection_ice_unittest.cc b/pc/peer_connection_ice_unittest.cc index 7d8ce57937..ab5a8f40ca 100644 --- a/pc/peer_connection_ice_unittest.cc +++ b/pc/peer_connection_ice_unittest.cc @@ -1202,10 +1202,10 @@ TEST_P(PeerConnectionIceUfragPwdAnswerTest, TestIncludedInAnswer) { auto offer = caller->CreateOffer(); auto* offer_transport_desc = GetFirstTransportDescription(offer.get()); if (offer_new_ufrag_) { - offer_transport_desc->ice_ufrag += "_new"; + offer_transport_desc->ice_ufrag += "+new"; } if (offer_new_pwd_) { - offer_transport_desc->ice_pwd += "_new"; + offer_transport_desc->ice_pwd += "+new"; } ASSERT_TRUE(callee->SetRemoteDescription(std::move(offer))); @@ -1248,8 +1248,8 @@ TEST_P(PeerConnectionIceTest, // Signal ICE restart on the first media section. auto* offer_transport_desc = GetFirstTransportDescription(offer.get()); - offer_transport_desc->ice_ufrag += "_new"; - offer_transport_desc->ice_pwd += "_new"; + offer_transport_desc->ice_ufrag += "+new"; + offer_transport_desc->ice_pwd += "+new"; ASSERT_TRUE(callee->SetRemoteDescription(std::move(offer)));