header_map/fuzz: improve robustness to embedded NULL in headers.

include/envoy/http/BUILD

@@ -73,7 +73,10 @@ envoy_cc_library(
 envoy_cc_library(
     name = "header_map_interface",
     hdrs = ["header_map.h"],
-    deps = ["//source/common/common:hash_lib"],
+    deps = [
+        "//source/common/common:assert_lib",
+        "//source/common/common:hash_lib",
+    ],
 )
 
 envoy_cc_library(

include/envoy/http/header_map.h

@@ -12,6 +12,7 @@
 
 #include "envoy/common/pure.h"
 
+#include "common/common/assert.h"
 #include "common/common/hash.h"
 
 #include "absl/strings/string_view.h"
@@ -25,9 +26,12 @@ namespace Http {
  */
 class LowerCaseString {
 public:
-  LowerCaseString(LowerCaseString&& rhs) : string_(std::move(rhs.string_)) {}
-  LowerCaseString(const LowerCaseString& rhs) : string_(rhs.string_) {}
-  explicit LowerCaseString(const std::string& new_string) : string_(new_string) { lower(); }
+  LowerCaseString(LowerCaseString&& rhs) : string_(std::move(rhs.string_)) { ASSERT(valid()); }
+  LowerCaseString(const LowerCaseString& rhs) : string_(rhs.string_) { ASSERT(valid()); }
+  explicit LowerCaseString(const std::string& new_string) : string_(new_string) {
+    ASSERT(valid());
+    lower();
+  }
 
   const std::string& get() const { return string_; }
   bool operator==(const LowerCaseString& rhs) const { return string_ == rhs.string_; }
@@ -36,6 +40,9 @@ class LowerCaseString {
 
 private:
   void lower() { std::transform(string_.begin(), string_.end(), string_.begin(), tolower); }
+  // Used by ASSERTs to validate internal consistency. E.g. valid HTTP header keys/values should
+  // never contain embedded NULLs.
+  bool valid() const { return string_.find('\0') == std::string::npos; }
 
   std::string string_;
 };
@@ -176,6 +183,9 @@ class HeaderString {
   };
 
   void freeDynamic();
+  // Used by ASSERTs to validate internal consistency. E.g. valid HTTP header keys/values should
+  // never contain embedded NULLs.
+  bool valid() const;
 
   uint32_t string_length_;
   Type type_;

source/common/http/header_map_impl.cc

@@ -39,16 +39,19 @@ HeaderString::HeaderString() : type_(Type::Inline) {
   clear();
   static_assert(sizeof(inline_buffer_) >= MaxIntegerLength, "");
   static_assert(MinDynamicCapacity >= MaxIntegerLength, "");
+  ASSERT(valid());
 }
 
 HeaderString::HeaderString(const LowerCaseString& ref_value) : type_(Type::Reference) {
   buffer_.ref_ = ref_value.get().c_str();
   string_length_ = ref_value.get().size();
+  ASSERT(valid());
 }
 
 HeaderString::HeaderString(const std::string& ref_value) : type_(Type::Reference) {
   buffer_.ref_ = ref_value.c_str();
   string_length_ = ref_value.size();
+  ASSERT(valid());
 }
 
 HeaderString::HeaderString(HeaderString&& move_value) {
@@ -76,6 +79,7 @@ HeaderString::HeaderString(HeaderString&& move_value) {
     break;
   }
   }
+  ASSERT(valid());
 }
 
 HeaderString::~HeaderString() { freeDynamic(); }
@@ -86,6 +90,10 @@ void HeaderString::freeDynamic() {
   }
 }
 
+bool HeaderString::valid() const {
+  return std::string(c_str(), string_length_).find('\0') == std::string::npos;
+}
+
 void HeaderString::append(const char* data, uint32_t size) {
   switch (type_) {
   case Type::Reference: {
@@ -143,6 +151,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());
 }
 
 void HeaderString::clear() {
@@ -203,6 +212,7 @@ void HeaderString::setCopy(const char* data, uint32_t size) {
   memcpy(buffer_.dynamic_, data, size);
   buffer_.dynamic_[size] = 0;
   string_length_ = size;
+  ASSERT(valid());
 }
 
 void HeaderString::setInteger(uint64_t value) {
@@ -235,6 +245,7 @@ void HeaderString::setReference(const std::string& ref_value) {
   type_ = Type::Reference;
   buffer_.ref_ = ref_value.c_str();
   string_length_ = ref_value.size();
+  ASSERT(valid());
 }
 
 // Specialization needed for HeaderMapImpl::HeaderList::insert() when key is LowerCaseString.

test/common/http/codec_impl_corpus/clusterfuzz-testcase-minimized-codec_impl_fuzz_test-5720162173452288

@@ -0,0 +1 @@
+actions {   new_stream {     request_headers {       headers {         key: ":path"       }       headers {         key: ":method"       }       headers {         key: "transfer-encodinG\0  "       }     }   } }

test/common/http/codec_impl_fuzz_test.cc

@@ -37,23 +37,7 @@ namespace Http {
 constexpr bool DebugMode = false;
 
 Http::TestHeaderMapImpl fromSanitizedHeaders(const test::fuzz::Headers& headers) {
-  // When we are injecting headers, we don't allow the key to ever be empty,
-  // since calling code is not supposed to do this. Also disallowed
-  // transfer-encoding.
-  test::fuzz::Headers sanitized_headers;
-  for (const auto& header : headers.headers()) {
-    const std::string key = StringUtil::toLower(header.key());
-
-    if (key == "transfer-encoding") {
-      continue;
-    }
-
-    auto* sane_header = sanitized_headers.add_headers();
-    sane_header->set_key(key.empty() ? "non-empty" : key);
-    sane_header->set_value(header.value());
-  }
-
-  return Fuzz::fromHeaders(sanitized_headers);
+  return Fuzz::fromHeaders(headers, {"transfer-encoding"});
 }
 
 // Convert from test proto Http1ServerSettings to Http1Settings.

test/fuzz/utility.h

@@ -11,12 +11,27 @@ namespace Envoy {
 namespace Fuzz {
 
 // Convert from test proto Headers to TestHeaderMapImpl.
-inline Http::TestHeaderMapImpl fromHeaders(const test::fuzz::Headers& headers) {
+inline Http::TestHeaderMapImpl
+fromHeaders(const test::fuzz::Headers& headers,
+            const std::unordered_set<std::string>& ignore_headers = {}) {
   Http::TestHeaderMapImpl header_map;
   for (const auto& header : headers.headers()) {
+    // HeaderMapImpl and places such as the route lookup should never see strings with embedded NULL
+    // values, the HTTP codecs should reject them. So, don't inject any such strings into the fuzz
+    // tests.
+    const auto clean = [](const std::string& s) {
+      const auto n = s.find('\0');
+      if (n == std::string::npos) {
+        return s;
+      }
+      return s.substr(0, n);
+    };
     // When we are injecting headers, we don't allow the key to ever be empty,
     // since calling code is not supposed to do this.
-    header_map.addCopy(header.key().empty() ? "not-empty" : header.key(), header.value());
+    const std::string key = header.key().empty() ? "not-empty" : clean(header.key());
+    if (ignore_headers.find(StringUtil::toLower(key)) != ignore_headers.end()) {
+      header_map.addCopy(key, clean(header.value()));
+    }
   }
   return header_map;
 }