Skip to content

Commit

Permalink
tls_inspector: enable TLSv1.3. (envoyproxy#118) (envoyproxy#173)
Browse files Browse the repository at this point in the history
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.

Signed-off-by: Piotr Sikora <piotrsikora@google.com>
  • Loading branch information
PiotrSikora committed Mar 3, 2020
1 parent 6d0cf31 commit 1af8b4e
Show file tree
Hide file tree
Showing 7 changed files with 53 additions and 23 deletions.
3 changes: 3 additions & 0 deletions docs/root/intro/version_history.rst
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@ Version history
1.13.1 (Pending)
================
* rbac: added :ref:`url_path <envoy_api_field_config.rbac.v2.Permission.url_path>` for matching URL path without the query and fragment string.
* http: added HTTP/1.1 flood protection. Can be temporarily disabled using the runtime feature `envoy.reloadable_features.http1_flood_protection`.
* 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: fix the SDS vulnerability that TLS validation context (e.g., subject alt name or hash) cannot be effectively validated in some cases.

1.13.0 (January 20, 2020)
=========================
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,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())),
Expand All @@ -32,6 +36,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(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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_;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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_)),
Expand Down Expand Up @@ -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};
Expand All @@ -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 {
Expand All @@ -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 {
Expand All @@ -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))
Expand Down Expand Up @@ -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 {
Expand All @@ -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))
Expand All @@ -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;

Expand All @@ -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(
Expand Down
7 changes: 4 additions & 3 deletions test/extensions/filters/listener/tls_inspector/tls_utility.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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()));

Expand Down
5 changes: 4 additions & 1 deletion test/extensions/filters/listener/tls_inspector/tls_utility.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit 1af8b4e

Please sign in to comment.