diff --git a/envoy/http/codec.h b/envoy/http/codec.h index 7445fb20eec5..d116c57cba1b 100644 --- a/envoy/http/codec.h +++ b/envoy/http/codec.h @@ -46,6 +46,14 @@ const char MaxResponseHeadersCountOverrideKey[] = class Stream; +/** + * Header validation mode that codecs should use internally. + */ +enum class CodecHeaderValidationMode { + Enabled, + Disabled, +}; + /** * Error codes used to convey the reason for a GOAWAY. */ diff --git a/source/common/http/codec_client.cc b/source/common/http/codec_client.cc index 64dcc55a2356..f60c8d13733f 100644 --- a/source/common/http/codec_client.cc +++ b/source/common/http/codec_client.cc @@ -183,7 +183,7 @@ NoConnectCodecClientProd::NoConnectCodecClientProd(CodecType type, case CodecType::HTTP1: { codec_ = std::make_unique( *connection_, host->cluster().http1CodecStats(), *this, host->cluster().http1Settings(), - host->cluster().maxResponseHeadersCount()); + host->cluster().maxResponseHeadersCount(), CodecHeaderValidationMode::Enabled); break; } case CodecType::HTTP2: { diff --git a/source/common/http/conn_manager_utility.cc b/source/common/http/conn_manager_utility.cc index c783760b84b4..8d8809b9e040 100644 --- a/source/common/http/conn_manager_utility.cc +++ b/source/common/http/conn_manager_utility.cc @@ -70,7 +70,8 @@ ServerConnectionPtr ConnectionManagerUtility::autoCreateCodec( Http1::CodecStats& stats = Http1::CodecStats::atomicGet(http1_codec_stats, scope); return std::make_unique( connection, stats, callbacks, http1_settings, max_request_headers_kb, - max_request_headers_count, headers_with_underscores_action); + max_request_headers_count, headers_with_underscores_action, + CodecHeaderValidationMode::Enabled); } } diff --git a/source/common/http/http1/BUILD b/source/common/http/http1/BUILD index e801968ec533..8be4e715befe 100644 --- a/source/common/http/http1/BUILD +++ b/source/common/http/http1/BUILD @@ -113,6 +113,7 @@ envoy_cc_library( external_deps = ["http_parser"], deps = [ ":parser_interface", + "//envoy/http:codec_interface", "//source/common/common:assert_lib", ], ) diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index 98e290b99259..dfceeab01420 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -472,7 +472,8 @@ int ConnectionImpl::setAndCheckCallbackStatusOr(Envoy::StatusOr&& ConnectionImpl::ConnectionImpl(Network::Connection& connection, CodecStats& stats, const Http1Settings& settings, MessageType type, - uint32_t max_headers_kb, const uint32_t max_headers_count) + uint32_t max_headers_kb, const uint32_t max_headers_count, + const CodecHeaderValidationMode codec_header_validation_mode) : connection_(connection), stats_(stats), codec_settings_(settings), encode_only_header_key_formatter_(encodeOnlyFormatterFromSettings(settings)), processing_trailers_(false), handling_upgrade_(false), reset_stream_called_(false), @@ -483,7 +484,7 @@ ConnectionImpl::ConnectionImpl(Network::Connection& connection, CodecStats& stat []() -> void { /* TODO(adisuissa): Handle overflow watermark */ })), max_headers_kb_(max_headers_kb), max_headers_count_(max_headers_count) { output_buffer_->setWatermarks(connection.bufferLimit()); - parser_ = std::make_unique(type, this); + parser_ = std::make_unique(type, this, codec_header_validation_mode); } Status ConnectionImpl::completeLastHeader() { @@ -957,9 +958,10 @@ ServerConnectionImpl::ServerConnectionImpl( const Http1Settings& settings, uint32_t max_request_headers_kb, const uint32_t max_request_headers_count, envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction - headers_with_underscores_action) + headers_with_underscores_action, + const CodecHeaderValidationMode codec_header_validation_mode) : ConnectionImpl(connection, stats, settings, MessageType::Request, max_request_headers_kb, - max_request_headers_count), + max_request_headers_count, codec_header_validation_mode), callbacks_(callbacks), response_buffer_releasor_([this](const Buffer::OwnedBufferFragmentImpl* fragment) { releaseOutboundResponse(fragment); @@ -1241,11 +1243,12 @@ void ServerConnectionImpl::ActiveRequest::dumpState(std::ostream& os, int indent os << DUMP_MEMBER(response_encoder_.local_end_stream_); } -ClientConnectionImpl::ClientConnectionImpl(Network::Connection& connection, CodecStats& stats, - ConnectionCallbacks&, const Http1Settings& settings, - const uint32_t max_response_headers_count) +ClientConnectionImpl::ClientConnectionImpl( + Network::Connection& connection, CodecStats& stats, ConnectionCallbacks&, + const Http1Settings& settings, const uint32_t max_response_headers_count, + const CodecHeaderValidationMode codec_header_validation_mode) : ConnectionImpl(connection, stats, settings, MessageType::Response, MAX_RESPONSE_HEADERS_KB, - max_response_headers_count) {} + max_response_headers_count, codec_header_validation_mode) {} bool ClientConnectionImpl::cannotHaveBody() { if (pending_response_.has_value() && pending_response_.value().encoder_.headRequest()) { diff --git a/source/common/http/http1/codec_impl.h b/source/common/http/http1/codec_impl.h index e1e508747fa0..f21912830115 100644 --- a/source/common/http/http1/codec_impl.h +++ b/source/common/http/http1/codec_impl.h @@ -254,7 +254,8 @@ class ConnectionImpl : public virtual Connection, protected: ConnectionImpl(Network::Connection& connection, CodecStats& stats, const Http1Settings& settings, - MessageType type, uint32_t max_headers_kb, const uint32_t max_headers_count); + MessageType type, uint32_t max_headers_kb, const uint32_t max_headers_count, + CodecHeaderValidationMode codec_header_validation_mode); bool resetStreamCalled() { return reset_stream_called_; } @@ -436,7 +437,8 @@ class ServerConnectionImpl : public ServerConnection, public ConnectionImpl { ServerConnectionCallbacks& callbacks, const Http1Settings& settings, uint32_t max_request_headers_kb, const uint32_t max_request_headers_count, envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction - headers_with_underscores_action); + headers_with_underscores_action, + const CodecHeaderValidationMode codec_header_validation_mode); bool supportsHttp10() override { return codec_settings_.accept_http_10_; } protected: @@ -549,7 +551,8 @@ class ClientConnectionImpl : public ClientConnection, public ConnectionImpl { public: ClientConnectionImpl(Network::Connection& connection, CodecStats& stats, ConnectionCallbacks& callbacks, const Http1Settings& settings, - const uint32_t max_response_headers_count); + const uint32_t max_response_headers_count, + const CodecHeaderValidationMode codec_header_validation_mode); // Http::ClientConnection RequestEncoder& newStream(ResponseDecoder& response_decoder) override; diff --git a/source/common/http/http1/legacy_parser_impl.cc b/source/common/http/http1/legacy_parser_impl.cc index 02f9a5700e7f..c6b9d1b0598f 100644 --- a/source/common/http/http1/legacy_parser_impl.cc +++ b/source/common/http/http1/legacy_parser_impl.cc @@ -38,7 +38,11 @@ class LegacyHttpParserImpl::Impl { parser_.allow_chunked_length = 1; } - Impl(http_parser_type type, void* data) : Impl(type) { + Impl(http_parser_type type, void* data, + const Http::CodecHeaderValidationMode codec_header_validation_mode) + : Impl(type) { + unsigned permissive_parsing = + codec_header_validation_mode == Http::CodecHeaderValidationMode::Disabled ? 1 : 0; parser_.data = data; settings_ = { [](http_parser* parser) -> int { @@ -89,7 +93,8 @@ class LegacyHttpParserImpl::Impl { static_cast(parser->data)->onChunkHeader(is_final_chunk); return 0; }, - nullptr // on_chunk_complete + nullptr, // on_chunk_complete + permissive_parsing // permissive_parsing }; } @@ -136,7 +141,9 @@ class LegacyHttpParserImpl::Impl { http_parser_settings settings_; }; -LegacyHttpParserImpl::LegacyHttpParserImpl(MessageType type, ParserCallbacks* data) { +LegacyHttpParserImpl::LegacyHttpParserImpl( + MessageType type, ParserCallbacks* data, + const Http::CodecHeaderValidationMode codec_header_validation_mode) { http_parser_type parser_type; switch (type) { case MessageType::Request: @@ -147,7 +154,7 @@ LegacyHttpParserImpl::LegacyHttpParserImpl(MessageType type, ParserCallbacks* da break; } - impl_ = std::make_unique(parser_type, data); + impl_ = std::make_unique(parser_type, data, codec_header_validation_mode); } // Because we have a pointer-to-impl using std::unique_ptr, we must place the destructor in the diff --git a/source/common/http/http1/legacy_parser_impl.h b/source/common/http/http1/legacy_parser_impl.h index 2099f815824b..53033ba19904 100644 --- a/source/common/http/http1/legacy_parser_impl.h +++ b/source/common/http/http1/legacy_parser_impl.h @@ -2,6 +2,8 @@ #include +#include "envoy/http/codec.h" + #include "source/common/http/http1/parser.h" namespace Envoy { @@ -10,7 +12,8 @@ namespace Http1 { class LegacyHttpParserImpl : public Parser { public: - LegacyHttpParserImpl(MessageType type, ParserCallbacks* data); + LegacyHttpParserImpl(MessageType type, ParserCallbacks* data, + const CodecHeaderValidationMode codec_header_validation_mode); ~LegacyHttpParserImpl() override; // Http1::Parser diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index be209e012127..518c7649859f 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -333,7 +333,7 @@ bool Utility::Url::initialize(absl::string_view absolute_url, bool is_connect) { struct http_parser_url u; http_parser_url_init(&u); const int result = - http_parser_parse_url(absolute_url.data(), absolute_url.length(), is_connect, &u); + http_parser_parse_url(absolute_url.data(), absolute_url.length(), is_connect, &u, 0); if (result != 0) { return false; diff --git a/source/extensions/filters/listener/http_inspector/http_inspector.cc b/source/extensions/filters/listener/http_inspector/http_inspector.cc index f2c864010abc..82e965e7d6dd 100644 --- a/source/extensions/filters/listener/http_inspector/http_inspector.cc +++ b/source/extensions/filters/listener/http_inspector/http_inspector.cc @@ -28,9 +28,8 @@ Filter::Filter(const ConfigSharedPtr config) : config_(config) { http_parser_init(&parser_, HTTP_REQUEST); } -http_parser_settings Filter::settings_{ - nullptr, nullptr, nullptr, nullptr, nullptr, nullptr, nullptr, nullptr, nullptr, nullptr, -}; +http_parser_settings Filter::settings_{nullptr, nullptr, nullptr, nullptr, nullptr, nullptr, + nullptr, nullptr, nullptr, nullptr, 0}; Network::FilterStatus Filter::onAccept(Network::ListenerFilterCallbacks& cb) { ENVOY_LOG(debug, "http inspector: new connection accepted"); diff --git a/source/extensions/filters/network/http_connection_manager/config.cc b/source/extensions/filters/network/http_connection_manager/config.cc index 275997caee78..6cd966945cc0 100644 --- a/source/extensions/filters/network/http_connection_manager/config.cc +++ b/source/extensions/filters/network/http_connection_manager/config.cc @@ -696,7 +696,7 @@ HttpConnectionManagerConfig::createCodec(Network::Connection& connection, return std::make_unique( connection, Http::Http1::CodecStats::atomicGet(http1_codec_stats_, context_.scope()), callbacks, http1_settings_, maxRequestHeadersKb(), maxRequestHeadersCount(), - headersWithUnderscoresAction()); + headersWithUnderscoresAction(), Http::CodecHeaderValidationMode::Enabled); } case CodecType::HTTP2: { return std::make_unique( diff --git a/test/common/http/codec_impl_fuzz_test.cc b/test/common/http/codec_impl_fuzz_test.cc index a9ad2219ff59..316ad4eb7392 100644 --- a/test/common/http/codec_impl_fuzz_test.cc +++ b/test/common/http/codec_impl_fuzz_test.cc @@ -509,7 +509,7 @@ void codecFuzz(const test::common::http::CodecImplFuzzTestCase& input, HttpVersi } else { client = std::make_unique( client_connection, Http1::CodecStats::atomicGet(http1_stats, stats_store), client_callbacks, - client_http1settings, max_response_headers_count); + client_http1settings, max_response_headers_count, CodecHeaderValidationMode::Enabled); } if (http2) { @@ -524,7 +524,7 @@ void codecFuzz(const test::common::http::CodecImplFuzzTestCase& input, HttpVersi server = std::make_unique( server_connection, Http1::CodecStats::atomicGet(http1_stats, stats_store), server_callbacks, server_http1settings, max_request_headers_kb, max_request_headers_count, - headers_with_underscores_action); + headers_with_underscores_action, CodecHeaderValidationMode::Enabled); } // We track whether the connection should be closed for HTTP/1, since stream resets imply diff --git a/test/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc index 825b6431c335..1832faa0e116 100644 --- a/test/common/http/http1/codec_impl_test.cc +++ b/test/common/http/http1/codec_impl_test.cc @@ -83,7 +83,8 @@ class Http1ServerConnectionImplTest : public Http1CodecTestBase { void initialize() { codec_ = std::make_unique( connection_, http1CodecStats(), callbacks_, codec_settings_, max_request_headers_kb_, - max_request_headers_count_, headers_with_underscores_action_); + max_request_headers_count_, headers_with_underscores_action_, + codec_header_validation_mode_); } ~Http1ServerConnectionImplTest() override { @@ -96,6 +97,8 @@ class Http1ServerConnectionImplTest : public Http1CodecTestBase { NiceMock callbacks_; NiceMock codec_settings_; Http::ServerConnectionPtr codec_; + Http::CodecHeaderValidationMode codec_header_validation_mode_{ + Http::CodecHeaderValidationMode::Enabled}; void expectHeadersTest(Protocol p, bool allow_absolute_url, Buffer::OwnedImpl& buffer, TestRequestHeaderMapImpl& expected_headers); @@ -153,7 +156,8 @@ void Http1ServerConnectionImplTest::expect400(Protocol p, bool allow_absolute_ur codec_settings_.allow_absolute_url_ = allow_absolute_url; codec_ = std::make_unique( connection_, http1CodecStats(), callbacks_, codec_settings_, max_request_headers_kb_, - max_request_headers_count_, envoy::config::core::v3::HttpProtocolOptions::ALLOW); + max_request_headers_count_, envoy::config::core::v3::HttpProtocolOptions::ALLOW, + CodecHeaderValidationMode::Enabled); } MockRequestDecoder decoder; @@ -183,7 +187,8 @@ void Http1ServerConnectionImplTest::expectHeadersTest(Protocol p, bool allow_abs codec_settings_.allow_absolute_url_ = allow_absolute_url; codec_ = std::make_unique( connection_, http1CodecStats(), callbacks_, codec_settings_, max_request_headers_kb_, - max_request_headers_count_, envoy::config::core::v3::HttpProtocolOptions::ALLOW); + max_request_headers_count_, envoy::config::core::v3::HttpProtocolOptions::ALLOW, + CodecHeaderValidationMode::Enabled); } MockRequestDecoder decoder; @@ -204,7 +209,8 @@ void Http1ServerConnectionImplTest::expectTrailersTest(bool enable_trailers) { codec_settings_.enable_trailers_ = enable_trailers; codec_ = std::make_unique( connection_, http1CodecStats(), callbacks_, codec_settings_, max_request_headers_kb_, - max_request_headers_count_, envoy::config::core::v3::HttpProtocolOptions::ALLOW); + max_request_headers_count_, envoy::config::core::v3::HttpProtocolOptions::ALLOW, + CodecHeaderValidationMode::Enabled); } InSequence sequence; @@ -241,7 +247,8 @@ void Http1ServerConnectionImplTest::testTrailersExceedLimit(std::string trailer_ codec_settings_.enable_trailers_ = enable_trailers; codec_ = std::make_unique( connection_, http1CodecStats(), callbacks_, codec_settings_, max_request_headers_kb_, - max_request_headers_count_, envoy::config::core::v3::HttpProtocolOptions::ALLOW); + max_request_headers_count_, envoy::config::core::v3::HttpProtocolOptions::ALLOW, + CodecHeaderValidationMode::Enabled); std::string exception_reason; NiceMock decoder; EXPECT_CALL(callbacks_, newStream(_, _)) @@ -325,7 +332,8 @@ void Http1ServerConnectionImplTest::testServerAllowChunkedContentLength(uint32_t codec_settings_.allow_chunked_length_ = allow_chunked_length; codec_ = std::make_unique( connection_, http1CodecStats(), callbacks_, codec_settings_, max_request_headers_kb_, - max_request_headers_count_, envoy::config::core::v3::HttpProtocolOptions::ALLOW); + max_request_headers_count_, envoy::config::core::v3::HttpProtocolOptions::ALLOW, + CodecHeaderValidationMode::Enabled); MockRequestDecoder decoder; Http::ResponseEncoder* response_encoder = nullptr; @@ -710,7 +718,8 @@ TEST_F(Http1ServerConnectionImplTest, CodecHasCorrectStreamErrorIfTrue) { codec_settings_.stream_error_on_invalid_http_message_ = true; codec_ = std::make_unique( connection_, http1CodecStats(), callbacks_, codec_settings_, max_request_headers_kb_, - max_request_headers_count_, envoy::config::core::v3::HttpProtocolOptions::ALLOW); + max_request_headers_count_, envoy::config::core::v3::HttpProtocolOptions::ALLOW, + CodecHeaderValidationMode::Enabled); Buffer::OwnedImpl buffer("GET / HTTP/1.1\r\n"); NiceMock decoder; @@ -729,7 +738,8 @@ TEST_F(Http1ServerConnectionImplTest, CodecHasCorrectStreamErrorIfFalse) { codec_settings_.stream_error_on_invalid_http_message_ = false; codec_ = std::make_unique( connection_, http1CodecStats(), callbacks_, codec_settings_, max_request_headers_kb_, - max_request_headers_count_, envoy::config::core::v3::HttpProtocolOptions::ALLOW); + max_request_headers_count_, envoy::config::core::v3::HttpProtocolOptions::ALLOW, + CodecHeaderValidationMode::Enabled); Buffer::OwnedImpl buffer("GET / HTTP/1.1\r\n"); NiceMock decoder; @@ -2127,7 +2137,8 @@ class Http1ClientConnectionImplTest : public Http1CodecTestBase { public: void initialize() { codec_ = std::make_unique( - connection_, http1CodecStats(), callbacks_, codec_settings_, max_response_headers_count_); + connection_, http1CodecStats(), callbacks_, codec_settings_, max_response_headers_count_, + codec_header_validation_mode_); } void readDisableOnRequestEncoder(RequestEncoder* request_encoder, bool disable) { @@ -2138,6 +2149,8 @@ class Http1ClientConnectionImplTest : public Http1CodecTestBase { NiceMock callbacks_; NiceMock codec_settings_; Http::ClientConnectionPtr codec_; + Http::CodecHeaderValidationMode codec_header_validation_mode_{ + Http::CodecHeaderValidationMode::Enabled}; void testClientAllowChunkedContentLength(uint32_t content_length, bool allow_chunked_length); @@ -2150,7 +2163,8 @@ void Http1ClientConnectionImplTest::testClientAllowChunkedContentLength(uint32_t bool allow_chunked_length) { codec_settings_.allow_chunked_length_ = allow_chunked_length; codec_ = std::make_unique( - connection_, http1CodecStats(), callbacks_, codec_settings_, max_response_headers_count_); + connection_, http1CodecStats(), callbacks_, codec_settings_, max_response_headers_count_, + CodecHeaderValidationMode::Enabled); NiceMock response_decoder; Http::RequestEncoder& request_encoder = codec_->newStream(response_decoder); @@ -3006,6 +3020,39 @@ TEST_F(Http1ServerConnectionImplTest, ManyLargeRequestHeadersAccepted) { testRequestHeadersAccepted(createLargeHeaderFragment(64)); } +TEST_F(Http1ServerConnectionImplTest, PermissiveParsing) { + codec_header_validation_mode_ = CodecHeaderValidationMode::Disabled; + initialize(); + + InSequence sequence; + + MockRequestDecoder decoder; + EXPECT_CALL(callbacks_, newStream(_, _)).WillOnce(ReturnRef(decoder)); + + TestRequestHeaderMapImpl expected_headers{ + {":path", "/δ¶/δt/pope?q=1#narf"}, + {":method", "GET"}, + }; + EXPECT_CALL(decoder, decodeHeaders_(HeaderMapEqual(&expected_headers), true)); + + Buffer::OwnedImpl buffer("GET /δ¶/δt/pope?q=1#narf HXXP/1.1\r\n\r\n"); + auto status = codec_->dispatch(buffer); + EXPECT_TRUE(status.ok()); + EXPECT_EQ(0U, buffer.length()); +} + +TEST_F(Http1ServerConnectionImplTest, StrictParsing) { + initialize(); + + MockRequestDecoder decoder; + EXPECT_CALL(callbacks_, newStream(_, _)).WillOnce(ReturnRef(decoder)); + + Buffer::OwnedImpl buffer("GET /δ¶/δt/pope?q=1#narf HXXP/1.1\r\n\r\n"); + EXPECT_CALL(decoder, sendLocalReply(_, _, _, _, _)); + auto status = codec_->dispatch(buffer); + EXPECT_TRUE(isCodecProtocolError(status)); +} + // Tests that incomplete response headers of 80 kB header value fails. TEST_F(Http1ClientConnectionImplTest, ResponseHeadersWithLargeValueRejected) { initialize();