diff --git a/api/envoy/config/core/v3/protocol.proto b/api/envoy/config/core/v3/protocol.proto index 0ab6289e9659..f397882e3199 100644 --- a/api/envoy/config/core/v3/protocol.proto +++ b/api/envoy/config/core/v3/protocol.proto @@ -100,7 +100,7 @@ message HttpProtocolOptions { HeadersWithUnderscoresAction headers_with_underscores_action = 5; } -// [#next-free-field: 6] +// [#next-free-field: 7] message Http1ProtocolOptions { option (udpa.annotations.versioning).previous_message_type = "envoy.api.v2.core.Http1ProtocolOptions"; @@ -157,6 +157,16 @@ message Http1ProtocolOptions { // - Not a response to a HEAD request. // - The content length header is not present. bool enable_trailers = 5; + + // Allows Envoy to process requests/responses with both `Content-Length` and `Transfer-Encoding` + // headers set. By default such messages are rejected, but if option is enabled - Envoy will + // remove Content-Length header and process message. + // See `RFC7230, sec. 3.3.3 ` for details. + // + // .. attention:: + // Enabling this option might lead to request smuggling vulnerability, especially if traffic + // is proxied via multiple layers of proxies. + bool allow_chunked_length = 6; } // [#next-free-field: 15] diff --git a/api/envoy/config/core/v4alpha/protocol.proto b/api/envoy/config/core/v4alpha/protocol.proto index 955c29335a3f..746d53c557ee 100644 --- a/api/envoy/config/core/v4alpha/protocol.proto +++ b/api/envoy/config/core/v4alpha/protocol.proto @@ -100,7 +100,7 @@ message HttpProtocolOptions { HeadersWithUnderscoresAction headers_with_underscores_action = 5; } -// [#next-free-field: 6] +// [#next-free-field: 7] message Http1ProtocolOptions { option (udpa.annotations.versioning).previous_message_type = "envoy.config.core.v3.Http1ProtocolOptions"; @@ -157,6 +157,16 @@ message Http1ProtocolOptions { // - Not a response to a HEAD request. // - The content length header is not present. bool enable_trailers = 5; + + // Allows Envoy to process requests/responses with both `Content-Length` and `Transfer-Encoding` + // headers set. By default such messages are rejected, but if option is enabled - Envoy will + // remove Content-Length header and process message. + // See `RFC7230, sec. 3.3.3 ` for details. + // + // .. attention:: + // Enabling this option might lead to request smuggling vulnerability, especially if traffic + // is proxied via multiple layers of proxies. + bool allow_chunked_length = 6; } // [#next-free-field: 15] diff --git a/bazel/repository_locations.bzl b/bazel/repository_locations.bzl index c31bc141b997..b5161ffd330d 100644 --- a/bazel/repository_locations.bzl +++ b/bazel/repository_locations.bzl @@ -389,10 +389,13 @@ DEPENDENCY_REPOSITORIES_SPEC = dict( com_github_nodejs_http_parser = dict( project_name = "HTTP Parser", project_url = "https://github.com/nodejs/http-parser", - version = "2.9.3", - sha256 = "8fa0ab8770fd8425a9b431fdbf91623c4d7a9cdb842b9339289bd2b0b01b0d3d", + # 2020-07-10 + # This SHA includes fix for https://github.com/nodejs/http-parser/issues/517 which allows (opt-in) to serve + # requests with both Content-Legth and Transfer-Encoding: chunked headers set. + version = "4f15b7d510dc7c6361a26a7c6d2f7c3a17f8d878", + sha256 = "6a12896313ce1ca630cf516a0ee43a79b5f13f5a5d8143f56560ac0b21c98fac", strip_prefix = "http-parser-{version}", - urls = ["https://github.com/nodejs/http-parser/archive/v{version}.tar.gz"], + urls = ["https://github.com/nodejs/http-parser/archive/{version}.tar.gz"], use_category = ["dataplane"], cpe = "cpe:2.3:a:nodejs:node.js:*", ), diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index a6cc62277f5a..17b660beeeeb 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -69,6 +69,7 @@ New Features * grpc-json: support specifying `response_body` field in for `google.api.HttpBody` message. * hds: added :ref:`cluster_endpoints_health ` to HDS responses, keeping endpoints in the same groupings as they were configured in the HDS specifier by cluster and locality instead of as a flat list. * http: added support for :ref:`%DOWNSTREAM_PEER_FINGERPRINT_1% ` as custom header. +* http: added :ref:`allow_chunked_length ` configuration option for HTTP/1 codec to allow processing requests/responses with both Content-Length and Transfer-Encoding: chunked headers. If such message is served and option is enabled - per RFC Content-Length is ignored and removed. * http: introduced new HTTP/1 and HTTP/2 codec implementations that will remove the use of exceptions for control flow due to high risk factors and instead use error statuses. The old behavior is used by default, but the new codecs can be enabled for testing by setting the runtime feature `envoy.reloadable_features.new_codec_behavior` to true. The new codecs will be in development for one month, and then enabled by default while the old codecs are deprecated. * load balancer: added a :ref:`configuration` option to specify the active request bias used by the least request load balancer. * lua: added Lua APIs to access :ref:`SSL connection info ` object. diff --git a/generated_api_shadow/envoy/config/core/v3/protocol.proto b/generated_api_shadow/envoy/config/core/v3/protocol.proto index 0ab6289e9659..f397882e3199 100644 --- a/generated_api_shadow/envoy/config/core/v3/protocol.proto +++ b/generated_api_shadow/envoy/config/core/v3/protocol.proto @@ -100,7 +100,7 @@ message HttpProtocolOptions { HeadersWithUnderscoresAction headers_with_underscores_action = 5; } -// [#next-free-field: 6] +// [#next-free-field: 7] message Http1ProtocolOptions { option (udpa.annotations.versioning).previous_message_type = "envoy.api.v2.core.Http1ProtocolOptions"; @@ -157,6 +157,16 @@ message Http1ProtocolOptions { // - Not a response to a HEAD request. // - The content length header is not present. bool enable_trailers = 5; + + // Allows Envoy to process requests/responses with both `Content-Length` and `Transfer-Encoding` + // headers set. By default such messages are rejected, but if option is enabled - Envoy will + // remove Content-Length header and process message. + // See `RFC7230, sec. 3.3.3 ` for details. + // + // .. attention:: + // Enabling this option might lead to request smuggling vulnerability, especially if traffic + // is proxied via multiple layers of proxies. + bool allow_chunked_length = 6; } // [#next-free-field: 15] diff --git a/generated_api_shadow/envoy/config/core/v4alpha/protocol.proto b/generated_api_shadow/envoy/config/core/v4alpha/protocol.proto index cc1b99d0a048..5afb71210fbb 100644 --- a/generated_api_shadow/envoy/config/core/v4alpha/protocol.proto +++ b/generated_api_shadow/envoy/config/core/v4alpha/protocol.proto @@ -100,7 +100,7 @@ message HttpProtocolOptions { HeadersWithUnderscoresAction headers_with_underscores_action = 5; } -// [#next-free-field: 6] +// [#next-free-field: 7] message Http1ProtocolOptions { option (udpa.annotations.versioning).previous_message_type = "envoy.config.core.v3.Http1ProtocolOptions"; @@ -157,6 +157,16 @@ message Http1ProtocolOptions { // - Not a response to a HEAD request. // - The content length header is not present. bool enable_trailers = 5; + + // Allows Envoy to process requests/responses with both `Content-Length` and `Transfer-Encoding` + // headers set. By default such messages are rejected, but if option is enabled - Envoy will + // remove Content-Length header and process message. + // See `RFC7230, sec. 3.3.3 ` for details. + // + // .. attention:: + // Enabling this option might lead to request smuggling vulnerability, especially if traffic + // is proxied via multiple layers of proxies. + bool allow_chunked_length = 6; } // [#next-free-field: 15] diff --git a/include/envoy/http/codec.h b/include/envoy/http/codec.h index ccc04af094a6..23d94197585a 100644 --- a/include/envoy/http/codec.h +++ b/include/envoy/http/codec.h @@ -374,6 +374,10 @@ struct Http1Settings { // - Is neither a HEAD only request nor a HTTP Upgrade // - Not a HEAD request bool enable_trailers_{false}; + // Allows Envoy to process requests/responses with both `Content-Length` and `Transfer-Encoding` + // headers set. By default such messages are rejected, but if option is enabled - Envoy will + // remove Content-Length header and process message. + bool allow_chunked_length_{false}; enum class HeaderKeyFormat { // By default no formatting is performed, presenting all headers in lowercase (as Envoy diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index 3c4fde96ebe1..0ef6c9bef322 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -42,6 +42,7 @@ struct Http1ResponseCodeDetailValues { const absl::string_view TransferEncodingNotAllowed = "http1.transfer_encoding_not_allowed"; const absl::string_view ContentLengthNotAllowed = "http1.content_length_not_allowed"; const absl::string_view InvalidUnderscore = "http1.unexpected_underscore"; + const absl::string_view ChunkedContentLength = "http1.content_length_and_chunked_not_allowed"; }; struct Http1HeaderTypesValues { @@ -471,13 +472,12 @@ http_parser_settings ConnectionImpl::settings_{ }; ConnectionImpl::ConnectionImpl(Network::Connection& connection, CodecStats& stats, - http_parser_type type, uint32_t max_headers_kb, - const uint32_t max_headers_count, - HeaderKeyFormatterPtr&& header_key_formatter, bool enable_trailers) - : connection_(connection), stats_(stats), + const Http1Settings& settings, http_parser_type type, + uint32_t max_headers_kb, const uint32_t max_headers_count, + HeaderKeyFormatterPtr&& header_key_formatter) + : connection_(connection), stats_(stats), codec_settings_(settings), header_key_formatter_(std::move(header_key_formatter)), processing_trailers_(false), handling_upgrade_(false), reset_stream_called_(false), deferred_end_stream_headers_(false), - enable_trailers_(enable_trailers), strict_1xx_and_204_headers_(Runtime::runtimeFeatureEnabled( "envoy.reloadable_features.strict_1xx_and_204_response_headers")), dispatching_(false), @@ -487,6 +487,7 @@ ConnectionImpl::ConnectionImpl(Network::Connection& connection, CodecStats& stat max_headers_kb_(max_headers_kb), max_headers_count_(max_headers_count) { output_buffer_.setWatermarks(connection.bufferLimit()); http_parser_init(&parser_, type); + parser_.allow_chunked_length = 1; parser_.data = this; } @@ -632,7 +633,7 @@ Status ConnectionImpl::onHeaderField(const char* data, size_t length) { // We previously already finished up the headers, these headers are // now trailers. if (header_parsing_state_ == HeaderParsingState::Done) { - if (!enable_trailers_) { + if (!enableTrailers()) { // Ignore trailers. return okStatus(); } @@ -651,7 +652,7 @@ Status ConnectionImpl::onHeaderField(const char* data, size_t length) { Status ConnectionImpl::onHeaderValue(const char* data, size_t length) { ASSERT(dispatching_); - if (header_parsing_state_ == HeaderParsingState::Done && !enable_trailers_) { + if (header_parsing_state_ == HeaderParsingState::Done && !enableTrailers()) { // Ignore trailers. return okStatus(); } @@ -728,6 +729,29 @@ Envoy::StatusOr ConnectionImpl::onHeadersCompleteBase() { handling_upgrade_ = true; } + // https://tools.ietf.org/html/rfc7230#section-3.3.3 + // If a message is received with both a Transfer-Encoding and a + // Content-Length header field, the Transfer-Encoding overrides the + // Content-Length. Such a message might indicate an attempt to + // perform request smuggling (Section 9.5) or response splitting + // (Section 9.4) and ought to be handled as an error. A sender MUST + // remove the received Content-Length field prior to forwarding such + // a message. + + // Reject message with Http::Code::BadRequest if both Transfer-Encoding and Content-Length + // headers are present or if allowed by http1 codec settings and 'Transfer-Encoding' + // is chunked - remove Content-Length and serve request. + if (parser_.uses_transfer_encoding != 0 && request_or_response_headers.ContentLength()) { + if ((parser_.flags & F_CHUNKED) && codec_settings_.allow_chunked_length_) { + request_or_response_headers.removeContentLength(); + } else { + error_code_ = Http::Code::BadRequest; + RETURN_IF_ERROR(sendProtocolError(Http1ResponseCodeDetails::get().ChunkedContentLength)); + return codecProtocolError( + "http/1.1 protocol error: both 'Content-Length' and 'Transfer-Encoding' are set."); + } + } + // Per https://tools.ietf.org/html/rfc7230#section-3.3.1 Envoy should reject // transfer-codings it does not understand. // Per https://tools.ietf.org/html/rfc7231#section-4.3.6 a payload with a @@ -822,9 +846,9 @@ ServerConnectionImpl::ServerConnectionImpl( const uint32_t max_request_headers_count, envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction headers_with_underscores_action) - : ConnectionImpl(connection, stats, HTTP_REQUEST, max_request_headers_kb, - max_request_headers_count, formatter(settings), settings.enable_trailers_), - callbacks_(callbacks), codec_settings_(settings), + : ConnectionImpl(connection, stats, settings, HTTP_REQUEST, max_request_headers_kb, + max_request_headers_count, formatter(settings)), + callbacks_(callbacks), response_buffer_releasor_([this](const Buffer::OwnedBufferFragmentImpl* fragment) { releaseOutboundResponse(fragment); }), @@ -1123,15 +1147,16 @@ Status ServerConnectionImpl::checkHeaderNameForUnderscores() { ClientConnectionImpl::ClientConnectionImpl(Network::Connection& connection, CodecStats& stats, ConnectionCallbacks&, const Http1Settings& settings, const uint32_t max_response_headers_count) - : ConnectionImpl(connection, stats, HTTP_RESPONSE, MAX_RESPONSE_HEADERS_KB, - max_response_headers_count, formatter(settings), settings.enable_trailers_) {} + : ConnectionImpl(connection, stats, settings, HTTP_RESPONSE, MAX_RESPONSE_HEADERS_KB, + max_response_headers_count, formatter(settings)) {} bool ClientConnectionImpl::cannotHaveBody() { if (pending_response_.has_value() && pending_response_.value().encoder_.headRequest()) { ASSERT(!pending_response_done_); return true; } else if (parser_.status_code == 204 || parser_.status_code == 304 || - (parser_.status_code >= 200 && parser_.content_length == 0)) { + (parser_.status_code >= 200 && parser_.content_length == 0 && + !(parser_.flags & F_CHUNKED))) { return true; } else { return false; diff --git a/source/common/http/http1/codec_impl.h b/source/common/http/http1/codec_impl.h index 3d2bd4e59cce..16575bf9b98f 100644 --- a/source/common/http/http1/codec_impl.h +++ b/source/common/http/http1/codec_impl.h @@ -204,7 +204,7 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable active_request_; - Http1Settings codec_settings_; const Buffer::OwnedBufferFragmentImpl::Releasor response_buffer_releasor_; uint32_t outbound_responses_{}; // This defaults to 2, which functionally disables pipelining. If any users @@ -558,7 +557,6 @@ class ClientConnectionImpl : public ClientConnection, public ConnectionImpl { ClientConnectionImpl(Network::Connection& connection, CodecStats& stats, ConnectionCallbacks& callbacks, const Http1Settings& settings, const uint32_t max_response_headers_count); - // Http::ClientConnection RequestEncoder& newStream(ResponseDecoder& response_decoder) override; diff --git a/source/common/http/http1/codec_impl_legacy.cc b/source/common/http/http1/codec_impl_legacy.cc index f6ed38e70c62..b0cc69da3220 100644 --- a/source/common/http/http1/codec_impl_legacy.cc +++ b/source/common/http/http1/codec_impl_legacy.cc @@ -41,6 +41,7 @@ struct Http1ResponseCodeDetailValues { const absl::string_view TransferEncodingNotAllowed = "http1.transfer_encoding_not_allowed"; const absl::string_view ContentLengthNotAllowed = "http1.content_length_not_allowed"; const absl::string_view InvalidUnderscore = "http1.unexpected_underscore"; + const absl::string_view ChunkedContentLength = "http1.content_length_and_chunked_not_allowed"; }; struct Http1HeaderTypesValues { @@ -447,13 +448,12 @@ http_parser_settings ConnectionImpl::settings_{ }; ConnectionImpl::ConnectionImpl(Network::Connection& connection, CodecStats& stats, - http_parser_type type, uint32_t max_headers_kb, - const uint32_t max_headers_count, - HeaderKeyFormatterPtr&& header_key_formatter, bool enable_trailers) - : connection_(connection), stats_(stats), + const Http1Settings& settings, http_parser_type type, + uint32_t max_headers_kb, const uint32_t max_headers_count, + HeaderKeyFormatterPtr&& header_key_formatter) + : connection_(connection), stats_(stats), codec_settings_(settings), header_key_formatter_(std::move(header_key_formatter)), processing_trailers_(false), handling_upgrade_(false), reset_stream_called_(false), deferred_end_stream_headers_(false), - enable_trailers_(enable_trailers), strict_1xx_and_204_headers_(Runtime::runtimeFeatureEnabled( "envoy.reloadable_features.strict_1xx_and_204_response_headers")), output_buffer_([&]() -> void { this->onBelowLowWatermark(); }, @@ -462,6 +462,7 @@ ConnectionImpl::ConnectionImpl(Network::Connection& connection, CodecStats& stat max_headers_kb_(max_headers_kb), max_headers_count_(max_headers_count) { output_buffer_.setWatermarks(connection.bufferLimit()); http_parser_init(&parser_, type); + parser_.allow_chunked_length = 1; parser_.data = this; } @@ -583,7 +584,7 @@ void ConnectionImpl::onHeaderField(const char* data, size_t length) { // We previously already finished up the headers, these headers are // now trailers. if (header_parsing_state_ == HeaderParsingState::Done) { - if (!enable_trailers_) { + if (!enableTrailers()) { // Ignore trailers. return; } @@ -601,7 +602,7 @@ void ConnectionImpl::onHeaderField(const char* data, size_t length) { } void ConnectionImpl::onHeaderValue(const char* data, size_t length) { - if (header_parsing_state_ == HeaderParsingState::Done && !enable_trailers_) { + if (header_parsing_state_ == HeaderParsingState::Done && !enableTrailers()) { // Ignore trailers. return; } @@ -677,6 +678,29 @@ int ConnectionImpl::onHeadersCompleteBase() { handling_upgrade_ = true; } + // https://tools.ietf.org/html/rfc7230#section-3.3.3 + // If a message is received with both a Transfer-Encoding and a + // Content-Length header field, the Transfer-Encoding overrides the + // Content-Length. Such a message might indicate an attempt to + // perform request smuggling (Section 9.5) or response splitting + // (Section 9.4) and ought to be handled as an error. A sender MUST + // remove the received Content-Length field prior to forwarding such + // a message. + + // Reject message with Http::Code::BadRequest if both Transfer-Encoding and Content-Length + // headers are present or if allowed by http1 codec settings and 'Transfer-Encoding' + // is chunked - remove Content-Length and serve request. + if (parser_.uses_transfer_encoding != 0 && request_or_response_headers.ContentLength()) { + if ((parser_.flags & F_CHUNKED) && codec_settings_.allow_chunked_length_) { + request_or_response_headers.removeContentLength(); + } else { + error_code_ = Http::Code::BadRequest; + sendProtocolError(Http1ResponseCodeDetails::get().ChunkedContentLength); + throw CodecProtocolException( + "http/1.1 protocol error: both 'Content-Length' and 'Transfer-Encoding' are set."); + } + } + // Per https://tools.ietf.org/html/rfc7230#section-3.3.1 Envoy should reject // transfer-codings it does not understand. // Per https://tools.ietf.org/html/rfc7231#section-4.3.6 a payload with a @@ -765,9 +789,9 @@ ServerConnectionImpl::ServerConnectionImpl( const uint32_t max_request_headers_count, envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction headers_with_underscores_action) - : ConnectionImpl(connection, stats, HTTP_REQUEST, max_request_headers_kb, - max_request_headers_count, formatter(settings), settings.enable_trailers_), - callbacks_(callbacks), codec_settings_(settings), + : ConnectionImpl(connection, stats, settings, HTTP_REQUEST, max_request_headers_kb, + max_request_headers_count, formatter(settings)), + callbacks_(callbacks), response_buffer_releasor_([this](const Buffer::OwnedBufferFragmentImpl* fragment) { releaseOutboundResponse(fragment); }), @@ -1058,15 +1082,16 @@ void ServerConnectionImpl::checkHeaderNameForUnderscores() { ClientConnectionImpl::ClientConnectionImpl(Network::Connection& connection, CodecStats& stats, ConnectionCallbacks&, const Http1Settings& settings, const uint32_t max_response_headers_count) - : ConnectionImpl(connection, stats, HTTP_RESPONSE, MAX_RESPONSE_HEADERS_KB, - max_response_headers_count, formatter(settings), settings.enable_trailers_) {} + : ConnectionImpl(connection, stats, settings, HTTP_RESPONSE, MAX_RESPONSE_HEADERS_KB, + max_response_headers_count, formatter(settings)) {} bool ClientConnectionImpl::cannotHaveBody() { if (pending_response_.has_value() && pending_response_.value().encoder_.headRequest()) { ASSERT(!pending_response_done_); return true; } else if (parser_.status_code == 204 || parser_.status_code == 304 || - (parser_.status_code >= 200 && parser_.content_length == 0)) { + (parser_.status_code >= 200 && parser_.content_length == 0 && + !(parser_.flags & F_CHUNKED))) { return true; } else { return false; diff --git a/source/common/http/http1/codec_impl_legacy.h b/source/common/http/http1/codec_impl_legacy.h index b553c5f9f9c7..642c504a2258 100644 --- a/source/common/http/http1/codec_impl_legacy.h +++ b/source/common/http/http1/codec_impl_legacy.h @@ -208,7 +208,7 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable active_request_; - Http1Settings codec_settings_; const Buffer::OwnedBufferFragmentImpl::Releasor response_buffer_releasor_; uint32_t outbound_responses_{}; // This defaults to 2, which functionally disables pipelining. If any users diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index 28e8872851cd..2e73b6e443c4 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -404,6 +404,7 @@ Utility::parseHttp1Settings(const envoy::config::core::v3::Http1ProtocolOptions& ret.accept_http_10_ = config.accept_http_10(); ret.default_host_for_http_10_ = config.default_host_for_http_10(); ret.enable_trailers_ = config.enable_trailers(); + ret.allow_chunked_length_ = config.allow_chunked_length(); if (config.header_key_format().has_proper_case_words()) { ret.header_key_format_ = Http1Settings::HeaderKeyFormat::ProperCase; diff --git a/test/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc index f6da689eacd6..9cd59c74c5c0 100644 --- a/test/common/http/http1/codec_impl_test.cc +++ b/test/common/http/http1/codec_impl_test.cc @@ -97,6 +97,8 @@ class Http1ServerConnectionImplTest : public Http1CodecTestBase, // Used to test if trailers are decoded/encoded void expectTrailersTest(bool enable_trailers); + void testServerAllowChunkedContentLength(uint32_t content_length, bool allow_chunked_length); + // Send the request, and validate the received request headers. // Then send a response just to clean up. void @@ -328,6 +330,65 @@ void Http1ServerConnectionImplTest::testRequestHeadersAccepted(std::string heade EXPECT_TRUE(status.ok()); } +void Http1ServerConnectionImplTest::testServerAllowChunkedContentLength(uint32_t content_length, + bool allow_chunked_length) { + codec_settings_.allow_chunked_length_ = allow_chunked_length; + if (testingNewCodec()) { + codec_ = std::make_unique( + connection_, http1CodecStats(), callbacks_, codec_settings_, max_request_headers_kb_, + max_request_headers_count_, envoy::config::core::v3::HttpProtocolOptions::ALLOW); + } else { + codec_ = std::make_unique( + connection_, http1CodecStats(), callbacks_, codec_settings_, max_request_headers_kb_, + max_request_headers_count_, envoy::config::core::v3::HttpProtocolOptions::ALLOW); + } + + MockRequestDecoder decoder; + Http::ResponseEncoder* response_encoder = nullptr; + + EXPECT_CALL(callbacks_, newStream(_, _)) + .WillOnce(Invoke([&](ResponseEncoder& encoder, bool) -> RequestDecoder& { + response_encoder = &encoder; + return decoder; + })); + + TestRequestHeaderMapImpl expected_headers{ + {":path", "/"}, + {":method", "POST"}, + {"transfer-encoding", "chunked"}, + }; + Buffer::OwnedImpl expected_data("Hello World"); + + if (allow_chunked_length) { + EXPECT_CALL(decoder, decodeHeaders_(HeaderMapEqual(&expected_headers), false)); + EXPECT_CALL(decoder, decodeData(BufferEqual(&expected_data), false)); + EXPECT_CALL(decoder, decodeData(_, true)); + } else { + EXPECT_CALL(decoder, decodeHeaders_(_, _)).Times(0); + EXPECT_CALL(decoder, decodeData(_, _)).Times(0); + EXPECT_CALL(decoder, sendLocalReply(false, Http::Code::BadRequest, "Bad Request", _, _, _)); + } + + Buffer::OwnedImpl buffer( + fmt::format("POST / HTTP/1.1\r\ntransfer-encoding: chunked\r\ncontent-length: {}\r\n\r\n" + "6\r\nHello \r\n" + "5\r\nWorld\r\n" + "0\r\n\r\n", + content_length)); + + auto status = codec_->dispatch(buffer); + + if (allow_chunked_length) { + EXPECT_TRUE(status.ok()); + } else { + EXPECT_TRUE(isCodecProtocolError(status)); + EXPECT_EQ(status.message(), + "http/1.1 protocol error: both 'Content-Length' and 'Transfer-Encoding' are set."); + EXPECT_EQ("http1.content_length_and_chunked_not_allowed", + response_encoder->getStream().responseDetails()); + } +} + INSTANTIATE_TEST_SUITE_P(Codecs, Http1ServerConnectionImplTest, testing::Bool(), [](const testing::TestParamInfo& param) { return param.param ? "New" : "Legacy"; @@ -1840,6 +1901,30 @@ TEST_P(Http1ServerConnectionImplTest, WatermarkTest) { ->onUnderlyingConnectionBelowWriteBufferLowWatermark(); } +TEST_P(Http1ServerConnectionImplTest, TestSmugglingDisallowChunkedContentLength0) { + testServerAllowChunkedContentLength(0, false); +} +TEST_P(Http1ServerConnectionImplTest, TestSmugglingDisallowChunkedContentLength1) { + // content-length less than POST body size + testServerAllowChunkedContentLength(1, false); +} +TEST_P(Http1ServerConnectionImplTest, TestSmugglingDisallowChunkedContentLength100) { + // content-length greater than POST body size + testServerAllowChunkedContentLength(100, false); +} + +TEST_P(Http1ServerConnectionImplTest, TestSmugglingAllowChunkedContentLength0) { + testServerAllowChunkedContentLength(0, true); +} +TEST_P(Http1ServerConnectionImplTest, TestSmugglingAllowChunkedContentLength1) { + // content-length less than POST body size + testServerAllowChunkedContentLength(1, true); +} +TEST_P(Http1ServerConnectionImplTest, TestSmugglingAllowChunkedContentLength100) { + // content-length greater than POST body size + testServerAllowChunkedContentLength(100, true); +} + class Http1ClientConnectionImplTest : public Http1CodecTestBase, public testing::TestWithParam { public: @@ -1868,6 +1953,8 @@ class Http1ClientConnectionImplTest : public Http1CodecTestBase, NiceMock codec_settings_; Http::ClientConnectionPtr codec_; + void testClientAllowChunkedContentLength(uint32_t content_length, bool allow_chunked_length); + protected: Stats::TestUtil::TestStore store_; uint32_t max_response_headers_count_{Http::DEFAULT_MAX_HEADERS_COUNT}; @@ -1878,6 +1965,52 @@ INSTANTIATE_TEST_SUITE_P(Codecs, Http1ClientConnectionImplTest, testing::Bool(), return param.param ? "New" : "Legacy"; }); +void Http1ClientConnectionImplTest::testClientAllowChunkedContentLength(uint32_t content_length, + bool allow_chunked_length) { + codec_settings_.allow_chunked_length_ = allow_chunked_length; + if (testingNewCodec()) { + codec_ = std::make_unique( + connection_, http1CodecStats(), callbacks_, codec_settings_, max_response_headers_count_); + } else { + codec_ = std::make_unique( + connection_, http1CodecStats(), callbacks_, codec_settings_, max_response_headers_count_); + } + + NiceMock response_decoder; + Http::RequestEncoder& request_encoder = codec_->newStream(response_decoder); + + TestRequestHeaderMapImpl headers{{":method", "GET"}, {":path", "/"}, {":authority", "host"}}; + request_encoder.encodeHeaders(headers, true); + + TestResponseHeaderMapImpl expected_headers{{":status", "200"}, {"transfer-encoding", "chunked"}}; + Buffer::OwnedImpl expected_data("Hello World"); + + if (allow_chunked_length) { + EXPECT_CALL(response_decoder, decodeHeaders_(HeaderMapEqual(&expected_headers), false)); + EXPECT_CALL(response_decoder, decodeData(BufferEqual(&expected_data), false)); + EXPECT_CALL(response_decoder, decodeData(_, true)); + } else { + EXPECT_CALL(response_decoder, decodeHeaders_(_, _)).Times(0); + EXPECT_CALL(response_decoder, decodeData(_, _)).Times(0); + } + + Buffer::OwnedImpl buffer( + fmt::format("HTTP/1.1 200 OK\r\ntransfer-encoding: chunked\r\ncontent-length: {}\r\n\r\n" + "6\r\nHello \r\n" + "5\r\nWorld\r\n" + "0\r\n\r\n", + content_length)); + auto status = codec_->dispatch(buffer); + + if (allow_chunked_length) { + EXPECT_TRUE(status.ok()); + } else { + EXPECT_TRUE(isCodecProtocolError(status)); + EXPECT_EQ(status.message(), + "http/1.1 protocol error: both 'Content-Length' and 'Transfer-Encoding' are set."); + }; +} + TEST_P(Http1ClientConnectionImplTest, SimpleGet) { initialize(); @@ -2841,5 +2974,29 @@ TEST_P(Http1ClientConnectionImplTest, ManyResponseHeadersAccepted) { status = codec_->dispatch(buffer); } +TEST_P(Http1ClientConnectionImplTest, TestResponseSplit0) { + testClientAllowChunkedContentLength(0, false); +} + +TEST_P(Http1ClientConnectionImplTest, TestResponseSplit1) { + testClientAllowChunkedContentLength(1, false); +} + +TEST_P(Http1ClientConnectionImplTest, TestResponseSplit100) { + testClientAllowChunkedContentLength(100, false); +} + +TEST_P(Http1ClientConnectionImplTest, TestResponseSplitAllowChunkedLength0) { + testClientAllowChunkedContentLength(0, true); +} + +TEST_P(Http1ClientConnectionImplTest, TestResponseSplitAllowChunkedLength1) { + testClientAllowChunkedContentLength(1, true); +} + +TEST_P(Http1ClientConnectionImplTest, TestResponseSplitAllowChunkedLength100) { + testClientAllowChunkedContentLength(100, true); +} + } // namespace Http } // namespace Envoy diff --git a/test/integration/integration_test.cc b/test/integration/integration_test.cc index e968016cee1b..d4fee8815495 100644 --- a/test/integration/integration_test.cc +++ b/test/integration/integration_test.cc @@ -429,6 +429,78 @@ TEST_P(IntegrationTest, TestSmuggling) { } } +TEST_P(IntegrationTest, TestServerAllowChunkedLength) { + config_helper_.addConfigModifier( + [&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& + hcm) -> void { + hcm.mutable_http_protocol_options()->set_allow_chunked_length(true); + }); + initialize(); + + auto tcp_client = makeTcpConnection(lookupPort("http")); + ASSERT_TRUE(tcp_client->write("POST / HTTP/1.1\r\n" + "Host: host\r\n" + "Content-length: 100\r\n" + "Transfer-Encoding: chunked\r\n\r\n" + "4\r\nbody\r\n" + "0\r\n\r\n")); + + FakeRawConnectionPtr fake_upstream_connection; + ASSERT_TRUE(fake_upstreams_[0]->waitForRawConnection(fake_upstream_connection)); + std::string data; + ASSERT_TRUE(fake_upstream_connection->waitForData( + FakeRawConnection::waitForInexactMatch("\r\n\r\n"), &data)); + + ASSERT_THAT(data, HasSubstr("POST / HTTP/1.1")); + ASSERT_THAT(data, HasSubstr("transfer-encoding: chunked")); + // verify no 'content-length' header + ASSERT_THAT(data, Not(HasSubstr("ontent-length"))); + + ASSERT_TRUE( + fake_upstream_connection->write("HTTP/1.1 200 OK\r\nTransfer-encoding: chunked\r\n\r\n")); + ASSERT_TRUE(fake_upstream_connection->close()); + ASSERT_TRUE(fake_upstream_connection->waitForDisconnect()); + tcp_client->close(); +} + +TEST_P(IntegrationTest, TestClientAllowChunkedLength) { + config_helper_.addConfigModifier([&](envoy::config::bootstrap::v3::Bootstrap& bootstrap) -> void { + RELEASE_ASSERT(bootstrap.mutable_static_resources()->clusters_size() == 1, ""); + if (fake_upstreams_[0]->httpType() == FakeHttpConnection::Type::HTTP1) { + auto* cluster = bootstrap.mutable_static_resources()->mutable_clusters(0); + cluster->mutable_http_protocol_options()->set_allow_chunked_length(true); + } + }); + + initialize(); + + auto tcp_client = makeTcpConnection(lookupPort("http")); + ASSERT_TRUE(tcp_client->write("GET / HTTP/1.1\r\nHost: host\r\n\r\n")); + + FakeRawConnectionPtr fake_upstream_connection; + ASSERT_TRUE(fake_upstreams_[0]->waitForRawConnection(fake_upstream_connection)); + std::string data; + ASSERT_TRUE(fake_upstream_connection->waitForData( + FakeRawConnection::waitForInexactMatch("\r\n\r\n"), &data)); + + ASSERT_TRUE(fake_upstream_connection->write("HTTP/1.1 200 OK\r\n" + "Transfer-encoding: chunked\r\n" + "Content-Length: 100\r\n\r\n" + "4\r\nbody\r\n" + "0\r\n\r\n")); + tcp_client->waitForData("\r\n\r\n", false); + std::string response = tcp_client->data(); + + EXPECT_THAT(response, HasSubstr("HTTP/1.1 200 OK\r\n")); + EXPECT_THAT(response, Not(HasSubstr("content-length"))); + EXPECT_THAT(response, HasSubstr("transfer-encoding: chunked\r\n")); + EXPECT_THAT(response, EndsWith("\r\n\r\n")); + + ASSERT_TRUE(fake_upstream_connection->close()); + ASSERT_TRUE(fake_upstream_connection->waitForDisconnect()); + tcp_client->close(); +} + TEST_P(IntegrationTest, BadFirstline) { initialize(); std::string response;