From 539e912ef8db279e3fbc3b0e6c6951a204069935 Mon Sep 17 00:00:00 2001 From: Matthew Stevenson <52979934+matthewstevenson88@users.noreply.github.com> Date: Wed, 4 Oct 2023 12:45:09 -0700 Subject: [PATCH] [ssl] Do not crash if ssl session cache capacity is zero. (#34539) This behavior is dangerous because we will crash when the cache is created, which is not necessarily on application startup and is likely when you first try to establish an SSL connection. Instead, we log an error. If the SSL library attempts to put a session ticket in the cache it will fail to do so, but everything else will continue as normal. In particular, we will always seamlessly fall back to a full SSL handshake. Along the way, we also ensure that you cannot put a null `SSL_SESSION` into the cache, which would lead to a segfault when it is fetched from the cache. --- .../ssl/session_cache/ssl_session_cache.cc | 10 +++- test/core/tsi/ssl_session_cache_test.cc | 40 ++++++++++++++++ test/cpp/end2end/ssl_credentials_test.cc | 48 +++++++++++++++++++ 3 files changed, 97 insertions(+), 1 deletion(-) diff --git a/src/core/tsi/ssl/session_cache/ssl_session_cache.cc b/src/core/tsi/ssl/session_cache/ssl_session_cache.cc index 5427c989b2f..d4bafaa0538 100644 --- a/src/core/tsi/ssl/session_cache/ssl_session_cache.cc +++ b/src/core/tsi/ssl/session_cache/ssl_session_cache.cc @@ -62,7 +62,11 @@ class SslSessionLRUCache::Node { }; SslSessionLRUCache::SslSessionLRUCache(size_t capacity) : capacity_(capacity) { - GPR_ASSERT(capacity > 0); + if (capacity == 0) { + gpr_log( + GPR_ERROR, + "SslSessionLRUCache capacity is zero. SSL sessions cannot be resumed."); + } } SslSessionLRUCache::~SslSessionLRUCache() { @@ -94,6 +98,10 @@ SslSessionLRUCache::Node* SslSessionLRUCache::FindLocked( } void SslSessionLRUCache::Put(const char* key, SslSessionPtr session) { + if (session == nullptr) { + gpr_log(GPR_ERROR, "Attempted to put null SSL session in session cache."); + return; + } grpc_core::MutexLock lock(&lock_); Node* node = FindLocked(key); if (node != nullptr) { diff --git a/test/core/tsi/ssl_session_cache_test.cc b/test/core/tsi/ssl_session_cache_test.cc index 9a7da12a9f3..5890f15bc63 100644 --- a/test/core/tsi/ssl_session_cache_test.cc +++ b/test/core/tsi/ssl_session_cache_test.cc @@ -103,6 +103,7 @@ TEST(SslSessionCacheTest, LruCache) { { RefCountedPtr cache = tsi::SslSessionLRUCache::Create(3); + EXPECT_EQ(cache->Size(), 0); tsi::SslSessionPtr sess2 = tracker.NewSession(2); SSL_SESSION* sess2_ptr = sess2.get(); cache->Put("first.dropbox.com", std::move(sess2)); @@ -143,6 +144,45 @@ TEST(SslSessionCacheTest, LruCache) { EXPECT_EQ(tracker.AliveCount(), 0); } +TEST(SslSessionCacheTest, PutAndGet) { + // Set up an empty cache and an SSL session. + SSL_CTX* ssl_ctx = SSL_CTX_new(TLS_method()); + tsi::SslSessionPtr ssl_session_ptr(SSL_SESSION_new(ssl_ctx)); + RefCountedPtr cache = + tsi::SslSessionLRUCache::Create(1); + EXPECT_EQ(cache->Size(), 0); + // Put the SSL session in the cache. + cache->Put("foo.domain", std::move(ssl_session_ptr)); + EXPECT_EQ(cache->Size(), 1); + // Get a copy of the SSL session from the cache. + EXPECT_EQ(cache->Size(), 1); + EXPECT_NE(cache->Get("foo.domain"), nullptr); + // Try to put a null SSL session in the cache and check that it was not + // successful. + cache->Put("foo.domain.2", /*session=*/nullptr); + EXPECT_EQ(cache->Size(), 1); + EXPECT_NE(cache->Get("foo.domain"), nullptr); + EXPECT_EQ(cache->Get("foo.domain.2"), nullptr); + // Cleanup. + SSL_CTX_free(ssl_ctx); +} + +TEST(SslSessionCacheTest, CapacityZeroCache) { + // Set up an empty cache and an SSL session. + SSL_CTX* ssl_ctx = SSL_CTX_new(TLS_method()); + tsi::SslSessionPtr ssl_session_ptr(SSL_SESSION_new(ssl_ctx)); + RefCountedPtr cache = + tsi::SslSessionLRUCache::Create(0); + EXPECT_EQ(cache->Size(), 0); + // Try to put the SSL session in the cache and check that it was not + // successful. + cache->Put("foo.domain", std::move(ssl_session_ptr)); + EXPECT_EQ(cache->Size(), 0); + EXPECT_EQ(cache->Get("foo.domain"), nullptr); + // Cleanup. + SSL_CTX_free(ssl_ctx); +} + } // namespace } // namespace grpc_core diff --git a/test/cpp/end2end/ssl_credentials_test.cc b/test/cpp/end2end/ssl_credentials_test.cc index 91022228986..b1a51dafccd 100644 --- a/test/cpp/end2end/ssl_credentials_test.cc +++ b/test/cpp/end2end/ssl_credentials_test.cc @@ -125,6 +125,31 @@ void DoRpc(const std::string& server_addr, } } +TEST_F(SslCredentialsTest, SequentialResumption) { + server_addr_ = absl::StrCat("localhost:", + std::to_string(grpc_pick_unused_port_or_die())); + absl::Notification notification; + server_thread_ = new std::thread([&]() { RunServer(¬ification); }); + notification.WaitForNotification(); + + std::string root_cert = ReadFile(kCaCertPath); + std::string client_key = ReadFile(kClientKeyPath); + std::string client_cert = ReadFile(kClientCertPath); + grpc::SslCredentialsOptions ssl_options; + ssl_options.pem_root_certs = root_cert; + ssl_options.pem_private_key = client_key; + ssl_options.pem_cert_chain = client_cert; + + grpc_ssl_session_cache* cache = grpc_ssl_session_cache_create_lru(16); + + DoRpc(server_addr_, ssl_options, cache, /*expect_session_reuse=*/false); + for (int i = 0; i < 10; i++) { + DoRpc(server_addr_, ssl_options, cache, /*expect_session_reuse=*/true); + } + + grpc_ssl_session_cache_destroy(cache); +} + TEST_F(SslCredentialsTest, ConcurrentResumption) { server_addr_ = absl::StrCat("localhost:", std::to_string(grpc_pick_unused_port_or_die())); @@ -157,6 +182,29 @@ TEST_F(SslCredentialsTest, ConcurrentResumption) { grpc_ssl_session_cache_destroy(cache); } +TEST_F(SslCredentialsTest, ResumptionFailsDueToNoCapacityInCache) { + server_addr_ = absl::StrCat("localhost:", + std::to_string(grpc_pick_unused_port_or_die())); + absl::Notification notification; + server_thread_ = new std::thread([&]() { RunServer(¬ification); }); + notification.WaitForNotification(); + + std::string root_cert = ReadFile(kCaCertPath); + std::string client_key = ReadFile(kClientKeyPath); + std::string client_cert = ReadFile(kClientCertPath); + grpc::SslCredentialsOptions ssl_options; + ssl_options.pem_root_certs = root_cert; + ssl_options.pem_private_key = client_key; + ssl_options.pem_cert_chain = client_cert; + + grpc_ssl_session_cache* cache = grpc_ssl_session_cache_create_lru(0); + + DoRpc(server_addr_, ssl_options, cache, /*expect_session_reuse=*/false); + DoRpc(server_addr_, ssl_options, cache, /*expect_session_reuse=*/false); + + grpc_ssl_session_cache_destroy(cache); +} + } // namespace } // namespace testing } // namespace grpc