Revert "Reclaim one byte from HeaderString. (envoyproxy#6826)"

This is failing header_map_impl_fuzz due to memcpy heap buffer overflow and should be considered a
serious security issue. Given this was only introduced in the last day and does not affect any
release, a revert and bump is the consensus of Envoy security team.

This reverts commit 977907d.

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

Risk level: Low
Testing: Corpus entry added, bazel test //test/common/http:header_map_impl_fuzz_test --test_output=all -c dbg --config=clang-asan

Signed-off-by: Harvey Tuch <htuch@google.com>

include/envoy/http/header_map.h

@@ -119,8 +119,6 @@ class HeaderString {
   char* buffer() { return buffer_.dynamic_; }
 
   /**
-   * Get an absl::string_view. It will NOT be NUL terminated!
-   *
    * @return an absl::string_view.
    */
   absl::string_view getStringView() const {

source/common/http/header_map_impl.cc

@@ -73,7 +73,7 @@ HeaderString::HeaderString(HeaderString&& move_value) {
   }
   case Type::Inline: {
     buffer_.dynamic_ = inline_buffer_;
-    memcpy(inline_buffer_, move_value.inline_buffer_, string_length_);
+    memcpy(inline_buffer_, move_value.inline_buffer_, string_length_ + 1);
     move_value.string_length_ = 0;
     move_value.inline_buffer_[0] = 0;
     break;
@@ -113,7 +113,7 @@ void HeaderString::append(const char* data, uint32_t size) {
   }
 
   case Type::Inline: {
-    const uint64_t new_capacity = static_cast<uint64_t>(size) + string_length_;
+    const uint64_t new_capacity = static_cast<uint64_t>(size) + 1 + string_length_;
     if (new_capacity <= sizeof(inline_buffer_)) {
       // Already inline and the new value fits in inline storage.
       break;
@@ -133,7 +133,7 @@ void HeaderString::append(const char* data, uint32_t size) {
       dynamic_capacity_ = new_capacity;
       type_ = Type::Dynamic;
     } else {
-      if (size + string_length_ > dynamic_capacity_) {
+      if (size + 1 + string_length_ > dynamic_capacity_) {
         const uint64_t new_capacity = newCapacity(string_length_, size);
         validateCapacity(new_capacity);
 
@@ -148,6 +148,7 @@ void HeaderString::append(const char* data, uint32_t size) {
 
   memcpy(buffer_.dynamic_ + string_length_, data, size);
   string_length_ += size;
+  buffer_.dynamic_[string_length_] = 0;
   ASSERT(valid());
 }
 
@@ -177,7 +178,7 @@ void HeaderString::setCopy(const char* data, uint32_t size) {
   }
 
   case Type::Inline: {
-    if (size <= sizeof(inline_buffer_)) {
+    if (size + 1 <= sizeof(inline_buffer_)) {
       // Already inline and the new value fits in inline storage.
       break;
     }
@@ -194,7 +195,7 @@ void HeaderString::setCopy(const char* data, uint32_t size) {
       RELEASE_ASSERT(buffer_.dynamic_ != nullptr, "");
       type_ = Type::Dynamic;
     } else {
-      if (size > dynamic_capacity_) {
+      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_);

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

@@ -0,0 +1 @@
+actions {   add_reference {   } } actions {   get_and_mutate {     append: ""   } } actions {   get_and_mutate { set_copy: "                                "   } }

test/common/http/header_map_impl_test.cc

@@ -180,7 +180,7 @@ TEST(HeaderStringTest, All) {
   // Append, small buffer to dynamic
   {
     HeaderString string;
-    std::string test(128, 'a');
+    std::string test(127, 'a');
     string.append(test.c_str(), test.size());
     EXPECT_EQ(HeaderString::Type::Inline, string.type());
     string.append("a", 1);
@@ -208,28 +208,28 @@ TEST(HeaderStringTest, All) {
   // Append, realloc dynamic.
   {
     HeaderString string;
-    std::string large(129, 'a');
+    std::string large(128, 'a');
     string.append(large.c_str(), large.size());
     EXPECT_EQ(HeaderString::Type::Dynamic, string.type());
     std::string large2 = large + large;
     string.append(large2.c_str(), large2.size());
     large += large2;
     EXPECT_EQ(large, string.getStringView());
-    EXPECT_EQ(387U, string.size());
+    EXPECT_EQ(384U, string.size());
   }
 
   // Append, realloc close to limit with small buffer.
   {
     HeaderString string;
-    std::string large(129, 'a');
+    std::string large(128, 'a');
     string.append(large.c_str(), large.size());
     EXPECT_EQ(HeaderString::Type::Dynamic, string.type());
     std::string large2(120, 'b');
     string.append(large2.c_str(), large2.size());
     std::string large3(32, 'c');
     string.append(large3.c_str(), large3.size());
     EXPECT_EQ((large + large2 + large3), string.getStringView());
-    EXPECT_EQ(281U, string.size());
+    EXPECT_EQ(280U, string.size());
   }
 
   // Set integer, inline
@@ -243,7 +243,7 @@ TEST(HeaderStringTest, All) {
   // Set integer, dynamic
   {
     HeaderString string;
-    std::string large(129, 'a');
+    std::string large(128, 'a');
     string.append(large.c_str(), large.size());
     string.setInteger(123456789);
     EXPECT_EQ("123456789", string.getStringView());
@@ -260,7 +260,7 @@ TEST(HeaderStringTest, All) {
     EXPECT_EQ(11U, string.size());
     EXPECT_EQ(HeaderString::Type::Reference, string.type());
 
-    const std::string large(129, 'a');
+    const std::string large(128, 'a');
     string.setCopy(large.c_str(), large.size());
     EXPECT_NE(string.getStringView().data(), large.c_str());
     EXPECT_EQ(HeaderString::Type::Dynamic, string.type());