Fix grpc-timeout integer overflow (envoyproxy#237)

Fixes CVE-2021-28682, a remotely exploitable integer overflow.

Signed-off-by: Asra Ali <asraa@google.com>
Co-authored-by: Tony Allen <tony@allen.gg>
Signed-off-by: Tony Allen <tony@allen.gg>
Signed-off-by: Gokul Nair <gnair@twitter.com>

source/common/grpc/common.cc

@@ -164,12 +164,18 @@ Common::getGrpcTimeout(const Http::RequestHeaderMap& request_headers) {
   const Http::HeaderEntry* header_grpc_timeout_entry = request_headers.GrpcTimeout();
   std::chrono::milliseconds timeout;
   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 absl::nullopt;
+    }
+    // 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':
         return std::chrono::hours(grpc_timeout);
       case 'M':
@@ -204,7 +210,6 @@ void Common::toGrpcTimeout(const std::chrono::milliseconds& timeout,
   uint64_t time = timeout.count();
   static const char units[] = "mSMH";
   const char* unit = units; // start with milliseconds
-  static constexpr size_t MAX_GRPC_TIMEOUT_VALUE = 99999999;
   if (time > MAX_GRPC_TIMEOUT_VALUE) {
     time /= 1000; // Convert from milliseconds to seconds
     unit++;

source/common/grpc/common.h

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

test/common/grpc/common_test.cc

@@ -78,6 +78,9 @@ TEST(GrpcContextTest, GetGrpcTimeout) {
   Http::TestRequestHeaderMapImpl missing_unit{{"grpc-timeout", "123"}};
   EXPECT_EQ(absl::nullopt, Common::getGrpcTimeout(missing_unit));
 
+  Http::TestRequestHeaderMapImpl small_missing_unit{{"grpc-timeout", "1"}};
+  EXPECT_EQ(absl::nullopt, Common::getGrpcTimeout(small_missing_unit));
+
   Http::TestRequestHeaderMapImpl illegal_unit{{"grpc-timeout", "123F"}};
   EXPECT_EQ(absl::nullopt, Common::getGrpcTimeout(illegal_unit));
 
@@ -102,8 +105,19 @@ 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.
+  // Test max 8 digits to prevent millisecond overflow.
+  Http::TestRequestHeaderMapImpl value_overflow{{"grpc-timeout", "6666666666666H"}};
+  EXPECT_EQ(absl::nullopt, Common::getGrpcTimeout(value_overflow));
+
+  // Reject negative values.
+  Http::TestRequestHeaderMapImpl value_negative{{"grpc-timeout", "-1S"}};
+  EXPECT_EQ(absl::nullopt, 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/conn_manager_impl_test.cc

@@ -2533,6 +2533,19 @@ TEST_F(HttpConnectionManagerImplTest, DurationTimeout) {
     decoder_filters_[0]->callbacks_->clusterInfo();
   }
 
+  // With an invalid gRPC timeout, refreshing cached route will not use header and use stream
+  // duration.
+  latched_headers->setGrpcTimeout("6666666666666H");
+  {
+    // 25ms used already from previous case so timer is set to be 5ms.
+    EXPECT_CALL(*timer, enableTimer(std::chrono::milliseconds(5), _));
+    EXPECT_CALL(route_config_provider_.route_config_->route_->route_entry_, maxStreamDuration())
+        .Times(2)
+        .WillRepeatedly(Return(std::chrono::milliseconds(30)));
+    decoder_filters_[0]->callbacks_->clearRouteCache();
+    decoder_filters_[0]->callbacks_->clusterInfo();
+  }
+
   // Cleanup.
   EXPECT_CALL(*timer, disableTimer());
   EXPECT_CALL(*decoder_filters_[0], onStreamComplete());

test/common/router/router_test.cc

@@ -5376,6 +5376,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)));