diff --git a/source/common/http/header_utility.cc b/source/common/http/header_utility.cc index b0fc8d02fe5e3..4dfd9b9556a0a 100644 --- a/source/common/http/header_utility.cc +++ b/source/common/http/header_utility.cc @@ -368,7 +368,7 @@ bool HeaderUtility::isModifiableHeader(absl::string_view header) { } HeaderUtility::HeaderValidationResult HeaderUtility::checkHeaderNameForUnderscores( - const std::string& header_name, + absl::string_view header_name, envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction headers_with_underscores_action, Stats::Counter& dropped_headers_with_underscores, diff --git a/source/common/http/header_utility.h b/source/common/http/header_utility.h index 50b1d571f60e1..1053f893c34f4 100644 --- a/source/common/http/header_utility.h +++ b/source/common/http/header_utility.h @@ -250,7 +250,7 @@ class HeaderUtility { * headers_with_underscores_action. */ static HeaderValidationResult checkHeaderNameForUnderscores( - const std::string& header_name, + absl::string_view header_name, envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction headers_with_underscores_action, Stats::Counter& dropped_headers_with_underscores, diff --git a/source/common/quic/envoy_quic_client_stream.cc b/source/common/quic/envoy_quic_client_stream.cc index 804a39934fd85..09e6a921b5966 100644 --- a/source/common/quic/envoy_quic_client_stream.cc +++ b/source/common/quic/envoy_quic_client_stream.cc @@ -244,13 +244,13 @@ void EnvoyQuicClientStream::maybeDecodeTrailers() { if (sequencer()->IsClosed() && !FinishedReadingTrailers()) { // Only decode trailers after finishing decoding body. end_stream_decoded_ = true; - if (received_trailers().size() > filterManagerConnection()->maxIncomingHeadersCount()) { - details_ = Http3ResponseCodeDetailValues::too_many_trailers; + auto trailers = spdyHeaderBlockToEnvoyTrailers( + received_trailers(), filterManagerConnection()->maxIncomingHeadersCount(), *this, details_); + if (trailers == nullptr) { onStreamError(close_connection_upon_invalid_header_, quic::QUIC_STREAM_EXCESSIVE_LOAD); return; } - response_decoder_->decodeTrailers( - spdyHeaderBlockToEnvoyHeaders(received_trailers())); + response_decoder_->decodeTrailers(std::move(trailers)); MarkTrailersConsumed(); } } diff --git a/source/common/quic/envoy_quic_server_stream.cc b/source/common/quic/envoy_quic_server_stream.cc index aa100414b93be..eef432bb57f7e 100644 --- a/source/common/quic/envoy_quic_server_stream.cc +++ b/source/common/quic/envoy_quic_server_stream.cc @@ -245,13 +245,13 @@ void EnvoyQuicServerStream::maybeDecodeTrailers() { if (sequencer()->IsClosed() && !FinishedReadingTrailers()) { // Only decode trailers after finishing decoding body. end_stream_decoded_ = true; - if (received_trailers().size() > filterManagerConnection()->maxIncomingHeadersCount()) { - details_ = Http3ResponseCodeDetailValues::too_many_trailers; + auto trailers = spdyHeaderBlockToEnvoyTrailers( + received_trailers(), filterManagerConnection()->maxIncomingHeadersCount(), *this, details_); + if (trailers == nullptr) { onStreamError(close_connection_upon_invalid_header_); return; } - request_decoder_->decodeTrailers( - spdyHeaderBlockToEnvoyHeaders(received_trailers())); + request_decoder_->decodeTrailers(std::move(trailers)); MarkTrailersConsumed(); } } @@ -337,7 +337,7 @@ QuicFilterManagerConnectionImpl* EnvoyQuicServerStream::filterManagerConnection( } Http::HeaderUtility::HeaderValidationResult -EnvoyQuicServerStream::validateHeader(const std::string& header_name, +EnvoyQuicServerStream::validateHeader(absl::string_view header_name, absl::string_view header_value) { Http::HeaderUtility::HeaderValidationResult result = EnvoyQuicStream::validateHeader(header_name, header_value); diff --git a/source/common/quic/envoy_quic_server_stream.h b/source/common/quic/envoy_quic_server_stream.h index e0b98e835fdad..fcc41c2eb5e21 100644 --- a/source/common/quic/envoy_quic_server_stream.h +++ b/source/common/quic/envoy_quic_server_stream.h @@ -72,7 +72,7 @@ class EnvoyQuicServerStream : public quic::QuicSpdyServerStreamBase, // EnvoyQuicStream Http::HeaderUtility::HeaderValidationResult - validateHeader(const std::string& header_name, absl::string_view header_value) override; + validateHeader(absl::string_view header_name, absl::string_view header_value) override; protected: // EnvoyQuicStream diff --git a/source/common/quic/envoy_quic_stream.h b/source/common/quic/envoy_quic_stream.h index f28cae4cf0119..d0ddd69d39872 100644 --- a/source/common/quic/envoy_quic_stream.h +++ b/source/common/quic/envoy_quic_stream.h @@ -95,9 +95,12 @@ class EnvoyQuicStream : public virtual Http::StreamEncoder, } Http::HeaderUtility::HeaderValidationResult - validateHeader(const std::string& header_name, absl::string_view header_value) override { + validateHeader(absl::string_view header_name, absl::string_view header_value) override { bool override_stream_error_on_invalid_http_message = http3_options_.override_stream_error_on_invalid_http_message().value(); + if (!Http::HeaderUtility::headerValueIsValid(header_value)) { + return Http::HeaderUtility::HeaderValidationResult::REJECT; + } if (header_name == "content-length") { return Http::HeaderUtility::validateContentLength( header_value, override_stream_error_on_invalid_http_message, diff --git a/source/common/quic/envoy_quic_utils.h b/source/common/quic/envoy_quic_utils.h index 434fd939572c1..e2beaa9009c1f 100644 --- a/source/common/quic/envoy_quic_utils.h +++ b/source/common/quic/envoy_quic_utils.h @@ -66,7 +66,7 @@ class HeaderValidator { public: virtual ~HeaderValidator() = default; virtual Http::HeaderUtility::HeaderValidationResult - validateHeader(const std::string& header_name, absl::string_view header_value) = 0; + validateHeader(absl::string_view name, absl::string_view header_value) = 0; }; // The returned header map has all keys in lower case. @@ -105,15 +105,36 @@ quicHeadersToEnvoyHeaders(const quic::QuicHeaderList& header_list, HeaderValidat } template -std::unique_ptr spdyHeaderBlockToEnvoyHeaders(const spdy::SpdyHeaderBlock& header_block) { +std::unique_ptr spdyHeaderBlockToEnvoyTrailers(const spdy::SpdyHeaderBlock& header_block, + uint32_t max_headers_allowed, + HeaderValidator& validator, + absl::string_view& details) { auto headers = T::create(); + if (header_block.size() > max_headers_allowed) { + details = Http3ResponseCodeDetailValues::too_many_trailers; + return nullptr; + } for (auto entry : header_block) { // TODO(danzh): Avoid temporary strings and addCopy() with string_view. std::string key(entry.first); // QUICHE coalesces multiple trailer values with the same key with '\0'. std::vector values = absl::StrSplit(entry.second, '\0'); for (const absl::string_view& value : values) { - headers->addCopy(Http::LowerCaseString(key), value); + if (max_headers_allowed == 0) { + details = Http3ResponseCodeDetailValues::too_many_trailers; + return nullptr; + } + max_headers_allowed--; + Http::HeaderUtility::HeaderValidationResult result = + validator.validateHeader(entry.first, value); + switch (result) { + case Http::HeaderUtility::HeaderValidationResult::REJECT: + return nullptr; + case Http::HeaderUtility::HeaderValidationResult::DROP: + continue; + case Http::HeaderUtility::HeaderValidationResult::ACCEPT: + headers->addCopy(Http::LowerCaseString(key), value); + } } } return headers; diff --git a/test/common/quic/envoy_quic_utils_test.cc b/test/common/quic/envoy_quic_utils_test.cc index 20a1dfe989690..8cc0aa18b0cc2 100644 --- a/test/common/quic/envoy_quic_utils_test.cc +++ b/test/common/quic/envoy_quic_utils_test.cc @@ -19,6 +19,7 @@ #include "gtest/gtest.h" using testing::_; +using testing::NiceMock; using testing::Return; namespace Envoy { @@ -49,7 +50,7 @@ class MockHeaderValidator : public HeaderValidator { public: ~MockHeaderValidator() override = default; MOCK_METHOD(Http::HeaderUtility::HeaderValidationResult, validateHeader, - (const std::string& header_name, absl::string_view header_value)); + (absl::string_view header_name, absl::string_view header_value)); }; TEST(EnvoyQuicUtilsTest, HeadersConversion) { @@ -61,7 +62,10 @@ TEST(EnvoyQuicUtilsTest, HeadersConversion) { // converting to Envoy headers. headers_block.AppendValueOrAddHeader("key", "value1"); headers_block.AppendValueOrAddHeader("key", "value2"); - auto envoy_headers = spdyHeaderBlockToEnvoyHeaders(headers_block); + NiceMock validator; + absl::string_view details; + auto envoy_headers = spdyHeaderBlockToEnvoyTrailers( + headers_block, 100, validator, details); // Envoy header block is 1 header larger because QUICHE header block does coalescing. EXPECT_EQ(headers_block.size() + 1u, envoy_headers->size()); EXPECT_EQ("www.google.com", envoy_headers->getHostValue()); @@ -79,15 +83,13 @@ TEST(EnvoyQuicUtilsTest, HeadersConversion) { quic_headers.OnHeader("key", "value2"); quic_headers.OnHeader("key-to-drop", ""); quic_headers.OnHeaderBlockEnd(0, 0); - MockHeaderValidator validator; EXPECT_CALL(validator, validateHeader(_, _)) - .WillRepeatedly([](const std::string& header_name, absl::string_view) { + .WillRepeatedly([](absl::string_view header_name, absl::string_view) { if (header_name == "key-to-drop") { return Http::HeaderUtility::HeaderValidationResult::DROP; } return Http::HeaderUtility::HeaderValidationResult::ACCEPT; }); - absl::string_view details; auto envoy_headers2 = quicHeadersToEnvoyHeaders(quic_headers, validator, 100, details); EXPECT_EQ(*envoy_headers, *envoy_headers2); @@ -100,7 +102,7 @@ TEST(EnvoyQuicUtilsTest, HeadersConversion) { quic_headers2.OnHeader("invalid_key", ""); quic_headers2.OnHeaderBlockEnd(0, 0); EXPECT_CALL(validator, validateHeader(_, _)) - .WillRepeatedly([](const std::string& header_name, absl::string_view) { + .WillRepeatedly([](absl::string_view header_name, absl::string_view) { if (header_name == "invalid_key") { return Http::HeaderUtility::HeaderValidationResult::REJECT; } @@ -110,5 +112,37 @@ TEST(EnvoyQuicUtilsTest, HeadersConversion) { 100, details)); } +TEST(EnvoyQuicUtilsTest, HeadersSizeBounds) { + spdy::SpdyHeaderBlock headers_block; + headers_block[":authority"] = "www.google.com"; + headers_block[":path"] = "/index.hml"; + headers_block[":scheme"] = "https"; + headers_block["foo"] = std::string("bar\0eep\0baz", 11); + absl::string_view details; + // 6 headers are allowed. + NiceMock validator; + EXPECT_NE(nullptr, spdyHeaderBlockToEnvoyTrailers( + headers_block, 6, validator, details)); + // Given the cap is 6, make sure anything lower, exact or otherwise, is rejected. + EXPECT_EQ(nullptr, spdyHeaderBlockToEnvoyTrailers( + headers_block, 5, validator, details)); + EXPECT_EQ("http3.too_many_trailers", details); + EXPECT_EQ(nullptr, spdyHeaderBlockToEnvoyTrailers( + headers_block, 4, validator, details)); +} + +TEST(EnvoyQuicUtilsTest, TrailerCharacters) { + spdy::SpdyHeaderBlock headers_block; + headers_block[":authority"] = "www.google.com"; + headers_block[":path"] = "/index.hml"; + headers_block[":scheme"] = "https"; + absl::string_view details; + NiceMock validator; + EXPECT_CALL(validator, validateHeader(_, _)) + .WillRepeatedly(Return(Http::HeaderUtility::HeaderValidationResult::REJECT)); + EXPECT_EQ(nullptr, spdyHeaderBlockToEnvoyTrailers( + headers_block, 5, validator, details)); +} + } // namespace Quic } // namespace Envoy diff --git a/test/integration/multiplexed_integration_test.cc b/test/integration/multiplexed_integration_test.cc index cf9980620f788..f139528b9723e 100644 --- a/test/integration/multiplexed_integration_test.cc +++ b/test/integration/multiplexed_integration_test.cc @@ -1817,4 +1817,24 @@ TEST_P(Http2IntegrationTest, OnLocalReply) { } } +TEST_P(Http2IntegrationTest, InvalidTrailers) { + useAccessLog("%RESPONSE_CODE_DETAILS%"); + autonomous_upstream_ = true; + initialize(); + codec_client_ = makeHttpConnection(lookupPort("http")); + + // Start the request. + auto encoder_decoder = codec_client_->startRequest(default_request_headers_); + auto response = std::move(encoder_decoder.second); + request_encoder_ = &encoder_decoder.first; + + std::string value = std::string(1, 2); + EXPECT_FALSE(Http::HeaderUtility::headerValueIsValid(value)); + codec_client_->sendTrailers(*request_encoder_, + Http::TestRequestTrailerMapImpl{{"trailer", value}}); + ASSERT_TRUE(response->waitForReset()); + // http2.invalid.header.field or http3.invalid_header_field + EXPECT_THAT(waitForAccessLog(access_log_name_), HasSubstr("invalid")); +} + } // namespace Envoy diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc index 8a31ce5dda486..291596fb5308e 100644 --- a/test/integration/protocol_integration_test.cc +++ b/test/integration/protocol_integration_test.cc @@ -98,6 +98,32 @@ TEST_P(DownstreamProtocolIntegrationTest, RouterClusterNotFound404) { EXPECT_EQ("404", response->headers().getStatusValue()); } +TEST_P(DownstreamProtocolIntegrationTest, TestHostWhitespacee) { + config_helper_.addConfigModifier(&setDoNotValidateRouteConfig); + auto host = config_helper_.createVirtualHost("foo.com", "/unknown", "unknown_cluster"); + host.mutable_routes(0)->mutable_route()->set_cluster_not_found_response_code( + envoy::config::route::v3::RouteAction::NOT_FOUND); + config_helper_.addVirtualHost(host); + initialize(); + + codec_client_ = makeHttpConnection(lookupPort("http")); + auto encoder_decoder = codec_client_->startRequest(Http::TestRequestHeaderMapImpl{ + {":method", "GET"}, {":authority", " foo.com "}, {":path", "/unknown"}}); + request_encoder_ = &encoder_decoder.first; + auto response = std::move(encoder_decoder.second); + + // For HTTP/1 the whitespace will be stripped, and 404 returned as above. + if (downstreamProtocol() == Http::CodecType::HTTP1) { + ASSERT_TRUE(response->waitForEndStream()); + EXPECT_EQ("404", response->headers().getStatusValue()); + EXPECT_TRUE(response->complete()); + } else { + // For HTTP/2 and above, the whitespace is illegal. + ASSERT_TRUE(response->waitForReset()); + ASSERT_TRUE(codec_client_->waitForDisconnect()); + } +} + // Add a route that uses unknown cluster (expect 503 Service Unavailable). TEST_P(DownstreamProtocolIntegrationTest, RouterClusterNotFound503) { config_helper_.addConfigModifier(&setDoNotValidateRouteConfig); @@ -1825,10 +1851,7 @@ TEST_P(DownstreamProtocolIntegrationTest, ManyRequestTrailersRejected) { config_helper_.addConfigModifier(setEnableUpstreamTrailersHttp1()); Http::TestRequestTrailerMapImpl request_trailers; for (int i = 0; i < 150; i++) { - // TODO(alyssawilk) QUIC fails without the trailers being distinct because - // the checks are done before transformation. Either make the transformation - // use commas, or do QUIC checks before and after. - request_trailers.addCopy(absl::StrCat("trailer", i), std::string(1, 'a')); + request_trailers.addCopy("trailer", std::string(1, 'a')); } initialize(); diff --git a/test/integration/quic_http_integration_test.cc b/test/integration/quic_http_integration_test.cc index a32fda3ddbee4..16f0aa1870ade 100644 --- a/test/integration/quic_http_integration_test.cc +++ b/test/integration/quic_http_integration_test.cc @@ -414,5 +414,20 @@ TEST_P(QuicHttpIntegrationTest, ResetRequestWithoutAuthorityHeader) { EXPECT_EQ("400", response->headers().getStatusValue()); } +TEST_P(QuicHttpIntegrationTest, ResetRequestWithInvalidCharacter) { + initialize(); + + codec_client_ = makeHttpConnection(lookupPort("http")); + + std::string value = std::string(1, 2); + EXPECT_FALSE(Http::HeaderUtility::headerValueIsValid(value)); + default_request_headers_.addCopy("illegal_header", value); + auto encoder_decoder = codec_client_->startRequest(default_request_headers_); + request_encoder_ = &encoder_decoder.first; + auto response = std::move(encoder_decoder.second); + + ASSERT_TRUE(response->waitForReset()); +} + } // namespace Quic } // namespace Envoy