backport 1.14: grpc: fix grpc-timeout integer-overflow (#257)

Fixes CVE-2021-28682, a remotely exploitable integer overflow. Signed-off-by: Asra Ali <[email protected]> Co-authored-by: Tony Allen <[email protected]> Signed-off-by: Tony Allen <[email protected]>
envoyproxy · Apr 15, 2021 · 96e889d · 96e889d
1 parent 4105bfb
commit 96e889d
Show file tree

Hide file tree

Showing 5 changed files with 40 additions and 10 deletions.
diff --git a/source/common/grpc/common.cc b/source/common/grpc/common.cc
@@ -160,12 +160,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;

diff --git a/source/common/grpc/common.h b/source/common/grpc/common.h
@@ -169,6 +169,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
@@ -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
@@ -4948,6 +4948,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)));

diff --git a/test/integration/http2_integration_test.cc b/test/integration/http2_integration_test.cc
@@ -1904,8 +1904,7 @@ TEST_P(Http2FloodMitigationTest, DownstreamSendingEmptyMetadata) {
 
   const uint32_t client_stream_idx = 1;
   // Send request.
-  const Http2Frame request =
-      Http2Frame::makePostRequest(client_stream_idx, "host", "/");
+  const Http2Frame request = Http2Frame::makePostRequest(client_stream_idx, "host", "/");
   sendFame(request);
   ASSERT_TRUE(fake_upstreams_[0]->waitForHttpConnection(*dispatcher_, fake_upstream_connection));
   ASSERT_TRUE(fake_upstream_connection->waitForNewStream(*dispatcher_, fake_upstream_request));