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.

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
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
25 changes: 17 additions & 8 deletions source/common/http/header_map_impl.cc
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,11 @@ 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);
dynamic_capacity_ = std::max(MinDynamicCapacity, new_capacity);
if (dynamic_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?

}
char* buf = static_cast<char*>(malloc(dynamic_capacity_));
RELEASE_ASSERT(buf != nullptr, "");
memcpy(buf, buffer_.ref_, string_length_);
Expand All @@ -108,7 +115,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 +124,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
142 changes: 82 additions & 60 deletions test/common/router/header_formatter_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,28 @@ TEST_F(RequestInfoHeaderFormatterTest, TestFormatWithUpstreamMetadataVariable) {
testFormatting(request_info, "UPSTREAM_METADATA([\"namespace\", \"nested\", \"list_key\"])", "");
}

TEST_F(RequestInfoHeaderFormatterTest, 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(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 +382,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 +409,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 +431,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 +458,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 +499,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 +518,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 +584,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 +661,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