Envoy security fixes

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

@@ -131,6 +131,7 @@ All http2 statistics are rooted at *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/grpc/common.cc

@@ -165,12 +165,18 @@ std::chrono::milliseconds Common::getGrpcTimeout(const Http::RequestHeaderMap& r
   std::chrono::milliseconds timeout(0);
   const Http::HeaderEntry* header_grpc_timeout_entry = request_headers.GrpcTimeout();
   if (header_grpc_timeout_entry) {
-    uint64_t grpc_timeout;
-    // TODO(dnoe): Migrate to pure string_view (#6580)
-    std::string grpc_timeout_string(header_grpc_timeout_entry->value().getStringView());
-    const char* unit = StringUtil::strtoull(grpc_timeout_string.c_str(), grpc_timeout);
-    if (unit != nullptr && *unit != '\0') {
-      switch (*unit) {
+    int64_t grpc_timeout;
+    absl::string_view timeout_entry = header_grpc_timeout_entry->value().getStringView();
+    if (timeout_entry.empty()) {
+      // Must be of the form TimeoutValue TimeoutUnit. See
+      // https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md#requests.
+      return timeout;
+    }
+    // TimeoutValue must be a positive integer of at most 8 digits.
+    if (absl::SimpleAtoi(timeout_entry.substr(0, timeout_entry.size() - 1), &grpc_timeout) &&
+        grpc_timeout >= 0 && static_cast<uint64_t>(grpc_timeout) <= MAX_GRPC_TIMEOUT_VALUE) {
+      const char unit = timeout_entry[timeout_entry.size() - 1];
+      switch (unit) {
       case 'H':
         timeout = std::chrono::hours(grpc_timeout);
         break;

source/common/grpc/common.h

@@ -177,6 +177,8 @@ class Common {
 
 private:
   static void checkForHeaderOnlyError(Http::ResponseMessage& http_response);
+
+  static constexpr size_t MAX_GRPC_TIMEOUT_VALUE = 99999999;
 };
 
 } // namespace Grpc

source/common/http/http2/codec_impl.cc

@@ -482,7 +482,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/grpc/common_test.cc

@@ -78,6 +78,9 @@ TEST(GrpcContextTest, GetGrpcTimeout) {
   Http::TestRequestHeaderMapImpl missing_unit{{"grpc-timeout", "123"}};
   EXPECT_EQ(std::chrono::milliseconds(0), Common::getGrpcTimeout(missing_unit));
 
+  Http::TestRequestHeaderMapImpl small_missing_unit{{"grpc-timeout", "1"}};
+  EXPECT_EQ(std::chrono::milliseconds(0), Common::getGrpcTimeout(small_missing_unit));
+
   Http::TestRequestHeaderMapImpl illegal_unit{{"grpc-timeout", "123F"}};
   EXPECT_EQ(std::chrono::milliseconds(0), Common::getGrpcTimeout(illegal_unit));
 
@@ -99,8 +102,18 @@ TEST(GrpcContextTest, GetGrpcTimeout) {
   Http::TestRequestHeaderMapImpl unit_nanoseconds{{"grpc-timeout", "12345678n"}};
   EXPECT_EQ(std::chrono::milliseconds(13), Common::getGrpcTimeout(unit_nanoseconds));
 
-  // Max 8 digits and no leading whitespace or +- signs are not enforced on decode,
-  // so we don't test for them.
+  Http::TestRequestHeaderMapImpl value_overflow{{"grpc-timeout", "6666666666666H"}};
+  EXPECT_EQ(std::chrono::milliseconds(0), Common::getGrpcTimeout(value_overflow));
+
+  // Reject negative values.
+  Http::TestRequestHeaderMapImpl value_negative{{"grpc-timeout", "-1S"}};
+  EXPECT_EQ(std::chrono::milliseconds(0), Common::getGrpcTimeout(value_negative));
+
+  // Allow positive values marked with +.
+  Http::TestRequestHeaderMapImpl value_positive{{"grpc-timeout", "+1S"}};
+  EXPECT_EQ(std::chrono::milliseconds(1000), Common::getGrpcTimeout(value_positive));
+
+  // No leading whitespace are not enforced on decode so we don't test for them.
 }
 
 TEST(GrpcCommonTest, GrpcStatusDetailsBin) {

test/common/http/http2/BUILD

@@ -72,7 +72,11 @@ envoy_cc_test_library(
     name = "http2_frame",
     srcs = ["http2_frame.cc"],
     hdrs = ["http2_frame.h"],
+    external_deps = [
+        "nghttp2",
+    ],
     deps = [
+        "//include/envoy/http:metadata_interface_with_external_headers",
         "//source/common/common:assert_lib",
         "//source/common/common:macros",
     ],

test/common/http/http2/http2_frame.cc

@@ -4,11 +4,17 @@
 
 #include "envoy/common/platform.h"
 
+#include "absl/container/fixed_array.h"
+#include "nghttp2/nghttp2.h"
+
 namespace {
 
 // Make request stream ID in the network byte order
 uint32_t makeRequestStreamId(uint32_t stream_id) { return htonl((stream_id << 1) | 1); }
 
+// Converts stream ID to the network byte order. Supports all values in the range [0, 2^30).
+uint32_t makeNetworkOrderStreamId(uint32_t stream_id) { return htonl(stream_id); }
+
 // All this templatized stuff is for the typesafe constexpr bitwise ORing of the "enum class" values
 template <typename First, typename... Rest> struct FirstArgType {
   using type = First; // NOLINT(readability-identifier-naming)
@@ -210,6 +216,43 @@ Http2Frame Http2Frame::makeWindowUpdateFrame(uint32_t stream_index, uint32_t inc
   return frame;
 }
 
+// Note: encoder in codebase persists multiple maps, with each map representing an individual frame.
+Http2Frame Http2Frame::makeMetadataFrameFromMetadataMap(uint32_t stream_index,
+                                                        const MetadataMap& metadata_map,
+                                                        MetadataFlags flags) {
+  const int numberOfNameValuePairs = metadata_map.size();
+  absl::FixedArray<nghttp2_nv> nameValues(numberOfNameValuePairs);
+  absl::FixedArray<nghttp2_nv>::iterator iterator = nameValues.begin();
+  for (const auto& metadata : metadata_map) {
+    *iterator = {const_cast<uint8_t*>(reinterpret_cast<const uint8_t*>(metadata.first.data())),
+                 const_cast<uint8_t*>(reinterpret_cast<const uint8_t*>(metadata.second.data())),
+                 metadata.first.size(), metadata.second.size(), NGHTTP2_NV_FLAG_NO_INDEX};
+    ++iterator;
+  }
+
+  nghttp2_hd_deflater* deflater;
+  // Note: this has no effect, as metadata frames do not add onto Dynamic table.
+  const int maxDynamicTableSize = 4096;
+  nghttp2_hd_deflate_new(&deflater, maxDynamicTableSize);
+
+  const size_t upperBoundBufferLength =
+      nghttp2_hd_deflate_bound(deflater, nameValues.begin(), numberOfNameValuePairs);
+
+  uint8_t* buffer = new uint8_t[upperBoundBufferLength];
+
+  const size_t numberOfBytesInMetadataPayload = nghttp2_hd_deflate_hd(
+      deflater, buffer, upperBoundBufferLength, nameValues.begin(), numberOfNameValuePairs);
+
+  Http2Frame frame;
+  frame.buildHeader(Type::Metadata, numberOfBytesInMetadataPayload, static_cast<uint8_t>(flags),
+                    makeNetworkOrderStreamId(stream_index));
+  std::vector<uint8_t> bufferVector(buffer, buffer + numberOfBytesInMetadataPayload);
+  frame.appendDataAfterHeaders(bufferVector);
+  delete[] buffer;
+  nghttp2_hd_deflate_del(deflater);
+  return frame;
+}
+
 Http2Frame Http2Frame::makeMalformedRequest(uint32_t stream_index) {
   Http2Frame frame;
   frame.buildHeader(Type::Headers, 0, orFlags(HeadersFlags::EndStream, HeadersFlags::EndHeaders),

test/common/http/http2/http2_frame.h

@@ -4,6 +4,8 @@
 #include <string>
 #include <vector>
 
+#include "envoy/http/metadata_interface.h"
+
 #include "common/common/assert.h"
 
 #include "absl/strings/string_view.h"
@@ -35,7 +37,8 @@ class Http2Frame {
     Ping,
     GoAway,
     WindowUpdate,
-    Continuation
+    Continuation,
+    Metadata = 77,
   };
 
   enum class SettingsFlags : uint8_t {
@@ -54,6 +57,11 @@ class Http2Frame {
     EndStream = 1,
   };
 
+  enum class MetadataFlags : uint8_t {
+    None = 0,
+    EndMetadata = 4,
+  };
+
   // See https://tools.ietf.org/html/rfc7541#appendix-A for static header indexes
   enum class StaticHeaderIndex : uint8_t {
     Unknown,
@@ -101,6 +109,9 @@ class Http2Frame {
   static Http2Frame makeEmptyGoAwayFrame(uint32_t last_stream_index, ErrorCode error_code);
 
   static Http2Frame makeWindowUpdateFrame(uint32_t stream_index, uint32_t increment);
+  static Http2Frame makeMetadataFrameFromMetadataMap(uint32_t stream_index,
+                                                     const MetadataMap& metadata_map,
+                                                     MetadataFlags flags);
   static Http2Frame makeMalformedRequest(uint32_t stream_index);
   static Http2Frame makeMalformedRequestWithZerolenHeader(uint32_t stream_index,
                                                           absl::string_view host,
@@ -155,6 +166,9 @@ class Http2Frame {
   // header.
   void appendHpackInt(uint64_t value, unsigned char prefix_mask);
   void appendData(absl::string_view data) { data_.insert(data_.end(), data.begin(), data.end()); }
+  void appendDataAfterHeaders(std::vector<uint8_t> data) {
+    std::copy(data.begin(), data.end(), data_.begin() + 9);
+  }
 
   // Headers are directly encoded
   void appendStaticHeader(StaticHeaderIndex index);

test/common/router/router_test.cc

@@ -5561,6 +5561,16 @@ TEST(RouterFilterUtilityTest, FinalTimeout) {
     EXPECT_EQ("5", headers.get_("x-envoy-expected-rq-timeout-ms"));
     EXPECT_EQ("5m", headers.get_("grpc-timeout"));
   }
+  {
+    NiceMock<MockRouteEntry> route;
+    EXPECT_CALL(route, maxGrpcTimeout())
+        .WillRepeatedly(Return(absl::optional<std::chrono::milliseconds>(10000)));
+    Http::TestRequestHeaderMapImpl headers{{"content-type", "application/grpc"},
+                                           {"grpc-timeout", "6666666666666H"}};
+    FilterUtility::finalTimeout(route, headers, true, true, false, false);
+    EXPECT_EQ("10000", headers.get_("x-envoy-expected-rq-timeout-ms"));
+    EXPECT_EQ("10000m", headers.get_("grpc-timeout"));
+  }
   {
     NiceMock<MockRouteEntry> route;
     EXPECT_CALL(route, timeout()).WillOnce(Return(std::chrono::milliseconds(10)));

test/integration/fake_upstream.cc

@@ -632,6 +632,21 @@ void FakeUpstream::sendUdpDatagram(const std::string& buffer,
   });
 }
 
+AssertionResult FakeUpstream::rawWriteConnection(uint32_t index, const std::string& data,
+                                                 bool end_stream,
+                                                 std::chrono::milliseconds timeout) {
+  Thread::LockGuard lock(lock_);
+  auto iter = consumed_connections_.begin();
+  std::advance(iter, index);
+  return (*iter)->shared_connection().executeOnDispatcher(
+      [data, end_stream](Network::Connection& connection) {
+        ASSERT(connection.state() == Network::Connection::State::Open);
+        Buffer::OwnedImpl buffer(data);
+        connection.write(buffer, end_stream);
+      },
+      timeout);
+}
+
 AssertionResult FakeRawConnection::waitForData(uint64_t num_bytes, std::string* data,
                                                milliseconds timeout) {
   Thread::LockGuard lock(lock_);

test/integration/fake_upstream.h

@@ -652,6 +652,11 @@ class FakeUpstream : Logger::Loggable<Logger::Id::testing>,
     return Http::Http2::CodecStats::atomicGet(http2_codec_stats_, stats_store_);
   }
 
+  ABSL_MUST_USE_RESULT
+  testing::AssertionResult
+  rawWriteConnection(uint32_t index, const std::string& data, bool end_stream = false,
+                     std::chrono::milliseconds timeout = TestUtility::DefaultTimeout);
+
 protected:
   Stats::IsolatedStoreImpl stats_store_;
   const FakeHttpConnection::Type http_type_;