diff --git a/source/common/http/header_map_impl.cc b/source/common/http/header_map_impl.cc index f3807edd5285c..c9bf39570cae2 100644 --- a/source/common/http/header_map_impl.cc +++ b/source/common/http/header_map_impl.cc @@ -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) { + return (static_cast(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::max(), ""); + RELEASE_ASSERT(new_capacity <= std::numeric_limits::max(), + "Trying to allocate overly large headers."); ASSERT(new_capacity >= MinDynamicCapacity); } @@ -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((string_length_ + size) * 2)); - validateCapacity(dynamic_capacity_); + const uint64_t new_capacity = newCapacity(string_length_, size); + if (new_capacity > MinDynamicCapacity) { + validateCapacity(new_capacity); + dynamic_capacity_ = new_capacity; + } else { + dynamic_capacity_ = MinDynamicCapacity; + } char* buf = static_cast(malloc(dynamic_capacity_)); RELEASE_ASSERT(buf != nullptr, ""); memcpy(buf, buffer_.ref_, string_length_); @@ -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(size) + 1 + string_length_; + if (new_capacity <= sizeof(inline_buffer_)) { // Already inline and the new value fits in inline storage. break; } @@ -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(malloc(new_capacity)); RELEASE_ASSERT(buffer_.dynamic_ != nullptr, ""); @@ -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(realloc(buffer_.dynamic_, 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 dc55e0d2e7ae0..8a1df0f6369f4 100644 --- a/test/common/http/header_map_impl_test.cc +++ b/test/common/http/header_map_impl_test.cc @@ -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::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::max()), + "Trying to allocate overly large headers."); +} + TEST(HeaderMapImplTest, PseudoHeaderOrder) { typedef testing::MockFunction MockCb; MockCb cb; diff --git a/test/common/router/header_formatter_test.cc b/test/common/router/header_formatter_test.cc index 9cd76e4b877af..9c9048e6194e0 100644 --- a/test/common/router/header_formatter_test.cc +++ b/test/common/router/header_formatter_test.cc @@ -162,6 +162,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 to_add; + const uint32_t num_header_chunks = 10; + const uint64_t length = std::numeric_limits::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 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 request_info; std::shared_ptr> host; @@ -421,14 +446,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; } } @@ -448,10 +473,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 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) { @@ -470,15 +495,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> host( new NiceMock()); NiceMock request_info; auto metadata = std::make_shared(); 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) { @@ -497,11 +522,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 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) { @@ -541,7 +566,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 request_info; absl::optional protocol = Envoy::Http::Protocol::Http11; ON_CALL(request_info, protocol()).WillByDefault(ReturnPointee(&protocol)); @@ -565,34 +590,34 @@ match: { prefix: "/new_endpoint" } ON_CALL(request_info, perRequestState()).WillByDefault(ReturnRef(per_request_state)); ON_CALL(Const(request_info), perRequestState()).WillByDefault(ReturnRef(per_request_state)); - 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")); - EXPECT_TRUE(headerMap.has("x-per-request")); - EXPECT_EQ("test_value", headerMap.get_("x-per-request")); + EXPECT_TRUE(header_map.has("x-per-request")); + EXPECT_EQ("test_value", header_map.get_("x-per-request")); } TEST(HeaderParserTest, EvaluateHeadersWithAppendFalse) { @@ -634,29 +659,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 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 CountMap; CountMap counts; - headerMap.iterate( + header_map.iterate( [](const Http::HeaderEntry& header, void* cb_v) -> Http::HeaderMap::Iterate { CountMap* m = static_cast(cb_v); std::string key = std::string{header.key().c_str()}; @@ -711,29 +736,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 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