diff --git a/api/envoy/extensions/filters/http/ext_proc/v3/ext_proc.proto b/api/envoy/extensions/filters/http/ext_proc/v3/ext_proc.proto index b916f874f30b..a924b7625922 100644 --- a/api/envoy/extensions/filters/http/ext_proc/v3/ext_proc.proto +++ b/api/envoy/extensions/filters/http/ext_proc/v3/ext_proc.proto @@ -139,13 +139,16 @@ message ExternalProcessor { repeated string response_attributes = 6; // Specifies the timeout for each individual message sent on the stream and - // when the filter is running in synchronous mode. Whenever - // the proxy sends a message on the stream that requires a response, it will - // reset this timer, and will stop processing and return an error (subject - // to the processing mode) if the timer expires before a matching response - // is received. There is no timeout when the filter is running in asynchronous - // mode. Default is 200 milliseconds. - google.protobuf.Duration message_timeout = 7; + // when the filter is running in synchronous mode. Whenever the proxy sends + // a message on the stream that requires a response, it will reset this timer, + // and will stop processing and return an error (subject to the processing mode) + // if the timer expires before a matching response is received. There is no + // timeout when the filter is running in asynchronous mode. The + // ``message_timeout`` range is >= 0s and <= 3600s. Default is 200 milliseconds. + google.protobuf.Duration message_timeout = 7 [(validate.rules).duration = { + lte {seconds: 3600} + gte {} + }]; // Optional additional prefix to use when emitting statistics. This allows to distinguish // emitted statistics between configured *ext_proc* filters in an HTTP filter chain. @@ -166,8 +169,12 @@ message ExternalProcessor { // Specify the upper bound of // :ref:`override_message_timeout ` + // The ``max_message_timeout`` range is >= 0s and <= 3600s. // If not specified, by default it is 0, which will effectively disable the ``override_message_timeout`` API. - google.protobuf.Duration max_message_timeout = 10; + google.protobuf.Duration max_message_timeout = 10 [(validate.rules).duration = { + lte {seconds: 3600} + gte {} + }]; // Prevents clearing the route-cache when the // :ref:`clear_route_cache ` diff --git a/source/common/protobuf/utility.cc b/source/common/protobuf/utility.cc index 7cdc7d5042a8..ec8a55a36efb 100644 --- a/source/common/protobuf/utility.cc +++ b/source/common/protobuf/utility.cc @@ -645,14 +645,22 @@ ProtobufWkt::Value ValueUtil::listValue(const std::vector& v namespace { -void validateDuration(const ProtobufWkt::Duration& duration, int64_t max_seconds_value) { +absl::Status validateDurationNoThrow(const ProtobufWkt::Duration& duration, + int64_t max_seconds_value) { if (duration.seconds() < 0 || duration.nanos() < 0) { - throw DurationUtil::OutOfRangeException( + return absl::OutOfRangeError( fmt::format("Expected positive duration: {}", duration.DebugString())); } if (duration.nanos() > 999999999 || duration.seconds() > max_seconds_value) { - throw DurationUtil::OutOfRangeException( - fmt::format("Duration out-of-range: {}", duration.DebugString())); + return absl::OutOfRangeError(fmt::format("Duration out-of-range: {}", duration.DebugString())); + } + return absl::OkStatus(); +} + +void validateDuration(const ProtobufWkt::Duration& duration, int64_t max_seconds_value) { + const auto result = validateDurationNoThrow(duration, max_seconds_value); + if (!result.ok()) { + throw DurationUtil::OutOfRangeException(std::string(result.message())); } } @@ -669,6 +677,12 @@ void validateDurationAsMilliseconds(const ProtobufWkt::Duration& duration) { validateDuration(duration, kMaxInt64Nanoseconds); } +absl::Status validateDurationAsMillisecondsNoThrow(const ProtobufWkt::Duration& duration) { + constexpr int64_t kMaxInt64Nanoseconds = + std::numeric_limits::max() / (1000 * 1000 * 1000); + return validateDurationNoThrow(duration, kMaxInt64Nanoseconds); +} + } // namespace uint64_t DurationUtil::durationToMilliseconds(const ProtobufWkt::Duration& duration) { @@ -676,6 +690,15 @@ uint64_t DurationUtil::durationToMilliseconds(const ProtobufWkt::Duration& durat return Protobuf::util::TimeUtil::DurationToMilliseconds(duration); } +absl::StatusOr +DurationUtil::durationToMillisecondsNoThrow(const ProtobufWkt::Duration& duration) { + const auto result = validateDurationAsMillisecondsNoThrow(duration); + if (!result.ok()) { + return result; + } + return Protobuf::util::TimeUtil::DurationToMilliseconds(duration); +} + uint64_t DurationUtil::durationToSeconds(const ProtobufWkt::Duration& duration) { validateDuration(duration); return Protobuf::util::TimeUtil::DurationToSeconds(duration); diff --git a/source/common/protobuf/utility.h b/source/common/protobuf/utility.h index 7f5847e310a7..ad746301d01e 100644 --- a/source/common/protobuf/utility.h +++ b/source/common/protobuf/utility.h @@ -15,6 +15,7 @@ #include "source/common/singleton/const_singleton.h" #include "absl/status/status.h" +#include "absl/status/statusor.h" #include "absl/strings/str_join.h" // Obtain the value of a wrapped field (e.g. google.protobuf.UInt32Value) if set. Otherwise, return @@ -683,6 +684,14 @@ class DurationUtil { */ static uint64_t durationToMilliseconds(const ProtobufWkt::Duration& duration); + /** + * Same as DurationUtil::durationToMilliseconds but does not throw an exception. + * @param duration protobuf. + * @return duration in milliseconds or an error status. + */ + static absl::StatusOr + durationToMillisecondsNoThrow(const ProtobufWkt::Duration& duration); + /** * Same as Protobuf::util::TimeUtil::DurationToSeconds but with extra validation logic. * Specifically, we ensure that the duration is positive. diff --git a/source/extensions/filters/http/ext_proc/ext_proc.cc b/source/extensions/filters/http/ext_proc/ext_proc.cc index f849c0fb4794..34c4ce953a9d 100644 --- a/source/extensions/filters/http/ext_proc/ext_proc.cc +++ b/source/extensions/filters/http/ext_proc/ext_proc.cc @@ -527,12 +527,20 @@ void Filter::sendTrailers(ProcessorState& state, const Http::HeaderMap& trailers stats_.stream_msgs_sent_.inc(); } -void Filter::onNewTimeout(const uint32_t message_timeout_ms) { +void Filter::onNewTimeout(const ProtobufWkt::Duration& override_message_timeout) { + const auto result = DurationUtil::durationToMillisecondsNoThrow(override_message_timeout); + if (!result.ok()) { + ENVOY_LOG(warn, "Ext_proc server new timeout setting is out of duration range. " + "Ignoring the message."); + stats_.override_message_timeout_ignored_.inc(); + return; + } + const auto message_timeout_ms = result.value(); // The new timeout has to be >=1ms and <= max_message_timeout configured in filter. - const uint32_t min_timeout_ms = 1; - const uint32_t max_timeout_ms = config_->maxMessageTimeout(); + const uint64_t min_timeout_ms = 1; + const uint64_t max_timeout_ms = config_->maxMessageTimeout(); if (message_timeout_ms < min_timeout_ms || message_timeout_ms > max_timeout_ms) { - ENVOY_LOG(warn, "Ext_proc server new timeout setting is out of range. " + ENVOY_LOG(warn, "Ext_proc server new timeout setting is out of config range. " "Ignoring the message."); stats_.override_message_timeout_ignored_.inc(); return; @@ -559,7 +567,7 @@ void Filter::onReceiveMessage(std::unique_ptr&& r) { // Check whether the server is asking to extend the timer. if (response->has_override_message_timeout()) { - onNewTimeout(DurationUtil::durationToMilliseconds(response->override_message_timeout())); + onNewTimeout(response->override_message_timeout()); return; } diff --git a/source/extensions/filters/http/ext_proc/ext_proc.h b/source/extensions/filters/http/ext_proc/ext_proc.h index 0bc052997e88..2824cb4d1944 100644 --- a/source/extensions/filters/http/ext_proc/ext_proc.h +++ b/source/extensions/filters/http/ext_proc/ext_proc.h @@ -191,7 +191,7 @@ class Filter : public Logger::Loggable, void onGrpcClose() override; void onMessageTimeout(); - void onNewTimeout(const uint32_t message_timeout_ms); + void onNewTimeout(const ProtobufWkt::Duration& override_message_timeout); void sendBufferedData(ProcessorState& state, ProcessorState::CallbackState new_state, bool end_stream) { diff --git a/test/common/protobuf/utility_test.cc b/test/common/protobuf/utility_test.cc index 030ff3f0ba0e..4e2edbf3e687 100644 --- a/test/common/protobuf/utility_test.cc +++ b/test/common/protobuf/utility_test.cc @@ -1705,6 +1705,52 @@ TEST(DurationUtilTest, OutOfRange) { } } +TEST(DurationUtilTest, NoThrow) { + { + // In range test + ProtobufWkt::Duration duration; + duration.set_seconds(5); + duration.set_nanos(10000000); + const auto result = DurationUtil::durationToMillisecondsNoThrow(duration); + EXPECT_TRUE(result.ok()); + EXPECT_TRUE(result.value() == 5010); + } + + // Below are out-of-range tests + { + ProtobufWkt::Duration duration; + duration.set_seconds(-1); + const auto result = DurationUtil::durationToMillisecondsNoThrow(duration); + EXPECT_FALSE(result.ok()); + } + { + ProtobufWkt::Duration duration; + duration.set_nanos(-1); + const auto result = DurationUtil::durationToMillisecondsNoThrow(duration); + EXPECT_FALSE(result.ok()); + } + { + ProtobufWkt::Duration duration; + duration.set_nanos(1000000000); + const auto result = DurationUtil::durationToMillisecondsNoThrow(duration); + EXPECT_FALSE(result.ok()); + } + { + ProtobufWkt::Duration duration; + duration.set_seconds(Protobuf::util::TimeUtil::kDurationMaxSeconds + 1); + const auto result = DurationUtil::durationToMillisecondsNoThrow(duration); + EXPECT_FALSE(result.ok()); + } + { + ProtobufWkt::Duration duration; + constexpr int64_t kMaxInt64Nanoseconds = + std::numeric_limits::max() / (1000 * 1000 * 1000); + duration.set_seconds(kMaxInt64Nanoseconds + 1); + const auto result = DurationUtil::durationToMillisecondsNoThrow(duration); + EXPECT_FALSE(result.ok()); + } +} + // Verify WIP accounting of the file based annotations. This test uses the strict validator to test // that code path. TEST_F(ProtobufUtilityTest, MessageInWipFile) { diff --git a/test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc b/test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc index 55fb05d10d0f..1d4565a56470 100644 --- a/test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc +++ b/test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc @@ -315,7 +315,7 @@ class ExtProcIntegrationTest : public HttpIntegrationTest, // ext_proc server sends back a response to tell Envoy to stop the // original timer and start a new timer. - void serverSendNewTimeout(const uint32_t timeout_ms) { + void serverSendNewTimeout(const uint64_t timeout_ms) { ProcessingResponse response; if (timeout_ms < 1000) { response.mutable_override_message_timeout()->set_nanos(timeout_ms * 1000000); @@ -327,7 +327,7 @@ class ExtProcIntegrationTest : public HttpIntegrationTest, // The new timeout message is ignored by Envoy due to different reasons, like // new_timeout setting is out-of-range, or max_message_timeout is not configured. - void newTimeoutWrongConfigTest(const uint32_t timeout_ms) { + void newTimeoutWrongConfigTest(const uint64_t timeout_ms) { // Set envoy filter timeout to be 200ms. proto_config_.mutable_message_timeout()->set_nanos(200000000); // Config max_message_timeout proto to enable the new timeout API. @@ -1960,4 +1960,13 @@ TEST_P(ExtProcIntegrationTest, RequestMessageNewTimeoutNegativeTestTimeoutNotAcc newTimeoutWrongConfigTest(500); } +// Send the new timeout to be an extremely large number, which is out-of-range of duration. +// Verify the code appropriately handled it. +TEST_P(ExtProcIntegrationTest, RequestMessageNewTimeoutOutOfBounds) { + // Config max_message_timeout proto to 100ms to enable the new timeout API. + max_message_timeout_ms_ = 100; + const uint64_t override_message_timeout = 1000000000000000; + newTimeoutWrongConfigTest(override_message_timeout); +} + } // namespace Envoy diff --git a/test/extensions/filters/http/ext_proc/unit_test_fuzz/ext_proc_corpus/crash-caae576f1c5a5c4bd6f831dd7d159b2504f476b0 b/test/extensions/filters/http/ext_proc/unit_test_fuzz/ext_proc_corpus/crash-caae576f1c5a5c4bd6f831dd7d159b2504f476b0 new file mode 100644 index 000000000000..ec0f8a51e546 --- /dev/null +++ b/test/extensions/filters/http/ext_proc/unit_test_fuzz/ext_proc_corpus/crash-caae576f1c5a5c4bd6f831dd7d159b2504f476b0 @@ -0,0 +1,36 @@ +config { + grpc_service { + envoy_grpc { + cluster_name: "#" + retry_policy { + retry_back_off { + base_interval { + seconds: 137438953472 + } + } + } + } + timeout { + seconds: 137438953472 + nanos: 655360 + } + } + request_attributes: "#" + message_timeout { + seconds: 137438953472 + } + max_message_timeout { + seconds: 137438953472 + } + disable_clear_route_cache: true +} +request { +} +response { + request_headers { + } + override_message_timeout { + seconds: 137438953472 + nanos: 655360 + } +}