From 2c9cc74ccaf491d5162376c395ed13b51705e14e Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Fri, 24 Aug 2018 10:31:52 -0400 Subject: [PATCH 1/6] http: fix allocation bug introduced in #4211. 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 #4211. Testing: Unit test and corpus entry added. Signed-off-by: Harvey Tuch --- include/envoy/http/header_map.h | 3 +++ source/common/http/header_map_impl.cc | 17 ++++++++++++++++- ...d-header_map_impl_fuzz_test-5689833624698880 | 1 + test/common/http/header_map_impl_test.cc | 9 +++++++++ 4 files changed, 29 insertions(+), 1 deletion(-) create mode 100644 test/common/http/header_map_impl_corpus/clusterfuzz-testcase-minimized-header_map_impl_fuzz_test-5689833624698880 diff --git a/include/envoy/http/header_map.h b/include/envoy/http/header_map.h index 3e158dc608aa5..0558d7635b269 100644 --- a/include/envoy/http/header_map.h +++ b/include/envoy/http/header_map.h @@ -155,10 +155,13 @@ class HeaderString { private: union { + // 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]; uint32_t dynamic_capacity_; diff --git a/source/common/http/header_map_impl.cc b/source/common/http/header_map_impl.cc index 7f17f1e24b600..17397c28cea80 100644 --- a/source/common/http/header_map_impl.cc +++ b/source/common/http/header_map_impl.cc @@ -14,9 +14,17 @@ 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}; +} // 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 +78,8 @@ 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)); char* buf = static_cast(malloc(dynamic_capacity_)); RELEASE_ASSERT(buf != nullptr, ""); memcpy(buf, buffer_.ref_, string_length_); @@ -94,6 +103,7 @@ void HeaderString::append(const char* data, uint32_t size) { // 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); buffer_.dynamic_ = static_cast(malloc(new_capacity)); memcpy(buffer_.dynamic_, inline_buffer_, string_length_); RELEASE_ASSERT(buffer_.dynamic_ != nullptr, ""); @@ -182,8 +192,13 @@ 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(dynamic_capacity_ >= MaxIntegerLength); + // It's safe to use buffer.dynamic_, since buffer.ref_ is union aliased. + ASSERT(&buffer_.dynamic_ == &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 0000000000000..75d99a6a28576 --- /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 326b954002c09..c71f2c57cb0ce 100644 --- a/test/common/http/header_map_impl_test.cc +++ b/test/common/http/header_map_impl_test.cc @@ -692,6 +692,15 @@ TEST(HeaderMapImplTest, TestAppendHeader) { HeaderMapImpl::appendToHeader(value3, ""); EXPECT_EQ(value3, "empty"); } + // Regression test for appending to an empty string with empty, 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) { From 28d232d91d92f3cafe4a2864e28dbf4932a81979 Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Fri, 24 Aug 2018 13:22:58 -0400 Subject: [PATCH 2/6] Fix ASSERT fails in tests. Signed-off-by: Harvey Tuch --- include/envoy/http/header_map.h | 1 + source/common/http/header_map_impl.cc | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/include/envoy/http/header_map.h b/include/envoy/http/header_map.h index 0558d7635b269..ca27b2e9f0d1f 100644 --- a/include/envoy/http/header_map.h +++ b/include/envoy/http/header_map.h @@ -164,6 +164,7 @@ class HeaderString { // 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 17397c28cea80..b0f136879fd11 100644 --- a/source/common/http/header_map_impl.cc +++ b/source/common/http/header_map_impl.cc @@ -196,7 +196,7 @@ void HeaderString::setInteger(uint64_t value) { ASSERT(buffer_.dynamic_ == inline_buffer_); case Type::Dynamic: { // Whether dynamic or inline the buffer is guaranteed to be large enough. - ASSERT(dynamic_capacity_ >= MaxIntegerLength); + ASSERT(type_ == Type::Inline || dynamic_capacity_ >= MaxIntegerLength); // It's safe to use buffer.dynamic_, since buffer.ref_ is union aliased. ASSERT(&buffer_.dynamic_ == &buffer_.ref_); string_length_ = StringUtil::itoa(buffer_.dynamic_, 32, value); From 7c7c1228199d7d3afbfd5c98d08a9c9aa0d168b6 Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Fri, 24 Aug 2018 13:51:34 -0400 Subject: [PATCH 3/6] Review feedback and more belt-and-braces. Signed-off-by: Harvey Tuch --- source/common/http/header_map_impl.cc | 21 ++++++++++++++++----- test/common/http/header_map_impl_test.cc | 3 ++- 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/source/common/http/header_map_impl.cc b/source/common/http/header_map_impl.cc index b0f136879fd11..adec96d1d369c 100644 --- a/source/common/http/header_map_impl.cc +++ b/source/common/http/header_map_impl.cc @@ -18,6 +18,14 @@ 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) { @@ -80,6 +88,7 @@ void HeaderString::append(const char* data, uint32_t size) { type_ = Type::Dynamic; 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_); @@ -100,19 +109,17 @@ void HeaderString::append(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) { 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(), ""); - ASSERT(new_capacity >= MinDynamicCapacity); + 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, ""); } @@ -163,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, ""); } } } diff --git a/test/common/http/header_map_impl_test.cc b/test/common/http/header_map_impl_test.cc index c71f2c57cb0ce..dc55e0d2e7ae0 100644 --- a/test/common/http/header_map_impl_test.cc +++ b/test/common/http/header_map_impl_test.cc @@ -692,7 +692,8 @@ TEST(HeaderMapImplTest, TestAppendHeader) { HeaderMapImpl::appendToHeader(value3, ""); EXPECT_EQ(value3, "empty"); } - // Regression test for appending to an empty string with empty, then setting integer. + // Regression test for appending to an empty string with a short string, then + // setting integer. { const std::string empty; HeaderString value4(empty); From 36af343eda31900519bf637bfa57870a6df04a1a Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Fri, 24 Aug 2018 14:28:18 -0400 Subject: [PATCH 4/6] static_assert for union alias. Signed-off-by: Harvey Tuch --- include/envoy/http/header_map.h | 2 +- source/common/http/header_map_impl.cc | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/include/envoy/http/header_map.h b/include/envoy/http/header_map.h index ca27b2e9f0d1f..fa02952a2b280 100644 --- a/include/envoy/http/header_map.h +++ b/include/envoy/http/header_map.h @@ -154,7 +154,7 @@ 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_; diff --git a/source/common/http/header_map_impl.cc b/source/common/http/header_map_impl.cc index adec96d1d369c..3fb226e7f5560 100644 --- a/source/common/http/header_map_impl.cc +++ b/source/common/http/header_map_impl.cc @@ -209,7 +209,8 @@ void HeaderString::setInteger(uint64_t value) { // 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. - ASSERT(&buffer_.dynamic_ == &buffer_.ref_); + // 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); } } From 8e12de5e4374735a6bda96126a497b168a55a4ff Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Fri, 24 Aug 2018 14:29:11 -0400 Subject: [PATCH 5/6] uint64_t -> size_t. Signed-off-by: Harvey Tuch --- source/common/http/header_map_impl.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/http/header_map_impl.cc b/source/common/http/header_map_impl.cc index 3fb226e7f5560..86c3f8686e70d 100644 --- a/source/common/http/header_map_impl.cc +++ b/source/common/http/header_map_impl.cc @@ -108,7 +108,7 @@ 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; + const size_t new_capacity = (static_cast(string_length_) + size) * 2; validateCapacity(new_capacity); buffer_.dynamic_ = static_cast(malloc(new_capacity)); RELEASE_ASSERT(buffer_.dynamic_ != nullptr, ""); From cd531fa785ad47e308b8887cd24c6ac9ff335821 Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Fri, 24 Aug 2018 15:05:51 -0400 Subject: [PATCH 6/6] Fewer type gymnastics. Signed-off-by: Harvey Tuch --- source/common/http/header_map_impl.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/http/header_map_impl.cc b/source/common/http/header_map_impl.cc index 86c3f8686e70d..86c656901bc1c 100644 --- a/source/common/http/header_map_impl.cc +++ b/source/common/http/header_map_impl.cc @@ -108,7 +108,7 @@ 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 size_t new_capacity = (static_cast(string_length_) + size) * 2; + const size_t new_capacity = (string_length_ + size) * 2; validateCapacity(new_capacity); buffer_.dynamic_ = static_cast(malloc(new_capacity)); RELEASE_ASSERT(buffer_.dynamic_ != nullptr, "");