Skip to content

Commit

Permalink
Envoy ext_proc filter throw exception when received response timeout …
Browse files Browse the repository at this point in the history
…Duration is too large (envoyproxy#27260)

* Adding ext_proc filter config and response timeout Duration PGVs to avoid ext_proc filter fuzzer crash due to duration config out-of-bounds.

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
Signed-off-by: Ryan Eskin <ryan.eskin89@protonmail.com>
  • Loading branch information
yanjunxiang-google authored and reskin89 committed Jul 11, 2023
1 parent 96b9b85 commit e749b66
Show file tree
Hide file tree
Showing 8 changed files with 158 additions and 20 deletions.
23 changes: 15 additions & 8 deletions api/envoy/extensions/filters/http/ext_proc/v3/ext_proc.proto
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -166,8 +169,12 @@ message ExternalProcessor {

// Specify the upper bound of
// :ref:`override_message_timeout <envoy_v3_api_field_service.ext_proc.v3.ProcessingResponse.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 <envoy_v3_api_field_service.ext_proc.v3.CommonResponse.clear_route_cache>`
Expand Down
31 changes: 27 additions & 4 deletions source/common/protobuf/utility.cc
Original file line number Diff line number Diff line change
Expand Up @@ -645,14 +645,22 @@ ProtobufWkt::Value ValueUtil::listValue(const std::vector<ProtobufWkt::Value>& 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()));
}
}

Expand All @@ -669,13 +677,28 @@ void validateDurationAsMilliseconds(const ProtobufWkt::Duration& duration) {
validateDuration(duration, kMaxInt64Nanoseconds);
}

absl::Status validateDurationAsMillisecondsNoThrow(const ProtobufWkt::Duration& duration) {
constexpr int64_t kMaxInt64Nanoseconds =
std::numeric_limits<int64_t>::max() / (1000 * 1000 * 1000);
return validateDurationNoThrow(duration, kMaxInt64Nanoseconds);
}

} // namespace

uint64_t DurationUtil::durationToMilliseconds(const ProtobufWkt::Duration& duration) {
validateDurationAsMilliseconds(duration);
return Protobuf::util::TimeUtil::DurationToMilliseconds(duration);
}

absl::StatusOr<uint64_t>
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);
Expand Down
9 changes: 9 additions & 0 deletions source/common/protobuf/utility.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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<uint64_t>
durationToMillisecondsNoThrow(const ProtobufWkt::Duration& duration);

/**
* Same as Protobuf::util::TimeUtil::DurationToSeconds but with extra validation logic.
* Specifically, we ensure that the duration is positive.
Expand Down
18 changes: 13 additions & 5 deletions source/extensions/filters/http/ext_proc/ext_proc.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -559,7 +567,7 @@ void Filter::onReceiveMessage(std::unique_ptr<ProcessingResponse>&& 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;
}

Expand Down
2 changes: 1 addition & 1 deletion source/extensions/filters/http/ext_proc/ext_proc.h
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@ class Filter : public Logger::Loggable<Logger::Id::ext_proc>,
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) {
Expand Down
46 changes: 46 additions & 0 deletions test/common/protobuf/utility_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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<int64_t>::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) {
Expand Down
13 changes: 11 additions & 2 deletions test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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.
Expand Down Expand Up @@ -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

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

0 comments on commit e749b66

Please sign in to comment.