Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

headers: fixing fast fail of size-validation #4269

Merged
merged 7 commits into from
Aug 30, 2018
Merged

headers: fixing fast fail of size-validation #4269

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
176472e
one more OOM vector
alyssawilk Aug 27, 2018
f668193
reviewer comments
alyssawilk Aug 27, 2018
f39ec4c
Hopefully unbreaking the mac build. will write test tomorrow
alyssawilk Aug 27, 2018
8745ebd
one more test, fixing EXPECT_DEATH macro for asan/tsan
alyssawilk Aug 28, 2018
580e760
disabling proof of concept test
alyssawilk Aug 28, 2018
d3ffa9d
trying to fix coverage build
alyssawilk Aug 28, 2018
eefc16f
Merge branch 'master' into RELEASE_ASSERT
alyssawilk Aug 29, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 21 additions & 9 deletions source/common/http/header_map_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,15 @@ 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) {
uint64_t newCapacity(uint32_t existing_capacity, uint32_t size_to_append) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: you could make existing_capacity a uint64_t to avoid the cast, but this doesn't really matter.

return (static_cast<uint64_t>(existing_capacity) + size_to_append) * 2;
}

void validateCapacity(uint64_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<uint32_t>::max(), "");
RELEASE_ASSERT(new_capacity <= std::numeric_limits<uint32_t>::max(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

std::numeric_limits::max() returns type T; should that part be cast to size_t before comparison?

"Trying to allocate overly large headers.");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should there be a TODO here for a more graceful failure handling? Or is dying the only appropriate action at this point?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dying before overflow is appropriate but I agree I'd like something before that.

I've had this discussion with Matt before. For some context on how headers get added it's
[incoming headers from client/upstream] which are subject to built in header limits
filters you've chosen to add (i.e. Google's internal "let's add googly trace headers") which should be scoped
Lua (which if you're not sanity checking, well joke's on you)
User defined headers (which I just filed an issue tracker for being too easy an OOM vector)

I think if each header input has at least some scoped / sane limits, having a relesae assert for bugs is Ok-but-not-great.

What I'd prefer is that we have a soft-fail option a lot earlier - a configured max header limit where instead of appending headers we do the logical equivalent of an internal GFE_BUG macro to note the error for both logging and monitoring, but not kill Envoy except on configuration of a build-in bugs-should-be-fatal flag for the benefit of folks on the fail-fast fail-hard side of the fence.

This is on my list of things to do to secure Envoy before widespread use for my project, but not high enough up I've filed an issue tracker for it.

ASSERT(new_capacity >= MinDynamicCapacity);
}

Expand Down Expand Up @@ -86,9 +91,13 @@ 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_ =
std::max(MinDynamicCapacity, static_cast<size_t>((string_length_ + size) * 2));
validateCapacity(dynamic_capacity_);
const uint64_t new_capacity = newCapacity(string_length_, size);
if (new_capacity > MinDynamicCapacity) {
validateCapacity(new_capacity);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For some reason, my comment was lost here. I was wondering if we could move validateCapacity up and make it unconditional?

dynamic_capacity_ = new_capacity;
} else {
dynamic_capacity_ = MinDynamicCapacity;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, much cleaner now, thanks.

}
char* buf = static_cast<char*>(malloc(dynamic_capacity_));
RELEASE_ASSERT(buf != nullptr, "");
memcpy(buf, buffer_.ref_, string_length_);
Expand All @@ -97,7 +106,8 @@ void HeaderString::append(const char* data, uint32_t size) {
}

case Type::Inline: {
if (size + 1 + string_length_ <= sizeof(inline_buffer_)) {
const uint64_t new_capacity = static_cast<uint64_t>(size) + 1 + string_length_;
if (new_capacity <= sizeof(inline_buffer_)) {
// Already inline and the new value fits in inline storage.
break;
}
Expand All @@ -108,7 +118,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 = (string_length_ + size) * 2;
const uint64_t new_capacity = newCapacity(string_length_, size);
validateCapacity(new_capacity);
buffer_.dynamic_ = static_cast<char*>(malloc(new_capacity));
RELEASE_ASSERT(buffer_.dynamic_ != nullptr, "");
Expand All @@ -117,9 +127,11 @@ void HeaderString::append(const char* data, uint32_t size) {
type_ = Type::Dynamic;
} else {
if (size + 1 + string_length_ > dynamic_capacity_) {
const uint64_t new_capacity = newCapacity(string_length_, size);
validateCapacity(new_capacity);

// Need to reallocate.
dynamic_capacity_ = (string_length_ + size) * 2;
validateCapacity(dynamic_capacity_);
dynamic_capacity_ = new_capacity;
buffer_.dynamic_ = static_cast<char*>(realloc(buffer_.dynamic_, dynamic_capacity_));
RELEASE_ASSERT(buffer_.dynamic_ != nullptr, "");
}
Expand Down
13 changes: 13 additions & 0 deletions test/common/http/header_map_impl_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -704,6 +704,19 @@ TEST(HeaderMapImplTest, TestAppendHeader) {
}
}

TEST(HeaderMapImplTest, TestHeaderLengthChecks) {
HeaderString value;
value.setCopy("some;", 5);
EXPECT_DEATH_LOG_TO_STDERR(value.append(nullptr, std::numeric_limits<uint32_t>::max()),
"Trying to allocate overly large headers.");

std::string source("hello");
HeaderString reference;
reference.setReference(source);
EXPECT_DEATH_LOG_TO_STDERR(reference.append(nullptr, std::numeric_limits<uint32_t>::max()),
"Trying to allocate overly large headers.");
}

TEST(HeaderMapImplTest, PseudoHeaderOrder) {
typedef testing::MockFunction<void(const std::string&, const std::string&)> MockCb;
MockCb cb;
Expand Down
145 changes: 85 additions & 60 deletions test/common/router/header_formatter_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,31 @@ TEST_F(RequestInfoHeaderFormatterTest, TestFormatWithUpstreamMetadataVariable) {
testFormatting(request_info, "UPSTREAM_METADATA([\"namespace\", \"nested\", \"list_key\"])", "");
}

// Breaks tsan/asan builds by trying to allocate a lot of memory.
// Works on debug builds and needs to be fixed. See
// https://github.com/envoyproxy/envoy/issues/4268
TEST_F(RequestInfoHeaderFormatterTest, DISABLED_UserDefinedHeadersConsideredHarmful) {
// This must be an inline header to get the append-in-place semantics.
const char* header_name = "connection";
Protobuf::RepeatedPtrField<envoy::api::v2::core::HeaderValueOption> to_add;
const uint32_t num_header_chunks = 10;
const uint64_t length = std::numeric_limits<uint32_t>::max() / num_header_chunks;
std::string really_long_string(length + 1, 'a');
for (uint32_t i = 0; i < num_header_chunks; ++i) {
envoy::api::v2::core::HeaderValueOption* header = to_add.Add();
header->mutable_header()->set_key(header_name);
header->mutable_header()->set_value(really_long_string);
header->mutable_append()->set_value(true);
}

HeaderParserPtr req_header_parser = HeaderParser::configure(to_add);

Http::TestHeaderMapImpl header_map{{":method", "POST"}};
NiceMock<Envoy::RequestInfo::MockRequestInfo> request_info;
EXPECT_DEATH_LOG_TO_STDERR(req_header_parser->evaluateHeaders(header_map, request_info),
"Trying to allocate overly large headers.");
}

TEST_F(RequestInfoHeaderFormatterTest, TestFormatWithUpstreamMetadataVariableMissingHost) {
NiceMock<Envoy::RequestInfo::MockRequestInfo> request_info;
std::shared_ptr<NiceMock<Envoy::Upstream::MockHostDescription>> host;
Expand Down Expand Up @@ -360,14 +385,14 @@ TEST(HeaderParserTest, TestParseInternal) {

HeaderParserPtr req_header_parser = HeaderParser::configure(to_add);

Http::TestHeaderMapImpl headerMap{{":method", "POST"}};
req_header_parser->evaluateHeaders(headerMap, request_info);
Http::TestHeaderMapImpl header_map{{":method", "POST"}};
req_header_parser->evaluateHeaders(header_map, request_info);

std::string descriptor = fmt::format("for test case input: {}", test_case.input_);

EXPECT_TRUE(headerMap.has("x-header")) << descriptor;
EXPECT_TRUE(header_map.has("x-header")) << descriptor;
EXPECT_TRUE(test_case.expected_output_) << descriptor;
EXPECT_EQ(test_case.expected_output_.value(), headerMap.get_("x-header")) << descriptor;
EXPECT_EQ(test_case.expected_output_.value(), header_map.get_("x-header")) << descriptor;
}
}

Expand All @@ -387,10 +412,10 @@ TEST(HeaderParserTest, EvaluateHeaders) {
)EOF";
HeaderParserPtr req_header_parser =
HeaderParser::configure(parseRouteFromJson(json).route().request_headers_to_add());
Http::TestHeaderMapImpl headerMap{{":method", "POST"}};
Http::TestHeaderMapImpl header_map{{":method", "POST"}};
NiceMock<Envoy::RequestInfo::MockRequestInfo> request_info;
req_header_parser->evaluateHeaders(headerMap, request_info);
EXPECT_TRUE(headerMap.has("x-client-ip"));
req_header_parser->evaluateHeaders(header_map, request_info);
EXPECT_TRUE(header_map.has("x-client-ip"));
}

TEST(HeaderParserTest, EvaluateEmptyHeaders) {
Expand All @@ -409,15 +434,15 @@ TEST(HeaderParserTest, EvaluateEmptyHeaders) {
)EOF";
HeaderParserPtr req_header_parser =
HeaderParser::configure(parseRouteFromJson(json).route().request_headers_to_add());
Http::TestHeaderMapImpl headerMap{{":method", "POST"}};
Http::TestHeaderMapImpl header_map{{":method", "POST"}};
std::shared_ptr<NiceMock<Envoy::Upstream::MockHostDescription>> host(
new NiceMock<Envoy::Upstream::MockHostDescription>());
NiceMock<Envoy::RequestInfo::MockRequestInfo> request_info;
auto metadata = std::make_shared<envoy::api::v2::core::Metadata>();
ON_CALL(request_info, upstreamHost()).WillByDefault(Return(host));
ON_CALL(*host, metadata()).WillByDefault(Return(metadata));
req_header_parser->evaluateHeaders(headerMap, request_info);
EXPECT_FALSE(headerMap.has("x-key"));
req_header_parser->evaluateHeaders(header_map, request_info);
EXPECT_FALSE(header_map.has("x-key"));
}

TEST(HeaderParserTest, EvaluateStaticHeaders) {
Expand All @@ -436,11 +461,11 @@ TEST(HeaderParserTest, EvaluateStaticHeaders) {
)EOF";
HeaderParserPtr req_header_parser =
HeaderParser::configure(parseRouteFromJson(json).route().request_headers_to_add());
Http::TestHeaderMapImpl headerMap{{":method", "POST"}};
Http::TestHeaderMapImpl header_map{{":method", "POST"}};
NiceMock<Envoy::RequestInfo::MockRequestInfo> request_info;
req_header_parser->evaluateHeaders(headerMap, request_info);
EXPECT_TRUE(headerMap.has("static-header"));
EXPECT_EQ("static-value", headerMap.get_("static-header"));
req_header_parser->evaluateHeaders(header_map, request_info);
EXPECT_TRUE(header_map.has("static-header"));
EXPECT_EQ("static-value", header_map.get_("static-header"));
}

TEST(HeaderParserTest, EvaluateCompoundHeaders) {
Expand Down Expand Up @@ -477,7 +502,7 @@ match: { prefix: "/new_endpoint" }

HeaderParserPtr req_header_parser =
HeaderParser::configure(parseRouteFromV2Yaml(yaml).route().request_headers_to_add());
Http::TestHeaderMapImpl headerMap{{":method", "POST"}};
Http::TestHeaderMapImpl header_map{{":method", "POST"}};
NiceMock<Envoy::RequestInfo::MockRequestInfo> request_info;
absl::optional<Envoy::Http::Protocol> protocol = Envoy::Http::Protocol::Http11;
ON_CALL(request_info, protocol()).WillByDefault(ReturnPointee(&protocol));
Expand All @@ -496,31 +521,31 @@ match: { prefix: "/new_endpoint" }
)EOF"));
ON_CALL(*host, metadata()).WillByDefault(Return(metadata));

req_header_parser->evaluateHeaders(headerMap, request_info);
req_header_parser->evaluateHeaders(header_map, request_info);

EXPECT_TRUE(headerMap.has("x-prefix"));
EXPECT_EQ("prefix-127.0.0.1", headerMap.get_("x-prefix"));
EXPECT_TRUE(header_map.has("x-prefix"));
EXPECT_EQ("prefix-127.0.0.1", header_map.get_("x-prefix"));

EXPECT_TRUE(headerMap.has("x-suffix"));
EXPECT_EQ("127.0.0.1-suffix", headerMap.get_("x-suffix"));
EXPECT_TRUE(header_map.has("x-suffix"));
EXPECT_EQ("127.0.0.1-suffix", header_map.get_("x-suffix"));

EXPECT_TRUE(headerMap.has("x-both"));
EXPECT_EQ("prefix-127.0.0.1-suffix", headerMap.get_("x-both"));
EXPECT_TRUE(header_map.has("x-both"));
EXPECT_EQ("prefix-127.0.0.1-suffix", header_map.get_("x-both"));

EXPECT_TRUE(headerMap.has("x-escaping-1"));
EXPECT_EQ("%127.0.0.1%", headerMap.get_("x-escaping-1"));
EXPECT_TRUE(header_map.has("x-escaping-1"));
EXPECT_EQ("%127.0.0.1%", header_map.get_("x-escaping-1"));

EXPECT_TRUE(headerMap.has("x-escaping-2"));
EXPECT_EQ("%%%", headerMap.get_("x-escaping-2"));
EXPECT_TRUE(header_map.has("x-escaping-2"));
EXPECT_EQ("%%%", header_map.get_("x-escaping-2"));

EXPECT_TRUE(headerMap.has("x-multi"));
EXPECT_EQ("HTTP/1.1 from 127.0.0.1", headerMap.get_("x-multi"));
EXPECT_TRUE(header_map.has("x-multi"));
EXPECT_EQ("HTTP/1.1 from 127.0.0.1", header_map.get_("x-multi"));

EXPECT_TRUE(headerMap.has("x-multi-back-to-back"));
EXPECT_EQ("HTTP/1.1127.0.0.1", headerMap.get_("x-multi-back-to-back"));
EXPECT_TRUE(header_map.has("x-multi-back-to-back"));
EXPECT_EQ("HTTP/1.1127.0.0.1", header_map.get_("x-multi-back-to-back"));

EXPECT_TRUE(headerMap.has("x-metadata"));
EXPECT_EQ("value", headerMap.get_("x-metadata"));
EXPECT_TRUE(header_map.has("x-metadata"));
EXPECT_EQ("value", header_map.get_("x-metadata"));
}

TEST(HeaderParserTest, EvaluateHeadersWithAppendFalse) {
Expand Down Expand Up @@ -562,29 +587,29 @@ TEST(HeaderParserTest, EvaluateHeadersWithAppendFalse) {

HeaderParserPtr req_header_parser =
Router::HeaderParser::configure(route_action.request_headers_to_add());
Http::TestHeaderMapImpl headerMap{
Http::TestHeaderMapImpl header_map{
{":method", "POST"}, {"static-header", "old-value"}, {"x-client-ip", "0.0.0.0"}};

NiceMock<Envoy::RequestInfo::MockRequestInfo> request_info;
const SystemTime start_time(std::chrono::microseconds(1522796769123456));
EXPECT_CALL(request_info, startTime()).Times(3).WillRepeatedly(Return(start_time));

req_header_parser->evaluateHeaders(headerMap, request_info);
EXPECT_TRUE(headerMap.has("static-header"));
EXPECT_EQ("static-value", headerMap.get_("static-header"));
EXPECT_TRUE(headerMap.has("x-client-ip"));
EXPECT_EQ("127.0.0.1", headerMap.get_("x-client-ip"));
EXPECT_TRUE(headerMap.has("x-request-start"));
EXPECT_EQ("1522796769123", headerMap.get_("x-request-start"));
EXPECT_TRUE(headerMap.has("x-request-start-default"));
EXPECT_EQ("2018-04-03T23:06:09.123Z", headerMap.get_("x-request-start-default"));
EXPECT_TRUE(headerMap.has("x-request-start-range"));
req_header_parser->evaluateHeaders(header_map, request_info);
EXPECT_TRUE(header_map.has("static-header"));
EXPECT_EQ("static-value", header_map.get_("static-header"));
EXPECT_TRUE(header_map.has("x-client-ip"));
EXPECT_EQ("127.0.0.1", header_map.get_("x-client-ip"));
EXPECT_TRUE(header_map.has("x-request-start"));
EXPECT_EQ("1522796769123", header_map.get_("x-request-start"));
EXPECT_TRUE(header_map.has("x-request-start-default"));
EXPECT_EQ("2018-04-03T23:06:09.123Z", header_map.get_("x-request-start-default"));
EXPECT_TRUE(header_map.has("x-request-start-range"));
EXPECT_EQ("123456000, 1, 12, 123, 1234, 12345, 123456, 1234560, 12345600, 123456000",
headerMap.get_("x-request-start-range"));
header_map.get_("x-request-start-range"));

typedef std::map<std::string, int> CountMap;
CountMap counts;
headerMap.iterate(
header_map.iterate(
[](const Http::HeaderEntry& header, void* cb_v) -> Http::HeaderMap::Iterate {
CountMap* m = static_cast<CountMap*>(cb_v);
std::string key = std::string{header.key().c_str()};
Expand Down Expand Up @@ -639,29 +664,29 @@ match: { prefix: "/new_endpoint" }
const auto route = parseRouteFromV2Yaml(yaml).route();
HeaderParserPtr resp_header_parser =
HeaderParser::configure(route.response_headers_to_add(), route.response_headers_to_remove());
Http::TestHeaderMapImpl headerMap{{":method", "POST"}, {"x-safe", "safe"}, {"x-nope", "nope"}};
Http::TestHeaderMapImpl header_map{{":method", "POST"}, {"x-safe", "safe"}, {"x-nope", "nope"}};
NiceMock<Envoy::RequestInfo::MockRequestInfo> request_info;

// Initialize start_time as 2018-04-03T23:06:09.123Z in microseconds.
const SystemTime start_time(std::chrono::microseconds(1522796769123456));
EXPECT_CALL(request_info, startTime()).Times(7).WillRepeatedly(Return(start_time));

resp_header_parser->evaluateHeaders(headerMap, request_info);
EXPECT_TRUE(headerMap.has("x-client-ip"));
EXPECT_TRUE(headerMap.has("x-request-start-multiple"));
EXPECT_TRUE(headerMap.has("x-safe"));
EXPECT_FALSE(headerMap.has("x-nope"));
EXPECT_TRUE(headerMap.has("x-request-start"));
EXPECT_EQ("1522796769.123", headerMap.get_("x-request-start"));
resp_header_parser->evaluateHeaders(header_map, request_info);
EXPECT_TRUE(header_map.has("x-client-ip"));
EXPECT_TRUE(header_map.has("x-request-start-multiple"));
EXPECT_TRUE(header_map.has("x-safe"));
EXPECT_FALSE(header_map.has("x-nope"));
EXPECT_TRUE(header_map.has("x-request-start"));
EXPECT_EQ("1522796769.123", header_map.get_("x-request-start"));
EXPECT_EQ("1522796769.123 2018-04-03T23:06:09.123Z 1522796769",
headerMap.get_("x-request-start-multiple"));
EXPECT_TRUE(headerMap.has("x-request-start-f"));
EXPECT_EQ("f", headerMap.get_("x-request-start-f"));
EXPECT_TRUE(headerMap.has("x-request-start-default"));
EXPECT_EQ("2018-04-03T23:06:09.123Z", headerMap.get_("x-request-start-default"));
EXPECT_TRUE(headerMap.has("x-request-start-range"));
header_map.get_("x-request-start-multiple"));
EXPECT_TRUE(header_map.has("x-request-start-f"));
EXPECT_EQ("f", header_map.get_("x-request-start-f"));
EXPECT_TRUE(header_map.has("x-request-start-default"));
EXPECT_EQ("2018-04-03T23:06:09.123Z", header_map.get_("x-request-start-default"));
EXPECT_TRUE(header_map.has("x-request-start-range"));
EXPECT_EQ("123456000, 1, 12, 123, 1234, 12345, 123456, 1234560, 12345600, 123456000",
headerMap.get_("x-request-start-range"));
header_map.get_("x-request-start-range"));
}

} // namespace Router
Expand Down