ssl: serialize accesses to SSL socket factory contexts (#4345)

akonradi · dnoe · commit f936fc60f488 · 2018-09-05T15:54:10.000-04:00
Description:
The ssl_ctx_ fields of the ServerSslSocketFactory and ClientSslSocketFactory are accessed and mutated from different threads without external serialization. This is a bug since instances of std::shared_ptr are not thread-safe (though different instances pointing to the same object are). This patch fixes the bug by using a mutex to serialize accesses.

Risk Level: Low
Testing: ran test suite
Docs Changes: n/a
Release Notes: n/a
diff --git a/source/common/ssl/BUILD b/source/common/ssl/BUILD
@@ -12,7 +12,10 @@ envoy_cc_library(
     name = "ssl_socket_lib",
     srcs = ["ssl_socket.cc"],
     hdrs = ["ssl_socket.h"],
-    external_deps = ["ssl"],
+    external_deps = [
+        "abseil_synchronization",
+        "ssl",
+    ],
     deps = [
         ":context_config_lib",
         ":context_lib",
@@ -23,6 +26,7 @@ envoy_cc_library(
         "//source/common/common:assert_lib",
         "//source/common/common:empty_string",
         "//source/common/common:minimal_logger_lib",
+        "//source/common/common:thread_annotations",
         "//source/common/http:headers_lib",
     ],
 )
diff --git a/source/common/ssl/ssl_socket.cc b/source/common/ssl/ssl_socket.cc
@@ -422,7 +422,11 @@ Network::TransportSocketPtr ClientSslSocketFactory::createTransportSocket() cons
   // onAddOrUpdateSecret() could be invoked in the middle of checking the existence of ssl_ctx and
   // creating SslSocket using ssl_ctx. Capture ssl_ctx_ into a local variable so that we check and
   // use the same ssl_ctx to create SslSocket.
-  auto ssl_ctx = ssl_ctx_;
+  ClientContextSharedPtr ssl_ctx;
+  {
+    absl::ReaderMutexLock l(&ssl_ctx_mu_);
+    ssl_ctx = ssl_ctx_;
+  }
   if (ssl_ctx) {
     return std::make_unique<Ssl::SslSocket>(std::move(ssl_ctx), Ssl::InitialState::Client);
   } else {
@@ -436,7 +440,10 @@ bool ClientSslSocketFactory::implementsSecureTransport() const { return true; }
 
 void ClientSslSocketFactory::onAddOrUpdateSecret() {
   ENVOY_LOG(debug, "Secret is updated.");
-  ssl_ctx_ = manager_.createSslClientContext(stats_scope_, *config_);
+  {
+    absl::WriterMutexLock l(&ssl_ctx_mu_);
+    ssl_ctx_ = manager_.createSslClientContext(stats_scope_, *config_);
+  }
   stats_.ssl_context_update_by_sds_.inc();
 }
 
@@ -454,7 +461,11 @@ Network::TransportSocketPtr ServerSslSocketFactory::createTransportSocket() cons
   // onAddOrUpdateSecret() could be invoked in the middle of checking the existence of ssl_ctx and
   // creating SslSocket using ssl_ctx. Capture ssl_ctx_ into a local variable so that we check and
   // use the same ssl_ctx to create SslSocket.
-  auto ssl_ctx = ssl_ctx_;
+  ServerContextSharedPtr ssl_ctx;
+  {
+    absl::ReaderMutexLock l(&ssl_ctx_mu_);
+    ssl_ctx = ssl_ctx_;
+  }
   if (ssl_ctx) {
     return std::make_unique<Ssl::SslSocket>(std::move(ssl_ctx), Ssl::InitialState::Server);
   } else {
@@ -468,7 +479,10 @@ bool ServerSslSocketFactory::implementsSecureTransport() const { return true; }
 
 void ServerSslSocketFactory::onAddOrUpdateSecret() {
   ENVOY_LOG(debug, "Secret is updated.");
-  ssl_ctx_ = manager_.createSslServerContext(stats_scope_, *config_, server_names_);
+  {
+    absl::WriterMutexLock l(&ssl_ctx_mu_);
+    ssl_ctx_ = manager_.createSslServerContext(stats_scope_, *config_, server_names_);
+  }
   stats_.ssl_context_update_by_sds_.inc();
 }
 
diff --git a/source/common/ssl/ssl_socket.h b/source/common/ssl/ssl_socket.h
@@ -12,6 +12,7 @@
 #include "common/common/logger.h"
 #include "common/ssl/context_impl.h"
 
+#include "absl/synchronization/mutex.h"
 #include "openssl/ssl.h"
 
 namespace Envoy {
@@ -101,7 +102,8 @@ class ClientSslSocketFactory : public Network::TransportSocketFactory,
   Stats::Scope& stats_scope_;
   SslSocketFactoryStats stats_;
   ClientContextConfigPtr config_;
-  ClientContextSharedPtr ssl_ctx_;
+  mutable absl::Mutex ssl_ctx_mu_;
+  ClientContextSharedPtr ssl_ctx_ GUARDED_BY(ssl_ctx_mu_);
 };
 
 class ServerSslSocketFactory : public Network::TransportSocketFactory,
@@ -123,7 +125,8 @@ class ServerSslSocketFactory : public Network::TransportSocketFactory,
   SslSocketFactoryStats stats_;
   ServerContextConfigPtr config_;
   const std::vector<std::string> server_names_;
-  ServerContextSharedPtr ssl_ctx_;
+  mutable absl::Mutex ssl_ctx_mu_;
+  ServerContextSharedPtr ssl_ctx_ GUARDED_BY(ssl_ctx_mu_);
 };
 
 } // namespace Ssl
