diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index 1e821f7a0dec..96dd8a939ea7 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -503,6 +503,22 @@ void ConnectionImpl::completeLastHeader() { ASSERT(current_header_value_.empty()); } +uint32_t ConnectionImpl::getHeadersSize() { + return current_header_field_.size() + current_header_value_.size() + + headersOrTrailers().byteSize(); +} + +void ConnectionImpl::checkMaxHeadersSize() { + const uint32_t total = getHeadersSize(); + if (total > (max_headers_kb_ * 1024)) { + const absl::string_view header_type = + processing_trailers_ ? Http1HeaderTypes::get().Trailers : Http1HeaderTypes::get().Headers; + error_code_ = Http::Code::RequestHeaderFieldsTooLarge; + sendProtocolError(Http1ResponseCodeDetails::get().HeadersTooLarge); + throw CodecProtocolException(absl::StrCat(header_type, " size exceeds limit")); + } +} + bool ConnectionImpl::maybeDirectDispatch(Buffer::Instance& data) { if (!handling_upgrade_) { // Only direct dispatch for Upgrade requests. @@ -581,12 +597,15 @@ void ConnectionImpl::onHeaderField(const char* data, size_t length) { } processing_trailers_ = true; header_parsing_state_ = HeaderParsingState::Field; + allocTrailers(); } if (header_parsing_state_ == HeaderParsingState::Value) { completeLastHeader(); } current_header_field_.append(data, length); + + checkMaxHeadersSize(); } void ConnectionImpl::onHeaderValue(const char* data, size_t length) { @@ -595,12 +614,7 @@ void ConnectionImpl::onHeaderValue(const char* data, size_t length) { return; } - if (processing_trailers_) { - maybeAllocTrailers(); - } - absl::string_view header_value{data, length}; - if (strict_header_validation_) { if (!Http::HeaderUtility::headerValueIsValid(header_value)) { ENVOY_CONN_LOG(debug, "invalid header value: {}", connection_, header_value); @@ -620,15 +634,7 @@ void ConnectionImpl::onHeaderValue(const char* data, size_t length) { } current_header_value_.append(header_value.data(), header_value.length()); - const uint32_t total = - current_header_field_.size() + current_header_value_.size() + headersOrTrailers().byteSize(); - if (total > (max_headers_kb_ * 1024)) { - const absl::string_view header_type = - processing_trailers_ ? Http1HeaderTypes::get().Trailers : Http1HeaderTypes::get().Headers; - error_code_ = Http::Code::RequestHeaderFieldsTooLarge; - sendProtocolError(Http1ResponseCodeDetails::get().HeadersTooLarge); - throw CodecProtocolException(absl::StrCat(header_type, " size exceeds limit")); - } + checkMaxHeadersSize(); } int ConnectionImpl::onHeadersCompleteBase() { @@ -786,6 +792,14 @@ ServerConnectionImpl::ServerConnectionImpl( Runtime::runtimeFeatureEnabled("envoy.reloadable_features.http1_flood_protection")), headers_with_underscores_action_(headers_with_underscores_action) {} +uint32_t ServerConnectionImpl::getHeadersSize() { + // Add in the the size of the request URL if processing request headers. + const uint32_t url_size = (!processing_trailers_ && active_request_.has_value()) + ? active_request_.value().request_url_.size() + : 0; + return url_size + ConnectionImpl::getHeadersSize(); +} + void ServerConnectionImpl::onEncodeComplete() { if (active_request_.value().remote_complete_) { // Only do this if remote is complete. If we are replying before the request is complete the @@ -918,6 +932,8 @@ void ServerConnectionImpl::onMessageBegin() { void ServerConnectionImpl::onUrl(const char* data, size_t length) { if (active_request_.has_value()) { active_request_.value().request_url_.append(data, length); + + checkMaxHeadersSize(); } } diff --git a/source/common/http/http1/codec_impl.h b/source/common/http/http1/codec_impl.h index 44e4282742c9..b18a482872d7 100644 --- a/source/common/http/http1/codec_impl.h +++ b/source/common/http/http1/codec_impl.h @@ -220,6 +220,20 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable& activeRequest() { return active_request_; } // ConnectionImpl void onMessageComplete() override; + // Add the size of the request_url to the reported header size when processing request headers. + uint32_t getHeadersSize() override; private: /** @@ -462,9 +478,10 @@ class ServerConnectionImpl : public ServerConnection, public ConnectionImpl { } void allocHeaders() override { ASSERT(nullptr == absl::get(headers_or_trailers_)); + ASSERT(!processing_trailers_); headers_or_trailers_.emplace(RequestHeaderMapImpl::create()); } - void maybeAllocTrailers() override { + void allocTrailers() override { ASSERT(processing_trailers_); if (!absl::holds_alternative(headers_or_trailers_)) { headers_or_trailers_.emplace(RequestTrailerMapImpl::create()); @@ -547,9 +564,10 @@ class ClientConnectionImpl : public ClientConnection, public ConnectionImpl { } void allocHeaders() override { ASSERT(nullptr == absl::get(headers_or_trailers_)); + ASSERT(!processing_trailers_); headers_or_trailers_.emplace(ResponseHeaderMapImpl::create()); } - void maybeAllocTrailers() override { + void allocTrailers() override { ASSERT(processing_trailers_); if (!absl::holds_alternative(headers_or_trailers_)) { headers_or_trailers_.emplace(ResponseTrailerMapImpl::create()); @@ -567,9 +585,9 @@ class ClientConnectionImpl : public ClientConnection, public ConnectionImpl { bool ignore_message_complete_for_100_continue_{}; // TODO(mattklein123): This should be a member of PendingResponse but this change needs dedicated // thought as some of the reset and no header code paths make this difficult. Headers are - // populated on message begin. Trailers are populated on the first parsed trailer field (if - // trailers are enabled). The variant is reset to null headers on message complete for assertion - // purposes. + // populated on message begin. Trailers are populated when the switch to trailer processing is + // detected while parsing the first trailer field (if trailers are enabled). The variant is reset + // to null headers on message complete for assertion purposes. absl::variant headers_or_trailers_; // The default limit of 80 KiB is the vanilla http_parser behaviour. diff --git a/test/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc index 347a26780f44..e0c450f2f26c 100644 --- a/test/common/http/http1/codec_impl_test.cc +++ b/test/common/http/http1/codec_impl_test.cc @@ -238,7 +238,7 @@ void Http1ServerConnectionImplTest::testTrailersExceedLimit(std::string trailer_ "body\r\n0\r\n"); auto status = codec_->dispatch(buffer); EXPECT_TRUE(status.ok()); - buffer = Buffer::OwnedImpl(trailer_string + "\r\n\r\n"); + buffer = Buffer::OwnedImpl(trailer_string); if (enable_trailers) { EXPECT_CALL(decoder, sendLocalReply(_, _, _, _, _, _, _)); status = codec_->dispatch(buffer); @@ -2520,26 +2520,60 @@ TEST_F(Http1ClientConnectionImplTest, LowWatermarkDuringClose) { TEST_F(Http1ServerConnectionImplTest, LargeTrailersRejected) { // Default limit of 60 KiB - std::string long_string = "big: " + std::string(60 * 1024, 'q') + "\r\n"; + std::string long_string = "big: " + std::string(60 * 1024, 'q') + "\r\n\r\n\r\n"; + testTrailersExceedLimit(long_string, true); +} + +TEST_F(Http1ServerConnectionImplTest, LargeTrailerFieldRejected) { + // Construct partial headers with a long field name that exceeds the default limit of 60KiB. + std::string long_string = "bigfield" + std::string(60 * 1024, 'q'); testTrailersExceedLimit(long_string, true); } // Tests that the default limit for the number of request headers is 100. TEST_F(Http1ServerConnectionImplTest, ManyTrailersRejected) { // Send a request with 101 headers. - testTrailersExceedLimit(createHeaderFragment(101), true); + testTrailersExceedLimit(createHeaderFragment(101) + "\r\n\r\n", true); } TEST_F(Http1ServerConnectionImplTest, LargeTrailersRejectedIgnored) { // Default limit of 60 KiB - std::string long_string = "big: " + std::string(60 * 1024, 'q') + "\r\n"; + std::string long_string = "big: " + std::string(60 * 1024, 'q') + "\r\n\r\n\r\n"; + testTrailersExceedLimit(long_string, false); +} + +TEST_F(Http1ServerConnectionImplTest, LargeTrailerFieldRejectedIgnored) { + // Default limit of 60 KiB + std::string long_string = "bigfield" + std::string(60 * 1024, 'q') + ": value\r\n\r\n\r\n"; testTrailersExceedLimit(long_string, false); } // Tests that the default limit for the number of request headers is 100. TEST_F(Http1ServerConnectionImplTest, ManyTrailersIgnored) { // Send a request with 101 headers. - testTrailersExceedLimit(createHeaderFragment(101), false); + testTrailersExceedLimit(createHeaderFragment(101) + "\r\n\r\n", false); +} + +TEST_F(Http1ServerConnectionImplTest, LargeRequestUrlRejected) { + initialize(); + + std::string exception_reason; + NiceMock decoder; + Http::ResponseEncoder* response_encoder = nullptr; + EXPECT_CALL(callbacks_, newStream(_, _)) + .WillOnce(Invoke([&](ResponseEncoder& encoder, bool) -> RequestDecoder& { + response_encoder = &encoder; + return decoder; + })); + + // Default limit of 60 KiB + std::string long_url = "/" + std::string(60 * 1024, 'q'); + Buffer::OwnedImpl buffer("GET " + long_url + " HTTP/1.1\r\n"); + + auto status = codec_->dispatch(buffer); + EXPECT_TRUE(isCodecProtocolError(status)); + EXPECT_EQ(status.message(), "headers size exceeds limit"); + EXPECT_EQ("http1.headers_too_large", response_encoder->getStream().responseDetails()); } TEST_F(Http1ServerConnectionImplTest, LargeRequestHeadersRejected) { @@ -2631,8 +2665,27 @@ TEST_F(Http1ServerConnectionImplTest, ManyRequestHeadersAccepted) { testRequestHeadersAccepted(createHeaderFragment(150)); } -// Tests that response headers of 80 kB fails. -TEST_F(Http1ClientConnectionImplTest, LargeResponseHeadersRejected) { +// Tests that incomplete response headers of 80 kB header value fails. +TEST_F(Http1ClientConnectionImplTest, ResponseHeadersWithLargeValueRejected) { + initialize(); + + NiceMock response_decoder; + Http::RequestEncoder& request_encoder = codec_->newStream(response_decoder); + TestRequestHeaderMapImpl headers{{":method", "GET"}, {":path", "/"}, {":authority", "host"}}; + request_encoder.encodeHeaders(headers, true); + + Buffer::OwnedImpl buffer("HTTP/1.1 200 OK\r\nContent-Length: 0\r\n"); + auto status = codec_->dispatch(buffer); + EXPECT_TRUE(status.ok()); + std::string long_header = "big: " + std::string(80 * 1024, 'q'); + buffer = Buffer::OwnedImpl(long_header); + status = codec_->dispatch(buffer); + EXPECT_TRUE(isCodecProtocolError(status)); + EXPECT_EQ(status.message(), "headers size exceeds limit"); +} + +// Tests that incomplete response headers with a 80 kB header field fails. +TEST_F(Http1ClientConnectionImplTest, ResponseHeadersWithLargeFieldRejected) { initialize(); NiceMock decoder; @@ -2644,7 +2697,7 @@ TEST_F(Http1ClientConnectionImplTest, LargeResponseHeadersRejected) { Buffer::OwnedImpl buffer("HTTP/1.1 200 OK\r\nContent-Length: 0\r\n"); auto status = codec_->dispatch(buffer); EXPECT_TRUE(status.ok()); - std::string long_header = "big: " + std::string(80 * 1024, 'q') + "\r\n"; + std::string long_header = "big: " + std::string(80 * 1024, 'q'); buffer = Buffer::OwnedImpl(long_header); status = codec_->dispatch(buffer); EXPECT_TRUE(isCodecProtocolError(status)); diff --git a/test/integration/http_integration.cc b/test/integration/http_integration.cc index 01c6de72ad65..ca5fca51e403 100644 --- a/test/integration/http_integration.cc +++ b/test/integration/http_integration.cc @@ -958,6 +958,44 @@ void HttpIntegrationTest::testTwoRequests(bool network_backup) { EXPECT_EQ(1024U, response->body().size()); } +void HttpIntegrationTest::testLargeRequestUrl(uint32_t url_size, uint32_t max_headers_size) { + // `size` parameter dictates the size of each header that will be added to the request and `count` + // parameter is the number of headers to be added. The actual request byte size will exceed `size` + // due to the keys and other headers. The actual request header count will exceed `count` by four + // due to default headers. + + config_helper_.addConfigModifier( + [&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& + hcm) -> void { hcm.mutable_max_request_headers_kb()->set_value(max_headers_size); }); + max_request_headers_kb_ = max_headers_size; + + Http::TestRequestHeaderMapImpl big_headers{{":method", "GET"}, + {":path", "/" + std::string(url_size * 1024, 'a')}, + {":scheme", "http"}, + {":authority", "host"}}; + + initialize(); + codec_client_ = makeHttpConnection(lookupPort("http")); + if (url_size >= max_headers_size) { + // header size includes keys too, so expect rejection when equal + auto encoder_decoder = codec_client_->startRequest(big_headers); + auto response = std::move(encoder_decoder.second); + + if (downstream_protocol_ == Http::CodecClient::Type::HTTP1) { + codec_client_->waitForDisconnect(); + EXPECT_TRUE(response->complete()); + EXPECT_EQ("431", response->headers().Status()->value().getStringView()); + } else { + response->waitForReset(); + codec_client_->close(); + } + } else { + auto response = sendRequestAndWaitForResponse(big_headers, 0, default_response_headers_, 0); + EXPECT_TRUE(response->complete()); + EXPECT_EQ("200", response->headers().Status()->value().getStringView()); + } +} + void HttpIntegrationTest::testLargeRequestHeaders(uint32_t size, uint32_t count, uint32_t max_size, uint32_t max_count) { // `size` parameter dictates the size of each header that will be added to the request and `count` diff --git a/test/integration/http_integration.h b/test/integration/http_integration.h index 25cad95bb37b..bc04a94be36a 100644 --- a/test/integration/http_integration.h +++ b/test/integration/http_integration.h @@ -196,6 +196,7 @@ class HttpIntegrationTest : public BaseIntegrationTest { void testLargeHeaders(Http::TestRequestHeaderMapImpl request_headers, Http::TestRequestTrailerMapImpl request_trailers, uint32_t size, uint32_t max_size); + void testLargeRequestUrl(uint32_t url_size, uint32_t max_headers_size); void testLargeRequestHeaders(uint32_t size, uint32_t count, uint32_t max_size = 60, uint32_t max_count = 100); void testLargeRequestTrailers(uint32_t size, uint32_t max_size = 60); diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc index ccb9654c1b54..074181c73699 100644 --- a/test/integration/protocol_integration_test.cc +++ b/test/integration/protocol_integration_test.cc @@ -1352,6 +1352,16 @@ name: decode-headers-only EXPECT_EQ(0, upstream_request_->body().length()); } +TEST_P(DownstreamProtocolIntegrationTest, LargeRequestUrlRejected) { + // Send one 95 kB URL with limit 60 kB headers. + testLargeRequestUrl(95, 60); +} + +TEST_P(DownstreamProtocolIntegrationTest, LargeRequestUrlAccepted) { + // Send one 95 kB URL with limit 96 kB headers. + testLargeRequestUrl(95, 96); +} + TEST_P(DownstreamProtocolIntegrationTest, LargeRequestHeadersRejected) { // Send one 95 kB header with limit 60 kB and 100 headers. testLargeRequestHeaders(95, 1, 60, 100);