quiche: enforce content-length header consistency

docs/root/configuration/http/http_conn_man/response_code_details.rst

@@ -120,4 +120,5 @@ All http3 details are rooted at *http3.*
     http3.too_many_trailers, Either incoming request or response trailers contained too many entries.
     http3.remote_refuse, The peer refused the stream.
     http3.remote_reset, The peer reset the stream.
+    http3.inconsistent_content_length, The payload size is different from what was indicated by the content-length header.
 

source/common/http/header_utility.cc

@@ -405,7 +405,7 @@ HeaderUtility::HeaderValidationResult HeaderUtility::checkHeaderNameForUnderscor
 HeaderUtility::HeaderValidationResult
 HeaderUtility::validateContentLength(absl::string_view header_value,
                                      bool override_stream_error_on_invalid_http_message,
-                                     bool& should_close_connection) {
+                                     bool& should_close_connection, size_t& content_length_output) {
   should_close_connection = false;
   std::vector<absl::string_view> values = absl::StrSplit(header_value, ',');
   absl::optional<uint64_t> content_length;
@@ -430,6 +430,7 @@ HeaderUtility::validateContentLength(absl::string_view header_value,
       return HeaderValidationResult::REJECT;
     }
   }
+  content_length_output = content_length.value();
   return HeaderValidationResult::ACCEPT;
 }
 

source/common/http/header_utility.h

@@ -258,13 +258,14 @@ class HeaderUtility {
 
   /**
    * Check if header_value represents a valid value for HTTP content-length header.
-   * Return HeaderValidationResult and populate should_close_connection
-   * according to override_stream_error_on_invalid_http_message.
+   * Return HeaderValidationResult and populate content_length_output if the value is valid,
+   * otherwise populate should_close_connection according to
+   * override_stream_error_on_invalid_http_message.
    */
   static HeaderValidationResult
   validateContentLength(absl::string_view header_value,
                         bool override_stream_error_on_invalid_http_message,
-                        bool& should_close_connection);
+                        bool& should_close_connection, size_t& content_length_output);
 };
 
 } // namespace Http

source/common/quic/envoy_quic_client_stream.cc

@@ -53,15 +53,19 @@ Http::Status EnvoyQuicClientStream::encodeHeaders(const Http::RequestHeaderMap&
   local_end_stream_ = end_stream;
   SendBufferMonitor::ScopedWatermarkBufferUpdater updater(this, this);
   auto spdy_headers = envoyHeadersToSpdyHeaderBlock(headers);
-  if (headers.Method() && headers.Method()->value() == "CONNECT") {
-    // It is a bytestream connect and should have :path and :protocol set accordingly
-    // As HTTP/1.1 does not require a path for CONNECT, we may have to add one
-    // if shifting codecs. For now, default to "/" - this can be made
-    // configurable if necessary.
-    // https://tools.ietf.org/html/draft-kinnear-httpbis-http2-transport-02
-    spdy_headers[":protocol"] = Http::Headers::get().ProtocolValues.Bytestream;
-    if (!headers.Path()) {
-      spdy_headers[":path"] = "/";
+  if (headers.Method()) {
+    if (headers.Method()->value() == "CONNECT") {
+      // It is a bytestream connect and should have :path and :protocol set accordingly
+      // As HTTP/1.1 does not require a path for CONNECT, we may have to add one
+      // if shifting codecs. For now, default to "/" - this can be made
+      // configurable if necessary.
+      // https://tools.ietf.org/html/draft-kinnear-httpbis-http2-transport-02
+      spdy_headers[":protocol"] = Http::Headers::get().ProtocolValues.Bytestream;
+      if (!headers.Path()) {
+        spdy_headers[":path"] = "/";
+      }
+    } else if (headers.Method()->value() == "HEAD") {
+      sent_head_request_ = true;
     }
   }
   WriteHeaders(std::move(spdy_headers), end_stream, nullptr);
@@ -190,6 +194,9 @@ void EnvoyQuicClientStream::OnInitialHeadersComplete(bool fin, size_t frame_len,
   } else if (status != enumToInt(Http::Code::Continue)) {
     response_decoder_->decodeHeaders(std::move(headers),
                                      /*end_stream=*/fin);
+    if (status == enumToInt(Http::Code::NotModified)) {
+      got_304_response_ = true;
+    }
   }
 
   ConsumeHeaderList();
@@ -224,6 +231,11 @@ void EnvoyQuicClientStream::OnBodyAvailable() {
     if (fin_read_and_no_trailers) {
       end_stream_decoded_ = true;
     }
+    updateReceivedContentBytes(buffer->length(), fin_read_and_no_trailers);
+    if (stream_error() != quic::QUIC_STREAM_NO_ERROR) {
+      // A stream error has occurred, stop processing.
+      return;
+    }
     response_decoder_->decodeData(*buffer, fin_read_and_no_trailers);
   }
 
@@ -255,6 +267,11 @@ void EnvoyQuicClientStream::maybeDecodeTrailers() {
   if (sequencer()->IsClosed() && !FinishedReadingTrailers()) {
     // Only decode trailers after finishing decoding body.
     end_stream_decoded_ = true;
+    updateReceivedContentBytes(0, true);
+    if (stream_error() != quic::QUIC_STREAM_NO_ERROR) {
+      // A stream error has occurred, stop processing.
+      return;
+    }
     quic::QuicRstStreamErrorCode transform_rst = quic::QUIC_STREAM_NO_ERROR;
     auto trailers = spdyHeaderBlockToEnvoyTrailers<Http::ResponseTrailerMapImpl>(
         received_trailers(), filterManagerConnection()->maxIncomingHeadersCount(), *this, details_,

source/common/quic/envoy_quic_client_stream.h

@@ -73,17 +73,15 @@ class EnvoyQuicClientStream : public quic::QuicSpdyClientStream,
   // Http::MultiplexedStreamImplBase
   bool hasPendingData() override;
 
+  void onStreamError(absl::optional<bool> should_close_connection,
+                     quic::QuicRstStreamErrorCode rst_code) override;
+
 private:
   QuicFilterManagerConnectionImpl* filterManagerConnection();
 
   // Deliver awaiting trailers if body has been delivered.
   void maybeDecodeTrailers();
 
-  // Either reset the stream or close the connection according to
-  // should_close_connection and configured http3 options.
-  void onStreamError(absl::optional<bool> should_close_connection,
-                     quic::QuicRstStreamErrorCode rst_code);
-
   Http::ResponseDecoder* response_decoder_{nullptr};
 
   bool decoded_100_continue_{false};

source/common/quic/envoy_quic_server_stream.cc

@@ -211,6 +211,11 @@ void EnvoyQuicServerStream::OnBodyAvailable() {
     if (fin_read_and_no_trailers) {
       end_stream_decoded_ = true;
     }
+    updateReceivedContentBytes(buffer->length(), fin_read_and_no_trailers);
+    if (stream_error() != quic::QUIC_STREAM_NO_ERROR) {
+      // A stream error has occurred, stop processing.
+      return;
+    }
     request_decoder_->decodeData(*buffer, fin_read_and_no_trailers);
   }
 
@@ -248,6 +253,11 @@ void EnvoyQuicServerStream::maybeDecodeTrailers() {
   if (sequencer()->IsClosed() && !FinishedReadingTrailers()) {
     // Only decode trailers after finishing decoding body.
     end_stream_decoded_ = true;
+    updateReceivedContentBytes(0, true);
+    if (stream_error() != quic::QUIC_STREAM_NO_ERROR) {
+      // A stream error has occurred, stop processing.
+      return;
+    }
     quic::QuicRstStreamErrorCode rst = quic::QUIC_STREAM_NO_ERROR;
     auto trailers = spdyHeaderBlockToEnvoyTrailers<Http::RequestTrailerMapImpl>(
         received_trailers(), filterManagerConnection()->maxIncomingHeadersCount(), *this, details_,

source/common/quic/envoy_quic_server_stream.h

@@ -80,17 +80,16 @@ class EnvoyQuicServerStream : public quic::QuicSpdyServerStreamBase,
   void onPendingFlushTimer() override;
   bool hasPendingData() override;
 
+  void
+  onStreamError(absl::optional<bool> should_close_connection,
+                quic::QuicRstStreamErrorCode rst = quic::QUIC_BAD_APPLICATION_PAYLOAD) override;
+
 private:
   QuicFilterManagerConnectionImpl* filterManagerConnection();
 
   // Deliver awaiting trailers if body has been delivered.
   void maybeDecodeTrailers();
 
-  // Either reset the stream or close the connection according to
-  // should_close_connection and configured http3 options.
-  void onStreamError(absl::optional<bool> should_close_connection,
-                     quic::QuicRstStreamErrorCode rst = quic::QUIC_BAD_APPLICATION_PAYLOAD);
-
   Http::RequestDecoder* request_decoder_{nullptr};
   envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction
       headers_with_underscores_action_;

source/common/quic/envoy_quic_stream.h

@@ -107,9 +107,13 @@ class EnvoyQuicStream : public virtual Http::StreamEncoder,
       return Http::HeaderUtility::HeaderValidationResult::REJECT;
     }
     if (header_name == "content-length") {
-      return Http::HeaderUtility::validateContentLength(
-          header_value, override_stream_error_on_invalid_http_message,
-          close_connection_upon_invalid_header_);
+      size_t content_length = 0;
+      Http::HeaderUtility::HeaderValidationResult result =
+          Http::HeaderUtility::validateContentLength(
+              header_value, override_stream_error_on_invalid_http_message,
+              close_connection_upon_invalid_header_, content_length);
+      content_length_ = content_length;
+      return result;
     }
     return Http::HeaderUtility::HeaderValidationResult::ACCEPT;
   }
@@ -122,6 +126,26 @@ class EnvoyQuicStream : public virtual Http::StreamEncoder,
   // Needed for ENVOY_STREAM_LOG.
   virtual uint32_t streamId() PURE;
   virtual Network::Connection* connection() PURE;
+  // Either reset the stream or close the connection according to
+  // should_close_connection and configured http3 options.
+  virtual void
+  onStreamError(absl::optional<bool> should_close_connection,
+                quic::QuicRstStreamErrorCode rst = quic::QUIC_BAD_APPLICATION_PAYLOAD) PURE;
+
+  // TODO(danzh) remove this once QUICHE enforces content-length consistency.
+  void updateReceivedContentBytes(size_t payload_length, bool end_stream) {
+    received_content_bytes_ += payload_length;
+    if (!content_length_.has_value()) {
+      return;
+    }
+    if (received_content_bytes_ > content_length_.value() ||
+        (end_stream && received_content_bytes_ != content_length_.value() &&
+         !(got_304_response_ && received_content_bytes_ == 0) && !(sent_head_request_))) {
+      details_ = Http3ResponseCodeDetailValues::inconsistent_content_length;
+      // Reset instead of closing the connection to align with nghttp2.
+      onStreamError(false);
+    }
+  }
 
   // True once end of stream is propagated to Envoy. Envoy doesn't expect to be
   // notified more than once about end of stream. So once this is true, no need
@@ -141,6 +165,8 @@ class EnvoyQuicStream : public virtual Http::StreamEncoder,
   // TODO(kbaichoo): bind the account to the QUIC buffers to enable tracking of
   // memory allocated within QUIC buffers.
   Buffer::BufferMemoryAccountSharedPtr buffer_memory_account_ = nullptr;
+  bool got_304_response_{false};
+  bool sent_head_request_{false};
 
 private:
   // Keeps track of bytes buffered in the stream send buffer in QUICHE and reacts
@@ -157,6 +183,8 @@ class EnvoyQuicStream : public virtual Http::StreamEncoder,
   // state change in its own call stack. And Envoy upstream doesn't like quic stream to be unblocked
   // in its callstack either because the stream will push data right away.
   Event::SchedulableCallbackPtr async_stream_blockage_change_;
+  absl::optional<size_t> content_length_;
+  size_t received_content_bytes_{0};
 };
 
 } // namespace Quic

source/common/quic/envoy_quic_utils.h

@@ -53,6 +53,9 @@ class Http3ResponseCodeDetailValues {
   static constexpr absl::string_view too_many_trailers = "http3.too_many_trailers";
   // Too many headers were sent.
   static constexpr absl::string_view too_many_headers = "http3.too_many_headers";
+  // The payload size is different from what the content-length header indicated.
+  static constexpr absl::string_view inconsistent_content_length =
+      "http3.inconsistent_content_length";
 };
 
 // TODO(danzh): this is called on each write. Consider to return an address instance on the stack if

test/common/http/header_utility_test.cc

@@ -916,20 +916,26 @@ TEST(ValidateHeaders, Connect) {
 
 TEST(ValidateHeaders, ContentLength) {
   bool should_close_connection;
-  EXPECT_EQ(HeaderUtility::HeaderValidationResult::ACCEPT,
-            HeaderUtility::validateContentLength("1,1", true, should_close_connection));
+  size_t content_length{0};
+  EXPECT_EQ(
+      HeaderUtility::HeaderValidationResult::ACCEPT,
+      HeaderUtility::validateContentLength("1,1", true, should_close_connection, content_length));
   EXPECT_FALSE(should_close_connection);
+  EXPECT_EQ(1, content_length);
 
-  EXPECT_EQ(HeaderUtility::HeaderValidationResult::REJECT,
-            HeaderUtility::validateContentLength("1,2", true, should_close_connection));
+  EXPECT_EQ(
+      HeaderUtility::HeaderValidationResult::REJECT,
+      HeaderUtility::validateContentLength("1,2", true, should_close_connection, content_length));
   EXPECT_FALSE(should_close_connection);
 
-  EXPECT_EQ(HeaderUtility::HeaderValidationResult::REJECT,
-            HeaderUtility::validateContentLength("1,2", false, should_close_connection));
+  EXPECT_EQ(
+      HeaderUtility::HeaderValidationResult::REJECT,
+      HeaderUtility::validateContentLength("1,2", false, should_close_connection, content_length));
   EXPECT_TRUE(should_close_connection);
 
-  EXPECT_EQ(HeaderUtility::HeaderValidationResult::REJECT,
-            HeaderUtility::validateContentLength("-1", false, should_close_connection));
+  EXPECT_EQ(
+      HeaderUtility::HeaderValidationResult::REJECT,
+      HeaderUtility::validateContentLength("-1", false, should_close_connection, content_length));
   EXPECT_TRUE(should_close_connection);
 }
 

test/integration/multiplexed_integration_test.cc

@@ -1880,4 +1880,35 @@ TEST_P(Http2IntegrationTest, InvalidTrailers) {
   EXPECT_THAT(waitForAccessLog(access_log_name_), HasSubstr("invalid"));
 }
 
+TEST_P(Http2IntegrationTest, InconsistentContentLength) {
+  useAccessLog("%RESPONSE_CODE_DETAILS%");
+  initialize();
+  codec_client_ = makeHttpConnection(lookupPort("http"));
+  auto encoder_decoder =
+      codec_client_->startRequest(Http::TestRequestHeaderMapImpl{{":method", "POST"},
+                                                                 {":path", "/test/long/url"},
+                                                                 {":scheme", "http"},
+                                                                 {":authority", "host"},
+                                                                 {"content-length", "1025"}});
+
+  auto response = std::move(encoder_decoder.second);
+  request_encoder_ = &encoder_decoder.first;
+  codec_client_->sendData(*request_encoder_, 1024, false);
+  codec_client_->sendTrailers(*request_encoder_,
+                              Http::TestRequestTrailerMapImpl{{"trailer", "value"}});
+
+  // Inconsistency in content-length header and the actually body length should be treated as a
+  // stream error.
+  ASSERT_TRUE(response->waitForReset());
+  // http3.inconsistent_content_length.
+  if (downstreamProtocol() == Http::CodecType::HTTP3) {
+    EXPECT_EQ(Http::StreamResetReason::RemoteReset, response->resetReason());
+    EXPECT_THAT(waitForAccessLog(access_log_name_), HasSubstr("inconsistent_content_length"));
+  } else {
+    EXPECT_EQ(Http::StreamResetReason::ConnectionTermination, response->resetReason());
+    // http2.violation.of.messaging.rule
+    EXPECT_THAT(waitForAccessLog(access_log_name_), HasSubstr("violation"));
+  }
+}
+
 } // namespace Envoy

test/integration/protocol_integration_test.cc

@@ -493,6 +493,27 @@ TEST_P(ProtocolIntegrationTest, 304HeadResponseWithoutContentLengthLegacy) {
   EXPECT_TRUE(response->headers().get(Http::LowerCaseString("content-length")).empty());
 }
 
+// Tests that the response to a HEAD request can have content-length header but empty body.
+TEST_P(ProtocolIntegrationTest, 200HeadResponseWithContentLength) {
+  initialize();
+  codec_client_ = makeHttpConnection(lookupPort("http"));
+  auto response = codec_client_->makeHeaderOnlyRequest(
+      Http::TestRequestHeaderMapImpl{{":method", "HEAD"},
+                                     {":path", "/test/long/url"},
+                                     {":scheme", "http"},
+                                     {":authority", "host"},
+                                     {"if-none-match", "\"1234567890\""}});
+  waitForNextUpstreamRequest();
+  upstream_request_->encodeHeaders(
+      Http::TestResponseHeaderMapImpl{{":status", "200"}, {"content-length", "123"}}, true);
+  ASSERT_TRUE(response->waitForEndStream());
+  EXPECT_TRUE(response->complete());
+  EXPECT_EQ("200", response->headers().getStatusValue());
+  EXPECT_EQ(
+      "123",
+      response->headers().get(Http::LowerCaseString("content-length"))[0]->value().getStringView());
+}
+
 // Tests missing headers needed for H/1 codec first line.
 TEST_P(DownstreamProtocolIntegrationTest, DownstreamRequestWithFaultyFilter) {
   if (upstreamProtocol() == Http::CodecType::HTTP3) {
@@ -3158,4 +3179,54 @@ TEST_P(ProtocolIntegrationTest, FragmentStrippedFromPathWithOverride) {
   EXPECT_EQ("200", response->headers().getStatusValue());
 }
 
+TEST_P(DownstreamProtocolIntegrationTest, ContentLengthSmallerThanPayload) {
+  initialize();
+  codec_client_ = makeHttpConnection(lookupPort("http"));
+  auto response =
+      codec_client_->makeRequestWithBody(Http::TestRequestHeaderMapImpl{{":method", "POST"},
+                                                                        {":path", "/test/long/url"},
+                                                                        {":scheme", "http"},
+                                                                        {":authority", "host"},
+                                                                        {"content-length", "123"}},
+                                         1024);
+  if (downstreamProtocol() == Http::CodecType::HTTP1) {
+    waitForNextUpstreamRequest();
+    // HTTP/1.x requests get the payload length from Content-Length header. The remaining bytes is
+    // parsed as another request.
+    EXPECT_EQ(123u, upstream_request_->body().length());
+    upstream_request_->encodeHeaders(Http::TestResponseHeaderMapImpl{{":status", "200"}}, true);
+    ASSERT_TRUE(response->waitForEndStream());
+    EXPECT_EQ("200", response->headers().getStatusValue());
+    EXPECT_TRUE(response->complete());
+  } else {
+    // Inconsistency in content-length header and the actually body length should be treated as a
+    // stream error.
+    ASSERT_TRUE(response->waitForReset());
+    EXPECT_EQ(Http::StreamResetReason::RemoteReset, response->resetReason());
+  }
+}
+
+TEST_P(DownstreamProtocolIntegrationTest, ContentLengthLargerThanPayload) {
+  if (downstreamProtocol() == Http::CodecType::HTTP1) {
+    // HTTP/1.x request rely on Content-Length header to determine payload length. So there is no
+    // inconsistency but the request will hang there waiting for the rest bytes.
+    return;
+  }
+
+  initialize();
+  codec_client_ = makeHttpConnection(lookupPort("http"));
+  auto response =
+      codec_client_->makeRequestWithBody(Http::TestRequestHeaderMapImpl{{":method", "POST"},
+                                                                        {":path", "/test/long/url"},
+                                                                        {":scheme", "http"},
+                                                                        {":authority", "host"},
+                                                                        {"content-length", "1025"}},
+                                         1024);
+
+  // Inconsistency in content-length header and the actually body length should be treated as a
+  // stream error.
+  ASSERT_TRUE(response->waitForReset());
+  EXPECT_EQ(Http::StreamResetReason::RemoteReset, response->resetReason());
+}
+
 } // namespace Envoy