From 74b4c33ffe4f665c34567b962f51dff6828ab544 Mon Sep 17 00:00:00 2001 From: Ismo Puustinen Date: Fri, 8 Mar 2019 17:22:40 +0200 Subject: [PATCH 1/5] upstream: fix oss-fuzz issue #11095. Do not attempt to read IP address information from a unix domain socket address. Signed-off-by: Ismo Puustinen --- source/common/upstream/upstream_impl.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/source/common/upstream/upstream_impl.h b/source/common/upstream/upstream_impl.h index 4fa85a239ad66..b224fd3e7c150 100644 --- a/source/common/upstream/upstream_impl.h +++ b/source/common/upstream/upstream_impl.h @@ -69,7 +69,8 @@ class HostDescriptionImpl : virtual public HostDescription { const envoy::api::v2::endpoint::Endpoint::HealthCheckConfig& health_check_config, uint32_t priority) : cluster_(cluster), hostname_(hostname), address_(dest_address), - health_check_address_(health_check_config.port_value() == 0 + health_check_address_(health_check_config.port_value() == 0 || + dest_address->type() != Network::Address::Type::Ip ? dest_address : Network::Utility::getAddressWithPort( *dest_address, health_check_config.port_value())), From 21e7d1eaee515cf58fcff83b61f73f30e114484d Mon Sep 17 00:00:00 2001 From: Ismo Puustinen Date: Fri, 8 Mar 2019 17:23:11 +0200 Subject: [PATCH 2/5] test: add fuzz test case to corpus. Signed-off-by: Ismo Puustinen --- ...sterfuzz-testcase-minimized-server_fuzz_test-5742573780467712 | 1 + 1 file changed, 1 insertion(+) create mode 100644 test/server/server_corpus/clusterfuzz-testcase-minimized-server_fuzz_test-5742573780467712 diff --git a/test/server/server_corpus/clusterfuzz-testcase-minimized-server_fuzz_test-5742573780467712 b/test/server/server_corpus/clusterfuzz-testcase-minimized-server_fuzz_test-5742573780467712 new file mode 100644 index 0000000000000..672c449b2692f --- /dev/null +++ b/test/server/server_corpus/clusterfuzz-testcase-minimized-server_fuzz_test-5742573780467712 @@ -0,0 +1 @@ +static_resources { clusters { name: " " connect_timeout { nanos: 4 } load_assignment { cluster_name: " " endpoints { lb_endpoints { endpoint { address { pipe { path: " " } } health_check_config { port_value: 2 } } } } } } } \ No newline at end of file From ba1e4d01ab870878e92af4183de20303ba635e62 Mon Sep 17 00:00:00 2001 From: Ismo Puustinen Date: Fri, 8 Mar 2019 18:55:17 +0200 Subject: [PATCH 3/5] tests: add a test for HostDescriptionImpl constructor. Signed-off-by: Ismo Puustinen --- test/common/upstream/upstream_impl_test.cc | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/test/common/upstream/upstream_impl_test.cc b/test/common/upstream/upstream_impl_test.cc index e5314f0fc231e..bf1fc6a5f908c 100644 --- a/test/common/upstream/upstream_impl_test.cc +++ b/test/common/upstream/upstream_impl_test.cc @@ -866,6 +866,19 @@ TEST(HostImplTest, HealthFlags) { EXPECT_EQ(Host::Health::Unhealthy, host->health()); } +// Test that it's possible to do a HostDescriptionImpl with a unix +// domain socket host and a health check config with non-zero port. +// This is a regression test for oss-fuzz issue +// https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=11095 +TEST(HostImplTest, HealthPipeAddress) { + std::shared_ptr info{new NiceMock()}; + envoy::api::v2::endpoint::Endpoint::HealthCheckConfig config; + config.set_port_value(8000); + HostDescriptionImpl descr(info, "", Network::Utility::resolveUrl("unix://foo"), + envoy::api::v2::core::Metadata::default_instance(), + envoy::api::v2::core::Locality().default_instance(), config, 1); +} + class StaticClusterImplTest : public testing::Test, public UpstreamImplTestBase {}; TEST_F(StaticClusterImplTest, InitialHosts) { From d97a7705d8d35a817d35fb181cd09dbae3b366f0 Mon Sep 17 00:00:00 2001 From: Ismo Puustinen Date: Mon, 11 Mar 2019 11:28:43 +0200 Subject: [PATCH 4/5] If health check address is misconfigured, throw exception. Signed-off-by: Ismo Puustinen --- source/common/upstream/upstream_impl.h | 19 +++++++++++++------ test/common/upstream/upstream_impl_test.cc | 18 +++++++++++------- tools/spelling_dictionary.txt | 1 + 3 files changed, 25 insertions(+), 13 deletions(-) diff --git a/source/common/upstream/upstream_impl.h b/source/common/upstream/upstream_impl.h index b224fd3e7c150..5641671313d0a 100644 --- a/source/common/upstream/upstream_impl.h +++ b/source/common/upstream/upstream_impl.h @@ -69,18 +69,25 @@ class HostDescriptionImpl : virtual public HostDescription { const envoy::api::v2::endpoint::Endpoint::HealthCheckConfig& health_check_config, uint32_t priority) : cluster_(cluster), hostname_(hostname), address_(dest_address), - health_check_address_(health_check_config.port_value() == 0 || - dest_address->type() != Network::Address::Type::Ip - ? dest_address - : Network::Utility::getAddressWithPort( - *dest_address, health_check_config.port_value())), canary_(Config::Metadata::metadataValue(metadata, Config::MetadataFilters::get().ENVOY_LB, Config::MetadataEnvoyLbKeys::get().CANARY) .bool_value()), metadata_(std::make_shared(metadata)), locality_(locality), stats_{ALL_HOST_STATS(POOL_COUNTER(stats_store_), POOL_GAUGE(stats_store_))}, - priority_(priority) {} + priority_(priority) { + if (health_check_config.port_value() != 0 && + dest_address->type() != Network::Address::Type::Ip) { + // Setting the health check port to non-0 only works for IP-type addresses. Setting the port + // for a pipe address is a misconfiguration. Throw an exception. + throw EnvoyException( + fmt::format("Invalid host configuration: non-null port for non-IP address")); + } + health_check_address_ = + health_check_config.port_value() == 0 + ? dest_address + : Network::Utility::getAddressWithPort(*dest_address, health_check_config.port_value()); + } // Upstream::HostDescription bool canary() const override { return canary_; } diff --git a/test/common/upstream/upstream_impl_test.cc b/test/common/upstream/upstream_impl_test.cc index bf1fc6a5f908c..6a648e07b0b89 100644 --- a/test/common/upstream/upstream_impl_test.cc +++ b/test/common/upstream/upstream_impl_test.cc @@ -866,17 +866,21 @@ TEST(HostImplTest, HealthFlags) { EXPECT_EQ(Host::Health::Unhealthy, host->health()); } -// Test that it's possible to do a HostDescriptionImpl with a unix +// Test that it's not possible to do a HostDescriptionImpl with a unix // domain socket host and a health check config with non-zero port. // This is a regression test for oss-fuzz issue // https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=11095 TEST(HostImplTest, HealthPipeAddress) { - std::shared_ptr info{new NiceMock()}; - envoy::api::v2::endpoint::Endpoint::HealthCheckConfig config; - config.set_port_value(8000); - HostDescriptionImpl descr(info, "", Network::Utility::resolveUrl("unix://foo"), - envoy::api::v2::core::Metadata::default_instance(), - envoy::api::v2::core::Locality().default_instance(), config, 1); + EXPECT_THROW_WITH_MESSAGE( + { + std::shared_ptr info{new NiceMock()}; + envoy::api::v2::endpoint::Endpoint::HealthCheckConfig config; + config.set_port_value(8000); + HostDescriptionImpl descr(info, "", Network::Utility::resolveUrl("unix://foo"), + envoy::api::v2::core::Metadata::default_instance(), + envoy::api::v2::core::Locality().default_instance(), config, 1); + }, + EnvoyException, "Invalid host configuration: non-null port for non-IP address"); } class StaticClusterImplTest : public testing::Test, public UpstreamImplTestBase {}; diff --git a/tools/spelling_dictionary.txt b/tools/spelling_dictionary.txt index 34a034702ad1a..80345ddc7e22f 100644 --- a/tools/spelling_dictionary.txt +++ b/tools/spelling_dictionary.txt @@ -522,6 +522,7 @@ mem memcpy midp milli +misconfiguration misconfigured mixin mkdir From c7d08b64bff72307001f7fcbdd44872a55bdc666 Mon Sep 17 00:00:00 2001 From: Ismo Puustinen Date: Mon, 11 Mar 2019 21:44:22 +0200 Subject: [PATCH 5/5] Change non-null port -> non-zero port. Signed-off-by: Ismo Puustinen --- source/common/upstream/upstream_impl.h | 2 +- test/common/upstream/upstream_impl_test.cc | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/source/common/upstream/upstream_impl.h b/source/common/upstream/upstream_impl.h index 5641671313d0a..a8373afaed6c8 100644 --- a/source/common/upstream/upstream_impl.h +++ b/source/common/upstream/upstream_impl.h @@ -81,7 +81,7 @@ class HostDescriptionImpl : virtual public HostDescription { // Setting the health check port to non-0 only works for IP-type addresses. Setting the port // for a pipe address is a misconfiguration. Throw an exception. throw EnvoyException( - fmt::format("Invalid host configuration: non-null port for non-IP address")); + fmt::format("Invalid host configuration: non-zero port for non-IP address")); } health_check_address_ = health_check_config.port_value() == 0 diff --git a/test/common/upstream/upstream_impl_test.cc b/test/common/upstream/upstream_impl_test.cc index 6a648e07b0b89..f1610af265229 100644 --- a/test/common/upstream/upstream_impl_test.cc +++ b/test/common/upstream/upstream_impl_test.cc @@ -880,7 +880,7 @@ TEST(HostImplTest, HealthPipeAddress) { envoy::api::v2::core::Metadata::default_instance(), envoy::api::v2::core::Locality().default_instance(), config, 1); }, - EnvoyException, "Invalid host configuration: non-null port for non-IP address"); + EnvoyException, "Invalid host configuration: non-zero port for non-IP address"); } class StaticClusterImplTest : public testing::Test, public UpstreamImplTestBase {};