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>
envoyproxy · May 20, 2021 · d304a2f · d304a2f
1 parent fe58023
commit d304a2f
Show file tree

Hide file tree

Showing 2 changed files with 46 additions and 4 deletions.
diff --git 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
@@ -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");