Introduce length checking of all STUN byte string attributes

This will cause encoding of a STUN message with an over-long
byte string attribute to fail.

Bug: chromium:1144646
Change-Id: I265174577376ce01439835c03f2d46700842d211
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/191322
Commit-Queue: Harald Alvestrand <hta@webrtc.org>
Reviewed-by: Justin Uberti <juberti@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#32603}

api/transport/BUILD.gn

@@ -106,10 +106,12 @@ rtc_source_set("stun_types") {
   ]
 
   deps = [
+    "../../api:array_view",
     "../../rtc_base:checks",
     "../../rtc_base:rtc_base",
     "../../rtc_base:rtc_base_approved",
   ]
+  absl_deps = [ "//third_party/abseil-cpp/absl/strings" ]
 }
 
 if (rtc_include_tests) {

api/transport/stun.cc

@@ -11,8 +11,9 @@
 #include "api/transport/stun.h"
 
 #include <string.h>
-
 #include <algorithm>
+#include <cstdint>
+#include <iterator>
 #include <memory>
 #include <utility>
 
@@ -25,8 +26,14 @@
 using rtc::ByteBufferReader;
 using rtc::ByteBufferWriter;
 
+namespace cricket {
+
 namespace {
 
+const int k127Utf8CharactersLengthInBytes = 508;
+const int kDefaultMaxAttributeLength = 508;
+const int kMessageIntegrityAttributeLength = 20;
+
 uint32_t ReduceTransactionId(const std::string& transaction_id) {
   RTC_DCHECK(transaction_id.length() == cricket::kStunTransactionIdLength ||
              transaction_id.length() ==
@@ -40,9 +47,46 @@ uint32_t ReduceTransactionId(const std::string& transaction_id) {
   return result;
 }
 
-}  // namespace
+// Check the maximum length of a BYTE_STRING attribute against specifications.
+bool LengthValid(int type, int length) {
+  // "Less than 509 bytes" is intended to indicate a maximum of 127
+  // UTF-8 characters, which may take up to 4 bytes per character.
+  switch (type) {
+    case STUN_ATTR_USERNAME:
+      return length <=
+             k127Utf8CharactersLengthInBytes;  // RFC 8489 section 14.3
+    case STUN_ATTR_MESSAGE_INTEGRITY:
+      return length ==
+             kMessageIntegrityAttributeLength;  // RFC 8489 section 14.5
+    case STUN_ATTR_REALM:
+      return length <=
+             k127Utf8CharactersLengthInBytes;  // RFC 8489 section 14.9
+    case STUN_ATTR_NONCE:
+      return length <=
+             k127Utf8CharactersLengthInBytes;  // RFC 8489 section 14.10
+    case STUN_ATTR_SOFTWARE:
+      return length <=
+             k127Utf8CharactersLengthInBytes;  // RFC 8489 section 14.14
+    case STUN_ATTR_ORIGIN:
+      // 0x802F is unassigned by IANA.
+      // RESPONSE-ORIGIN is defined in RFC 5780 section 7.3, but does not
+      // specify a maximum length. It's an URL, so return an arbitrary
+      // restriction.
+      return length <= kDefaultMaxAttributeLength;
+    case STUN_ATTR_DATA:
+      // No length restriction in RFC; it's the content of an UDP datagram,
+      // which in theory can be up to 65.535 bytes.
+      // TODO(bugs.webrtc.org/12179): Write a test to find the real limit.
+      return length <= 65535;
+    default:
+      // Return an arbitrary restriction for all other types.
+      return length <= kDefaultMaxAttributeLength;
+  }
+  RTC_NOTREACHED();
+  return true;
+}
 
-namespace cricket {
+}  // namespace
 
 const char STUN_ERROR_REASON_TRY_ALTERNATE_SERVER[] = "Try Alternate Server";
 const char STUN_ERROR_REASON_BAD_REQUEST[] = "Bad Request";
@@ -993,6 +1037,10 @@ bool StunByteStringAttribute::Read(ByteBufferReader* buf) {
 }
 
 bool StunByteStringAttribute::Write(ByteBufferWriter* buf) const {
+  // Check that length is legal according to specs
+  if (!LengthValid(type(), length())) {
+    return false;
+  }
   buf->WriteBytes(bytes_, length());
   WritePadding(buf);
   return true;

api/transport/stun.h

@@ -16,11 +16,13 @@
 
 #include <stddef.h>
 #include <stdint.h>
-
+#include <functional>
 #include <memory>
 #include <string>
 #include <vector>
 
+#include "absl/strings/string_view.h"
+#include "api/array_view.h"
 #include "rtc_base/byte_buffer.h"
 #include "rtc_base/ip_address.h"
 #include "rtc_base/socket_address.h"
@@ -133,7 +135,6 @@ class StunAddressAttribute;
 class StunAttribute;
 class StunByteStringAttribute;
 class StunErrorCodeAttribute;
-
 class StunUInt16ListAttribute;
 class StunUInt32Attribute;
 class StunUInt64Attribute;

api/transport/stun_unittest.cc

@@ -1903,4 +1903,16 @@ TEST_F(StunTest, IsStunMethod) {
       sizeof(kRfc5769SampleRequest)));
 }
 
+TEST_F(StunTest, SizeRestrictionOnAttributes) {
+  StunMessage msg;
+  msg.SetType(STUN_BINDING_REQUEST);
+  msg.SetTransactionID("ABCDEFGH");
+  auto long_username = StunAttribute::CreateByteString(STUN_ATTR_USERNAME);
+  std::string long_string(509, 'x');
+  long_username->CopyBytes(long_string.c_str(), long_string.size());
+  msg.AddAttribute(std::move(long_username));
+  rtc::ByteBufferWriter out;
+  ASSERT_FALSE(msg.Write(&out));
+}
+
 }  // namespace cricket

tools_webrtc/iwyu/iwyu-filter-list

@@ -3,3 +3,4 @@
 # in certain configurations.
 #include <sys/socket.h>
 #include <ext/alloc_traits.h>
+#include <netinet/in.h>