From bee6408d7b671199206bbf3b27a6d52ad906532e Mon Sep 17 00:00:00 2001 From: Harald Alvestrand Date: Thu, 12 Nov 2020 11:17:41 +0000 Subject: [PATCH] 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 Reviewed-by: Justin Uberti Cr-Commit-Position: refs/heads/master@{#32603} --- api/transport/BUILD.gn | 2 ++ api/transport/stun.cc | 54 ++++++++++++++++++++++++++++-- api/transport/stun.h | 5 +-- api/transport/stun_unittest.cc | 12 +++++++ tools_webrtc/iwyu/iwyu-filter-list | 1 + 5 files changed, 69 insertions(+), 5 deletions(-) diff --git a/api/transport/BUILD.gn b/api/transport/BUILD.gn index 6a7cc57cd29..7bcda8b4a7e 100644 --- a/api/transport/BUILD.gn +++ b/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) { diff --git a/api/transport/stun.cc b/api/transport/stun.cc index 7fee6ea78af..c3f589a696f 100644 --- a/api/transport/stun.cc +++ b/api/transport/stun.cc @@ -11,8 +11,9 @@ #include "api/transport/stun.h" #include - #include +#include +#include #include #include @@ -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; diff --git a/api/transport/stun.h b/api/transport/stun.h index db37b8e3650..8893b2a1ffd 100644 --- a/api/transport/stun.h +++ b/api/transport/stun.h @@ -16,11 +16,13 @@ #include #include - +#include #include #include #include +#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; diff --git a/api/transport/stun_unittest.cc b/api/transport/stun_unittest.cc index 0884b2ca1c2..bf2717e0070 100644 --- a/api/transport/stun_unittest.cc +++ b/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 diff --git a/tools_webrtc/iwyu/iwyu-filter-list b/tools_webrtc/iwyu/iwyu-filter-list index f31b996e919..0c0c69558b6 100644 --- a/tools_webrtc/iwyu/iwyu-filter-list +++ b/tools_webrtc/iwyu/iwyu-filter-list @@ -3,3 +3,4 @@ # in certain configurations. #include #include +#include