diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index 46dd77f8b1b0..dd884bb78c3b 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -84,9 +84,11 @@ Version history * http: :ref:`AUTO ` codec protocol inference now requires the H2 magic bytes to be the first bytes transmitted by a downstream client. * http: remove h2c upgrade headers for HTTP/1 as h2c upgrades are currently not supported. * http: absolute URL support is now on by default. The prior behavior can be reinstated by setting :ref:`allow_absolute_url ` to false. +* http: added strict authority checking. This can be reversed temporarily by setting the runtime feature `envoy.reloadable_features.strict_authority_validation` to false. * http: support :ref:`host rewrite ` in the dynamic forward proxy. * http: support :ref:`disabling the filter per route ` in the grpc http1 reverse bridge filter. * http: added the ability to :ref:`configure max connection duration ` for downstream connections. +* http: trim LWS at the end of header keys, for correct HTTP/1.1 header parsing. * listeners: added :ref:`continue_on_listener_filters_timeout ` to configure whether a listener will still create a connection when listener filters time out. * listeners: added :ref:`HTTP inspector listener filter `. * listeners: added :ref:`connection balancer ` diff --git a/include/envoy/stream_info/stream_info.h b/include/envoy/stream_info/stream_info.h index 1b1ca9d55e2c..ccf5778be575 100644 --- a/include/envoy/stream_info/stream_info.h +++ b/include/envoy/stream_info/stream_info.h @@ -137,6 +137,8 @@ struct ResponseCodeDetailValues { // indicates that original "success" headers may have been sent downstream // despite the subsequent failure. const std::string LateUpstreamReset = "upstream_reset_after_response_started"; + // The request was rejected due to invalid characters in the HOST/:authority header. + const std::string InvalidAuthority = "invalid_authority"; }; using ResponseCodeDetails = ConstSingleton; diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 0feaa12d4e21..3b72848c6eb5 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -766,6 +766,15 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, } } + // Make sure the host is valid. + if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.strict_authority_validation") && + !HeaderUtility::authorityIsValid(request_headers_->Host()->value().getStringView())) { + sendLocalReply(Grpc::Common::hasGrpcContentType(*request_headers_), Code::BadRequest, "", + nullptr, is_head_request_, absl::nullopt, + StreamInfo::ResponseCodeDetails::get().InvalidAuthority); + return; + } + // Currently we only support relative paths at the application layer. We expect the codec to have // broken the path into pieces if applicable. NOTE: Currently the HTTP/1.1 codec only does this // when the allow_absolute_url flag is enabled on the HCM. diff --git a/source/common/http/header_utility.cc b/source/common/http/header_utility.cc index 71b390121fbb..7bb74e0ea932 100644 --- a/source/common/http/header_utility.cc +++ b/source/common/http/header_utility.cc @@ -135,6 +135,11 @@ bool HeaderUtility::headerIsValid(const absl::string_view header_value) { header_value.size()) != 0); } +bool HeaderUtility::authorityIsValid(const absl::string_view header_value) { + return (nghttp2_check_authority(reinterpret_cast(header_value.data()), + header_value.size()) != 0); +} + void HeaderUtility::addHeaders(HeaderMap& headers, const HeaderMap& headers_to_add) { headers_to_add.iterate( [](const HeaderEntry& header, void* context) -> HeaderMap::Iterate { diff --git a/source/common/http/header_utility.h b/source/common/http/header_utility.h index 51c5dc659857..aa2a92e3ece3 100644 --- a/source/common/http/header_utility.h +++ b/source/common/http/header_utility.h @@ -96,6 +96,12 @@ class HeaderUtility { */ static bool headerIsValid(const absl::string_view header_value); + /** + * Validates that the characters in the authority are valid. + * @return bool true if the header values are valid, false otherwise. + */ + static bool authorityIsValid(const absl::string_view authority_value); + /** * Add headers from one HeaderMap to another * @param headers target where headers will be added diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index 6e0ea8f1ea86..df25b453cbc5 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -469,7 +469,9 @@ void ConnectionImpl::onHeaderValue(const char* data, size_t length) { return; } - const absl::string_view header_value = absl::string_view(data, length); + // Work around a bug in http_parser where trailing whitespace is not trimmed + // as the spec requires: https://tools.ietf.org/html/rfc7230#section-3.2.4 + const absl::string_view header_value = StringUtil::trim(absl::string_view(data, length)); if (strict_header_validation_) { if (!Http::HeaderUtility::headerIsValid(header_value)) { @@ -487,7 +489,7 @@ void ConnectionImpl::onHeaderValue(const char* data, size_t length) { } header_parsing_state_ = HeaderParsingState::Value; - current_header_value_.append(data, length); + current_header_value_.append(header_value.data(), header_value.length()); const uint32_t total = current_header_field_.size() + current_header_value_.size() + current_header_map_->byteSize(); diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index d8bb32412732..483f4682e3cc 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -29,6 +29,8 @@ constexpr const char* runtime_features[] = { "envoy.reloadable_features.buffer_filter_populate_content_length", "envoy.reloadable_features.outlier_detection_support_for_grpc_status", "envoy.reloadable_features.connection_header_sanitization", + "envoy.reloadable_features.strict_header_validation", + "envoy.reloadable_features.strict_authority_validation", }; // This is a list of configuration fields which are disallowed by default in Envoy diff --git a/test/common/http/header_utility_test.cc b/test/common/http/header_utility_test.cc index 9b03bc64825e..a54dd4c10ffc 100644 --- a/test/common/http/header_utility_test.cc +++ b/test/common/http/header_utility_test.cc @@ -464,6 +464,13 @@ TEST(HeaderIsValidTest, ValidHeaderValuesAreAccepted) { EXPECT_TRUE(HeaderUtility::headerIsValid("Some Other Value")); } +TEST(HeaderIsValidTest, AuthIsValid) { + EXPECT_TRUE(HeaderUtility::authorityIsValid("strangebutlegal$-%&'")); + EXPECT_FALSE(HeaderUtility::authorityIsValid("illegal{}")); + // Full checks are done by Http2CodecImplTest.CheckAuthority, cross checking + // against nghttp2 compliance. +} + TEST(HeaderAddTest, HeaderAdd) { TestHeaderMapImpl headers{{"myheader1", "123value"}}; TestHeaderMapImpl headers_to_add{{"myheader2", "456value"}}; diff --git a/test/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc index 5a71a953efaa..cc77c9370984 100644 --- a/test/common/http/http1/codec_impl_test.cc +++ b/test/common/http/http1/codec_impl_test.cc @@ -61,6 +61,25 @@ class Http1ServerConnectionImplTest : public testing::Test { void testRequestHeadersExceedLimit(std::string header_string); void testRequestHeadersAccepted(std::string header_string); + // Send the request, and validate the received request headers. + // Then send a response just to clean up. + void sendAndValidateRequestAndSendResponse(absl::string_view raw_request, + const TestHeaderMapImpl& expected_request_headers) { + NiceMock decoder; + Http::StreamEncoder* response_encoder = nullptr; + EXPECT_CALL(callbacks_, newStream(_, _)) + .Times(1) + .WillOnce(Invoke([&](Http::StreamEncoder& encoder, bool) -> Http::StreamDecoder& { + response_encoder = &encoder; + return decoder; + })); + EXPECT_CALL(decoder, decodeHeaders_(HeaderMapEqual(&expected_request_headers), true)).Times(1); + Buffer::OwnedImpl buffer(raw_request); + codec_->dispatch(buffer); + EXPECT_EQ(0U, buffer.length()); + response_encoder->encodeHeaders(TestHeaderMapImpl{{":status", "200"}}, true); + } + protected: uint32_t max_request_headers_kb_{Http::DEFAULT_MAX_REQUEST_HEADERS_KB}; uint32_t max_request_headers_count_{Http::DEFAULT_MAX_HEADERS_COUNT}; @@ -167,6 +186,23 @@ TEST_F(Http1ServerConnectionImplTest, EmptyHeader) { EXPECT_EQ(0U, buffer.length()); } +TEST_F(Http1ServerConnectionImplTest, HostWithLWS) { + initialize(); + + TestHeaderMapImpl expected_headers{{":authority", "host"}, {":path", "/"}, {":method", "GET"}}; + + // Regression test spaces before and after the host header value. + sendAndValidateRequestAndSendResponse("GET / HTTP/1.1\r\nHost: host \r\n\r\n", expected_headers); + + // Regression test tabs before and after the host header value. + sendAndValidateRequestAndSendResponse("GET / HTTP/1.1\r\nHost: host \r\n\r\n", + expected_headers); + + // Regression test mixed spaces and tabs before and after the host header value. + sendAndValidateRequestAndSendResponse( + "GET / HTTP/1.1\r\nHost: host \r\n\r\n", expected_headers); +} + TEST_F(Http1ServerConnectionImplTest, Http10) { initialize(); diff --git a/test/common/http/http2/BUILD b/test/common/http/http2/BUILD index 3a93f99146e4..ded96525c02b 100644 --- a/test/common/http/http2/BUILD +++ b/test/common/http/http2/BUILD @@ -19,6 +19,7 @@ envoy_cc_test( "//source/common/event:dispatcher_lib", "//source/common/http:exception_lib", "//source/common/http:header_map_lib", + "//source/common/http:header_utility_lib", "//source/common/http/http2:codec_lib", "//source/common/stats:stats_lib", "//test/common/http:common_lib", diff --git a/test/integration/integration_test.cc b/test/integration/integration_test.cc index c65cbabdfd99..d9aadffa1f4a 100644 --- a/test/integration/integration_test.cc +++ b/test/integration/integration_test.cc @@ -446,13 +446,15 @@ TEST_P(IntegrationTest, TestInlineHeaders) { // Verify for HTTP/1.0 a keep-alive header results in no connection: close. // Also verify existing host headers are passed through for the HTTP/1.0 case. -TEST_P(IntegrationTest, Http10WithHostandKeepAlive) { +// This also regression tests proper handling of trailing whitespace after key +// values, specifically the host header. +TEST_P(IntegrationTest, Http10WithHostandKeepAliveAndLws) { autonomous_upstream_ = true; config_helper_.addConfigModifier(&setAllowHttp10WithDefaultHost); initialize(); std::string response; sendRawHttpAndWaitForResponse(lookupPort("http"), - "GET / HTTP/1.0\r\nHost: foo.com\r\nConnection:Keep-alive\r\n\r\n", + "GET / HTTP/1.0\r\nHost: foo.com \r\nConnection:Keep-alive\r\n\r\n", &response, true); EXPECT_THAT(response, HasSubstr("HTTP/1.0 200 OK\r\n")); EXPECT_THAT(response, Not(HasSubstr("connection: close"))); diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc index 31093c8b0e24..39e98f3e5ffb 100644 --- a/test/integration/protocol_integration_test.cc +++ b/test/integration/protocol_integration_test.cc @@ -1467,6 +1467,32 @@ TEST_P(ProtocolIntegrationTest, ConnDurationTimeoutNoHttpRequest) { test_server_->waitForCounterGe("http.config_test.downstream_cx_max_duration_reached", 1); } +// Make sure that invalid authority headers get blocked at or before the HCM. +TEST_P(DownstreamProtocolIntegrationTest, InvalidAuth) { + initialize(); + fake_upstreams_[0]->set_allow_unexpected_disconnects(true); + + codec_client_ = makeHttpConnection(lookupPort("http")); + + Http::TestHeaderMapImpl request_headers{{":method", "POST"}, + {":path", "/test/long/url"}, + {":scheme", "http"}, + {":authority", "ho|st|"}}; + + auto response = codec_client_->makeHeaderOnlyRequest(request_headers); + if (downstreamProtocol() == Http::CodecClient::Type::HTTP1) { + // For HTTP/1 this is handled by the HCM, which sends a full 400 response. + response->waitForEndStream(); + ASSERT_TRUE(response->complete()); + EXPECT_EQ("400", response->headers().Status()->value().getStringView()); + } else { + // For HTTP/2 this is handled by nghttp2 which resets the connection without + // sending an HTTP response. + codec_client_->waitForDisconnect(); + ASSERT_FALSE(response->complete()); + } +} + // For tests which focus on downstream-to-Envoy behavior, and don't need to be // run with both HTTP/1 and HTTP/2 upstreams. INSTANTIATE_TEST_SUITE_P(Protocols, DownstreamProtocolIntegrationTest,