config: strengthen validation for gRPC config sources. (envoyproxy#4171)

This addresses oss-fuzz issue https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=9335, where a bad config could cause the protobuf library to throw a non-EnvoyException CHECK exception, causing Envoy to abort. As a bonus, made sure we include the ApiConfigSource debug string in respective EnvoyExceptions, this makes pinpointing the specific part of the config easier in large configs. Risk level: Low Testing: Corpus entry and unit test added. Signed-off-by: Harvey Tuch <htuch@google.com>
rshriram · Aug 16, 2018 · 6d6fafd · 6d6fafd
1 parent 132302c
commit 6d6fafd
Show file tree

Hide file tree

Showing 4 changed files with 58 additions and 28 deletions.
diff --git a/source/common/config/utility.cc b/source/common/config/utility.cc
@@ -89,26 +89,33 @@ void Utility::checkApiConfigSourceNames(
       (api_config_source.api_type() == envoy::api::v2::core::ApiConfigSource::GRPC);
 
   if (api_config_source.cluster_names().empty() && api_config_source.grpc_services().empty()) {
-    throw EnvoyException("API configs must have either a gRPC service or a cluster name defined");
+    throw EnvoyException(
+        fmt::format("API configs must have either a gRPC service or a cluster name defined: {}",
+                    api_config_source.DebugString()));
   }
 
   if (is_grpc) {
     if (!api_config_source.cluster_names().empty()) {
-      throw EnvoyException(
-          "envoy::api::v2::core::ConfigSource::GRPC must not have a cluster name specified.");
+      throw EnvoyException(fmt::format(
+          "envoy::api::v2::core::ConfigSource::GRPC must not have a cluster name specified: {}",
+          api_config_source.DebugString()));
     }
     if (api_config_source.grpc_services().size() > 1) {
-      throw EnvoyException(
-          "envoy::api::v2::core::ConfigSource::GRPC must have a single gRPC service specified");
+      throw EnvoyException(fmt::format(
+          "envoy::api::v2::core::ConfigSource::GRPC must have a single gRPC service specified: {}",
+          api_config_source.DebugString()));
     }
   } else {
     if (!api_config_source.grpc_services().empty()) {
-      throw EnvoyException("envoy::api::v2::core::ConfigSource, if not of type gRPC, must not have "
-                           "a gRPC service specified");
+      throw EnvoyException(
+          fmt::format("envoy::api::v2::core::ConfigSource, if not of type gRPC, must not have "
+                      "a gRPC service specified: {}",
+                      api_config_source.DebugString()));
     }
     if (api_config_source.cluster_names().size() != 1) {
-      throw EnvoyException(
-          "envoy::api::v2::core::ConfigSource must have a singleton cluster name specified");
+      throw EnvoyException(fmt::format(
+          "envoy::api::v2::core::ConfigSource must have a singleton cluster name specified: {}",
+          api_config_source.DebugString()));
     }
   }
 }
@@ -226,6 +233,11 @@ Grpc::AsyncClientFactoryPtr Utility::factoryForGrpcApiConfigSource(
     const envoy::api::v2::core::ApiConfigSource& api_config_source, Stats::Scope& scope) {
   Utility::checkApiConfigSourceNames(api_config_source);
 
+  if (api_config_source.api_type() != envoy::api::v2::core::ApiConfigSource::GRPC) {
+    throw EnvoyException(fmt::format("envoy::api::v2::core::ConfigSource type must be GRPC: {}",
+                                     api_config_source.DebugString()));
+  }
+
   envoy::api::v2::core::GrpcService grpc_service;
   grpc_service.MergeFrom(api_config_source.grpc_services(0));
 

diff --git a/test/common/config/subscription_factory_test.cc b/test/common/config/subscription_factory_test.cc
@@ -68,9 +68,8 @@ TEST_F(SubscriptionFactoryTest, RestClusterEmpty) {
   config.mutable_api_config_source()->set_api_type(envoy::api::v2::core::ApiConfigSource::REST);
 
   EXPECT_CALL(cm_, clusters()).WillOnce(Return(cluster_map));
-  EXPECT_THROW_WITH_MESSAGE(
-      subscriptionFromConfigSource(config), EnvoyException,
-      "API configs must have either a gRPC service or a cluster name defined");
+  EXPECT_THROW_WITH_REGEX(subscriptionFromConfigSource(config), EnvoyException,
+                          "API configs must have either a gRPC service or a cluster name defined:");
 }
 
 TEST_F(SubscriptionFactoryTest, GrpcClusterEmpty) {
@@ -80,9 +79,8 @@ TEST_F(SubscriptionFactoryTest, GrpcClusterEmpty) {
   config.mutable_api_config_source()->set_api_type(envoy::api::v2::core::ApiConfigSource::GRPC);
 
   EXPECT_CALL(cm_, clusters()).WillOnce(Return(cluster_map));
-  EXPECT_THROW_WITH_MESSAGE(
-      subscriptionFromConfigSource(config), EnvoyException,
-      "API configs must have either a gRPC service or a cluster name defined");
+  EXPECT_THROW_WITH_REGEX(subscriptionFromConfigSource(config), EnvoyException,
+                          "API configs must have either a gRPC service or a cluster name defined:");
 }
 
 TEST_F(SubscriptionFactoryTest, RestClusterSingleton) {
@@ -150,9 +148,9 @@ TEST_F(SubscriptionFactoryTest, RestClusterMultiton) {
   EXPECT_CALL(cm_, clusters()).WillRepeatedly(Return(cluster_map));
   EXPECT_CALL(*cluster.info_, addedViaApi()).WillRepeatedly(Return(false));
   EXPECT_CALL(*cluster.info_, type()).WillRepeatedly(Return(envoy::api::v2::Cluster::STATIC));
-  EXPECT_THROW_WITH_MESSAGE(
+  EXPECT_THROW_WITH_REGEX(
       subscriptionFromConfigSource(config), EnvoyException,
-      "envoy::api::v2::core::ConfigSource must have a singleton cluster name specified");
+      "envoy::api::v2::core::ConfigSource must have a singleton cluster name specified:");
 }
 
 TEST_F(SubscriptionFactoryTest, GrpcClusterMultiton) {
@@ -174,9 +172,9 @@ TEST_F(SubscriptionFactoryTest, GrpcClusterMultiton) {
   EXPECT_CALL(*cluster.info_, addedViaApi()).WillRepeatedly(Return(false));
   EXPECT_CALL(*cluster.info_, type()).WillRepeatedly(Return(envoy::api::v2::Cluster::STATIC));
 
-  EXPECT_THROW_WITH_MESSAGE(
+  EXPECT_THROW_WITH_REGEX(
       subscriptionFromConfigSource(config), EnvoyException,
-      "envoy::api::v2::core::ConfigSource::GRPC must have a single gRPC service specified");
+      "envoy::api::v2::core::ConfigSource::GRPC must have a single gRPC service specified:");
 }
 
 TEST_F(SubscriptionFactoryTest, FilesystemSubscription) {

diff --git a/test/common/config/utility_test.cc b/test/common/config/utility_test.cc
@@ -225,7 +225,7 @@ TEST(UtilityTest, FactoryForGrpcApiConfigSource) {
     api_config_source.set_api_type(envoy::api::v2::core::ApiConfigSource::GRPC);
     EXPECT_THROW_WITH_REGEX(
         Utility::factoryForGrpcApiConfigSource(async_client_manager, api_config_source, scope),
-        EnvoyException, "API configs must have either a gRPC service or a cluster name defined");
+        EnvoyException, "API configs must have either a gRPC service or a cluster name defined:");
   }
 
   {
@@ -236,7 +236,7 @@ TEST(UtilityTest, FactoryForGrpcApiConfigSource) {
     EXPECT_THROW_WITH_REGEX(
         Utility::factoryForGrpcApiConfigSource(async_client_manager, api_config_source, scope),
         EnvoyException,
-        "envoy::api::v2::core::ConfigSource::GRPC must have a single gRPC service specified");
+        "envoy::api::v2::core::ConfigSource::GRPC must have a single gRPC service specified:");
   }
 
   {
@@ -247,7 +247,7 @@ TEST(UtilityTest, FactoryForGrpcApiConfigSource) {
     EXPECT_THROW_WITH_REGEX(
         Utility::factoryForGrpcApiConfigSource(async_client_manager, api_config_source, scope),
         EnvoyException,
-        "envoy::api::v2::core::ConfigSource::GRPC must not have a cluster name specified.");
+        "envoy::api::v2::core::ConfigSource::GRPC must not have a cluster name specified:");
   }
 
   {
@@ -258,7 +258,7 @@ TEST(UtilityTest, FactoryForGrpcApiConfigSource) {
     EXPECT_THROW_WITH_REGEX(
         Utility::factoryForGrpcApiConfigSource(async_client_manager, api_config_source, scope),
         EnvoyException,
-        "envoy::api::v2::core::ConfigSource::GRPC must not have a cluster name specified.");
+        "envoy::api::v2::core::ConfigSource::GRPC must not have a cluster name specified:");
   }
 
   {
@@ -270,7 +270,16 @@ TEST(UtilityTest, FactoryForGrpcApiConfigSource) {
         Utility::factoryForGrpcApiConfigSource(async_client_manager, api_config_source, scope),
         EnvoyException,
         "envoy::api::v2::core::ConfigSource, if not of type gRPC, must not have a gRPC service "
-        "specified");
+        "specified:");
+  }
+
+  {
+    envoy::api::v2::core::ApiConfigSource api_config_source;
+    api_config_source.set_api_type(envoy::api::v2::core::ApiConfigSource::REST);
+    api_config_source.add_cluster_names("foo");
+    EXPECT_THROW_WITH_REGEX(
+        Utility::factoryForGrpcApiConfigSource(async_client_manager, api_config_source, scope),
+        EnvoyException, "envoy::api::v2::core::ConfigSource type must be GRPC:");
   }
 
   {
@@ -303,9 +312,9 @@ TEST(CheckApiConfigSourceSubscriptionBackingClusterTest, GrpcClusterTestAcrossTy
   api_config_source->set_api_type(envoy::api::v2::core::ApiConfigSource::GRPC);
 
   // GRPC cluster without GRPC services.
-  EXPECT_THROW_WITH_MESSAGE(
+  EXPECT_THROW_WITH_REGEX(
       Utility::checkApiConfigSourceSubscriptionBackingCluster(cluster_map, *api_config_source),
-      EnvoyException, "API configs must have either a gRPC service or a cluster name defined");
+      EnvoyException, "API configs must have either a gRPC service or a cluster name defined:");
 
   // Non-existent cluster.
   api_config_source->add_grpc_services()->mutable_envoy_grpc()->set_cluster_name("foo_cluster");
@@ -344,10 +353,10 @@ TEST(CheckApiConfigSourceSubscriptionBackingClusterTest, GrpcClusterTestAcrossTy
 
   // API with cluster_names set should be rejected.
   api_config_source->add_cluster_names("foo_cluster");
-  EXPECT_THROW_WITH_MESSAGE(
+  EXPECT_THROW_WITH_REGEX(
       Utility::checkApiConfigSourceSubscriptionBackingCluster(cluster_map, *api_config_source),
       EnvoyException,
-      "envoy::api::v2::core::ConfigSource::GRPC must not have a cluster name specified.");
+      "envoy::api::v2::core::ConfigSource::GRPC must not have a cluster name specified:");
 }
 
 TEST(CheckApiConfigSourceSubscriptionBackingClusterTest, RestClusterTestAcrossTypes) {

diff --git a/test/server/server_corpus/clusterfuzz-testcase-server_fuzz_test-5809171076218880 b/test/server/server_corpus/clusterfuzz-testcase-server_fuzz_test-5809171076218880
@@ -0,0 +1,11 @@
+admin {
+  access_log_path: "@"
+  address {
+    pipe {
+      path: "@"
+    }
+  }
+}
+hds_config {
+  cluster_names: "+"
+}