From b0dc1c486c45b078f3b826b38f41d3a9f8076d25 Mon Sep 17 00:00:00 2001 From: Piotr Sikora Date: Tue, 7 Jul 2020 16:14:03 -0700 Subject: [PATCH] tls: improve wildcard matching (#11921) (#11927) Patching in 11885 with runtime guards and release notes Risk Level: Medium (changes to cert matching) Testing: new unit test Docs Changes: n/a Release Notes: inline Runtime guard: envoy.reloadable_features.fix_wildcard_matching Signed-off-by: Yann Soubeyrand Signed-off-by: Alyssa Wilk Signed-off-by: Piotr Sikora --- VERSION | 2 +- docs/root/intro/version_history.rst | 26 ++++++------- source/common/runtime/runtime_features.cc | 1 + source/extensions/transport_sockets/tls/BUILD | 1 + .../transport_sockets/tls/context_impl.cc | 10 ++++- test/extensions/transport_sockets/tls/BUILD | 1 + .../tls/context_impl_test.cc | 37 +++++++++++++++++++ 7 files changed, 61 insertions(+), 17 deletions(-) diff --git a/VERSION b/VERSION index 868b3aa82451..456e5c4ad803 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -1.12.5-dev +1.12.6 diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index f0c76944e7ce..52d976a9e82b 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -1,18 +1,18 @@ Version history --------------- -1.12.3 (Pending) -========================== -* buffer: force copy when appending small slices to OwnedImpl buffer to avoid fragmentation. -* listeners: fixed issue where :ref:`TLS inspector listener filter ` could have been bypassed by a client using only TLS 1.3. -* sds: fixed the SDS vulnerability that TLS validation context (e.g., subject alt name or hash) cannot be effectively validated in some cases. -* http: added HTTP/1.1 flood protection. Can be temporarily disabled using the runtime feature `envoy.reloadable_features.http1_flood_protection`. -1.12.5 (Pending) -================ -* http: the :ref:`stream_idle_timeout ` - now also defends against an HTTP/2 peer that does not open stream window once an entire response has been buffered to be sent to a downstream client. -* listener: add runtime support for `per-listener limits ` on active/accepted connections. -* overload management: add runtime support for :ref:`global limits ` on active/accepted connections. +1.12.6 (July 7, 2020) +===================== +* tls: fixed a bug where wilcard matching for "\*.foo.com" also matched domains of the form "a.b.foo.com". This behavior can be temporarily reverted by setting runtime feature `envoy.reloadable_features.fix_wildcard_matching` to false. + +1.12.5 (June 30, 2020) +====================== +* buffer: fixed CVE-2020-12603 by avoiding fragmentation, and tracking of HTTP/2 data and control frames in the output buffer. +* http: fixed CVE-2020-12604 by changing :ref:`stream_idle_timeout ` + to also defend against an HTTP/2 peer that does not open stream window once an entire response has been buffered to be sent to a downstream client. +* http: fixed CVE-2020-12605 by including request URL in request header size computation, and rejecting partial headers that exceed configured limits. +* listener: mitigated CVE-2020-8663 by adding runtime support for :ref:`per-listener limits ` on active/accepted connections. +* overload management: mitigated CVE-2020-8663 by adding runtime support for :ref:`global limits ` on active/accepted connections. 1.12.4 (June 8, 2020) ===================== @@ -87,11 +87,9 @@ Version history * http: :ref:`AUTO ` codec protocol inference now requires the H2 magic bytes to be the first bytes transmitted by a downstream client. * http: remove h2c upgrade headers for HTTP/1 as h2c upgrades are currently not supported. * http: absolute URL support is now on by default. The prior behavior can be reinstated by setting :ref:`allow_absolute_url ` to false. -* http: added strict authority checking. This can be reversed temporarily by setting the runtime feature `envoy.reloadable_features.strict_authority_validation` to false. * http: support :ref:`host rewrite ` in the dynamic forward proxy. * http: support :ref:`disabling the filter per route ` in the grpc http1 reverse bridge filter. * http: added the ability to :ref:`configure max connection duration ` for downstream connections. -* http: trim LWS at the end of header keys, for correct HTTP/1.1 header parsing. * listeners: added :ref:`continue_on_listener_filters_timeout ` to configure whether a listener will still create a connection when listener filters time out. * listeners: added :ref:`HTTP inspector listener filter `. * listeners: added :ref:`connection balancer ` diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index a3822558a1ba..4475f88c7bf4 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -31,6 +31,7 @@ constexpr const char* runtime_features[] = { "envoy.reloadable_features.outlier_detection_support_for_grpc_status", "envoy.reloadable_features.strict_header_validation", "envoy.reloadable_features.strict_authority_validation", + "envoy.reloadable_features.fix_wildcard_matching", }; // This is a list of configuration fields which are disallowed by default in Envoy diff --git a/source/extensions/transport_sockets/tls/BUILD b/source/extensions/transport_sockets/tls/BUILD index cdc81bb51608..9915c9cbc33e 100644 --- a/source/extensions/transport_sockets/tls/BUILD +++ b/source/extensions/transport_sockets/tls/BUILD @@ -101,6 +101,7 @@ envoy_cc_library( "//source/common/common:utility_lib", "//source/common/network:address_lib", "//source/common/protobuf:utility_lib", + "//source/common/runtime:runtime_lib", "//source/common/stats:symbol_table_lib", "//source/extensions/transport_sockets/tls/private_key:private_key_manager_lib", "@envoy_api//envoy/admin/v2alpha:pkg_cc_proto", diff --git a/source/extensions/transport_sockets/tls/context_impl.cc b/source/extensions/transport_sockets/tls/context_impl.cc index ed8e54c08449..dcbe45d9bf15 100644 --- a/source/extensions/transport_sockets/tls/context_impl.cc +++ b/source/extensions/transport_sockets/tls/context_impl.cc @@ -17,6 +17,7 @@ #include "common/common/utility.h" #include "common/network/address_impl.h" #include "common/protobuf/utility.h" +#include "common/runtime/runtime_impl.h" #include "extensions/transport_sockets/tls/utility.h" @@ -635,8 +636,13 @@ bool ContextImpl::dnsNameMatch(const std::string& dns_name, const char* pattern) size_t pattern_len = strlen(pattern); if (pattern_len > 1 && pattern[0] == '*' && pattern[1] == '.') { if (dns_name.length() > pattern_len - 1) { - size_t off = dns_name.length() - pattern_len + 1; - return dns_name.compare(off, pattern_len - 1, pattern + 1) == 0; + const size_t off = dns_name.length() - pattern_len + 1; + if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.fix_wildcard_matching")) { + return dns_name.substr(0, off).find('.') == std::string::npos && + dns_name.compare(off, pattern_len - 1, pattern + 1) == 0; + } else { + return dns_name.compare(off, pattern_len - 1, pattern + 1) == 0; + } } } diff --git a/test/extensions/transport_sockets/tls/BUILD b/test/extensions/transport_sockets/tls/BUILD index 94f93e62a6dd..57a60e46880d 100644 --- a/test/extensions/transport_sockets/tls/BUILD +++ b/test/extensions/transport_sockets/tls/BUILD @@ -82,6 +82,7 @@ envoy_cc_test( "//test/mocks/ssl:ssl_mocks", "//test/test_common:environment_lib", "//test/test_common:simulated_time_system_lib", + "//test/test_common:test_runtime_lib", ], ) diff --git a/test/extensions/transport_sockets/tls/context_impl_test.cc b/test/extensions/transport_sockets/tls/context_impl_test.cc index 3b6b596b5f1b..a219973f70d7 100644 --- a/test/extensions/transport_sockets/tls/context_impl_test.cc +++ b/test/extensions/transport_sockets/tls/context_impl_test.cc @@ -20,6 +20,7 @@ #include "test/mocks/ssl/mocks.h" #include "test/test_common/environment.h" #include "test/test_common/simulated_time_system.h" +#include "test/test_common/test_runtime.h" #include "test/test_common/utility.h" #include "gtest/gtest.h" @@ -44,6 +45,23 @@ class SslContextImplTest : public SslCertsTest { TEST_F(SslContextImplTest, TestDnsNameMatching) { EXPECT_TRUE(ContextImpl::dnsNameMatch("lyft.com", "lyft.com")); EXPECT_TRUE(ContextImpl::dnsNameMatch("a.lyft.com", "*.lyft.com")); + EXPECT_FALSE(ContextImpl::dnsNameMatch("a.b.lyft.com", "*.lyft.com")); + EXPECT_FALSE(ContextImpl::dnsNameMatch("foo.test.com", "*.lyft.com")); + EXPECT_FALSE(ContextImpl::dnsNameMatch("lyft.com", "*.lyft.com")); + EXPECT_FALSE(ContextImpl::dnsNameMatch("alyft.com", "*.lyft.com")); + EXPECT_FALSE(ContextImpl::dnsNameMatch("alyft.com", "*lyft.com")); + EXPECT_FALSE(ContextImpl::dnsNameMatch("lyft.com", "*lyft.com")); + EXPECT_FALSE(ContextImpl::dnsNameMatch("", "*lyft.com")); + EXPECT_FALSE(ContextImpl::dnsNameMatch("lyft.com", "")); +} + +TEST_F(SslContextImplTest, TestDnsNameMatchingLegacy) { + TestScopedRuntime scoped_runtime; + Runtime::LoaderSingleton::getExisting()->mergeValues( + {{"envoy.reloadable_features.fix_wildcard_matching", "false"}}); + EXPECT_TRUE(ContextImpl::dnsNameMatch("lyft.com", "lyft.com")); + EXPECT_TRUE(ContextImpl::dnsNameMatch("a.lyft.com", "*.lyft.com")); + // Legacy behavior EXPECT_TRUE(ContextImpl::dnsNameMatch("a.b.lyft.com", "*.lyft.com")); EXPECT_FALSE(ContextImpl::dnsNameMatch("foo.test.com", "*.lyft.com")); EXPECT_FALSE(ContextImpl::dnsNameMatch("lyft.com", "*.lyft.com")); @@ -70,6 +88,25 @@ TEST_F(SslContextImplTest, TestVerifySubjectAltNameURIMatched) { EXPECT_TRUE(ContextImpl::verifySubjectAltName(cert.get(), verify_subject_alt_name_list)); } +TEST_F(SslContextImplTest, TestVerifySubjectAltMultiDomain) { + bssl::UniquePtr cert = readCertFromFile(TestEnvironment::substitute( + "{{ test_rundir " + "}}/test/extensions/transport_sockets/tls/test_data/san_multiple_dns_cert.pem")); + std::vector verify_subject_alt_name_list = {"https://a.www.example.com"}; + EXPECT_FALSE(ContextImpl::verifySubjectAltName(cert.get(), verify_subject_alt_name_list)); +} + +TEST_F(SslContextImplTest, TestVerifySubjectAltMultiDomainLegacy) { + TestScopedRuntime scoped_runtime; + Runtime::LoaderSingleton::getExisting()->mergeValues( + {{"envoy.reloadable_features.fix_wildcard_matching", "false"}}); + bssl::UniquePtr cert = readCertFromFile(TestEnvironment::substitute( + "{{ test_rundir " + "}}/test/extensions/transport_sockets/tls/test_data/san_multiple_dns_cert.pem")); + std::vector verify_subject_alt_name_list = {"https://a.www.example.com"}; + EXPECT_TRUE(ContextImpl::verifySubjectAltName(cert.get(), verify_subject_alt_name_list)); +} + TEST_F(SslContextImplTest, TestVerifySubjectAltNameNotMatched) { bssl::UniquePtr cert = readCertFromFile(TestEnvironment::substitute( "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/san_dns_cert.pem"));