diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index 20ee702b5b7d..6ea617f7d142 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -5,6 +5,7 @@ Version history ================ * rbac: added :ref:`url_path ` for matching URL path without the query and fragment string. ========================== +* 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. 1.12.2 (December 10, 2019) diff --git a/source/extensions/filters/listener/tls_inspector/tls_inspector.cc b/source/extensions/filters/listener/tls_inspector/tls_inspector.cc index d38ca293f81c..62fd8aa801cf 100644 --- a/source/extensions/filters/listener/tls_inspector/tls_inspector.cc +++ b/source/extensions/filters/listener/tls_inspector/tls_inspector.cc @@ -23,6 +23,10 @@ namespace Extensions { namespace ListenerFilters { namespace TlsInspector { +// Min/max TLS version recognized by the underlying TLS/SSL library. +const unsigned Config::TLS_MIN_SUPPORTED_VERSION = TLS1_VERSION; +const unsigned Config::TLS_MAX_SUPPORTED_VERSION = TLS1_3_VERSION; + Config::Config(Stats::Scope& scope, uint32_t max_client_hello_size) : stats_{ALL_TLS_INSPECTOR_STATS(POOL_COUNTER_PREFIX(scope, "tls_inspector."))}, ssl_ctx_(SSL_CTX_new(TLS_with_buffers_method())), @@ -33,6 +37,8 @@ Config::Config(Stats::Scope& scope, uint32_t max_client_hello_size) max_client_hello_size_, size_t(TLS_MAX_CLIENT_HELLO))); } + SSL_CTX_set_min_proto_version(ssl_ctx_.get(), TLS_MIN_SUPPORTED_VERSION); + SSL_CTX_set_max_proto_version(ssl_ctx_.get(), TLS_MAX_SUPPORTED_VERSION); SSL_CTX_set_options(ssl_ctx_.get(), SSL_OP_NO_TICKET); SSL_CTX_set_session_cache_mode(ssl_ctx_.get(), SSL_SESS_CACHE_OFF); SSL_CTX_set_select_certificate_cb( diff --git a/source/extensions/filters/listener/tls_inspector/tls_inspector.h b/source/extensions/filters/listener/tls_inspector/tls_inspector.h index ee353ce9ef08..be232ce7928e 100644 --- a/source/extensions/filters/listener/tls_inspector/tls_inspector.h +++ b/source/extensions/filters/listener/tls_inspector/tls_inspector.h @@ -56,6 +56,8 @@ class Config { uint32_t maxClientHelloSize() const { return max_client_hello_size_; } static constexpr size_t TLS_MAX_CLIENT_HELLO = 64 * 1024; + static const unsigned TLS_MIN_SUPPORTED_VERSION; + static const unsigned TLS_MAX_SUPPORTED_VERSION; private: TlsInspectorStats stats_; diff --git a/test/extensions/filters/listener/tls_inspector/tls_inspector_benchmark.cc b/test/extensions/filters/listener/tls_inspector/tls_inspector_benchmark.cc index 5f8c1431ce6f..b580e6a80ee4 100644 --- a/test/extensions/filters/listener/tls_inspector/tls_inspector_benchmark.cc +++ b/test/extensions/filters/listener/tls_inspector/tls_inspector_benchmark.cc @@ -66,8 +66,9 @@ class FastMockOsSysCalls : public Api::MockOsSysCalls { }; static void BM_TlsInspector(benchmark::State& state) { - NiceMock os_sys_calls( - Tls::Test::generateClientHello("example.com", "\x02h2\x08http/1.1")); + NiceMock os_sys_calls(Tls::Test::generateClientHello( + Config::TLS_MIN_SUPPORTED_VERSION, Config::TLS_MAX_SUPPORTED_VERSION, "example.com", + "\x02h2\x08http/1.1")); TestThreadsafeSingletonInjector os_calls{&os_sys_calls}; NiceMock store; ConfigSharedPtr cfg(std::make_shared(store)); diff --git a/test/extensions/filters/listener/tls_inspector/tls_inspector_test.cc b/test/extensions/filters/listener/tls_inspector/tls_inspector_test.cc index c0416e52823e..3247ede588b8 100644 --- a/test/extensions/filters/listener/tls_inspector/tls_inspector_test.cc +++ b/test/extensions/filters/listener/tls_inspector/tls_inspector_test.cc @@ -27,7 +27,7 @@ namespace ListenerFilters { namespace TlsInspector { namespace { -class TlsInspectorTest : public testing::Test { +class TlsInspectorTest : public testing::TestWithParam> { public: TlsInspectorTest() : cfg_(std::make_shared(store_)), @@ -68,14 +68,22 @@ class TlsInspectorTest : public testing::Test { Network::IoHandlePtr io_handle_; }; +INSTANTIATE_TEST_SUITE_P(TlsProtocolVersions, TlsInspectorTest, + testing::Values(std::make_tuple(Config::TLS_MIN_SUPPORTED_VERSION, + Config::TLS_MAX_SUPPORTED_VERSION), + std::make_tuple(TLS1_VERSION, TLS1_VERSION), + std::make_tuple(TLS1_1_VERSION, TLS1_1_VERSION), + std::make_tuple(TLS1_2_VERSION, TLS1_2_VERSION), + std::make_tuple(TLS1_3_VERSION, TLS1_3_VERSION))); + // Test that an exception is thrown for an invalid value for max_client_hello_size -TEST_F(TlsInspectorTest, MaxClientHelloSize) { +TEST_P(TlsInspectorTest, MaxClientHelloSize) { EXPECT_THROW_WITH_MESSAGE(Config(store_, Config::TLS_MAX_CLIENT_HELLO + 1), EnvoyException, "max_client_hello_size of 65537 is greater than maximum of 65536."); } // Test that the filter detects Closed events and terminates. -TEST_F(TlsInspectorTest, ConnectionClosed) { +TEST_P(TlsInspectorTest, ConnectionClosed) { init(); EXPECT_CALL(cb_, continueFilterChain(false)); file_event_callback_(Event::FileReadyType::Closed); @@ -83,7 +91,7 @@ TEST_F(TlsInspectorTest, ConnectionClosed) { } // Test that the filter detects detects read errors. -TEST_F(TlsInspectorTest, ReadError) { +TEST_P(TlsInspectorTest, ReadError) { init(); EXPECT_CALL(os_sys_calls_, recv(42, _, _, MSG_PEEK)).WillOnce(InvokeWithoutArgs([]() { return Api::SysCallSizeResult{ssize_t(-1), ENOTSUP}; @@ -94,10 +102,11 @@ TEST_F(TlsInspectorTest, ReadError) { } // Test that a ClientHello with an SNI value causes the correct name notification. -TEST_F(TlsInspectorTest, SniRegistered) { +TEST_P(TlsInspectorTest, SniRegistered) { init(); const std::string servername("example.com"); - std::vector client_hello = Tls::Test::generateClientHello(servername, ""); + std::vector client_hello = Tls::Test::generateClientHello( + std::get<0>(GetParam()), std::get<1>(GetParam()), servername, ""); EXPECT_CALL(os_sys_calls_, recv(42, _, _, MSG_PEEK)) .WillOnce( Invoke([&client_hello](int, void* buffer, size_t length, int) -> Api::SysCallSizeResult { @@ -116,11 +125,12 @@ TEST_F(TlsInspectorTest, SniRegistered) { } // Test that a ClientHello with an ALPN value causes the correct name notification. -TEST_F(TlsInspectorTest, AlpnRegistered) { +TEST_P(TlsInspectorTest, AlpnRegistered) { init(); const std::vector alpn_protos = {absl::string_view("h2"), absl::string_view("http/1.1")}; - std::vector client_hello = Tls::Test::generateClientHello("", "\x02h2\x08http/1.1"); + std::vector client_hello = Tls::Test::generateClientHello( + std::get<0>(GetParam()), std::get<1>(GetParam()), "", "\x02h2\x08http/1.1"); EXPECT_CALL(os_sys_calls_, recv(42, _, _, MSG_PEEK)) .WillOnce( Invoke([&client_hello](int, void* buffer, size_t length, int) -> Api::SysCallSizeResult { @@ -139,11 +149,12 @@ TEST_F(TlsInspectorTest, AlpnRegistered) { } // Test with the ClientHello spread over multiple socket reads. -TEST_F(TlsInspectorTest, MultipleReads) { +TEST_P(TlsInspectorTest, MultipleReads) { init(); const std::vector alpn_protos = {absl::string_view("h2")}; const std::string servername("example.com"); - std::vector client_hello = Tls::Test::generateClientHello(servername, "\x02h2"); + std::vector client_hello = Tls::Test::generateClientHello( + std::get<0>(GetParam()), std::get<1>(GetParam()), servername, "\x02h2"); { InSequence s; EXPECT_CALL(os_sys_calls_, recv(42, _, _, MSG_PEEK)) @@ -177,9 +188,10 @@ TEST_F(TlsInspectorTest, MultipleReads) { } // Test that the filter correctly handles a ClientHello with no extensions present. -TEST_F(TlsInspectorTest, NoExtensions) { +TEST_P(TlsInspectorTest, NoExtensions) { init(); - std::vector client_hello = Tls::Test::generateClientHello("", ""); + std::vector client_hello = + Tls::Test::generateClientHello(std::get<0>(GetParam()), std::get<1>(GetParam()), "", ""); EXPECT_CALL(os_sys_calls_, recv(42, _, _, MSG_PEEK)) .WillOnce( Invoke([&client_hello](int, void* buffer, size_t length, int) -> Api::SysCallSizeResult { @@ -199,10 +211,11 @@ TEST_F(TlsInspectorTest, NoExtensions) { // Test that the filter fails if the ClientHello is larger than the // maximum allowed size. -TEST_F(TlsInspectorTest, ClientHelloTooBig) { +TEST_P(TlsInspectorTest, ClientHelloTooBig) { const size_t max_size = 50; cfg_ = std::make_shared(store_, max_size); - std::vector client_hello = Tls::Test::generateClientHello("example.com", ""); + std::vector client_hello = Tls::Test::generateClientHello( + std::get<0>(GetParam()), std::get<1>(GetParam()), "example.com", ""); ASSERT(client_hello.size() > max_size); init(); EXPECT_CALL(os_sys_calls_, recv(42, _, _, MSG_PEEK)) @@ -218,7 +231,7 @@ TEST_F(TlsInspectorTest, ClientHelloTooBig) { } // Test that the filter fails on non-SSL data -TEST_F(TlsInspectorTest, NotSsl) { +TEST_P(TlsInspectorTest, NotSsl) { init(); std::vector data; @@ -236,7 +249,7 @@ TEST_F(TlsInspectorTest, NotSsl) { EXPECT_EQ(1, cfg_->stats().tls_not_found_.value()); } -TEST_F(TlsInspectorTest, InlineReadSucceed) { +TEST_P(TlsInspectorTest, InlineReadSucceed) { filter_ = std::make_unique(cfg_); EXPECT_CALL(cb_, socket()).WillRepeatedly(ReturnRef(socket_)); @@ -244,7 +257,8 @@ TEST_F(TlsInspectorTest, InlineReadSucceed) { EXPECT_CALL(socket_, ioHandle()).WillRepeatedly(ReturnRef(*io_handle_)); const std::vector alpn_protos = {absl::string_view("h2")}; const std::string servername("example.com"); - std::vector client_hello = Tls::Test::generateClientHello(servername, "\x02h2"); + std::vector client_hello = Tls::Test::generateClientHello( + std::get<0>(GetParam()), std::get<1>(GetParam()), servername, "\x02h2"); EXPECT_CALL(os_sys_calls_, recv(42, _, _, MSG_PEEK)) .WillOnce(Invoke( diff --git a/test/extensions/filters/listener/tls_inspector/tls_utility.cc b/test/extensions/filters/listener/tls_inspector/tls_utility.cc index f41691dba736..70ed11b1f48c 100644 --- a/test/extensions/filters/listener/tls_inspector/tls_utility.cc +++ b/test/extensions/filters/listener/tls_inspector/tls_utility.cc @@ -8,11 +8,12 @@ namespace Envoy { namespace Tls { namespace Test { -std::vector generateClientHello(const std::string& sni_name, const std::string& alpn) { +std::vector generateClientHello(uint16_t tls_min_version, uint16_t tls_max_version, + const std::string& sni_name, const std::string& alpn) { bssl::UniquePtr ctx(SSL_CTX_new(TLS_with_buffers_method())); - const long flags = SSL_OP_NO_SSLv2 | SSL_OP_NO_SSLv3 | SSL_OP_NO_COMPRESSION; - SSL_CTX_set_options(ctx.get(), flags); + SSL_CTX_set_min_proto_version(ctx.get(), tls_min_version); + SSL_CTX_set_max_proto_version(ctx.get(), tls_max_version); bssl::UniquePtr ssl(SSL_new(ctx.get())); diff --git a/test/extensions/filters/listener/tls_inspector/tls_utility.h b/test/extensions/filters/listener/tls_inspector/tls_utility.h index 51cbe7e8c400..13e911e9e3c5 100644 --- a/test/extensions/filters/listener/tls_inspector/tls_utility.h +++ b/test/extensions/filters/listener/tls_inspector/tls_utility.h @@ -9,12 +9,15 @@ namespace Test { /** * Generate a TLS ClientHello in wire-format. + * @param tls_min_version Minimum supported TLS version to advertise. + * @param tls_max_version Maximum supported TLS version to advertise. * @param sni_name The name to include as a Server Name Indication. * No SNI extension is added if sni_name is empty. * @param alpn Protocol(s) list in the wire-format (i.e. 8-bit length-prefixed string) to advertise * in Application-Layer Protocol Negotiation. No ALPN is advertised if alpn is empty. */ -std::vector generateClientHello(const std::string& sni_name, const std::string& alpn); +std::vector generateClientHello(uint16_t tls_min_version, uint16_t tls_max_version, + const std::string& sni_name, const std::string& alpn); } // namespace Test } // namespace Tls