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}

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",

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[] = {

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<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

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

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<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 @@ 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,
+                            ice_parameters.ufrag, ice_parameters.pwd);
-                            jsep_description.transport_desc.ice_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<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 @@ 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(
+  ice_transport->SetRemoteIceParameters(ice_parameters);
-      remote_description_->transport_desc.GetIceParameters());
   ice_transport->SetRemoteIceMode(remote_description_->transport_desc.ice_mode);
 }
 

pc/jsep_transport.h

@@ -297,12 +297,9 @@ class JsepTransport : public sigslot::has_slots<> {
       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(

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_ufrag += "+new";
-  offer_transport_desc->ice_pwd += "_new";
+  offer_transport_desc->ice_pwd += "+new";
 
   ASSERT_TRUE(callee->SetRemoteDescription(std::move(offer)));