Change the size of the ICE ufrag to 4 bytes

webrtc/p2p/base/relayserver.cc

 /*
  *  Copyright 2004 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 "webrtc/p2p/base/relayserver.h"
 
 #ifdef WEBRTC_POSIX
 #include <errno.h>
 #endif  // WEBRTC_POSIX
 
 #include <algorithm>
 
 #include "webrtc/base/asynctcpsocket.h"
 #include "webrtc/base/helpers.h"
 #include "webrtc/base/logging.h"
 #include "webrtc/base/socketadapters.h"
 
 namespace cricket {
 
 // By default, we require a ping every 90 seconds.
 const int MAX_LIFETIME = 15 * 60 * 1000;
 
-// The number of bytes in each of the usernames we use.
-const uint32_t USERNAME_LENGTH = 16;
-
 // Calls SendTo on the given socket and logs any bad results.
 void Send(rtc::AsyncPacketSocket* socket, const char* bytes, size_t size,
           const rtc::SocketAddress& addr) {
   rtc::PacketOptions options;
   int result = socket->SendTo(bytes, size, addr, options);
   if (result < static_cast<int>(size)) {
     LOG(LS_ERROR) << "SendTo wrote only " << result << " of " << size
                   << " bytes";
   } else if (result < 0) {
     LOG_ERR(LS_ERROR) << "SendTo";
@@ skipping 214 matching lines @@
   }
 
   // The initial packet should have a username (which identifies the binding).
   const StunByteStringAttribute* username_attr =
       msg.GetByteString(STUN_ATTR_USERNAME);
   if (!username_attr) {
     LOG(LS_WARNING) << "Dropping packet: no username";
     return;
   }
 
-  uint32_t length =
-      std::min(static_cast<uint32_t>(username_attr->length()), USERNAME_LENGTH);
+  uint32_t length = static_cast<uint32_t>(username_attr->length());
   std::string username(username_attr->bytes(), length);
+  // The username is the substring before colon in username_attr
+  std::string::size_type colonpos = username.find(':');
+  if (colonpos != std::string::npos) {
+    username = username.substr(0, colonpos);

[pthatcher1, 2016/05/05 18:07:19]

I don't think this is correct.

The place where we put a binding into bindings_ gets a username from code kind
of like this:

  std::string username;
  const StunByteStringAttribute* username_attr =
msg->GetByteString(STUN_ATTR_USERNAME);
  username.append(username_attr->bytes(), username_attr->length());

So we should just make this code match and use the full username, not the
username up to a colon. 

The only thing the code did before that we might remove is set a maximum
username length to 16.  But this is legacy code and it's probably not worth
changing it for that.


In other words, just revert this file.  It doesn't need to be changed.

[Zhi Huang, 2016/05/05 18:27:09]

The reason I made these changes is some unit tests will fail.

I looked into this and I found that username_attr contains two parts (two names
I think) which is separated by a colon and the length of user_name is 2 *
length(ufrag) + 1. When the ufrag length is 16, the length of username_attr is
33. After min(33, 16), the length will be 16, which works fine. However, when
the ufrag length is 4, the length(username_attr) = 9. After min(9, 16), the
length will be 9, which will be a problem.

[pthatcher1, 2016/05/05 20:24:31]

I don't see why a length of 9 would cause this code to fail, but perhaps there
is something I missing.  In that case, we should fix the unittest to use 16
rather than 4.

Which unit test is it?

[Zhi Huang, 2016/05/05 23:10:59]

I changed code like this:
 const uint32_t USERNAME_LENGTH = 16;
 ...
 uint32_t length = 
  std::min(static_cast<uint32_t>(username_attr->length()),       
  USERNAME_LENGTH);
  std::string username(username_attr->bytes(), length);
  // TODO: Check the HMAC.
  // The binding should already be present.
  BindingMap::iterator biter = bindings_.find(username);
  if (biter == bindings_.end()) {
    LOG(LS_WARNING) << "Dropping packet: no binding with username";
    return;
  }

These tests will fail.
 out/Debug/rtc_unittests: PortTest.TestLocalToTcpGturn
 out/Debug/rtc_unittests: PortTest.TestLocalToGturn
 out/Debug/rtc_unittests: PortTest.TestPRNatToTcpGturn
 out/Debug/rtc_unittests: PortTest.TestLocalToSslTcpGturn
 out/Debug/rtc_unittests: PortTest.TestPRNatToGturn
 out/Debug/rtc_unittests: PortTest.TestSymNatToGturn
 out/Debug/rtc_unittests: PortTest.TestSymNatToTcpGturn
 out/Debug/rtc_unittests: PortTest.TestARNatToGturn
 out/Debug/rtc_unittests: PortTest.TestARNATNatToTcpGturn
 out/Debug/rtc_unittests: PortTest.TestConeNatToGturn
 out/Debug/rtc_unittests: PortTest.TestConeNatToTcpGturn

When I change USERNAME_LENGTH = 4, these tests will fail.
out/Debug/rtc_unittests: RelayServerTest.TestRemoteBind
 out/Debug/rtc_unittests: RelayServerTest.TestSendRequestBadUsername
 out/Debug/rtc_unittests: RelayServerTest.TestSendRequestNoDestinationAddress
 out/Debug/rtc_unittests: RelayServerTest.TestSendRequestNoData
 out/Debug/rtc_unittests: RelayServerTest.TestSendRaw

Since the USERNAME_LENGTH is only used here, I think its purpose is to extract
the substring from username_attribute. So using the colon to extract the
username might be more flexible.

[pthatcher1, 2016/05/09 23:59:25]

uint32_t length = static_cast<uint32_t>(username_attr->length()));
std::string username(username_attr->bytes(), length);
// TODO: Check the HMAC.
// The binding should already be present.
BindingMap::iterator biter = bindings_.find(username);
if (biter == bindings_.end()) {
  LOG(LS_WARNING) << "Dropping packet: no binding with username";
  return;
}

On 2016/05/05 23:10:59, Zhi Huang wrote:

+  }
   // TODO: Check the HMAC.
-
   // The binding should already be present.
   BindingMap::iterator biter = bindings_.find(username);
   if (biter == bindings_.end()) {
     LOG(LS_WARNING) << "Dropping packet: no binding with username";
     return;
   }
 
   // Add this authenticted connection to the binding.
   RelayServerConnection* ext_conn =
       new RelayServerConnection(biter->second, ap, socket);
@@ skipping 460 matching lines @@
     } else {
       server_->thread()->PostDelayed(lifetime_, this, MSG_LIFETIME_TIMER);
     }
 
   } else {
     ASSERT(false);
   }
 }
 
 }  // namespace cricket