From c94cacefe0793dff0c58a175a5a85eaad38fdead Mon Sep 17 00:00:00 2001 From: "Adi (Suissa) Peleg" Date: Thu, 25 Mar 2021 12:29:56 -0400 Subject: [PATCH 1/2] http: Fixing empty metadata map handling (#230) (#249) 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 Co-authored-by: Tony Allen Signed-off-by: Tony Allen --- .../http/http_conn_man/stats.rst | 1 + source/common/http/http2/codec_impl.cc | 8 +- source/common/http/http2/codec_stats.h | 1 + test/common/http/http2/BUILD | 4 + test/common/http/http2/http2_frame.cc | 43 +++++++++ test/common/http/http2/http2_frame.h | 16 +++- test/integration/fake_upstream.cc | 15 +++ test/integration/fake_upstream.h | 5 + test/integration/http2_integration_test.cc | 92 +++++++++++++++++++ test/integration/integration.cc | 1 + test/integration/integration.h | 2 + 11 files changed, 186 insertions(+), 2 deletions(-) diff --git a/docs/root/configuration/http/http_conn_man/stats.rst b/docs/root/configuration/http/http_conn_man/stats.rst index 2210bfc6dd5c..1d5b5c348096 100644 --- a/docs/root/configuration/http/http_conn_man/stats.rst +++ b/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 `. 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 `. 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 `. + 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 `. 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 `." 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 `. diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc index 532831198760..3592f195b74a 100644 --- a/source/common/http/http2/codec_impl.cc +++ b/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, diff --git a/source/common/http/http2/codec_stats.h b/source/common/http/http2/codec_stats.h index 32a31365d9d5..265d96c3a7e2 100644 --- a/source/common/http/http2/codec_stats.h +++ b/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) \ diff --git a/test/common/http/http2/BUILD b/test/common/http/http2/BUILD index 6876a13477a1..f465e96d4ea1 100644 --- a/test/common/http/http2/BUILD +++ b/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", ], diff --git a/test/common/http/http2/http2_frame.cc b/test/common/http/http2/http2_frame.cc index df61d5f88f31..f22f5330924b 100644 --- a/test/common/http/http2/http2_frame.cc +++ b/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 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 nameValues(numberOfNameValuePairs); + absl::FixedArray::iterator iterator = nameValues.begin(); + for (const auto& metadata : metadata_map) { + *iterator = {const_cast(reinterpret_cast(metadata.first.data())), + const_cast(reinterpret_cast(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(flags), + makeNetworkOrderStreamId(stream_index)); + std::vector 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), diff --git a/test/common/http/http2/http2_frame.h b/test/common/http/http2/http2_frame.h index 347061e77791..26667f3a11a8 100644 --- a/test/common/http/http2/http2_frame.h +++ b/test/common/http/http2/http2_frame.h @@ -4,6 +4,8 @@ #include #include +#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 data) { + std::copy(data.begin(), data.end(), data_.begin() + 9); + } // Headers are directly encoded void appendStaticHeader(StaticHeaderIndex index); diff --git a/test/integration/fake_upstream.cc b/test/integration/fake_upstream.cc index 4763c9bbfe05..02efa8a60c0a 100644 --- a/test/integration/fake_upstream.cc +++ b/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_); diff --git a/test/integration/fake_upstream.h b/test/integration/fake_upstream.h index 6afeb17b36a9..ccba27dee344 100644 --- a/test/integration/fake_upstream.h +++ b/test/integration/fake_upstream.h @@ -652,6 +652,11 @@ class FakeUpstream : Logger::Loggable, 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_; diff --git a/test/integration/http2_integration_test.cc b/test/integration/http2_integration_test.cc index 50ec78b68787..9cdf77fe5d87 100644 --- a/test/integration/http2_integration_test.cc +++ b/test/integration/http2_integration_test.cc @@ -152,6 +152,7 @@ TEST_P(Http2MetadataIntegrationTest, ProxyMetadataInResponse) { response->waitForEndStream(); ASSERT_TRUE(response->complete()); EXPECT_EQ(response->metadata_map().find(key)->second, value); + EXPECT_EQ(1, response->metadataMapsDecodedCount()); // Sends the second request. response = codec_client_->makeRequestWithBody(default_request_headers_, 10); @@ -171,6 +172,7 @@ TEST_P(Http2MetadataIntegrationTest, ProxyMetadataInResponse) { response->waitForEndStream(); ASSERT_TRUE(response->complete()); EXPECT_EQ(response->metadata_map().find(key)->second, value); + EXPECT_EQ(1, response->metadataMapsDecodedCount()); // Sends the third request. response = codec_client_->makeRequestWithBody(default_request_headers_, 10); @@ -190,6 +192,7 @@ TEST_P(Http2MetadataIntegrationTest, ProxyMetadataInResponse) { response->waitForEndStream(); ASSERT_TRUE(response->complete()); EXPECT_EQ(response->metadata_map().find(key)->second, value); + EXPECT_EQ(1, response->metadataMapsDecodedCount()); // Sends the fourth request. response = codec_client_->makeRequestWithBody(default_request_headers_, 10); @@ -210,6 +213,7 @@ TEST_P(Http2MetadataIntegrationTest, ProxyMetadataInResponse) { response->waitForEndStream(); ASSERT_TRUE(response->complete()); EXPECT_EQ(response->metadata_map().find(key)->second, value); + EXPECT_EQ(1, response->metadataMapsDecodedCount()); // Sends the fifth request. response = codec_client_->makeRequestWithBody(default_request_headers_, 10); @@ -230,6 +234,7 @@ TEST_P(Http2MetadataIntegrationTest, ProxyMetadataInResponse) { response->waitForEndStream(); ASSERT_TRUE(response->complete()); EXPECT_EQ(response->metadata_map().find(key)->second, value); + EXPECT_EQ(1, response->metadataMapsDecodedCount()); // Sends the sixth request. response = codec_client_->makeRequestWithBody(default_request_headers_, 10); @@ -280,6 +285,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->metadata_map().find(metadata.first)->second, metadata.second); @@ -313,6 +319,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->metadata_map().size(), 0); } @@ -354,6 +361,7 @@ TEST_P(Http2MetadataIntegrationTest, TestResponseMetadata) { expected_metadata_keys.insert("data"); verifyExpectedMetadata(response->metadata_map(), 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); @@ -368,6 +376,7 @@ TEST_P(Http2MetadataIntegrationTest, TestResponseMetadata) { expected_metadata_keys.insert("trailers"); verifyExpectedMetadata(response->metadata_map(), expected_metadata_keys); EXPECT_EQ(response->keyCount("duplicate"), 3); + EXPECT_EQ(4, response->metadataMapsDecodedCount()); // Upstream responds with headers, 100-continue and data. response = @@ -390,6 +399,7 @@ TEST_P(Http2MetadataIntegrationTest, TestResponseMetadata) { expected_metadata_keys.insert("100-continue"); verifyExpectedMetadata(response->metadata_map(), 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); @@ -408,6 +418,7 @@ TEST_P(Http2MetadataIntegrationTest, TestResponseMetadata) { expected_metadata_keys.insert("aaa"); expected_metadata_keys.insert("keep"); verifyExpectedMetadata(response->metadata_map(), 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); @@ -427,6 +438,7 @@ TEST_P(Http2MetadataIntegrationTest, TestResponseMetadata) { expected_metadata_keys.insert("replace"); verifyExpectedMetadata(response->metadata_map(), expected_metadata_keys); EXPECT_EQ(response->keyCount("duplicate"), 2); + EXPECT_EQ(3, response->metadataMapsDecodedCount()); } TEST_P(Http2MetadataIntegrationTest, ProxyMultipleMetadataReachSizeLimit) { @@ -1872,4 +1884,84 @@ TEST_P(Http2FloodMitigationTest, ZerolenHeaderAllowed) { EXPECT_THAT(waitForAccessLog(access_log_name_), HasSubstr("DPE")); } +// Tests sending an empty metadata map from downstream. +TEST_P(Http2FloodMitigationTest, 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 = 0; + // Send request. + const Http2Frame request = Http2Frame::makePostRequest(client_stream_idx, "host", "/"); + 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 << 1) | 1, 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()); +} + } // namespace Envoy diff --git a/test/integration/integration.cc b/test/integration/integration.cc index 6079451ac710..5f9260d8b510 100644 --- a/test/integration/integration.cc +++ b/test/integration/integration.cc @@ -133,6 +133,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]++; diff --git a/test/integration/integration.h b/test/integration/integration.h index c68efaf0963e..6cd9f8c7e7e2 100644 --- a/test/integration/integration.h +++ b/test/integration/integration.h @@ -46,6 +46,7 @@ class IntegrationStreamDecoder : public Http::ResponseDecoder, public Http::Stre const Http::ResponseTrailerMapPtr& trailers() { return trailers_; } const Http::MetadataMap& metadata_map() { 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() @@ -87,6 +88,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; From 2ac861a486755a78cb55225c312a9f8be1d363b8 Mon Sep 17 00:00:00 2001 From: Rei Shimizu Date: Fri, 26 Mar 2021 07:09:03 +0900 Subject: [PATCH 2/2] backport 1.15: grpc: fix grpc-timeout integer-overflow (#256) Fixes CVE-2021-28682, a remotely exploitable integer overflow. Signed-off-by: Asra Ali Co-authored-by: Tony Allen Signed-off-by: Tony Allen --- source/common/grpc/common.cc | 18 ++++++++++++------ source/common/grpc/common.h | 2 ++ test/common/grpc/common_test.cc | 17 +++++++++++++++-- test/common/router/router_test.cc | 10 ++++++++++ 4 files changed, 39 insertions(+), 8 deletions(-) diff --git a/source/common/grpc/common.cc b/source/common/grpc/common.cc index 5c4c9234c7bd..dd0e3124e95d 100644 --- a/source/common/grpc/common.cc +++ b/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(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; diff --git a/source/common/grpc/common.h b/source/common/grpc/common.h index cd94fe450568..f0829f602d36 100644 --- a/source/common/grpc/common.h +++ b/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 diff --git a/test/common/grpc/common_test.cc b/test/common/grpc/common_test.cc index 6847f159670e..7f874bd5c651 100644 --- a/test/common/grpc/common_test.cc +++ b/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) { diff --git a/test/common/router/router_test.cc b/test/common/router/router_test.cc index 169a8d801d29..f42e21a488af 100644 --- a/test/common/router/router_test.cc +++ b/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 route; + EXPECT_CALL(route, maxGrpcTimeout()) + .WillRepeatedly(Return(absl::optional(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 route; EXPECT_CALL(route, timeout()).WillOnce(Return(std::chrono::milliseconds(10)));