Validate ICE ufrag/pwd according to the spec
https://tools.ietf.org/html/draft-ietf-mmusic-ice-sip-sdp-39#section-5.4
Bug: chromium:1044521
Change-Id: Ia95718437dfc270b52cdf822e861a3da7cbbab76
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/167281
Commit-Queue: Steve Anton <steveanton@webrtc.org>
Reviewed-by: Qingsi Wang <qingsi@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#30375}
diff --git a/p2p/BUILD.gn b/p2p/BUILD.gn
index f3b5dd4..ae49deb 100644
--- a/p2p/BUILD.gn
+++ b/p2p/BUILD.gn
@@ -193,6 +193,7 @@
"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 b0a21d6d..dd7e38e 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<std::string> 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<std::string> 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> 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 15e2e91..e7934ba 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 @@
};
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<IceParameters> 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 0000000..41d7336
--- /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 37f3162..8a555f2 100644
--- a/pc/jsep_transport.cc
+++ b/pc/jsep_transport.cc
@@ -31,28 +31,6 @@
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 error;
RTC_DCHECK_RUN_ON(network_thread_);
- if (!VerifyIceParams(jsep_description)) {
+
+ webrtc::RTCErrorOr<IceParameters> 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 @@
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 @@
{
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 error;
RTC_DCHECK_RUN_ON(network_thread_);
- if (!VerifyIceParams(jsep_description)) {
+
+ webrtc::RTCErrorOr<IceParameters> 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 @@
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::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 6edf0ae..6d88def 100644
--- a/pc/jsep_transport.h
+++ b/pc/jsep_transport.h
@@ -297,12 +297,9 @@
ConnectionRole remote_connection_role,
absl::optional<rtc::SSLRole>* 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 7d8ce57..ab5a8f4 100644
--- a/pc/peer_connection_ice_unittest.cc
+++ b/pc/peer_connection_ice_unittest.cc
@@ -1202,10 +1202,10 @@
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 @@
// 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)));
