From 2e8ddf3f1821816bfb6ae93352fcc5dcddbb058f Mon Sep 17 00:00:00 2001 From: Dylan Carney Date: Mon, 15 Jul 2019 14:28:01 -0700 Subject: [PATCH] http/1.1 codec: Rejects requests w/ invalid HTTP header values (#7306) Description: This PR addresses the behavior described in #7061 by modifying the HTTP/1.1 codec, such that it verifies the value of each HTTP header when onHeaderValue is called. The codec raises an exception, which results in a 400 response being returned if an invalid header value (per RFC 7230, section 3.2) is found. The actual validation is done via the nghttp2_check_header_value function, which is wrapped in Envoy::Http::HeaderUtility::headerIsValid. (NOTE: this has been discussed as something that nghttp2 could do itself, but the issue seems to have languished. Also note that Go handles this: Go uses the httpguts.ValidHeaderFieldValue function (which is analogous to nghttp2_check_header_value) to ensure that all the HTTP header values conform to the relevant RFC specs before an http.Transport instance will round-trip the request. Risk Level: Low/medium This stricter validation semantics are controlled with the envoy.reloadable_features.validate_header_values runtime-guarded feature. Originally, the PR used a new boolean on the HTTP connection manager proto to toggle the behavior, but during the course of PR review, it was decided that this would be more appropriate for a runtime guard. Testing: new unit tests, manual tests Release Notes: Updated Fixes #7061 Signed-off-by: Dylan Carney --- docs/root/intro/version_history.rst | 1 + source/common/http/BUILD | 4 + source/common/http/codec_client.cc | 5 +- source/common/http/codec_client.h | 3 +- source/common/http/conn_manager_config.h | 6 +- source/common/http/conn_manager_impl.cc | 7 +- source/common/http/conn_manager_impl.h | 2 + source/common/http/conn_manager_utility.cc | 7 +- source/common/http/conn_manager_utility.h | 2 +- source/common/http/header_utility.cc | 6 + source/common/http/header_utility.h | 7 ++ source/common/http/http1/BUILD | 2 + source/common/http/http1/codec_impl.cc | 53 +++++++-- source/common/http/http1/codec_impl.h | 11 ++ source/common/http/http1/conn_pool.cc | 8 +- source/common/http/http2/conn_pool.cc | 2 +- source/common/upstream/health_checker_impl.cc | 7 +- .../network/http_connection_manager/config.cc | 15 ++- .../network/http_connection_manager/config.h | 3 +- source/server/http/admin.cc | 5 +- source/server/http/admin.h | 3 +- .../http/conn_manager_impl_fuzz_test.cc | 2 +- test/common/http/conn_manager_impl_test.cc | 2 +- test/common/http/conn_manager_utility_test.cc | 10 +- test/common/http/header_utility_test.cc | 19 +++ test/common/http/http1/BUILD | 4 + test/common/http/http1/codec_impl_test.cc | 111 ++++++++++++++++-- test/integration/http_integration.cc | 4 +- test/integration/utility.cc | 2 +- 29 files changed, 261 insertions(+), 52 deletions(-) diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index 70e8fe3e8c87..82b9475ea32c 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -3,6 +3,7 @@ Version history 1.12.0 (pending) ================ +* http: added the ability to reject HTTP/1.1 requests with invalid HTTP header values, using the runtime feature `envoy.reloadable_features.strict_header_validation`. 1.11.0 (July 11, 2019) ====================== diff --git a/source/common/http/BUILD b/source/common/http/BUILD index 88ff9675e93c..1dcf5045fa26 100644 --- a/source/common/http/BUILD +++ b/source/common/http/BUILD @@ -142,6 +142,7 @@ envoy_cc_library( ":conn_manager_config_interface", ":exception_lib", ":header_map_lib", + ":header_utility_lib", ":headers_lib", ":path_utility_lib", ":user_agent_lib", @@ -313,6 +314,9 @@ envoy_cc_library( name = "header_utility_lib", srcs = ["header_utility.cc"], hdrs = ["header_utility.h"], + external_deps = [ + "nghttp2", + ], deps = [ "//include/envoy/http:header_map_interface", "//include/envoy/json:json_object_interface", diff --git a/source/common/http/codec_client.cc b/source/common/http/codec_client.cc index 6c5488ef7883..fbfd421b0220 100644 --- a/source/common/http/codec_client.cc +++ b/source/common/http/codec_client.cc @@ -136,11 +136,12 @@ void CodecClient::onData(Buffer::Instance& data) { CodecClientProd::CodecClientProd(Type type, Network::ClientConnectionPtr&& connection, Upstream::HostDescriptionConstSharedPtr host, - Event::Dispatcher& dispatcher) + Event::Dispatcher& dispatcher, bool strict_header_validation) : CodecClient(type, std::move(connection), host, dispatcher) { switch (type) { case Type::HTTP1: { - codec_ = std::make_unique(*connection_, *this); + codec_ = std::make_unique(*connection_, *this, + strict_header_validation); break; } case Type::HTTP2: { diff --git a/source/common/http/codec_client.h b/source/common/http/codec_client.h index 4fa085d2e4fc..4bf2487da76c 100644 --- a/source/common/http/codec_client.h +++ b/source/common/http/codec_client.h @@ -241,7 +241,8 @@ using CodecClientPtr = std::unique_ptr; class CodecClientProd : public CodecClient { public: CodecClientProd(Type type, Network::ClientConnectionPtr&& connection, - Upstream::HostDescriptionConstSharedPtr host, Event::Dispatcher& dispatcher); + Upstream::HostDescriptionConstSharedPtr host, Event::Dispatcher& dispatcher, + bool strict_header_validation); }; } // namespace Http diff --git a/source/common/http/conn_manager_config.h b/source/common/http/conn_manager_config.h index 5e08e267d5b9..78ebc7a19161 100644 --- a/source/common/http/conn_manager_config.h +++ b/source/common/http/conn_manager_config.h @@ -184,11 +184,15 @@ class ConnectionManagerConfig { * @param connection supplies the owning connection. * @param data supplies the currently available read data. * @param callbacks supplies the callbacks to install into the codec. + * @param strict_header_validation indicates whether or not the codec should validate the values + * of each HTTP header (NOTE: this argument only affects the H/1.1 codec; the H/2 codec always + * does this) * @return a codec or nullptr if no codec can be created. */ virtual ServerConnectionPtr createCodec(Network::Connection& connection, const Buffer::Instance& data, - ServerConnectionCallbacks& callbacks) PURE; + ServerConnectionCallbacks& callbacks, + const bool strict_header_validation) PURE; /** * @return DateProvider& the date provider to use for diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index cf3e4f90ded6..998642cd2cd6 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -33,6 +33,7 @@ #include "common/http/path_utility.h" #include "common/http/utility.h" #include "common/network/utility.h" +#include "common/runtime/runtime_impl.h" #include "absl/strings/escaping.h" #include "absl/strings/match.h" @@ -111,7 +112,8 @@ ConnectionManagerImpl::ConnectionManagerImpl(ConnectionManagerConfig& config, overload_manager ? overload_manager->getThreadLocalOverloadState().getState( Server::OverloadActionNames::get().DisableHttpKeepAlive) : Server::OverloadManager::getInactiveState()), - time_source_(time_source) {} + time_source_(time_source), strict_header_validation_(Runtime::runtimeFeatureEnabled( + "envoy.reloadable_features.strict_header_validation")) {} const HeaderMapImpl& ConnectionManagerImpl::continueHeader() { CONSTRUCT_ON_FIRST_USE(HeaderMapImpl, @@ -259,7 +261,8 @@ StreamDecoder& ConnectionManagerImpl::newStream(StreamEncoder& response_encoder, Network::FilterStatus ConnectionManagerImpl::onData(Buffer::Instance& data, bool) { if (!codec_) { - codec_ = config_.createCodec(read_callbacks_->connection(), data, *this); + codec_ = + config_.createCodec(read_callbacks_->connection(), data, *this, strict_header_validation_); if (codec_->protocol() == Protocol::Http2) { stats_.named_.downstream_cx_http2_total_.inc(); stats_.named_.downstream_cx_http2_active_.inc(); diff --git a/source/common/http/conn_manager_impl.h b/source/common/http/conn_manager_impl.h index 0a7139a40f12..e2a790b28683 100644 --- a/source/common/http/conn_manager_impl.h +++ b/source/common/http/conn_manager_impl.h @@ -678,6 +678,8 @@ class ConnectionManagerImpl : Logger::Loggable, const Server::OverloadActionState& overload_stop_accepting_requests_ref_; const Server::OverloadActionState& overload_disable_keepalive_ref_; TimeSource& time_source_; + + const bool strict_header_validation_; }; } // namespace Http diff --git a/source/common/http/conn_manager_utility.cc b/source/common/http/conn_manager_utility.cc index acc258499e56..6ec1bd5b9116 100644 --- a/source/common/http/conn_manager_utility.cc +++ b/source/common/http/conn_manager_utility.cc @@ -41,13 +41,14 @@ std::string ConnectionManagerUtility::determineNextProtocol(Network::Connection& ServerConnectionPtr ConnectionManagerUtility::autoCreateCodec( Network::Connection& connection, const Buffer::Instance& data, ServerConnectionCallbacks& callbacks, Stats::Scope& scope, const Http1Settings& http1_settings, - const Http2Settings& http2_settings, const uint32_t max_request_headers_kb) { + const Http2Settings& http2_settings, const uint32_t max_request_headers_kb, + bool strict_header_validation) { if (determineNextProtocol(connection, data) == Http2::ALPN_STRING) { return std::make_unique(connection, callbacks, scope, http2_settings, max_request_headers_kb); } else { - return std::make_unique(connection, callbacks, http1_settings, - max_request_headers_kb); + return std::make_unique( + connection, callbacks, http1_settings, max_request_headers_kb, strict_header_validation); } } diff --git a/source/common/http/conn_manager_utility.h b/source/common/http/conn_manager_utility.h index 802d0ef07add..c203fb154717 100644 --- a/source/common/http/conn_manager_utility.h +++ b/source/common/http/conn_manager_utility.h @@ -37,7 +37,7 @@ class ConnectionManagerUtility { autoCreateCodec(Network::Connection& connection, const Buffer::Instance& data, ServerConnectionCallbacks& callbacks, Stats::Scope& scope, const Http1Settings& http1_settings, const Http2Settings& http2_settings, - const uint32_t max_request_headers_kb); + const uint32_t max_request_headers_kb, bool strict_header_validation); /** * Mutates request headers in various ways. This functionality is broken out because of its diff --git a/source/common/http/header_utility.cc b/source/common/http/header_utility.cc index 396fdb624f17..4cecc97178e7 100644 --- a/source/common/http/header_utility.cc +++ b/source/common/http/header_utility.cc @@ -6,6 +6,7 @@ #include "common/protobuf/utility.h" #include "absl/strings/match.h" +#include "nghttp2/nghttp2.h" namespace Envoy { namespace Http { @@ -117,6 +118,11 @@ bool HeaderUtility::matchHeaders(const Http::HeaderMap& request_headers, return match != header_data.invert_match_; } +bool HeaderUtility::headerIsValid(const absl::string_view header_value) { + return (nghttp2_check_header_value(reinterpret_cast(header_value.data()), + header_value.size()) != 0); +} + void HeaderUtility::addHeaders(Http::HeaderMap& headers, const Http::HeaderMap& headers_to_add) { headers_to_add.iterate( [](const Http::HeaderEntry& header, void* context) -> Http::HeaderMap::Iterate { diff --git a/source/common/http/header_utility.h b/source/common/http/header_utility.h index 1e328691bf4c..d2b552da51fa 100644 --- a/source/common/http/header_utility.h +++ b/source/common/http/header_utility.h @@ -45,6 +45,13 @@ class HeaderUtility { static bool matchHeaders(const Http::HeaderMap& request_headers, const HeaderData& config_header); + /** + * Validates that a header value is valid, according to RFC 7230, section 3.2. + * http://tools.ietf.org/html/rfc7230#section-3.2 + * @return bool true if the header values are valid, according to the aforementioned RFC. + */ + static bool headerIsValid(const absl::string_view header_value); + /** * Add headers from one HeaderMap to another * @param headers target where headers will be added diff --git a/source/common/http/http1/BUILD b/source/common/http/http1/BUILD index edd4dff03073..22f47ba2f31f 100644 --- a/source/common/http/http1/BUILD +++ b/source/common/http/http1/BUILD @@ -27,6 +27,7 @@ envoy_cc_library( "//source/common/http:codes_lib", "//source/common/http:exception_lib", "//source/common/http:header_map_lib", + "//source/common/http:header_utility_lib", "//source/common/http:headers_lib", "//source/common/http:utility_lib", ], @@ -55,6 +56,7 @@ envoy_cc_library( "//source/common/http:conn_pool_base_lib", "//source/common/http:headers_lib", "//source/common/network:utility_lib", + "//source/common/runtime:runtime_lib", "//source/common/upstream:upstream_lib", ], ) diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index edab3707018e..863606b32c6c 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -13,6 +13,7 @@ #include "common/common/stack_array.h" #include "common/common/utility.h" #include "common/http/exception.h" +#include "common/http/header_utility.h" #include "common/http/headers.h" #include "common/http/utility.h" @@ -319,11 +320,20 @@ const ToLowerTable& ConnectionImpl::toLowerTable() { return *table; } +// TODO(alyssawilk) The overloaded constructors here and on {Client,Server}ConnectionImpl +// can be cleaned up once "strict_header_validation" becomes the default behavior, rather +// than a runtime-guarded one. The overloads were a workaround for the fact that Runtime +// doesn't work from integration test call sites in scenarios where the required +// thread-local storage is not available. ConnectionImpl::ConnectionImpl(Network::Connection& connection, http_parser_type type, uint32_t max_headers_kb) + : ConnectionImpl::ConnectionImpl(connection, type, max_headers_kb, false) {} + +ConnectionImpl::ConnectionImpl(Network::Connection& connection, http_parser_type type, + uint32_t max_headers_kb, bool strict_header_validation) : connection_(connection), output_buffer_([&]() -> void { this->onBelowLowWatermark(); }, [&]() -> void { this->onAboveHighWatermark(); }), - max_headers_kb_(max_headers_kb) { + max_headers_kb_(max_headers_kb), strict_header_validation_(strict_header_validation) { output_buffer_.setWatermarks(connection.bufferLimit()); http_parser_init(&parser_, type); parser_.data = this; @@ -421,11 +431,21 @@ void ConnectionImpl::onHeaderValue(const char* data, size_t length) { // Ignore trailers. return; } - // http-parser should filter for this - // (https://tools.ietf.org/html/rfc7230#section-3.2.6), but it doesn't today. HeaderStrings - // have an invariant that they must not contain embedded zero characters - // (NUL, ASCII 0x0). - if (absl::string_view(data, length).find('\0') != absl::string_view::npos) { + + const absl::string_view header_value = absl::string_view(data, length); + + if (strict_header_validation_) { + if (!Http::HeaderUtility::headerIsValid(header_value)) { + ENVOY_CONN_LOG(debug, "invalid header value: {}", connection_, header_value); + error_code_ = Http::Code::BadRequest; + sendProtocolError(); + throw CodecProtocolException("http/1.1 protocol error: header value contains invalid chars"); + } + } else if (header_value.find('\0') != absl::string_view::npos) { + // http-parser should filter for this + // (https://tools.ietf.org/html/rfc7230#section-3.2.6), but it doesn't today. HeaderStrings + // have an invariant that they must not contain embedded zero characters + // (NUL, ASCII 0x0). throw CodecProtocolException("http/1.1 protocol error: header value contains NUL"); } @@ -496,8 +516,15 @@ void ConnectionImpl::onResetStreamBase(StreamResetReason reason) { ServerConnectionImpl::ServerConnectionImpl(Network::Connection& connection, ServerConnectionCallbacks& callbacks, Http1Settings settings, uint32_t max_request_headers_kb) - : ConnectionImpl(connection, HTTP_REQUEST, max_request_headers_kb), callbacks_(callbacks), - codec_settings_(settings) {} + : ServerConnectionImpl::ServerConnectionImpl(connection, callbacks, settings, + max_request_headers_kb, false) {} + +ServerConnectionImpl::ServerConnectionImpl(Network::Connection& connection, + ServerConnectionCallbacks& callbacks, + Http1Settings settings, uint32_t max_request_headers_kb, + bool strict_header_validation) + : ConnectionImpl(connection, HTTP_REQUEST, max_request_headers_kb, strict_header_validation), + callbacks_(callbacks), codec_settings_(settings) {} void ServerConnectionImpl::onEncodeComplete() { ASSERT(active_request_); @@ -668,8 +695,14 @@ void ServerConnectionImpl::onBelowLowWatermark() { } } -ClientConnectionImpl::ClientConnectionImpl(Network::Connection& connection, ConnectionCallbacks&) - : ConnectionImpl(connection, HTTP_RESPONSE, MAX_RESPONSE_HEADERS_KB) {} +ClientConnectionImpl::ClientConnectionImpl(Network::Connection& connection, + ConnectionCallbacks& callbacks) + : ClientConnectionImpl::ClientConnectionImpl(connection, callbacks, false) {} + +ClientConnectionImpl::ClientConnectionImpl(Network::Connection& connection, ConnectionCallbacks&, + bool strict_header_validation) + : ConnectionImpl(connection, HTTP_RESPONSE, MAX_RESPONSE_HEADERS_KB, strict_header_validation) { +} bool ClientConnectionImpl::cannotHaveBody() { if ((!pending_responses_.empty() && pending_responses_.front().head_request_) || diff --git a/source/common/http/http1/codec_impl.h b/source/common/http/http1/codec_impl.h index 27b80e5e5b81..a9bf7111b73b 100644 --- a/source/common/http/http1/codec_impl.h +++ b/source/common/http/http1/codec_impl.h @@ -174,6 +174,8 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable(Http::CodecClient::Type::HTTP2, std::move(data.connection_), - data.host_description_, dispatcher_); + data.host_description_, dispatcher_, false); } std::ostream& operator<<(std::ostream& out, HealthState state) { diff --git a/source/extensions/filters/network/http_connection_manager/config.cc b/source/extensions/filters/network/http_connection_manager/config.cc index 04edfdba79c4..f5596e83bfe0 100644 --- a/source/extensions/filters/network/http_connection_manager/config.cc +++ b/source/extensions/filters/network/http_connection_manager/config.cc @@ -360,21 +360,20 @@ void HttpConnectionManagerConfig::processFilter( filter_factories.push_back(callback); } -Http::ServerConnectionPtr -HttpConnectionManagerConfig::createCodec(Network::Connection& connection, - const Buffer::Instance& data, - Http::ServerConnectionCallbacks& callbacks) { +Http::ServerConnectionPtr HttpConnectionManagerConfig::createCodec( + Network::Connection& connection, const Buffer::Instance& data, + Http::ServerConnectionCallbacks& callbacks, const bool strict_header_validation) { switch (codec_type_) { case CodecType::HTTP1: return std::make_unique( - connection, callbacks, http1_settings_, maxRequestHeadersKb()); + connection, callbacks, http1_settings_, maxRequestHeadersKb(), strict_header_validation); case CodecType::HTTP2: return std::make_unique( connection, callbacks, context_.scope(), http2_settings_, maxRequestHeadersKb()); case CodecType::AUTO: - return Http::ConnectionManagerUtility::autoCreateCodec(connection, data, callbacks, - context_.scope(), http1_settings_, - http2_settings_, maxRequestHeadersKb()); + return Http::ConnectionManagerUtility::autoCreateCodec( + connection, data, callbacks, context_.scope(), http1_settings_, http2_settings_, + maxRequestHeadersKb(), strict_header_validation); } NOT_REACHED_GCOVR_EXCL_LINE; diff --git a/source/extensions/filters/network/http_connection_manager/config.h b/source/extensions/filters/network/http_connection_manager/config.h index 796b67fc9728..eb97a9bf21db 100644 --- a/source/extensions/filters/network/http_connection_manager/config.h +++ b/source/extensions/filters/network/http_connection_manager/config.h @@ -99,7 +99,8 @@ class HttpConnectionManagerConfig : Logger::Loggable, const std::list& accessLogs() override { return access_logs_; } Http::ServerConnectionPtr createCodec(Network::Connection& connection, const Buffer::Instance& data, - Http::ServerConnectionCallbacks& callbacks) override; + Http::ServerConnectionCallbacks& callbacks, + const bool strict_header_validation) override; Http::DateProvider& dateProvider() override { return date_provider_; } std::chrono::milliseconds drainTimeout() override { return drain_timeout_; } FilterChainFactory& filterFactory() override { return *this; } diff --git a/source/server/http/admin.cc b/source/server/http/admin.cc index 6537b978123b..598699859d90 100644 --- a/source/server/http/admin.cc +++ b/source/server/http/admin.cc @@ -1241,10 +1241,11 @@ AdminImpl::AdminImpl(const std::string& profile_path, Server::Instance& server) Http::ServerConnectionPtr AdminImpl::createCodec(Network::Connection& connection, const Buffer::Instance& data, - Http::ServerConnectionCallbacks& callbacks) { + Http::ServerConnectionCallbacks& callbacks, + const bool strict_header_validation) { return Http::ConnectionManagerUtility::autoCreateCodec( connection, data, callbacks, server_.stats(), Http::Http1Settings(), Http::Http2Settings(), - maxRequestHeadersKb()); + maxRequestHeadersKb(), strict_header_validation); } bool AdminImpl::createNetworkFilterChain(Network::Connection& connection, diff --git a/source/server/http/admin.h b/source/server/http/admin.h index 6fc5d3a6e254..f13129a0e679 100644 --- a/source/server/http/admin.h +++ b/source/server/http/admin.h @@ -104,7 +104,8 @@ class AdminImpl : public Admin, const std::list& accessLogs() override { return access_logs_; } Http::ServerConnectionPtr createCodec(Network::Connection& connection, const Buffer::Instance& data, - Http::ServerConnectionCallbacks& callbacks) override; + Http::ServerConnectionCallbacks& callbacks, + const bool strict_header_validation) override; Http::DateProvider& dateProvider() override { return date_provider_; } std::chrono::milliseconds drainTimeout() override { return std::chrono::milliseconds(100); } Http::FilterChainFactory& filterFactory() override { return *this; } diff --git a/test/common/http/conn_manager_impl_fuzz_test.cc b/test/common/http/conn_manager_impl_fuzz_test.cc index 3c11424b5964..e15e88e97473 100644 --- a/test/common/http/conn_manager_impl_fuzz_test.cc +++ b/test/common/http/conn_manager_impl_fuzz_test.cc @@ -72,7 +72,7 @@ class FuzzConfig : public ConnectionManagerConfig { // Http::ConnectionManagerConfig const std::list& accessLogs() override { return access_logs_; } ServerConnectionPtr createCodec(Network::Connection&, const Buffer::Instance&, - ServerConnectionCallbacks&) override { + ServerConnectionCallbacks&, const bool) override { return ServerConnectionPtr{codec_}; } DateProvider& dateProvider() override { return date_provider_; } diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index ce4e8af46284..31865f3f7287 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -241,7 +241,7 @@ class HttpConnectionManagerImplTest : public testing::Test, public ConnectionMan // Http::ConnectionManagerConfig const std::list& accessLogs() override { return access_logs_; } ServerConnectionPtr createCodec(Network::Connection&, const Buffer::Instance&, - ServerConnectionCallbacks&) override { + ServerConnectionCallbacks&, const bool) override { return ServerConnectionPtr{codec_}; } DateProvider& dateProvider() override { return date_provider_; } diff --git a/test/common/http/conn_manager_utility_test.cc b/test/common/http/conn_manager_utility_test.cc index 46937cf17ab0..48fa93d14174 100644 --- a/test/common/http/conn_manager_utility_test.cc +++ b/test/common/http/conn_manager_utility_test.cc @@ -43,13 +43,15 @@ class MockConnectionManagerConfig : public ConnectionManagerConfig { // Http::ConnectionManagerConfig ServerConnectionPtr createCodec(Network::Connection& connection, const Buffer::Instance& instance, - ServerConnectionCallbacks& callbacks) override { - return ServerConnectionPtr{createCodec_(connection, instance, callbacks)}; + ServerConnectionCallbacks& callbacks, + const bool strict_header_validation) override { + return ServerConnectionPtr{ + createCodec_(connection, instance, callbacks, strict_header_validation)}; } MOCK_METHOD0(accessLogs, const std::list&()); - MOCK_METHOD3(createCodec_, ServerConnection*(Network::Connection&, const Buffer::Instance&, - ServerConnectionCallbacks&)); + MOCK_METHOD4(createCodec_, ServerConnection*(Network::Connection&, const Buffer::Instance&, + ServerConnectionCallbacks&, const bool)); MOCK_METHOD0(dateProvider, DateProvider&()); MOCK_METHOD0(drainTimeout, std::chrono::milliseconds()); MOCK_METHOD0(filterFactory, FilterChainFactory&()); diff --git a/test/common/http/header_utility_test.cc b/test/common/http/header_utility_test.cc index b2fe1de01cd8..a20f4ca43f49 100644 --- a/test/common/http/header_utility_test.cc +++ b/test/common/http/header_utility_test.cc @@ -403,6 +403,25 @@ invert_match: true EXPECT_FALSE(HeaderUtility::matchHeaders(unmatching_headers, header_data)); } +TEST(HeaderIsValidTest, InvalidHeaderValuesAreRejected) { + // ASCII values 1-31 are control characters (with the exception of ASCII + // values 9, 10, and 13 which are a horizontal tab, line feed, and carriage + // return, respectively), and are not valid in an HTTP header, per + // RFC 7230, section 3.2 + for (uint i = 0; i < 32; i++) { + if (i == 9) { + continue; + } + + EXPECT_FALSE(HeaderUtility::headerIsValid(std::string(1, i))); + } +} + +TEST(HeaderIsValidTest, ValidHeaderValuesAreAccepted) { + EXPECT_TRUE(HeaderUtility::headerIsValid("some-value")); + EXPECT_TRUE(HeaderUtility::headerIsValid("Some Other Value")); +} + TEST(HeaderAddTest, HeaderAdd) { TestHeaderMapImpl headers{{"myheader1", "123value"}}; TestHeaderMapImpl headers_to_add{{"myheader2", "456value"}}; diff --git a/test/common/http/http1/BUILD b/test/common/http/http1/BUILD index e685bff6f328..efee73f47d1b 100644 --- a/test/common/http/http1/BUILD +++ b/test/common/http/http1/BUILD @@ -21,7 +21,11 @@ envoy_cc_test( "//source/common/http/http1:codec_lib", "//test/mocks/buffer:buffer_mocks", "//test/mocks/http:http_mocks", + "//test/mocks/init:init_mocks", + "//test/mocks/local_info:local_info_mocks", "//test/mocks/network:network_mocks", + "//test/mocks/protobuf:protobuf_mocks", + "//test/mocks/thread_local:thread_local_mocks", "//test/mocks/upstream:upstream_mocks", ], ) diff --git a/test/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc index c65d0ba18cb3..1db1fee44f4e 100644 --- a/test/common/http/http1/codec_impl_test.cc +++ b/test/common/http/http1/codec_impl_test.cc @@ -8,11 +8,18 @@ #include "common/http/exception.h" #include "common/http/header_map_impl.h" #include "common/http/http1/codec_impl.h" +#include "common/runtime/runtime_impl.h" #include "test/mocks/buffer/mocks.h" #include "test/mocks/http/mocks.h" +#include "test/mocks/init/mocks.h" +#include "test/mocks/local_info/mocks.h" #include "test/mocks/network/mocks.h" +#include "test/mocks/protobuf/mocks.h" +#include "test/mocks/runtime/mocks.h" +#include "test/mocks/thread_local/mocks.h" #include "test/test_common/printers.h" +#include "test/test_common/utility.h" #include "gmock/gmock.h" #include "gtest/gtest.h" @@ -30,9 +37,23 @@ namespace Http1 { class Http1ServerConnectionImplTest : public testing::Test { public: + Http1ServerConnectionImplTest() : api_(Api::createApiForTest()) { + envoy::config::bootstrap::v2::LayeredRuntime config; + config.add_layers()->mutable_admin_layer(); + + // Create a runtime loader, so that tests can manually manipulate runtime + // guarded features. + loader_ = std::make_unique(Runtime::LoaderPtr{ + new Runtime::LoaderImpl(dispatcher_, tls_, config, local_info_, init_manager_, store_, + generator_, validation_visitor_, *api_)}); + } + void initialize() { - codec_ = std::make_unique(connection_, callbacks_, codec_settings_, - max_request_headers_kb_); + strict_header_validation_ = + Runtime::runtimeFeatureEnabled("envoy.reloadable_features.strict_header_validation"); + codec_ = + std::make_unique(connection_, callbacks_, codec_settings_, + max_request_headers_kb_, strict_header_validation_); } NiceMock connection_; @@ -46,6 +67,16 @@ class Http1ServerConnectionImplTest : public testing::Test { protected: uint32_t max_request_headers_kb_{Http::DEFAULT_MAX_REQUEST_HEADERS_KB}; + bool strict_header_validation_; + Event::MockDispatcher dispatcher_; + NiceMock tls_; + Stats::IsolatedStoreImpl store_; + Runtime::MockRandomGenerator generator_; + Api::ApiPtr api_; + NiceMock local_info_; + Init::MockManager init_manager_; + NiceMock validation_visitor_; + std::unique_ptr loader_; }; void Http1ServerConnectionImplTest::expect400(Protocol p, bool allow_absolute_url, @@ -57,8 +88,9 @@ void Http1ServerConnectionImplTest::expect400(Protocol p, bool allow_absolute_ur if (allow_absolute_url) { codec_settings_.allow_absolute_url_ = allow_absolute_url; - codec_ = std::make_unique(connection_, callbacks_, codec_settings_, - max_request_headers_kb_); + codec_ = + std::make_unique(connection_, callbacks_, codec_settings_, + max_request_headers_kb_, strict_header_validation_); } Http::MockStreamDecoder decoder; @@ -77,8 +109,9 @@ void Http1ServerConnectionImplTest::expectHeadersTest(Protocol p, bool allow_abs // Make a new 'codec' with the right settings if (allow_absolute_url) { codec_settings_.allow_absolute_url_ = allow_absolute_url; - codec_ = std::make_unique(connection_, callbacks_, codec_settings_, - max_request_headers_kb_); + codec_ = + std::make_unique(connection_, callbacks_, codec_settings_, + max_request_headers_kb_, strict_header_validation_); } Http::MockStreamDecoder decoder; @@ -333,6 +366,43 @@ TEST_F(Http1ServerConnectionImplTest, HostHeaderTranslation) { EXPECT_EQ(0U, buffer.length()); } +// Ensures that requests with invalid HTTP header values are not rejected +// when the runtime guard is not enabled for the feature. +TEST_F(Http1ServerConnectionImplTest, HeaderInvalidCharsRuntimeGuard) { + // When the runtime-guarded feature is NOT enabled, invalid header values + // should be accepted by the codec. + Runtime::LoaderSingleton::getExisting()->mergeValues( + {{"envoy.reloadable_features.strict_header_validation", "false"}}); + + initialize(); + + Http::MockStreamDecoder decoder; + EXPECT_CALL(callbacks_, newStream(_, _)).WillOnce(ReturnRef(decoder)); + + Buffer::OwnedImpl buffer( + absl::StrCat("GET / HTTP/1.1\r\nHOST: h.com\r\nfoo: ", std::string(1, 3), "\r\n")); + codec_->dispatch(buffer); +} + +// Ensures that requests with invalid HTTP header values are properly rejected +// when the runtime guard is enabled for the feature. +TEST_F(Http1ServerConnectionImplTest, HeaderInvalidCharsRejection) { + // When the runtime-guarded feature is enabled, invalid header values + // should result in a rejection. + Runtime::LoaderSingleton::getExisting()->mergeValues( + {{"envoy.reloadable_features.strict_header_validation", "true"}}); + + initialize(); + + Http::MockStreamDecoder decoder; + EXPECT_CALL(callbacks_, newStream(_, _)).WillOnce(ReturnRef(decoder)); + + Buffer::OwnedImpl buffer( + absl::StrCat("GET / HTTP/1.1\r\nHOST: h.com\r\nfoo: ", std::string(1, 3), "\r\n")); + EXPECT_THROW_WITH_MESSAGE(codec_->dispatch(buffer), CodecProtocolException, + "http/1.1 protocol error: header value contains invalid chars"); +} + // Regression test for http-parser allowing embedded NULs in header values, // verify we reject them. TEST_F(Http1ServerConnectionImplTest, HeaderEmbeddedNulRejection) { @@ -784,11 +854,38 @@ TEST_F(Http1ServerConnectionImplTest, WatermarkTest) { class Http1ClientConnectionImplTest : public testing::Test { public: - void initialize() { codec_ = std::make_unique(connection_, callbacks_); } + Http1ClientConnectionImplTest() : api_(Api::createApiForTest()) { + envoy::config::bootstrap::v2::LayeredRuntime config; + + // Create a runtime loader, so that tests can manually manipulate runtime + // guarded features. + loader_ = std::make_unique(Runtime::LoaderPtr{ + new Runtime::LoaderImpl(dispatcher_, tls_, config, local_info_, init_manager_, store_, + generator_, validation_visitor_, *api_)}); + } + + void initialize() { + strict_header_validation_ = + Runtime::runtimeFeatureEnabled("envoy.reloadable_features.strict_header_validation"); + codec_ = + std::make_unique(connection_, callbacks_, strict_header_validation_); + } NiceMock connection_; NiceMock callbacks_; std::unique_ptr codec_; + +protected: + Event::MockDispatcher dispatcher_; + NiceMock tls_; + Stats::IsolatedStoreImpl store_; + Runtime::MockRandomGenerator generator_; + Api::ApiPtr api_; + NiceMock local_info_; + Init::MockManager init_manager_; + NiceMock validation_visitor_; + std::unique_ptr loader_; + bool strict_header_validation_; }; TEST_F(Http1ClientConnectionImplTest, SimpleGet) { diff --git a/test/integration/http_integration.cc b/test/integration/http_integration.cc index fbd7308fd4a6..eec74c73d452 100644 --- a/test/integration/http_integration.cc +++ b/test/integration/http_integration.cc @@ -62,8 +62,8 @@ typeToCodecType(Http::CodecClient::Type type) { IntegrationCodecClient::IntegrationCodecClient( Event::Dispatcher& dispatcher, Network::ClientConnectionPtr&& conn, Upstream::HostDescriptionConstSharedPtr host_description, CodecClient::Type type) - : CodecClientProd(type, std::move(conn), host_description, dispatcher), dispatcher_(dispatcher), - callbacks_(*this), codec_callbacks_(*this) { + : CodecClientProd(type, std::move(conn), host_description, dispatcher, false), + dispatcher_(dispatcher), callbacks_(*this), codec_callbacks_(*this) { connection_->addConnectionCallbacks(callbacks_); setCodecConnectionCallbacks(codec_callbacks_); dispatcher.run(Event::Dispatcher::RunType::Block); diff --git a/test/integration/utility.cc b/test/integration/utility.cc index ecf5fd26ae7d..de4375118daf 100644 --- a/test/integration/utility.cc +++ b/test/integration/utility.cc @@ -76,7 +76,7 @@ IntegrationUtil::makeSingleRequest(const Network::Address::InstanceConstSharedPt type, dispatcher->createClientConnection(addr, Network::Address::InstanceConstSharedPtr(), Network::Test::createRawBufferSocket(), nullptr), - host_description, *dispatcher); + host_description, *dispatcher, false); BufferingStreamDecoderPtr response(new BufferingStreamDecoder([&]() -> void { client.close(); dispatcher->exit();