From 6d6fafdb303aee7e49f159c33d9de695c9bef9ae Mon Sep 17 00:00:00 2001 From: htuch Date: Thu, 16 Aug 2018 11:44:01 -0400 Subject: [PATCH] config: strengthen validation for gRPC config sources. (#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 --- source/common/config/utility.cc | 30 +++++++++++++------ .../config/subscription_factory_test.cc | 18 +++++------ test/common/config/utility_test.cc | 27 +++++++++++------ ...testcase-server_fuzz_test-5809171076218880 | 11 +++++++ 4 files changed, 58 insertions(+), 28 deletions(-) create mode 100644 test/server/server_corpus/clusterfuzz-testcase-server_fuzz_test-5809171076218880 diff --git a/source/common/config/utility.cc b/source/common/config/utility.cc index b8369bb2cf0..3d848fd24c2 100644 --- 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 index b9155566914..6a2384a35eb 100644 --- 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 index ecefb84b37d..d71ab74e2ac 100644 --- 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 new file mode 100644 index 00000000000..fd67cec0fc8 --- /dev/null +++ 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: "+" +}