From a9c92a9f45ec42c06bc98fa9811395e00221b1aa Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Mon, 24 May 2021 13:27:20 -0400 Subject: [PATCH 1/7] http3: improving header limit checks Signed-off-by: Alyssa Wilk --- source/common/quic/envoy_quic_client_stream.cc | 7 ++++--- source/common/quic/envoy_quic_server_stream.cc | 7 ++++--- source/common/quic/envoy_quic_utils.h | 10 +++++++++- test/common/quic/envoy_quic_utils_test.cc | 16 +++++++++++++++- test/integration/protocol_integration_test.cc | 5 +---- 5 files changed, 33 insertions(+), 12 deletions(-) diff --git a/source/common/quic/envoy_quic_client_stream.cc b/source/common/quic/envoy_quic_client_stream.cc index 804a39934fd85..95009b027a6b5 100644 --- a/source/common/quic/envoy_quic_client_stream.cc +++ b/source/common/quic/envoy_quic_client_stream.cc @@ -244,13 +244,14 @@ void EnvoyQuicClientStream::maybeDecodeTrailers() { if (sequencer()->IsClosed() && !FinishedReadingTrailers()) { // Only decode trailers after finishing decoding body. end_stream_decoded_ = true; - if (received_trailers().size() > filterManagerConnection()->maxIncomingHeadersCount()) { + auto trailers = spdyHeaderBlockToEnvoyHeaders( + received_trailers(), filterManagerConnection()->maxIncomingHeadersCount()); + if (trailers.get() == nullptr) { details_ = Http3ResponseCodeDetailValues::too_many_trailers; 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..ec0b63c45965a 100644 --- a/source/common/quic/envoy_quic_server_stream.cc +++ b/source/common/quic/envoy_quic_server_stream.cc @@ -245,13 +245,14 @@ void EnvoyQuicServerStream::maybeDecodeTrailers() { if (sequencer()->IsClosed() && !FinishedReadingTrailers()) { // Only decode trailers after finishing decoding body. end_stream_decoded_ = true; - if (received_trailers().size() > filterManagerConnection()->maxIncomingHeadersCount()) { + auto trailers = spdyHeaderBlockToEnvoyHeaders( + received_trailers(), filterManagerConnection()->maxIncomingHeadersCount()); + if (trailers.get() == nullptr) { details_ = Http3ResponseCodeDetailValues::too_many_trailers; onStreamError(close_connection_upon_invalid_header_); return; } - request_decoder_->decodeTrailers( - spdyHeaderBlockToEnvoyHeaders(received_trailers())); + request_decoder_->decodeTrailers(std::move(trailers)); MarkTrailersConsumed(); } } diff --git a/source/common/quic/envoy_quic_utils.h b/source/common/quic/envoy_quic_utils.h index 434fd939572c1..c1f83a509397a 100644 --- a/source/common/quic/envoy_quic_utils.h +++ b/source/common/quic/envoy_quic_utils.h @@ -105,14 +105,22 @@ quicHeadersToEnvoyHeaders(const quic::QuicHeaderList& header_list, HeaderValidat } template -std::unique_ptr spdyHeaderBlockToEnvoyHeaders(const spdy::SpdyHeaderBlock& header_block) { +std::unique_ptr spdyHeaderBlockToEnvoyHeaders(const spdy::SpdyHeaderBlock& header_block, + uint32_t max_headers_allowed) { auto headers = T::create(); + if (header_block.size() > max_headers_allowed) { + 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) { + if (max_headers_allowed == 0) { + return nullptr; + } + max_headers_allowed--; headers->addCopy(Http::LowerCaseString(key), value); } } diff --git a/test/common/quic/envoy_quic_utils_test.cc b/test/common/quic/envoy_quic_utils_test.cc index 20a1dfe989690..468daf727ac4e 100644 --- a/test/common/quic/envoy_quic_utils_test.cc +++ b/test/common/quic/envoy_quic_utils_test.cc @@ -61,7 +61,8 @@ TEST(EnvoyQuicUtilsTest, HeadersConversion) { // converting to Envoy headers. headers_block.AppendValueOrAddHeader("key", "value1"); headers_block.AppendValueOrAddHeader("key", "value2"); - auto envoy_headers = spdyHeaderBlockToEnvoyHeaders(headers_block); + auto envoy_headers = + spdyHeaderBlockToEnvoyHeaders(headers_block, 100); // 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()); @@ -110,5 +111,18 @@ 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); + // 6 headers are allowed. + EXPECT_NE(nullptr, spdyHeaderBlockToEnvoyHeaders(headers_block, 6)); + // Given the cap is 6, make sure anything lower, exact or otherwise, is rejected. + EXPECT_EQ(nullptr, spdyHeaderBlockToEnvoyHeaders(headers_block, 5)); + EXPECT_EQ(nullptr, spdyHeaderBlockToEnvoyHeaders(headers_block, 4)); +} + } // namespace Quic } // namespace Envoy diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc index 8a31ce5dda486..4c8a6bfa7c2ee 100644 --- a/test/integration/protocol_integration_test.cc +++ b/test/integration/protocol_integration_test.cc @@ -1825,10 +1825,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(); From b95a267821969b912b2e7bcd3c0a65929745eaad Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Mon, 24 May 2021 14:55:05 -0400 Subject: [PATCH 2/7] WIP: Signed-off-by: Alyssa Wilk --- source/common/quic/envoy_quic_stream.h | 3 ++ .../integration/quic_http_integration_test.cc | 49 +++++++++++++++++++ 2 files changed, 52 insertions(+) diff --git a/source/common/quic/envoy_quic_stream.h b/source/common/quic/envoy_quic_stream.h index f28cae4cf0119..21906f0735148 100644 --- a/source/common/quic/envoy_quic_stream.h +++ b/source/common/quic/envoy_quic_stream.h @@ -98,6 +98,9 @@ class EnvoyQuicStream : public virtual Http::StreamEncoder, validateHeader(const std::string& 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::validHeaderString(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/test/integration/quic_http_integration_test.cc b/test/integration/quic_http_integration_test.cc index a32fda3ddbee4..4ad5c4d541fe4 100644 --- a/test/integration/quic_http_integration_test.cc +++ b/test/integration/quic_http_integration_test.cc @@ -414,5 +414,54 @@ TEST_P(QuicHttpIntegrationTest, ResetRequestWithoutAuthorityHeader) { EXPECT_EQ("400", response->headers().getStatusValue()); } +TEST_P(QuicHttpIntegrationTest, ResetRequestWithNullCharacter) { + initialize(); + + codec_client_ = makeHttpConnection(lookupPort("http")); + + default_request_headers_.addCopy("illegal_header", std::string("foo\0bar", 7)); + auto encoder_decoder = codec_client_->startRequest(default_request_headers_); + request_encoder_ = &encoder_decoder.first; + auto response = std::move(encoder_decoder.second); + + ASSERT_TRUE(response->waitForEndStream()); + codec_client_->close(); + ASSERT_TRUE(response->complete()); + EXPECT_EQ("400", response->headers().getStatusValue()); +} + +TEST_P(QuicHttpIntegrationTest, MultipleSetCookieAndCookieHeaders) { + initialize(); + + codec_client_ = makeHttpConnection(lookupPort("http")); + auto encoder_decoder = + codec_client_->startRequest(Http::TestRequestHeaderMapImpl{{":method", "GET"}, + {":path", "/dynamo/url"}, + {":scheme", "http"}, + {":authority", "host"}, + {"cookie", "a=b"}, + {"cookie", "c=d"}}); + request_encoder_ = &encoder_decoder.first; + auto response = std::move(encoder_decoder.second); + codec_client_->sendData(*request_encoder_, 0, true); + waitForNextUpstreamRequest(); + if (Runtime::runtimeFeatureEnabled( + "envoy.reloadable_features.header_map_correctly_coalesce_cookies")) { + EXPECT_EQ(upstream_request_->headers().get(Http::Headers::get().Cookie)[0]->value(), + "a=b; c=d"); + } + + upstream_request_->encodeHeaders(Http::TestResponseHeaderMapImpl{{":status", "200"}, + {"set-cookie", "foo"}, + {"set-cookie", "bar"}}, + true); + ASSERT_TRUE(response->waitForEndStream()); + EXPECT_TRUE(response->complete()); + const auto out = response->headers().get(Http::LowerCaseString("set-cookie")); + ASSERT_EQ(out.size(), 2); + ASSERT_EQ(out[0]->value().getStringView(), "foo"); + ASSERT_EQ(out[1]->value().getStringView(), "bar"); +} + } // namespace Quic } // namespace Envoy From fe21d61fe387fecd2b58a962545b0473852b96f6 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Wed, 28 Jul 2021 08:38:05 -0400 Subject: [PATCH 3/7] quic: improving header size validation Signed-off-by: Alyssa Wilk --- source/common/quic/envoy_quic_stream.h | 2 +- .../integration/quic_http_integration_test.cc | 44 +++---------------- 2 files changed, 6 insertions(+), 40 deletions(-) diff --git a/source/common/quic/envoy_quic_stream.h b/source/common/quic/envoy_quic_stream.h index 21906f0735148..46b9a041224a5 100644 --- a/source/common/quic/envoy_quic_stream.h +++ b/source/common/quic/envoy_quic_stream.h @@ -98,7 +98,7 @@ class EnvoyQuicStream : public virtual Http::StreamEncoder, validateHeader(const std::string& 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::validHeaderString(header_value)) { + if (!Http::HeaderUtility::headerValueIsValid(header_value)) { return Http::HeaderUtility::HeaderValidationResult::REJECT; } if (header_name == "content-length") { diff --git a/test/integration/quic_http_integration_test.cc b/test/integration/quic_http_integration_test.cc index 4ad5c4d541fe4..16f0aa1870ade 100644 --- a/test/integration/quic_http_integration_test.cc +++ b/test/integration/quic_http_integration_test.cc @@ -414,53 +414,19 @@ TEST_P(QuicHttpIntegrationTest, ResetRequestWithoutAuthorityHeader) { EXPECT_EQ("400", response->headers().getStatusValue()); } -TEST_P(QuicHttpIntegrationTest, ResetRequestWithNullCharacter) { +TEST_P(QuicHttpIntegrationTest, ResetRequestWithInvalidCharacter) { initialize(); codec_client_ = makeHttpConnection(lookupPort("http")); - default_request_headers_.addCopy("illegal_header", std::string("foo\0bar", 7)); + 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->waitForEndStream()); - codec_client_->close(); - ASSERT_TRUE(response->complete()); - EXPECT_EQ("400", response->headers().getStatusValue()); -} - -TEST_P(QuicHttpIntegrationTest, MultipleSetCookieAndCookieHeaders) { - initialize(); - - codec_client_ = makeHttpConnection(lookupPort("http")); - auto encoder_decoder = - codec_client_->startRequest(Http::TestRequestHeaderMapImpl{{":method", "GET"}, - {":path", "/dynamo/url"}, - {":scheme", "http"}, - {":authority", "host"}, - {"cookie", "a=b"}, - {"cookie", "c=d"}}); - request_encoder_ = &encoder_decoder.first; - auto response = std::move(encoder_decoder.second); - codec_client_->sendData(*request_encoder_, 0, true); - waitForNextUpstreamRequest(); - if (Runtime::runtimeFeatureEnabled( - "envoy.reloadable_features.header_map_correctly_coalesce_cookies")) { - EXPECT_EQ(upstream_request_->headers().get(Http::Headers::get().Cookie)[0]->value(), - "a=b; c=d"); - } - - upstream_request_->encodeHeaders(Http::TestResponseHeaderMapImpl{{":status", "200"}, - {"set-cookie", "foo"}, - {"set-cookie", "bar"}}, - true); - ASSERT_TRUE(response->waitForEndStream()); - EXPECT_TRUE(response->complete()); - const auto out = response->headers().get(Http::LowerCaseString("set-cookie")); - ASSERT_EQ(out.size(), 2); - ASSERT_EQ(out[0]->value().getStringView(), "foo"); - ASSERT_EQ(out[1]->value().getStringView(), "bar"); + ASSERT_TRUE(response->waitForReset()); } } // namespace Quic From ee56815876a506ce2671a5fb5272ca8abf23612f Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Wed, 28 Jul 2021 10:07:41 -0400 Subject: [PATCH 4/7] adding whitespace test Signed-off-by: Alyssa Wilk --- test/integration/protocol_integration_test.cc | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc index 4c8a6bfa7c2ee..2d324879fb87d 100644 --- a/test/integration/protocol_integration_test.cc +++ b/test/integration/protocol_integration_test.cc @@ -98,6 +98,31 @@ TEST_P(DownstreamProtocolIntegrationTest, RouterClusterNotFound404) { EXPECT_EQ("404", response->headers().getStatusValue()); } +// Add a route that uses unknown cluster (expect 404 Not Found). +TEST_P(DownstreamProtocolIntegrationTest, RouterClusterUsedRegardlessOfWhitespace) { + 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", "CONNECT"}, {":authority", " foo.com "}}); + request_encoder_ = &encoder_decoder.first; + auto response = std::move(encoder_decoder.second); + + if (downstreamProtocol() == Http::CodecType::HTTP1) { + ASSERT_TRUE(response->waitForEndStream()); + EXPECT_EQ("400", response->headers().getStatusValue()); + EXPECT_TRUE(response->complete()); + } else { + 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); From b7bbdd17e0e68b9ed3094e2b9c17cb7e9cf82437 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Wed, 28 Jul 2021 10:20:16 -0400 Subject: [PATCH 5/7] adding whitespace test Signed-off-by: Alyssa Wilk --- test/integration/protocol_integration_test.cc | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc index 2d324879fb87d..291596fb5308e 100644 --- a/test/integration/protocol_integration_test.cc +++ b/test/integration/protocol_integration_test.cc @@ -98,8 +98,7 @@ TEST_P(DownstreamProtocolIntegrationTest, RouterClusterNotFound404) { EXPECT_EQ("404", response->headers().getStatusValue()); } -// Add a route that uses unknown cluster (expect 404 Not Found). -TEST_P(DownstreamProtocolIntegrationTest, RouterClusterUsedRegardlessOfWhitespace) { +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( @@ -108,16 +107,18 @@ TEST_P(DownstreamProtocolIntegrationTest, RouterClusterUsedRegardlessOfWhitespac initialize(); codec_client_ = makeHttpConnection(lookupPort("http")); - auto encoder_decoder = codec_client_->startRequest( - Http::TestRequestHeaderMapImpl{{":method", "CONNECT"}, {":authority", " foo.com "}}); + 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("400", response->headers().getStatusValue()); + 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()); } From a061c08368a87ee19ea945e16700039d6dcbea88 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Wed, 28 Jul 2021 14:48:02 -0400 Subject: [PATCH 6/7] tidy + trailers Signed-off-by: Alyssa Wilk --- source/common/http/header_utility.cc | 2 +- source/common/http/header_utility.h | 2 +- .../common/quic/envoy_quic_client_stream.cc | 5 ++-- .../common/quic/envoy_quic_server_stream.cc | 6 ++--- source/common/quic/envoy_quic_server_stream.h | 2 +- source/common/quic/envoy_quic_stream.h | 2 +- source/common/quic/envoy_quic_utils.h | 19 +++++++++++--- test/common/quic/envoy_quic_utils_test.cc | 26 ++++++++++++------- .../multiplexed_integration_test.cc | 17 ++++++++++++ 9 files changed, 58 insertions(+), 23 deletions(-) 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 95009b027a6b5..09778a5d81633 100644 --- a/source/common/quic/envoy_quic_client_stream.cc +++ b/source/common/quic/envoy_quic_client_stream.cc @@ -245,9 +245,8 @@ void EnvoyQuicClientStream::maybeDecodeTrailers() { // Only decode trailers after finishing decoding body. end_stream_decoded_ = true; auto trailers = spdyHeaderBlockToEnvoyHeaders( - received_trailers(), filterManagerConnection()->maxIncomingHeadersCount()); - if (trailers.get() == nullptr) { - details_ = Http3ResponseCodeDetailValues::too_many_trailers; + received_trailers(), filterManagerConnection()->maxIncomingHeadersCount(), *this, details_); + if (trailers == nullptr) { onStreamError(close_connection_upon_invalid_header_, quic::QUIC_STREAM_EXCESSIVE_LOAD); return; } diff --git a/source/common/quic/envoy_quic_server_stream.cc b/source/common/quic/envoy_quic_server_stream.cc index ec0b63c45965a..436f64766287e 100644 --- a/source/common/quic/envoy_quic_server_stream.cc +++ b/source/common/quic/envoy_quic_server_stream.cc @@ -246,8 +246,8 @@ void EnvoyQuicServerStream::maybeDecodeTrailers() { // Only decode trailers after finishing decoding body. end_stream_decoded_ = true; auto trailers = spdyHeaderBlockToEnvoyHeaders( - received_trailers(), filterManagerConnection()->maxIncomingHeadersCount()); - if (trailers.get() == nullptr) { + received_trailers(), filterManagerConnection()->maxIncomingHeadersCount(), *this, details_); + if (trailers == nullptr) { details_ = Http3ResponseCodeDetailValues::too_many_trailers; onStreamError(close_connection_upon_invalid_header_); return; @@ -338,7 +338,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 46b9a041224a5..d0ddd69d39872 100644 --- a/source/common/quic/envoy_quic_stream.h +++ b/source/common/quic/envoy_quic_stream.h @@ -95,7 +95,7 @@ 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)) { diff --git a/source/common/quic/envoy_quic_utils.h b/source/common/quic/envoy_quic_utils.h index c1f83a509397a..ee899df7d1041 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. @@ -106,7 +106,9 @@ quicHeadersToEnvoyHeaders(const quic::QuicHeaderList& header_list, HeaderValidat template std::unique_ptr spdyHeaderBlockToEnvoyHeaders(const spdy::SpdyHeaderBlock& header_block, - uint32_t max_headers_allowed) { + uint32_t max_headers_allowed, + HeaderValidator& validator, + absl::string_view& details) { auto headers = T::create(); if (header_block.size() > max_headers_allowed) { return nullptr; @@ -118,10 +120,21 @@ std::unique_ptr spdyHeaderBlockToEnvoyHeaders(const spdy::SpdyHeaderBlock& he std::vector values = absl::StrSplit(entry.second, '\0'); for (const absl::string_view& value : values) { if (max_headers_allowed == 0) { + details = Http3ResponseCodeDetailValues::invalid_http_header; return nullptr; } max_headers_allowed--; - headers->addCopy(Http::LowerCaseString(key), value); + Http::HeaderUtility::HeaderValidationResult result = + validator.validateHeader(entry.first, value); + switch (result) { + case Http::HeaderUtility::HeaderValidationResult::REJECT: + // The validator sets the details to Http3ResponseCodeDetailValues::invalid_underscore + 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 468daf727ac4e..2efa93cf9c39e 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,8 +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, 100); + NiceMock validator; + absl::string_view details; + auto envoy_headers = spdyHeaderBlockToEnvoyHeaders( + 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()); @@ -80,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); @@ -101,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; } @@ -117,11 +118,16 @@ TEST(EnvoyQuicUtilsTest, HeadersSizeBounds) { 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. - EXPECT_NE(nullptr, spdyHeaderBlockToEnvoyHeaders(headers_block, 6)); + NiceMock validator; + EXPECT_NE(nullptr, spdyHeaderBlockToEnvoyHeaders(headers_block, 6, + validator, details)); // Given the cap is 6, make sure anything lower, exact or otherwise, is rejected. - EXPECT_EQ(nullptr, spdyHeaderBlockToEnvoyHeaders(headers_block, 5)); - EXPECT_EQ(nullptr, spdyHeaderBlockToEnvoyHeaders(headers_block, 4)); + EXPECT_EQ(nullptr, spdyHeaderBlockToEnvoyHeaders(headers_block, 5, + validator, details)); + EXPECT_EQ(nullptr, spdyHeaderBlockToEnvoyHeaders(headers_block, 4, + validator, details)); } } // namespace Quic diff --git a/test/integration/multiplexed_integration_test.cc b/test/integration/multiplexed_integration_test.cc index cf9980620f788..35a50295b80a5 100644 --- a/test/integration/multiplexed_integration_test.cc +++ b/test/integration/multiplexed_integration_test.cc @@ -1817,4 +1817,21 @@ TEST_P(Http2IntegrationTest, OnLocalReply) { } } +TEST_P(Http2IntegrationTest, InvalidTrailers) { + 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()); +} + } // namespace Envoy From 1bc62ecee92f43036b9686104973a871bc544ad8 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Wed, 28 Jul 2021 16:55:18 -0400 Subject: [PATCH 7/7] fixes and more tests Signed-off-by: Alyssa Wilk --- .../common/quic/envoy_quic_client_stream.cc | 2 +- .../common/quic/envoy_quic_server_stream.cc | 3 +- source/common/quic/envoy_quic_utils.h | 12 ++++---- test/common/quic/envoy_quic_utils_test.cc | 28 ++++++++++++++----- .../multiplexed_integration_test.cc | 3 ++ 5 files changed, 32 insertions(+), 16 deletions(-) diff --git a/source/common/quic/envoy_quic_client_stream.cc b/source/common/quic/envoy_quic_client_stream.cc index 09778a5d81633..09e6a921b5966 100644 --- a/source/common/quic/envoy_quic_client_stream.cc +++ b/source/common/quic/envoy_quic_client_stream.cc @@ -244,7 +244,7 @@ void EnvoyQuicClientStream::maybeDecodeTrailers() { if (sequencer()->IsClosed() && !FinishedReadingTrailers()) { // Only decode trailers after finishing decoding body. end_stream_decoded_ = true; - auto trailers = spdyHeaderBlockToEnvoyHeaders( + auto trailers = spdyHeaderBlockToEnvoyTrailers( received_trailers(), filterManagerConnection()->maxIncomingHeadersCount(), *this, details_); if (trailers == nullptr) { onStreamError(close_connection_upon_invalid_header_, quic::QUIC_STREAM_EXCESSIVE_LOAD); diff --git a/source/common/quic/envoy_quic_server_stream.cc b/source/common/quic/envoy_quic_server_stream.cc index 436f64766287e..eef432bb57f7e 100644 --- a/source/common/quic/envoy_quic_server_stream.cc +++ b/source/common/quic/envoy_quic_server_stream.cc @@ -245,10 +245,9 @@ void EnvoyQuicServerStream::maybeDecodeTrailers() { if (sequencer()->IsClosed() && !FinishedReadingTrailers()) { // Only decode trailers after finishing decoding body. end_stream_decoded_ = true; - auto trailers = spdyHeaderBlockToEnvoyHeaders( + auto trailers = spdyHeaderBlockToEnvoyTrailers( received_trailers(), filterManagerConnection()->maxIncomingHeadersCount(), *this, details_); if (trailers == nullptr) { - details_ = Http3ResponseCodeDetailValues::too_many_trailers; onStreamError(close_connection_upon_invalid_header_); return; } diff --git a/source/common/quic/envoy_quic_utils.h b/source/common/quic/envoy_quic_utils.h index ee899df7d1041..e2beaa9009c1f 100644 --- a/source/common/quic/envoy_quic_utils.h +++ b/source/common/quic/envoy_quic_utils.h @@ -105,12 +105,13 @@ quicHeadersToEnvoyHeaders(const quic::QuicHeaderList& header_list, HeaderValidat } template -std::unique_ptr spdyHeaderBlockToEnvoyHeaders(const spdy::SpdyHeaderBlock& header_block, - uint32_t max_headers_allowed, - HeaderValidator& validator, - absl::string_view& details) { +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) { @@ -120,7 +121,7 @@ std::unique_ptr spdyHeaderBlockToEnvoyHeaders(const spdy::SpdyHeaderBlock& he std::vector values = absl::StrSplit(entry.second, '\0'); for (const absl::string_view& value : values) { if (max_headers_allowed == 0) { - details = Http3ResponseCodeDetailValues::invalid_http_header; + details = Http3ResponseCodeDetailValues::too_many_trailers; return nullptr; } max_headers_allowed--; @@ -128,7 +129,6 @@ std::unique_ptr spdyHeaderBlockToEnvoyHeaders(const spdy::SpdyHeaderBlock& he validator.validateHeader(entry.first, value); switch (result) { case Http::HeaderUtility::HeaderValidationResult::REJECT: - // The validator sets the details to Http3ResponseCodeDetailValues::invalid_underscore return nullptr; case Http::HeaderUtility::HeaderValidationResult::DROP: continue; diff --git a/test/common/quic/envoy_quic_utils_test.cc b/test/common/quic/envoy_quic_utils_test.cc index 2efa93cf9c39e..8cc0aa18b0cc2 100644 --- a/test/common/quic/envoy_quic_utils_test.cc +++ b/test/common/quic/envoy_quic_utils_test.cc @@ -64,7 +64,7 @@ TEST(EnvoyQuicUtilsTest, HeadersConversion) { headers_block.AppendValueOrAddHeader("key", "value2"); NiceMock validator; absl::string_view details; - auto envoy_headers = spdyHeaderBlockToEnvoyHeaders( + 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()); @@ -121,13 +121,27 @@ TEST(EnvoyQuicUtilsTest, HeadersSizeBounds) { absl::string_view details; // 6 headers are allowed. NiceMock validator; - EXPECT_NE(nullptr, spdyHeaderBlockToEnvoyHeaders(headers_block, 6, - validator, details)); + 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, spdyHeaderBlockToEnvoyHeaders(headers_block, 5, - validator, details)); - EXPECT_EQ(nullptr, spdyHeaderBlockToEnvoyHeaders(headers_block, 4, - validator, details)); + 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 diff --git a/test/integration/multiplexed_integration_test.cc b/test/integration/multiplexed_integration_test.cc index 35a50295b80a5..f139528b9723e 100644 --- a/test/integration/multiplexed_integration_test.cc +++ b/test/integration/multiplexed_integration_test.cc @@ -1818,6 +1818,7 @@ TEST_P(Http2IntegrationTest, OnLocalReply) { } TEST_P(Http2IntegrationTest, InvalidTrailers) { + useAccessLog("%RESPONSE_CODE_DETAILS%"); autonomous_upstream_ = true; initialize(); codec_client_ = makeHttpConnection(lookupPort("http")); @@ -1832,6 +1833,8 @@ TEST_P(Http2IntegrationTest, InvalidTrailers) { 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