From d5f6c658d6159c753fc5bee09bfe7bfb02582380 Mon Sep 17 00:00:00 2001 From: antonio Date: Wed, 29 Apr 2020 17:33:45 -0400 Subject: [PATCH 1/3] [http1] Preserve LWS from the middle of HTTP1 header values that require multiple dispatch calls to process (#10886) Correctly preserve linear whitespace in the middle of HTTP1 header values. The fix in 6a95a21 trimmed away both leading and trailing whitespace when accepting header value fragments which can result in inner LWS in header values being stripped away if the LWS lands at the beginning or end of a buffer slice. Signed-off-by: Antonio Vicente Signed-off-by: antonio Signed-off-by: Yuchen Dai --- include/envoy/http/header_map.h | 6 ++ source/common/http/header_map_impl.cc | 40 +++++++++++++ source/common/http/http1/codec_impl.cc | 16 +++++- test/common/http/header_map_impl_test.cc | 13 +++++ test/common/http/http1/codec_impl_test.cc | 57 ++++++++++++++++++- test/integration/protocol_integration_test.cc | 33 +++++++++++ 6 files changed, 159 insertions(+), 6 deletions(-) diff --git a/include/envoy/http/header_map.h b/include/envoy/http/header_map.h index 9c65dfa1bc9b..de7ff83e5c4e 100644 --- a/include/envoy/http/header_map.h +++ b/include/envoy/http/header_map.h @@ -129,6 +129,12 @@ class HeaderString { */ char* buffer() { return buffer_.dynamic_; } + /** + * Trim trailing whitespaces from the HeaderString. + * v1.13 support all implementation. + */ + void rtrim(); + /** * Get an absl::string_view. It will NOT be NUL terminated! * diff --git a/source/common/http/header_map_impl.cc b/source/common/http/header_map_impl.cc index ecc98017b841..129a93ef0fa6 100644 --- a/source/common/http/header_map_impl.cc +++ b/source/common/http/header_map_impl.cc @@ -150,6 +150,46 @@ void HeaderString::append(const char* data, uint32_t size) { string_length_ += size; } +void HeaderString::rtrim() { + switch (type_) { + case Type::Reference: { + absl::string_view original = getStringView(); + absl::string_view rtrimmed = StringUtil::rtrim(original); + const uint64_t new_size = rtrimmed.size(); + // rtrim does nothing. + if (new_size == string_length_) { + break; + } + if (new_size > sizeof(inline_buffer_)) { + type_ = Type::Dynamic; + char* buf = static_cast(malloc(dynamic_capacity_)); + RELEASE_ASSERT(buf != nullptr, ""); + memcpy(buf, buffer_.ref_, new_size); + buffer_.dynamic_ = buf; + dynamic_capacity_ = new_size; + string_length_ = new_size; + } else { + type_ = Type::Inline; + memcpy(inline_buffer_, buffer_.ref_, new_size); + string_length_ = new_size; + } + break; + } + case Type::Dynamic: { + // rtrim only manipulate length_. Share the same code with Inline. + FALLTHRU; + } + case Type::Inline: { + absl::string_view original = getStringView(); + absl::string_view rtrimmed = StringUtil::rtrim(original); + if (original.size() != rtrimmed.size()) { + string_length_ = rtrimmed.size(); + } + break; + } + } +} + void HeaderString::clear() { switch (type_) { case Type::Reference: { diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index 32bcb70db812..358b2dd07476 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -461,6 +461,10 @@ void ConnectionImpl::completeLastHeader() { checkHeaderNameForUnderscores(); if (!current_header_field_.empty()) { toLowerTable().toLowerCase(current_header_field_.buffer(), current_header_field_.size()); + // Strip trailing whitespace of the current header value if any. Leading whitespace was trimmed + // in ConnectionImpl::onHeaderValue. http_parser does not strip leading or trailing whitespace + // as the spec requires: https://tools.ietf.org/html/rfc7230#section-3.2.4 + current_header_value_.rtrim(); current_header_map_->addViaMove(std::move(current_header_field_), std::move(current_header_value_)); } @@ -584,9 +588,8 @@ void ConnectionImpl::onHeaderValue(const char* data, size_t length) { if (!current_header_map_) { current_header_map_ = std::make_unique(); } - // Work around a bug in http_parser where trailing whitespace is not trimmed - // as the spec requires: https://tools.ietf.org/html/rfc7230#section-3.2.4 - const absl::string_view header_value = StringUtil::trim(absl::string_view(data, length)); + + absl::string_view header_value{data, length}; if (strict_header_validation_) { if (!Http::HeaderUtility::headerValueIsValid(header_value)) { @@ -604,6 +607,13 @@ void ConnectionImpl::onHeaderValue(const char* data, size_t length) { } header_parsing_state_ = HeaderParsingState::Value; + if (current_header_value_.empty()) { + // Strip leading whitespace if the current header value input contains the first bytes of the + // encoded header value. Trailing whitespace is stripped once the full header value is known in + // ConnectionImpl::completeLastHeader. http_parser does not strip leading or trailing whitespace + // as the spec requires: https://tools.ietf.org/html/rfc7230#section-3.2.4 . + header_value = StringUtil::ltrim(header_value); + } current_header_value_.append(header_value.data(), header_value.length()); checkMaxHeadersSize(); diff --git a/test/common/http/header_map_impl_test.cc b/test/common/http/header_map_impl_test.cc index 74f2d225e308..aa20ffa5aa2f 100644 --- a/test/common/http/header_map_impl_test.cc +++ b/test/common/http/header_map_impl_test.cc @@ -115,6 +115,19 @@ TEST(HeaderStringTest, All) { EXPECT_EQ("HELLO", string.getStringView()); } + // Inline rtrim removes trailing whitespace only. + { + const std::string data_with_leading_lws = " \t\f\v data"; + const std::string data_with_leading_and_trailing_lws = data_with_leading_lws + " \t\f\v"; + HeaderString string; + string.append(data_with_leading_and_trailing_lws.data(), + data_with_leading_and_trailing_lws.size()); + EXPECT_EQ(data_with_leading_and_trailing_lws, string.getStringView()); + string.rtrim(); + EXPECT_NE(data_with_leading_and_trailing_lws, string.getStringView()); + EXPECT_EQ(data_with_leading_lws, string.getStringView()); + } + // Static clear() does nothing. { std::string static_string("HELLO"); diff --git a/test/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc index 8ea74a0337a3..d6f23bec8561 100644 --- a/test/common/http/http1/codec_impl_test.cc +++ b/test/common/http/http1/codec_impl_test.cc @@ -43,6 +43,16 @@ std::string createHeaderFragment(int num_headers) { } return headers; } + +Buffer::OwnedImpl createBufferWithNByteSlices(absl::string_view input, size_t max_slice_size) { + Buffer::OwnedImpl buffer; + for (size_t offset = 0; offset < input.size(); offset += max_slice_size) { + buffer.appendSliceForTest(input.substr(offset, max_slice_size)); + } + // Verify that the buffer contains the right number of slices. + ASSERT(buffer.getRawSlices(nullptr, 0) == (input.size() + max_slice_size - 1) / max_slice_size); + return buffer; +} } // namespace class Http1ServerConnectionImplTest : public testing::Test { @@ -72,7 +82,13 @@ class Http1ServerConnectionImplTest : public testing::Test { // Then send a response just to clean up. void sendAndValidateRequestAndSendResponse(absl::string_view raw_request, const TestHeaderMapImpl& expected_request_headers) { - NiceMock decoder; + Buffer::OwnedImpl buffer(raw_request); + sendAndValidateRequestAndSendResponse(buffer, expected_request_headers); + } + + void sendAndValidateRequestAndSendResponse(Buffer::Instance& buffer, + const TestHeaderMapImpl& expected_request_headers) { + NiceMock decoder; Http::StreamEncoder* response_encoder = nullptr; EXPECT_CALL(callbacks_, newStream(_, _)) .Times(1) @@ -80,8 +96,7 @@ class Http1ServerConnectionImplTest : public testing::Test { response_encoder = &encoder; return decoder; })); - EXPECT_CALL(decoder, decodeHeaders_(HeaderMapEqual(&expected_request_headers), true)).Times(1); - Buffer::OwnedImpl buffer(raw_request); + EXPECT_CALL(decoder, decodeHeaders_(HeaderMapEqual(&expected_request_headers), true)); codec_->dispatch(buffer); EXPECT_EQ(0U, buffer.length()); response_encoder->encodeHeaders(TestHeaderMapImpl{{":status", "200"}}, true); @@ -353,6 +368,42 @@ TEST_F(Http1ServerConnectionImplTest, HostWithLWS) { "GET / HTTP/1.1\r\nHost: host \r\n\r\n", expected_headers); } +// Regression test for https://github.com/envoyproxy/envoy/issues/10270. Linear whitespace at the +// beginning and end of a header value should be stripped. Whitespace in the middle should be +// preserved. +TEST_F(Http1ServerConnectionImplTest, InnerLWSIsPreserved) { + initialize(); + + // Header with many spaces surrounded by non-whitespace characters to ensure that dispatching is + // split across multiple dispatch calls. The threshold used here comes from Envoy preferring 16KB + // reads, but the important part is that the header value is split such that the pieces have + // leading and trailing whitespace characters. + const std::string header_value_with_inner_lws = "v" + std::string(32 * 1024, ' ') + "v"; + TestHeaderMapImpl expected_headers{{":authority", "host"}, + {":path", "/"}, + {":method", "GET"}, + {"header_field", header_value_with_inner_lws}}; + + { + // Regression test spaces in the middle are preserved + Buffer::OwnedImpl header_buffer = createBufferWithNByteSlices( + "GET / HTTP/1.1\r\nHost: host\r\nheader_field: " + header_value_with_inner_lws + "\r\n\r\n", + 16 * 1024); + EXPECT_EQ(3, header_buffer.getRawSlices(nullptr, 0)); + sendAndValidateRequestAndSendResponse(header_buffer, expected_headers); + } + + { + // Regression test spaces before and after are removed + Buffer::OwnedImpl header_buffer = createBufferWithNByteSlices( + "GET / HTTP/1.1\r\nHost: host\r\nheader_field: " + header_value_with_inner_lws + + " \r\n\r\n", + 16 * 1024); + EXPECT_EQ(3, header_buffer.getRawSlices(nullptr, 0)); + sendAndValidateRequestAndSendResponse(header_buffer, expected_headers); + } +} + TEST_F(Http1ServerConnectionImplTest, Http10) { initialize(); diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc index e0d69f3774bc..e28d6b9a07bb 100644 --- a/test/integration/protocol_integration_test.cc +++ b/test/integration/protocol_integration_test.cc @@ -241,6 +241,39 @@ TEST_P(ProtocolIntegrationTest, DrainClose) { test_server_->drainManager().draining_ = false; } +// Regression test for https://github.com/envoyproxy/envoy/issues/10270 +TEST_P(ProtocolIntegrationTest, LongHeaderValueWithSpaces) { + // Header with at least 20kb of spaces surrounded by non-whitespace characters to ensure that + // dispatching is split across 2 dispatch calls. This threshold comes from Envoy preferring 16KB + // reads, which the buffer rounds up to about 20KB when allocating slices in + // Buffer::OwnedImpl::reserve(). + const std::string long_header_value_with_inner_lws = "v" + std::string(32 * 1024, ' ') + "v"; + + initialize(); + codec_client_ = makeHttpConnection(lookupPort("http")); + auto response = codec_client_->makeHeaderOnlyRequest( + Http::TestHeaderMapImpl{{":method", "GET"}, + {":path", "/test/long/url"}, + {":scheme", "http"}, + {":authority", "host"}, + {"longrequestvalue", long_header_value_with_inner_lws}}); + waitForNextUpstreamRequest(); + EXPECT_EQ(long_header_value_with_inner_lws, upstream_request_->headers() + .get(Http::LowerCaseString("longrequestvalue")) + ->value() + .getStringView()); + upstream_request_->encodeHeaders( + Http::TestHeaderMapImpl{{":status", "200"}, + {"longresponsevalue", long_header_value_with_inner_lws}}, + true); + response->waitForEndStream(); + EXPECT_TRUE(response->complete()); + EXPECT_EQ("200", response->headers().Status()->value().getStringView()); + EXPECT_EQ( + long_header_value_with_inner_lws, + response->headers().get(Http::LowerCaseString("longresponsevalue"))->value().getStringView()); +} + TEST_P(ProtocolIntegrationTest, Retry) { initialize(); codec_client_ = makeHttpConnection(lookupPort("http")); From 62e5eca4604c026ea4e50257d67c2a6a3887cd03 Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Tue, 28 Jul 2020 21:50:18 +0000 Subject: [PATCH 2/3] rtrim made accurate Signed-off-by: Yuchen Dai --- include/envoy/http/header_map.h | 2 +- source/common/http/header_map_impl.cc | 24 ++---------------------- 2 files changed, 3 insertions(+), 23 deletions(-) diff --git a/include/envoy/http/header_map.h b/include/envoy/http/header_map.h index de7ff83e5c4e..7a48b841f2a0 100644 --- a/include/envoy/http/header_map.h +++ b/include/envoy/http/header_map.h @@ -131,7 +131,7 @@ class HeaderString { /** * Trim trailing whitespaces from the HeaderString. - * v1.13 support all implementation. + * v1.13 supports both Inline and Dynamic, but not Reference type. */ void rtrim(); diff --git a/source/common/http/header_map_impl.cc b/source/common/http/header_map_impl.cc index 129a93ef0fa6..057dc1ce4fc1 100644 --- a/source/common/http/header_map_impl.cc +++ b/source/common/http/header_map_impl.cc @@ -153,30 +153,10 @@ void HeaderString::append(const char* data, uint32_t size) { void HeaderString::rtrim() { switch (type_) { case Type::Reference: { - absl::string_view original = getStringView(); - absl::string_view rtrimmed = StringUtil::rtrim(original); - const uint64_t new_size = rtrimmed.size(); - // rtrim does nothing. - if (new_size == string_length_) { - break; - } - if (new_size > sizeof(inline_buffer_)) { - type_ = Type::Dynamic; - char* buf = static_cast(malloc(dynamic_capacity_)); - RELEASE_ASSERT(buf != nullptr, ""); - memcpy(buf, buffer_.ref_, new_size); - buffer_.dynamic_ = buf; - dynamic_capacity_ = new_size; - string_length_ = new_size; - } else { - type_ = Type::Inline; - memcpy(inline_buffer_, buffer_.ref_, new_size); - string_length_ = new_size; - } - break; + RELEASE_ASSERT(false, "rtrim does not support header string in reference type."); } case Type::Dynamic: { - // rtrim only manipulate length_. Share the same code with Inline. + // v1.13: rtrim manipulate length_. Share the same code with Inline. FALLTHRU; } case Type::Inline: { From 240b8fd03e31414f56f149103f7421d002a95154 Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Tue, 28 Jul 2020 23:14:56 +0000 Subject: [PATCH 3/3] More rtrim fix Signed-off-by: Yuchen Dai --- source/common/http/header_map_impl.cc | 21 +++++---------------- test/common/http/header_map_impl_test.cc | 18 ++++++++++++++++++ 2 files changed, 23 insertions(+), 16 deletions(-) diff --git a/source/common/http/header_map_impl.cc b/source/common/http/header_map_impl.cc index 057dc1ce4fc1..bcbc4f91aad8 100644 --- a/source/common/http/header_map_impl.cc +++ b/source/common/http/header_map_impl.cc @@ -151,22 +151,11 @@ void HeaderString::append(const char* data, uint32_t size) { } void HeaderString::rtrim() { - switch (type_) { - case Type::Reference: { - RELEASE_ASSERT(false, "rtrim does not support header string in reference type."); - } - case Type::Dynamic: { - // v1.13: rtrim manipulate length_. Share the same code with Inline. - FALLTHRU; - } - case Type::Inline: { - absl::string_view original = getStringView(); - absl::string_view rtrimmed = StringUtil::rtrim(original); - if (original.size() != rtrimmed.size()) { - string_length_ = rtrimmed.size(); - } - break; - } + ASSERT(type() == Type::Inline || type() == Type::Dynamic); + absl::string_view original = getStringView(); + absl::string_view rtrimmed = StringUtil::rtrim(original); + if (original.size() != rtrimmed.size()) { + string_length_ = rtrimmed.size(); } } diff --git a/test/common/http/header_map_impl_test.cc b/test/common/http/header_map_impl_test.cc index aa20ffa5aa2f..f3c60252e1f0 100644 --- a/test/common/http/header_map_impl_test.cc +++ b/test/common/http/header_map_impl_test.cc @@ -117,17 +117,35 @@ TEST(HeaderStringTest, All) { // Inline rtrim removes trailing whitespace only. { + // This header string is short enough to fit into Inline type. const std::string data_with_leading_lws = " \t\f\v data"; const std::string data_with_leading_and_trailing_lws = data_with_leading_lws + " \t\f\v"; HeaderString string; string.append(data_with_leading_and_trailing_lws.data(), data_with_leading_and_trailing_lws.size()); EXPECT_EQ(data_with_leading_and_trailing_lws, string.getStringView()); + EXPECT_EQ(string.type(), HeaderString::Type::Inline); string.rtrim(); EXPECT_NE(data_with_leading_and_trailing_lws, string.getStringView()); EXPECT_EQ(data_with_leading_lws, string.getStringView()); } + // Dynamic rtrim removes trailing whitespace only. + { + // Making this string longer than Inline can fit. + const std::string padding_data_with_leading_lws = " \t\f\v data" + std::string(128, 'a'); + const std::string data_with_leading_and_trailing_lws = + padding_data_with_leading_lws + " \t\f\v"; + HeaderString string; + string.append(data_with_leading_and_trailing_lws.data(), + data_with_leading_and_trailing_lws.size()); + EXPECT_EQ(data_with_leading_and_trailing_lws, string.getStringView()); + EXPECT_EQ(string.type(), HeaderString::Type::Dynamic); + string.rtrim(); + EXPECT_NE(data_with_leading_and_trailing_lws, string.getStringView()); + EXPECT_EQ(padding_data_with_leading_lws, string.getStringView()); + } + // Static clear() does nothing. { std::string static_string("HELLO");