quic: enforcing header size and contents

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,

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,

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<Http::ResponseTrailerMapImpl>(
+        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<Http::ResponseTrailerMapImpl>(received_trailers()));
+    response_decoder_->decodeTrailers(std::move(trailers));
     MarkTrailersConsumed();
   }
 }

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<Http::RequestTrailerMapImpl>(
+        received_trailers(), filterManagerConnection()->maxIncomingHeadersCount(), *this, details_);
+    if (trailers == nullptr) {
       onStreamError(close_connection_upon_invalid_header_);
       return;
     }
-    request_decoder_->decodeTrailers(
-        spdyHeaderBlockToEnvoyHeaders<Http::RequestTrailerMapImpl>(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);

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

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,

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 <class T>
-std::unique_ptr<T> spdyHeaderBlockToEnvoyHeaders(const spdy::SpdyHeaderBlock& header_block) {
+std::unique_ptr<T> 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<absl::string_view> 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;

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<Http::RequestHeaderMapImpl>(headers_block);
+  NiceMock<MockHeaderValidator> validator;
+  absl::string_view details;
+  auto envoy_headers = spdyHeaderBlockToEnvoyTrailers<Http::RequestHeaderMapImpl>(
+      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<Http::RequestHeaderMapImpl>(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<MockHeaderValidator> validator;
+  EXPECT_NE(nullptr, spdyHeaderBlockToEnvoyTrailers<Http::RequestHeaderMapImpl>(
+                         headers_block, 6, validator, details));
+  // Given the cap is 6, make sure anything lower, exact or otherwise, is rejected.
+  EXPECT_EQ(nullptr, spdyHeaderBlockToEnvoyTrailers<Http::RequestHeaderMapImpl>(
+                         headers_block, 5, validator, details));
+  EXPECT_EQ("http3.too_many_trailers", details);
+  EXPECT_EQ(nullptr, spdyHeaderBlockToEnvoyTrailers<Http::RequestHeaderMapImpl>(
+                         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<MockHeaderValidator> validator;
+  EXPECT_CALL(validator, validateHeader(_, _))
+      .WillRepeatedly(Return(Http::HeaderUtility::HeaderValidationResult::REJECT));
+  EXPECT_EQ(nullptr, spdyHeaderBlockToEnvoyTrailers<Http::RequestHeaderMapImpl>(
+                         headers_block, 5, validator, details));
+}
+
 } // namespace Quic
 } // namespace Envoy

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

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();

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