From d304a2f3260c0aa8839c4623ebadf1265b748a3d Mon Sep 17 00:00:00 2001
From: Omri Zohar <omri.zohar@gmail.com>
Date: Thu, 20 May 2021 06:17:43 +0300
Subject: [PATCH] Fixing GRPC initial metadata validation (#16414)

According to the GRPC spec https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md
The values in the initial metadata can be any valid ASCII character in
case of a non binary header type.

Risk Level: Low
Testing: Unit Tests
Docs Changes:N/A
Release Notes: Fixing GRPC initial metadata validation for ASCII characters

Signed-off-by: Omri Zohar <omriz@google.com>
---
 .../common/grpc/async_client_manager_impl.cc  | 26 +++++++++++++++++--
 .../grpc/async_client_manager_impl_test.cc    | 24 +++++++++++++++--
 2 files changed, 46 insertions(+), 4 deletions(-)

diff --git a/source/common/grpc/async_client_manager_impl.cc b/source/common/grpc/async_client_manager_impl.cc
index 14866aa25d0e..7b122e8bd59e 100644
--- a/source/common/grpc/async_client_manager_impl.cc
+++ b/source/common/grpc/async_client_manager_impl.cc
@@ -3,8 +3,11 @@
 #include "envoy/config/core/v3/grpc_service.pb.h"
 #include "envoy/stats/scope.h"
 
+#include "common/common/base64.h"
 #include "common/grpc/async_client_impl.h"
 
+#include "absl/strings/match.h"
+
 #ifdef ENVOY_GOOGLE_GRPC
 #include "common/grpc/google_async_client_impl.h"
 #endif
@@ -24,6 +27,15 @@ bool validateGrpcHeaderChars(absl::string_view key) {
   return true;
 }
 
+bool validateGrpcCompatibleAsciiHeaderValue(absl::string_view h_value) {
+  for (auto ch : h_value) {
+    if (ch < 0x20 || ch > 0x7e) {
+      return false;
+    }
+  }
+  return true;
+}
+
 } // namespace
 
 AsyncClientFactoryImpl::AsyncClientFactoryImpl(Upstream::ClusterManager& cm,
@@ -84,8 +96,18 @@ GoogleAsyncClientFactoryImpl::GoogleAsyncClientFactoryImpl(
   // Check metadata for gRPC API compliance. Uppercase characters are lowered in the HeaderParser.
   // https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md
   for (const auto& header : config.initial_metadata()) {
-    if (!validateGrpcHeaderChars(header.key()) || !validateGrpcHeaderChars(header.value())) {
-      throw EnvoyException("Illegal characters in gRPC initial metadata.");
+    // Validate key
+    if (!validateGrpcHeaderChars(header.key())) {
+      throw EnvoyException(
+          fmt::format("Illegal characters in gRPC initial metadata header key: {}.", header.key()));
+    }
+
+    // Validate value
+    // Binary base64 encoded - handled by the GRPC library
+    if (!::absl::EndsWith(header.key(), "-bin") &&
+        !validateGrpcCompatibleAsciiHeaderValue(header.value())) {
+      throw EnvoyException(fmt::format(
+          "Illegal ASCII value for gRPC initial metadata header key: {}.", header.key()));
     }
   }
 }
diff --git a/test/common/grpc/async_client_manager_impl_test.cc b/test/common/grpc/async_client_manager_impl_test.cc
index 76c7c869a3b8..72d8507ed81e 100644
--- a/test/common/grpc/async_client_manager_impl_test.cc
+++ b/test/common/grpc/async_client_manager_impl_test.cc
@@ -88,7 +88,7 @@ TEST_F(AsyncClientManagerImplTest, GoogleGrpc) {
 #endif
 }
 
-TEST_F(AsyncClientManagerImplTest, GoogleGrpcIllegalChars) {
+TEST_F(AsyncClientManagerImplTest, GoogleGrpcIllegalCharsInKey) {
   EXPECT_CALL(scope_, createScope_("grpc.foo."));
   envoy::config::core::v3::GrpcService grpc_service;
   grpc_service.mutable_google_grpc()->set_stat_prefix("foo");
@@ -100,7 +100,7 @@ TEST_F(AsyncClientManagerImplTest, GoogleGrpcIllegalChars) {
 #ifdef ENVOY_GOOGLE_GRPC
   EXPECT_THROW_WITH_MESSAGE(
       async_client_manager_.factoryForGrpcService(grpc_service, scope_, false), EnvoyException,
-      "Illegal characters in gRPC initial metadata.");
+      "Illegal characters in gRPC initial metadata header key: illegalcharacter;.");
 #else
   EXPECT_THROW_WITH_MESSAGE(
       async_client_manager_.factoryForGrpcService(grpc_service, scope_, false), EnvoyException,
@@ -126,6 +126,26 @@ TEST_F(AsyncClientManagerImplTest, LegalGoogleGrpcChar) {
 #endif
 }
 
+TEST_F(AsyncClientManagerImplTest, GoogleGrpcIllegalCharsInValue) {
+  EXPECT_CALL(scope_, createScope_("grpc.foo."));
+  envoy::config::core::v3::GrpcService grpc_service;
+  grpc_service.mutable_google_grpc()->set_stat_prefix("foo");
+
+  auto& metadata = *grpc_service.mutable_initial_metadata()->Add();
+  metadata.set_key("legal-key");
+  metadata.set_value("NonAsciValue.भारत");
+
+#ifdef ENVOY_GOOGLE_GRPC
+  EXPECT_THROW_WITH_MESSAGE(
+      async_client_manager_.factoryForGrpcService(grpc_service, scope_, false), EnvoyException,
+      "Illegal ASCII value for gRPC initial metadata header key: legal-key.");
+#else
+  EXPECT_THROW_WITH_MESSAGE(
+      async_client_manager_.factoryForGrpcService(grpc_service, scope_, false), EnvoyException,
+      "Google C++ gRPC client is not linked");
+#endif
+}
+
 TEST_F(AsyncClientManagerImplTest, EnvoyGrpcUnknownOk) {
   envoy::config::core::v3::GrpcService grpc_service;
   grpc_service.mutable_envoy_grpc()->set_cluster_name("foo");