tls_inspector: enable TLSv1.3. (#119)

Previously, TLS inspector didn't support TLSv1.3 and clients configured
to use only TLSv1.3 were not recognized as TLS clients.

Because TLS extensions (SNI, ALPN) were not inspected, those connections
might have been matched to a wrong filter chain, possibly bypassing some
security restrictions in the process.

Fixes istio/istio#18695.

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

docs/root/intro/version_history.rst

@@ -5,6 +5,7 @@ Version history
 ================
 * rbac: added :ref:`url_path <envoy_api_field_config.rbac.v2.Permission.url_path>` for matching URL path without the query and fragment string.
 ==========================
+* listeners: fixed issue where :ref:`TLS inspector listener filter <config_listener_filters_tls_inspector>` 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)

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(

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_;

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<FastMockOsSysCalls> os_sys_calls(
-      Tls::Test::generateClientHello("example.com", "\x02h2\x08http/1.1"));
+  NiceMock<FastMockOsSysCalls> os_sys_calls(Tls::Test::generateClientHello(
+      Config::TLS_MIN_SUPPORTED_VERSION, Config::TLS_MAX_SUPPORTED_VERSION, "example.com",
+      "\x02h2\x08http/1.1"));
   TestThreadsafeSingletonInjector<Api::OsSysCallsImpl> os_calls{&os_sys_calls};
   NiceMock<Stats::MockStore> store;
   ConfigSharedPtr cfg(std::make_shared<Config>(store));

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<std::tuple<uint16_t, uint16_t>> {
 public:
   TlsInspectorTest()
       : cfg_(std::make_shared<Config>(store_)),
@@ -68,22 +68,30 @@ 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);
   EXPECT_EQ(1, cfg_->stats().connection_closed_.value());
 }
 
 // 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<uint8_t> client_hello = Tls::Test::generateClientHello(servername, "");
+  std::vector<uint8_t> 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<absl::string_view> alpn_protos = {absl::string_view("h2"),
                                                       absl::string_view("http/1.1")};
-  std::vector<uint8_t> client_hello = Tls::Test::generateClientHello("", "\x02h2\x08http/1.1");
+  std::vector<uint8_t> 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<absl::string_view> alpn_protos = {absl::string_view("h2")};
   const std::string servername("example.com");
-  std::vector<uint8_t> client_hello = Tls::Test::generateClientHello(servername, "\x02h2");
+  std::vector<uint8_t> 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<uint8_t> client_hello = Tls::Test::generateClientHello("", "");
+  std::vector<uint8_t> 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<Config>(store_, max_size);
-  std::vector<uint8_t> client_hello = Tls::Test::generateClientHello("example.com", "");
+  std::vector<uint8_t> 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<uint8_t> data;
 
@@ -236,15 +249,16 @@ 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<Filter>(cfg_);
 
   EXPECT_CALL(cb_, socket()).WillRepeatedly(ReturnRef(socket_));
   EXPECT_CALL(cb_, dispatcher()).WillRepeatedly(ReturnRef(dispatcher_));
   EXPECT_CALL(socket_, ioHandle()).WillRepeatedly(ReturnRef(*io_handle_));
   const std::vector<absl::string_view> alpn_protos = {absl::string_view("h2")};
   const std::string servername("example.com");
-  std::vector<uint8_t> client_hello = Tls::Test::generateClientHello(servername, "\x02h2");
+  std::vector<uint8_t> 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(

test/extensions/filters/listener/tls_inspector/tls_utility.cc

@@ -8,11 +8,12 @@ namespace Envoy {
 namespace Tls {
 namespace Test {
 
-std::vector<uint8_t> generateClientHello(const std::string& sni_name, const std::string& alpn) {
+std::vector<uint8_t> generateClientHello(uint16_t tls_min_version, uint16_t tls_max_version,
+                                         const std::string& sni_name, const std::string& alpn) {
   bssl::UniquePtr<SSL_CTX> 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(SSL_new(ctx.get()));
 

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<uint8_t> generateClientHello(const std::string& sni_name, const std::string& alpn);
+std::vector<uint8_t> generateClientHello(uint16_t tls_min_version, uint16_t tls_max_version,
+                                         const std::string& sni_name, const std::string& alpn);
 
 } // namespace Test
 } // namespace Tls