http: fix allocation bug introduced in envoyproxy#4211. (envoyproxy#4245

)

There were some non-local invariants that header_map_impl_fuzz_test surfaced around minimum dynamic
buffer size. This PR improves comments and documentation of invariants and fixes the allocation issue
to maintain them.

Fixes oss-fuzz issue https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=10038.

Risk level: Low. It's recommended to bump to this for potential security reasons if you are already post
envoyproxy#4211.
Testing: Unit test and corpus entry added.

Signed-off-by: Harvey Tuch <[email protected]>

include/envoy/http/header_map.h

@@ -154,13 +154,17 @@ class HeaderString {
   bool operator!=(const char* rhs) const { return 0 != strcmp(c_str(), rhs); }
 
 private:
-  union {
+  union Buffer {
+    // This should reference inline_buffer_ for Type::Inline.
     char* dynamic_;
     const char* ref_;
   } buffer_;
 
+  // Capacity in both Type::Inline and Type::Dynamic cases must be at least MinDynamicCapacity in
+  // header_map_impl.cc.
   union {
     char inline_buffer_[128];
+    // Since this is a union, this is only valid for type_ == Type::Dynamic.
     uint32_t dynamic_capacity_;
   };
 

source/common/http/header_map_impl.cc

@@ -14,9 +14,25 @@
 namespace Envoy {
 namespace Http {
 
+namespace {
+constexpr size_t MinDynamicCapacity{32};
+// This includes the NULL (StringUtil::itoa technically only needs 21).
+constexpr size_t MaxIntegerLength{32};
+
+void validateCapacity(size_t new_capacity) {
+  // If the resizing will cause buffer overflow due to hitting uint32_t::max, an OOM is likely
+  // imminent. Fast-fail rather than allow a buffer overflow attack (issue #1421)
+  RELEASE_ASSERT(new_capacity <= std::numeric_limits<uint32_t>::max(), "");
+  ASSERT(new_capacity >= MinDynamicCapacity);
+}
+
+} // namespace
+
 HeaderString::HeaderString() : type_(Type::Inline) {
   buffer_.dynamic_ = inline_buffer_;
   clear();
+  static_assert(sizeof(inline_buffer_) >= MaxIntegerLength, "");
+  static_assert(MinDynamicCapacity >= MaxIntegerLength, "");
 }
 
 HeaderString::HeaderString(const LowerCaseString& ref_value) : type_(Type::Reference) {
@@ -70,7 +86,9 @@ void HeaderString::append(const char* data, uint32_t size) {
     // Rather than be too clever and optimize this uncommon case, we dynamically
     // allocate and copy.
     type_ = Type::Dynamic;
-    dynamic_capacity_ = (string_length_ + size) * 2;
+    dynamic_capacity_ =
+        std::max(MinDynamicCapacity, static_cast<size_t>((string_length_ + size) * 2));
+    validateCapacity(dynamic_capacity_);
     char* buf = static_cast<char*>(malloc(dynamic_capacity_));
     RELEASE_ASSERT(buf != nullptr, "");
     memcpy(buf, buffer_.ref_, string_length_);
@@ -90,19 +108,18 @@ void HeaderString::append(const char* data, uint32_t size) {
   case Type::Dynamic: {
     // We can get here either because we didn't fit in inline or we are already dynamic.
     if (type_ == Type::Inline) {
-      const uint64_t new_capacity = (static_cast<uint64_t>(string_length_) + size) * 2;
-      // If the resizing will cause buffer overflow due to hitting uint32_t::max, an OOM is likely
-      // imminent. Fast-fail rather than allow a buffer overflow attack (issue #1421)
-      RELEASE_ASSERT(new_capacity <= std::numeric_limits<uint32_t>::max(), "");
+      const size_t new_capacity = (string_length_ + size) * 2;
+      validateCapacity(new_capacity);
       buffer_.dynamic_ = static_cast<char*>(malloc(new_capacity));
-      memcpy(buffer_.dynamic_, inline_buffer_, string_length_);
       RELEASE_ASSERT(buffer_.dynamic_ != nullptr, "");
+      memcpy(buffer_.dynamic_, inline_buffer_, string_length_);
       dynamic_capacity_ = new_capacity;
       type_ = Type::Dynamic;
     } else {
       if (size + 1 + string_length_ > dynamic_capacity_) {
         // Need to reallocate.
         dynamic_capacity_ = (string_length_ + size) * 2;
+        validateCapacity(dynamic_capacity_);
         buffer_.dynamic_ = static_cast<char*>(realloc(buffer_.dynamic_, dynamic_capacity_));
         RELEASE_ASSERT(buffer_.dynamic_ != nullptr, "");
       }
@@ -153,14 +170,18 @@ void HeaderString::setCopy(const char* data, uint32_t size) {
     // We can get here either because we didn't fit in inline or we are already dynamic.
     if (type_ == Type::Inline) {
       dynamic_capacity_ = size * 2;
+      validateCapacity(dynamic_capacity_);
       buffer_.dynamic_ = static_cast<char*>(malloc(dynamic_capacity_));
+      RELEASE_ASSERT(buffer_.dynamic_ != nullptr, "");
       type_ = Type::Dynamic;
     } else {
       if (size + 1 > dynamic_capacity_) {
         // Need to reallocate. Use free/malloc to avoid the copy since we are about to overwrite.
         dynamic_capacity_ = size * 2;
+        validateCapacity(dynamic_capacity_);
         free(buffer_.dynamic_);
         buffer_.dynamic_ = static_cast<char*>(malloc(dynamic_capacity_));
+        RELEASE_ASSERT(buffer_.dynamic_ != nullptr, "");
       }
     }
   }
@@ -182,8 +203,14 @@ void HeaderString::setInteger(uint64_t value) {
   }
 
   case Type::Inline:
+    // buffer_.dynamic_ should always point at inline_buffer_ for Type::Inline.
+    ASSERT(buffer_.dynamic_ == inline_buffer_);
   case Type::Dynamic: {
     // Whether dynamic or inline the buffer is guaranteed to be large enough.
+    ASSERT(type_ == Type::Inline || dynamic_capacity_ >= MaxIntegerLength);
+    // It's safe to use buffer.dynamic_, since buffer.ref_ is union aliased.
+    // This better not change without verifying assumptions across this file.
+    static_assert(offsetof(Buffer, dynamic_) == offsetof(Buffer, ref_), "");
     string_length_ = StringUtil::itoa(buffer_.dynamic_, 32, value);
   }
   }

test/common/http/header_map_impl_corpus/clusterfuzz-testcase-minimized-header_map_impl_fuzz_test-5689833624698880

@@ -0,0 +1 @@
+actions { } actions { } actions { } actions { } actions { } actions { } actions { } actions { } actions { } actions { } actions { } actions { } actions { } actions { } actions { } actions { } actions { } actions { } actions {   set_reference {   } } actions { } actions {   get_and_mutate {     append: ""   } } actions { } actions {   get_and_mutate {     set_integer: 0   } } actions { } actions { } actions { } actions { } actions { } actions { } actions { } actions { } 

test/common/http/header_map_impl_test.cc

@@ -692,6 +692,16 @@ TEST(HeaderMapImplTest, TestAppendHeader) {
     HeaderMapImpl::appendToHeader(value3, "");
     EXPECT_EQ(value3, "empty");
   }
+  // Regression test for appending to an empty string with a short string, then
+  // setting integer.
+  {
+    const std::string empty;
+    HeaderString value4(empty);
+    HeaderMapImpl::appendToHeader(value4, " ");
+    value4.setInteger(0);
+    EXPECT_STREQ("0", value4.c_str());
+    EXPECT_EQ(1U, value4.size());
+  }
 }
 
 TEST(HeaderMapImplTest, PseudoHeaderOrder) {