backport to 1.17: http: Fixing empty metadata map handling

Commit Message: Fixing a crash when the decoder receives an empty metadata map.

Additional Description:
Upon receiving an empty metadata map and trying to decode it an assertion is triggered in debug mode, and a seg-fault occurs in release mode.
The proposed fix ignores the empty metadata maps and updates a stats if one is received.

Risk Level: Medium for Envoy's running with Metadata support.
Testing: Added integration tests.
Docs Changes: Added a codec stats counter description.
Release Notes: Added bug fix description.
Platform Specific Features: N/A.
Fixes a fuzz bug: 25303

Signed-off-by: Adi Suissa-Peleg <adip@google.com>
Signed-off-by: Tony Allen <tony@allen.gg>

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

@@ -135,6 +135,7 @@ On the upstream side all http2 statistics are rooted at *cluster.<name>.http2.*
    inbound_empty_frames_flood, Counter, Total number of connections terminated for exceeding the limit on consecutive inbound frames with an empty payload and no end stream flag. The limit is configured by setting the :ref:`max_consecutive_inbound_frames_with_empty_payload config setting <envoy_v3_api_field_config.core.v3.Http2ProtocolOptions.max_consecutive_inbound_frames_with_empty_payload>`.
    inbound_priority_frames_flood, Counter, Total number of connections terminated for exceeding the limit on inbound frames of type PRIORITY. The limit is configured by setting the :ref:`max_inbound_priority_frames_per_stream config setting <envoy_v3_api_field_config.core.v3.Http2ProtocolOptions.max_inbound_priority_frames_per_stream>`.
    inbound_window_update_frames_flood, Counter, Total number of connections terminated for exceeding the limit on inbound frames of type WINDOW_UPDATE. The limit is configured by setting the :ref:`max_inbound_window_updateframes_per_data_frame_sent config setting <envoy_v3_api_field_config.core.v3.Http2ProtocolOptions.max_inbound_window_update_frames_per_data_frame_sent>`.
+   metadata_empty_frames, Counter, Total number of metadata frames that were received and contained empty maps.
    outbound_flood, Counter, Total number of connections terminated for exceeding the limit on outbound frames of all types. The limit is configured by setting the :ref:`max_outbound_frames config setting <envoy_v3_api_field_config.core.v3.Http2ProtocolOptions.max_outbound_frames>`.
    outbound_control_flood, Counter, "Total number of connections terminated for exceeding the limit on outbound frames of types PING, SETTINGS and RST_STREAM. The limit is configured by setting the :ref:`max_outbound_control_frames config setting <envoy_v3_api_field_config.core.v3.Http2ProtocolOptions.max_outbound_control_frames>`."
    requests_rejected_with_underscores_in_headers, Counter, Total numbers of rejected requests due to header names containing underscores. This action is configured by setting the :ref:`headers_with_underscores_action config setting <envoy_v3_api_field_config.core.v3.HttpProtocolOptions.headers_with_underscores_action>`.

source/common/http/http2/codec_impl.cc

@@ -553,7 +553,13 @@ MetadataDecoder& ConnectionImpl::StreamImpl::getMetadataDecoder() {
 }
 
 void ConnectionImpl::StreamImpl::onMetadataDecoded(MetadataMapPtr&& metadata_map_ptr) {
-  decoder().decodeMetadata(std::move(metadata_map_ptr));
+  // Empty metadata maps should not be decoded.
+  if (metadata_map_ptr->empty()) {
+    ENVOY_CONN_LOG(debug, "decode metadata called with empty map, skipping", parent_.connection_);
+    parent_.stats_.metadata_empty_frames_.inc();
+  } else {
+    decoder().decodeMetadata(std::move(metadata_map_ptr));
+  }
 }
 
 ConnectionImpl::ConnectionImpl(Network::Connection& connection, CodecStats& stats,

source/common/http/http2/codec_stats.h

@@ -19,6 +19,7 @@ namespace Http2 {
   COUNTER(inbound_empty_frames_flood)                                                              \
   COUNTER(inbound_priority_frames_flood)                                                           \
   COUNTER(inbound_window_update_frames_flood)                                                      \
+  COUNTER(metadata_empty_frames)                                                                   \
   COUNTER(outbound_control_flood)                                                                  \
   COUNTER(outbound_flood)                                                                          \
   COUNTER(requests_rejected_with_underscores_in_headers)                                           \

test/common/http/http2/http2_frame.cc

@@ -248,7 +248,7 @@ Http2Frame Http2Frame::makeWindowUpdateFrame(uint32_t stream_index, uint32_t inc
 
 // Note: encoder in codebase persists multiple maps, with each map representing an individual frame.
 Http2Frame Http2Frame::makeMetadataFrameFromMetadataMap(uint32_t stream_index,
-                                                        MetadataMap& metadata_map,
+                                                        const MetadataMap& metadata_map,
                                                         MetadataFlags flags) {
   const int numberOfNameValuePairs = metadata_map.size();
   absl::FixedArray<nghttp2_nv> nameValues(numberOfNameValuePairs);

test/common/http/http2/http2_frame.h

@@ -149,7 +149,7 @@ class Http2Frame {
 
   static Http2Frame makeWindowUpdateFrame(uint32_t stream_index, uint32_t increment);
   static Http2Frame makeMetadataFrameFromMetadataMap(uint32_t stream_index,
-                                                     MetadataMap& metadata_map,
+                                                     const MetadataMap& metadata_map,
                                                      MetadataFlags flags);
 
   static Http2Frame makeMalformedRequest(uint32_t stream_index);

test/integration/http2_integration_test.cc

@@ -180,6 +180,7 @@ TEST_P(Http2MetadataIntegrationTest, ProxyMetadataInResponse) {
   response->waitForEndStream();
   ASSERT_TRUE(response->complete());
   EXPECT_EQ(response->metadataMap().find(key)->second, value);
+  EXPECT_EQ(1, response->metadataMapsDecodedCount());
 
   // Sends the second request.
   response = codec_client_->makeRequestWithBody(default_request_headers_, 10);
@@ -199,6 +200,7 @@ TEST_P(Http2MetadataIntegrationTest, ProxyMetadataInResponse) {
   response->waitForEndStream();
   ASSERT_TRUE(response->complete());
   EXPECT_EQ(response->metadataMap().find(key)->second, value);
+  EXPECT_EQ(1, response->metadataMapsDecodedCount());
 
   // Sends the third request.
   response = codec_client_->makeRequestWithBody(default_request_headers_, 10);
@@ -218,6 +220,7 @@ TEST_P(Http2MetadataIntegrationTest, ProxyMetadataInResponse) {
   response->waitForEndStream();
   ASSERT_TRUE(response->complete());
   EXPECT_EQ(response->metadataMap().find(key)->second, value);
+  EXPECT_EQ(1, response->metadataMapsDecodedCount());
 
   // Sends the fourth request.
   response = codec_client_->makeRequestWithBody(default_request_headers_, 10);
@@ -238,6 +241,7 @@ TEST_P(Http2MetadataIntegrationTest, ProxyMetadataInResponse) {
   response->waitForEndStream();
   ASSERT_TRUE(response->complete());
   EXPECT_EQ(response->metadataMap().find(key)->second, value);
+  EXPECT_EQ(1, response->metadataMapsDecodedCount());
 
   // Sends the fifth request.
   response = codec_client_->makeRequestWithBody(default_request_headers_, 10);
@@ -258,6 +262,7 @@ TEST_P(Http2MetadataIntegrationTest, ProxyMetadataInResponse) {
   response->waitForEndStream();
   ASSERT_TRUE(response->complete());
   EXPECT_EQ(response->metadataMap().find(key)->second, value);
+  EXPECT_EQ(1, response->metadataMapsDecodedCount());
 
   // Sends the sixth request.
   response = codec_client_->makeRequestWithBody(default_request_headers_, 10);
@@ -308,6 +313,7 @@ TEST_P(Http2MetadataIntegrationTest, ProxyMultipleMetadata) {
   // Verifies multiple metadata are received by the client.
   response->waitForEndStream();
   ASSERT_TRUE(response->complete());
+  EXPECT_EQ(4, response->metadataMapsDecodedCount());
   for (int i = 0; i < size; i++) {
     for (const auto& metadata : *multiple_vecs[i][0]) {
       EXPECT_EQ(response->metadataMap().find(metadata.first)->second, metadata.second);
@@ -341,6 +347,7 @@ TEST_P(Http2MetadataIntegrationTest, ProxyInvalidMetadata) {
   // Verifies metadata is not received by the client.
   response->waitForEndStream();
   ASSERT_TRUE(response->complete());
+  EXPECT_EQ(0, response->metadataMapsDecodedCount());
   EXPECT_EQ(response->metadataMap().size(), 0);
 }
 
@@ -382,6 +389,7 @@ TEST_P(Http2MetadataIntegrationTest, TestResponseMetadata) {
   expected_metadata_keys.insert("data");
   verifyExpectedMetadata(response->metadataMap(), expected_metadata_keys);
   EXPECT_EQ(response->keyCount("duplicate"), 2);
+  EXPECT_EQ(2, response->metadataMapsDecodedCount());
 
   // Upstream responds with headers, data and trailers.
   response = codec_client_->makeRequestWithBody(default_request_headers_, 10);
@@ -396,6 +404,7 @@ TEST_P(Http2MetadataIntegrationTest, TestResponseMetadata) {
   expected_metadata_keys.insert("trailers");
   verifyExpectedMetadata(response->metadataMap(), expected_metadata_keys);
   EXPECT_EQ(response->keyCount("duplicate"), 3);
+  EXPECT_EQ(4, response->metadataMapsDecodedCount());
 
   // Upstream responds with headers, 100-continue and data.
   response =
@@ -418,6 +427,7 @@ TEST_P(Http2MetadataIntegrationTest, TestResponseMetadata) {
   expected_metadata_keys.insert("100-continue");
   verifyExpectedMetadata(response->metadataMap(), expected_metadata_keys);
   EXPECT_EQ(response->keyCount("duplicate"), 4);
+  EXPECT_EQ(4, response->metadataMapsDecodedCount());
 
   // Upstream responds with headers and metadata that will not be consumed.
   response = codec_client_->makeRequestWithBody(default_request_headers_, 10);
@@ -436,6 +446,7 @@ TEST_P(Http2MetadataIntegrationTest, TestResponseMetadata) {
   expected_metadata_keys.insert("aaa");
   expected_metadata_keys.insert("keep");
   verifyExpectedMetadata(response->metadataMap(), expected_metadata_keys);
+  EXPECT_EQ(2, response->metadataMapsDecodedCount());
 
   // Upstream responds with headers, data and metadata that will be consumed.
   response = codec_client_->makeRequestWithBody(default_request_headers_, 10);
@@ -455,6 +466,7 @@ TEST_P(Http2MetadataIntegrationTest, TestResponseMetadata) {
   expected_metadata_keys.insert("replace");
   verifyExpectedMetadata(response->metadataMap(), expected_metadata_keys);
   EXPECT_EQ(response->keyCount("duplicate"), 2);
+  EXPECT_EQ(3, response->metadataMapsDecodedCount());
 }
 
 TEST_P(Http2MetadataIntegrationTest, ProxyMultipleMetadataReachSizeLimit) {
@@ -1568,6 +1580,87 @@ INSTANTIATE_TEST_SUITE_P(IpVersions, Http2FrameIntegrationTest,
                          testing::ValuesIn(TestEnvironment::getIpVersionsForTest()),
                          TestUtility::ipTestParamsToString);
 
+// Tests sending an empty metadata map from downstream.
+TEST_P(Http2FrameIntegrationTest, DownstreamSendingEmptyMetadata) {
+  // Allow metadata usage.
+  config_helper_.addConfigModifier([&](envoy::config::bootstrap::v3::Bootstrap& bootstrap) -> void {
+    RELEASE_ASSERT(bootstrap.mutable_static_resources()->clusters_size() >= 1, "");
+    auto* cluster = bootstrap.mutable_static_resources()->mutable_clusters(0);
+    cluster->mutable_http2_protocol_options()->set_allow_metadata(true);
+  });
+  config_helper_.addConfigModifier(
+      [&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager&
+              hcm) -> void { hcm.mutable_http2_protocol_options()->set_allow_metadata(true); });
+
+  // This test uses an Http2Frame and not the encoder's encodeMetadata method,
+  // because encodeMetadata fails when an empty metadata map is sent.
+  beginSession();
+  FakeHttpConnectionPtr fake_upstream_connection;
+  FakeStreamPtr fake_upstream_request;
+
+  const uint32_t client_stream_idx = 1;
+  // Send request.
+  const Http2Frame request =
+      Http2Frame::makePostRequest(client_stream_idx, "host", "/path/to/long/url");
+  sendFrame(request);
+  ASSERT_TRUE(fake_upstreams_[0]->waitForHttpConnection(*dispatcher_, fake_upstream_connection));
+  ASSERT_TRUE(fake_upstream_connection->waitForNewStream(*dispatcher_, fake_upstream_request));
+
+  // Send metadata frame with empty metadata map.
+  const Http::MetadataMap empty_metadata_map;
+  const Http2Frame empty_metadata_map_frame = Http2Frame::makeMetadataFrameFromMetadataMap(
+      client_stream_idx, empty_metadata_map, Http2Frame::MetadataFlags::EndMetadata);
+  sendFrame(empty_metadata_map_frame);
+
+  // Send an empty data frame to close the stream.
+  const Http2Frame empty_data_frame =
+      Http2Frame::makeEmptyDataFrame(client_stream_idx, Http2Frame::DataFlags::EndStream);
+  sendFrame(empty_data_frame);
+
+  // Upstream sends a reply.
+  ASSERT_TRUE(fake_upstream_request->waitForEndStream(*dispatcher_));
+  const Http::TestResponseHeaderMapImpl response_headers{{":status", "200"}};
+  fake_upstream_request->encodeHeaders(response_headers, true);
+
+  // Make sure that a response from upstream is received by the client, and
+  // close the connection.
+  const auto response = readFrame();
+  EXPECT_EQ(Http2Frame::Type::Headers, response.type());
+  EXPECT_EQ(Http2Frame::ResponseStatus::Ok, response.responseStatus());
+  EXPECT_EQ(1, test_server_->counter("http2.metadata_empty_frames")->value());
+
+  // Cleanup.
+  tcp_client_->close();
+}
+
+// Tests that an empty metadata map from upstream is ignored.
+TEST_P(Http2MetadataIntegrationTest, UpstreamSendingEmptyMetadata) {
+  initialize();
+
+  // Send a request and make sure an upstream connection is established.
+  codec_client_ = makeHttpConnection(lookupPort("http"));
+  auto response = codec_client_->makeHeaderOnlyRequest(default_request_headers_);
+  waitForNextUpstreamRequest();
+  auto* upstream = fake_upstreams_.front().get();
+
+  // Send response headers.
+  upstream_request_->encodeHeaders(default_response_headers_, false);
+  // Send an empty metadata map back from upstream.
+  const Http::MetadataMap empty_metadata_map;
+  const Http2Frame empty_metadata_frame = Http2Frame::makeMetadataFrameFromMetadataMap(
+      1, empty_metadata_map, Http2Frame::MetadataFlags::EndMetadata);
+  ASSERT_TRUE(upstream->rawWriteConnection(
+      0, std::string(empty_metadata_frame.begin(), empty_metadata_frame.end())));
+  // Send an empty data frame after the metadata frame to end the stream.
+  upstream_request_->encodeData(0, true);
+
+  // Verifies that no metadata was received by the client.
+  response->waitForEndStream();
+  ASSERT_TRUE(response->complete());
+  EXPECT_EQ(0, response->metadataMapsDecodedCount());
+  EXPECT_EQ(1, test_server_->counter("cluster.cluster_0.http2.metadata_empty_frames")->value());
+}
+
 // Tests upstream sending a metadata frame after ending a stream.
 TEST_P(Http2MetadataIntegrationTest, UpstreamMetadataAfterEndStream) {
   initialize();

test/integration/integration_stream_decoder.cc

@@ -97,6 +97,7 @@ void IntegrationStreamDecoder::decodeTrailers(Http::ResponseTrailerMapPtr&& trai
 }
 
 void IntegrationStreamDecoder::decodeMetadata(Http::MetadataMapPtr&& metadata_map) {
+  metadata_maps_decoded_count_++;
   // Combines newly received metadata with the existing metadata.
   for (const auto& metadata : *metadata_map) {
     duplicated_metadata_key_count_[metadata.first]++;

test/integration/integration_stream_decoder.h

@@ -29,6 +29,7 @@ class IntegrationStreamDecoder : public Http::ResponseDecoder, public Http::Stre
   const Http::ResponseTrailerMapPtr& trailers() { return trailers_; }
   const Http::MetadataMap& metadataMap() { return *metadata_map_; }
   uint64_t keyCount(std::string key) { return duplicated_metadata_key_count_[key]; }
+  uint32_t metadataMapsDecodedCount() const { return metadata_maps_decoded_count_; }
   void waitForContinueHeaders();
   void waitForHeaders();
   // This function waits until body_ has at least size bytes in it (it might have more). clearBody()
@@ -70,6 +71,7 @@ class IntegrationStreamDecoder : public Http::ResponseDecoder, public Http::Stre
   bool waiting_for_headers_{};
   bool saw_reset_{};
   Http::StreamResetReason reset_reason_{};
+  uint32_t metadata_maps_decoded_count_{};
 };
 
 using IntegrationStreamDecoderPtr = std::unique_ptr<IntegrationStreamDecoder>;