diff --git a/include/envoy/http/BUILD b/include/envoy/http/BUILD index bdd112be4e1ed..4f0355015ce3c 100644 --- a/include/envoy/http/BUILD +++ b/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( diff --git a/include/envoy/http/header_map.h b/include/envoy/http/header_map.h index 780fc61b0ae49..f67a55c607963 100644 --- a/include/envoy/http/header_map.h +++ b/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_; diff --git a/source/common/http/header_map_impl.cc b/source/common/http/header_map_impl.cc index e5c642349176a..68bfc4365d531 100644 --- a/source/common/http/header_map_impl.cc +++ b/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. diff --git a/test/common/http/codec_impl_corpus/clusterfuzz-testcase-minimized-codec_impl_fuzz_test-5720162173452288 b/test/common/http/codec_impl_corpus/clusterfuzz-testcase-minimized-codec_impl_fuzz_test-5720162173452288 new file mode 100644 index 0000000000000..cdd0aad714ba7 --- /dev/null +++ b/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 " } } } } diff --git a/test/common/http/codec_impl_fuzz_test.cc b/test/common/http/codec_impl_fuzz_test.cc index ca23aa5060c50..7cfbcaf8d5c55 100644 --- a/test/common/http/codec_impl_fuzz_test.cc +++ b/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. diff --git a/test/fuzz/utility.h b/test/fuzz/utility.h index 8ea13ded8c297..ff90f7e1d394a 100644 --- a/test/fuzz/utility.h +++ b/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& 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; }