diff --git a/include/envoy/http/header_map.h b/include/envoy/http/header_map.h index 3e158dc608aa..fa02952a2b28 100644 --- a/include/envoy/http/header_map.h +++ b/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_; }; diff --git a/source/common/http/header_map_impl.cc b/source/common/http/header_map_impl.cc index 7f17f1e24b60..86c656901bc1 100644 --- a/source/common/http/header_map_impl.cc +++ b/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::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((string_length_ + size) * 2)); + validateCapacity(dynamic_capacity_); char* buf = static_cast(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(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::max(), ""); + const size_t new_capacity = (string_length_ + size) * 2; + validateCapacity(new_capacity); buffer_.dynamic_ = static_cast(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(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(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(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); } } diff --git a/test/common/http/header_map_impl_corpus/clusterfuzz-testcase-minimized-header_map_impl_fuzz_test-5689833624698880 b/test/common/http/header_map_impl_corpus/clusterfuzz-testcase-minimized-header_map_impl_fuzz_test-5689833624698880 new file mode 100644 index 000000000000..75d99a6a2857 --- /dev/null +++ b/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 { } diff --git a/test/common/http/header_map_impl_test.cc b/test/common/http/header_map_impl_test.cc index 326b954002c0..dc55e0d2e7ae 100644 --- a/test/common/http/header_map_impl_test.cc +++ b/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) {