Skip to content

Commit f7d3cb6

Browse files
htuchhtuch
authored
http: fix allocation bug introduced in #4211. (#4245)
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 <[email protected]>
1 parent cb892b4 commit f7d3cb6

File tree

4 files changed

+49
-7
lines changed

4 files changed

+49
-7
lines changed

‎include/envoy/http/header_map.h‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -154,13 +154,17 @@ class HeaderString {
154154
bool operator!=(const char* rhs) const { return 0 != strcmp(c_str(), rhs); }
155155

156156
private:
157-
union {
157+
union Buffer {
158+
// This should reference inline_buffer_ for Type::Inline.
158159
char* dynamic_;
159160
const char* ref_;
160161
} buffer_;
161162

163+
// Capacity in both Type::Inline and Type::Dynamic cases must be at least MinDynamicCapacity in
164+
// header_map_impl.cc.
162165
union {
163166
char inline_buffer_[128];
167+
// Since this is a union, this is only valid for type_ == Type::Dynamic.
164168
uint32_t dynamic_capacity_;
165169
};
166170

‎source/common/http/header_map_impl.cc‎

Lines changed: 33 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -14,9 +14,25 @@
1414
namespace Envoy {
1515
namespace Http {
1616

17+
namespace {
18+
constexpr size_t MinDynamicCapacity{32};
19+
// This includes the NULL (StringUtil::itoa technically only needs 21).
20+
constexpr size_t MaxIntegerLength{32};
21+
22+
void validateCapacity(size_t new_capacity) {
23+
// If the resizing will cause buffer overflow due to hitting uint32_t::max, an OOM is likely
24+
// imminent. Fast-fail rather than allow a buffer overflow attack (issue #1421)
25+
RELEASE_ASSERT(new_capacity <= std::numeric_limits<uint32_t>::max(), "");
26+
ASSERT(new_capacity >= MinDynamicCapacity);
27+
}
28+
29+
} // namespace
30+
1731
HeaderString::HeaderString() : type_(Type::Inline) {
1832
buffer_.dynamic_ = inline_buffer_;
1933
clear();
34+
static_assert(sizeof(inline_buffer_) >= MaxIntegerLength, "");
35+
static_assert(MinDynamicCapacity >= MaxIntegerLength, "");
2036
}
2137

2238
HeaderString::HeaderString(const LowerCaseString& ref_value) : type_(Type::Reference) {
@@ -70,7 +86,9 @@ void HeaderString::append(const char* data, uint32_t size) {
7086
// Rather than be too clever and optimize this uncommon case, we dynamically
7187
// allocate and copy.
7288
type_ = Type::Dynamic;
73-
dynamic_capacity_ = (string_length_ + size) * 2;
89+
dynamic_capacity_ =
90+
std::max(MinDynamicCapacity, static_cast<size_t>((string_length_ + size) * 2));
91+
validateCapacity(dynamic_capacity_);
7492
char* buf = static_cast<char*>(malloc(dynamic_capacity_));
7593
RELEASE_ASSERT(buf != nullptr, "");
7694
memcpy(buf, buffer_.ref_, string_length_);
@@ -90,19 +108,18 @@ void HeaderString::append(const char* data, uint32_t size) {
90108
case Type::Dynamic: {
91109
// We can get here either because we didn't fit in inline or we are already dynamic.
92110
if (type_ == Type::Inline) {
93-
const uint64_t new_capacity = (static_cast<uint64_t>(string_length_) + size) * 2;
94-
// If the resizing will cause buffer overflow due to hitting uint32_t::max, an OOM is likely
95-
// imminent. Fast-fail rather than allow a buffer overflow attack (issue #1421)
96-
RELEASE_ASSERT(new_capacity <= std::numeric_limits<uint32_t>::max(), "");
111+
const size_t new_capacity = (string_length_ + size) * 2;
112+
validateCapacity(new_capacity);
97113
buffer_.dynamic_ = static_cast<char*>(malloc(new_capacity));
98-
memcpy(buffer_.dynamic_, inline_buffer_, string_length_);
99114
RELEASE_ASSERT(buffer_.dynamic_ != nullptr, "");
115+
memcpy(buffer_.dynamic_, inline_buffer_, string_length_);
100116
dynamic_capacity_ = new_capacity;
101117
type_ = Type::Dynamic;
102118
} else {
103119
if (size + 1 + string_length_ > dynamic_capacity_) {
104120
// Need to reallocate.
105121
dynamic_capacity_ = (string_length_ + size) * 2;
122+
validateCapacity(dynamic_capacity_);
106123
buffer_.dynamic_ = static_cast<char*>(realloc(buffer_.dynamic_, dynamic_capacity_));
107124
RELEASE_ASSERT(buffer_.dynamic_ != nullptr, "");
108125
}
@@ -153,14 +170,18 @@ void HeaderString::setCopy(const char* data, uint32_t size) {
153170
// We can get here either because we didn't fit in inline or we are already dynamic.
154171
if (type_ == Type::Inline) {
155172
dynamic_capacity_ = size * 2;
173+
validateCapacity(dynamic_capacity_);
156174
buffer_.dynamic_ = static_cast<char*>(malloc(dynamic_capacity_));
175+
RELEASE_ASSERT(buffer_.dynamic_ != nullptr, "");
157176
type_ = Type::Dynamic;
158177
} else {
159178
if (size + 1 > dynamic_capacity_) {
160179
// Need to reallocate. Use free/malloc to avoid the copy since we are about to overwrite.
161180
dynamic_capacity_ = size * 2;
181+
validateCapacity(dynamic_capacity_);
162182
free(buffer_.dynamic_);
163183
buffer_.dynamic_ = static_cast<char*>(malloc(dynamic_capacity_));
184+
RELEASE_ASSERT(buffer_.dynamic_ != nullptr, "");
164185
}
165186
}
166187
}
@@ -182,8 +203,14 @@ void HeaderString::setInteger(uint64_t value) {
182203
}
183204

184205
case Type::Inline:
206+
// buffer_.dynamic_ should always point at inline_buffer_ for Type::Inline.
207+
ASSERT(buffer_.dynamic_ == inline_buffer_);
185208
case Type::Dynamic: {
186209
// Whether dynamic or inline the buffer is guaranteed to be large enough.
210+
ASSERT(type_ == Type::Inline || dynamic_capacity_ >= MaxIntegerLength);
211+
// It's safe to use buffer.dynamic_, since buffer.ref_ is union aliased.
212+
// This better not change without verifying assumptions across this file.
213+
static_assert(offsetof(Buffer, dynamic_) == offsetof(Buffer, ref_), "");
187214
string_length_ = StringUtil::itoa(buffer_.dynamic_, 32, value);
188215
}
189216
}

‎test/common/http/header_map_impl_corpus/clusterfuzz-testcase-minimized-header_map_impl_fuzz_test-5689833624698880‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
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 { }

‎test/common/http/header_map_impl_test.cc‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -692,6 +692,16 @@ TEST(HeaderMapImplTest, TestAppendHeader) {
692692
HeaderMapImpl::appendToHeader(value3, "");
693693
EXPECT_EQ(value3, "empty");
694694
}
695+
// Regression test for appending to an empty string with a short string, then
696+
// setting integer.
697+
{
698+
const std::string empty;
699+
HeaderString value4(empty);
700+
HeaderMapImpl::appendToHeader(value4, " ");
701+
value4.setInteger(0);
702+
EXPECT_STREQ("0", value4.c_str());
703+
EXPECT_EQ(1U, value4.size());
704+
}
695705
}
696706

697707
TEST(HeaderMapImplTest, PseudoHeaderOrder) {

0 commit comments

Comments
 (0)