THRIFT-4164: update openssl cleanup to match current requirements and document TSSLSocketFactory lifetime requirements

Client: cpp

This closes #1235

lib/cpp/README.md

@@ -279,3 +279,13 @@ overridden if it's not strong enough for you.
 
 In the pthread mutex implementation, the contention profiling code was enabled
 by default in all builds.  This changed to be disabled by default.  (THRIFT-4151)
+
+In older releases, if a TSSLSocketFactory's lifetime was not at least as long
+as the TSSLSockets it created, we silently reverted openssl to unsafe multithread behavior
+and so the results were undefined.  Changes were made in 0.11.0 that cause either an
+assertion or a core instead of undefined behavior.  The lifetime of a TSSLSocketFactory
+*must* be longer than any TSSLSocket that it creates, otherwise openssl will be cleaned
+up too early.  If the static boolean is set to disable openssl initialization and
+cleanup and leave it up to the consuming application, this requirement is not needed.
+(THRIFT-4164)
+

lib/cpp/src/thrift/transport/TSSLSocket.cpp

@@ -35,8 +35,14 @@
 #include <fcntl.h>
 #endif
 
+#define OPENSSL_VERSION_NO_THREAD_ID_BEFORE    0x10000000L
+#define OPENSSL_ENGINE_CLEANUP_REQUIRED_BEFORE 0x10100000L
 
 #include <boost/shared_array.hpp>
+#include <openssl/opensslv.h>
+#if (OPENSSL_VERSION_NUMBER < OPENSSL_ENGINE_CLEANUP_REQUIRED_BEFORE)
+#include <openssl/engine.h>
+#endif
 #include <openssl/err.h>
 #include <openssl/rand.h>
 #include <openssl/ssl.h>
@@ -46,8 +52,6 @@
 #include <thrift/transport/PlatformSocket.h>
 #include <thrift/TToString.h>
 
-#define OPENSSL_VERSION_NO_THREAD_ID 0x10000000L
-
 using namespace std;
 using namespace apache::thrift::concurrency;
 
@@ -66,13 +70,16 @@ static boost::shared_array<Mutex> mutexes;
 
 static void callbackLocking(int mode, int n, const char*, int) {
   if (mode & CRYPTO_LOCK) {
+    // assertion of (px != 0) here typically means that a TSSLSocket's lifetime
+    // exceeded the lifetime of the TSSLSocketFactory that created it, and the
+    // TSSLSocketFactory already ran cleanupOpenSSL(), which deleted "mutexes".
     mutexes[n].lock();
   } else {
     mutexes[n].unlock();
   }
 }
 
-#if (OPENSSL_VERSION_NUMBER < OPENSSL_VERSION_NO_THREAD_ID)
+#if (OPENSSL_VERSION_NUMBER < OPENSSL_VERSION_NO_THREAD_ID_BEFORE)
 static unsigned long callbackThreadID() {
 #ifdef _WIN32
   return (unsigned long)GetCurrentThreadId();
@@ -107,6 +114,8 @@ void initializeOpenSSL() {
   openSSLInitialized = true;
   SSL_library_init();
   SSL_load_error_strings();
+  ERR_load_crypto_strings();
+
   // static locking
   // newer versions of OpenSSL changed CRYPTO_num_locks - see THRIFT-3878
 #ifdef CRYPTO_num_locks
@@ -114,15 +123,13 @@ void initializeOpenSSL() {
 #else
   mutexes = boost::shared_array<Mutex>(new Mutex[ ::CRYPTO_num_locks()]);
 #endif
-  if (mutexes == NULL) {
-    throw TTransportException(TTransportException::INTERNAL_ERROR,
-                              "initializeOpenSSL() failed, "
-                              "out of memory while creating mutex array");
-  }
-#if (OPENSSL_VERSION_NUMBER < OPENSSL_VERSION_NO_THREAD_ID)
+
+#if (OPENSSL_VERSION_NUMBER < OPENSSL_VERSION_NO_THREAD_ID_BEFORE)
   CRYPTO_set_id_callback(callbackThreadID);
 #endif
+
   CRYPTO_set_locking_callback(callbackLocking);
+
   // dynamic locking
   CRYPTO_set_dynlock_create_callback(dyn_create);
   CRYPTO_set_dynlock_lock_callback(dyn_lock);
@@ -134,17 +141,18 @@ void cleanupOpenSSL() {
     return;
   }
   openSSLInitialized = false;
-#if (OPENSSL_VERSION_NUMBER < OPENSSL_VERSION_NO_THREAD_ID)
-  CRYPTO_set_id_callback(NULL);
+
+  // https://wiki.openssl.org/index.php/Library_Initialization#Cleanup
+  // we purposefully do NOT call FIPS_mode_set(0) and leave it up to the enclosing application to manage FIPS entirely
+#if (OPENSSL_VERSION_NUMBER < OPENSSL_ENGINE_CLEANUP_REQUIRED_BEFORE)
+  ENGINE_cleanup();             // https://www.openssl.org/docs/man1.1.0/crypto/ENGINE_cleanup.html - cleanup call is needed before 1.1.0
 #endif
-  CRYPTO_set_locking_callback(NULL);
-  CRYPTO_set_dynlock_create_callback(NULL);
-  CRYPTO_set_dynlock_lock_callback(NULL);
-  CRYPTO_set_dynlock_destroy_callback(NULL);
-  ERR_free_strings();
+  CONF_modules_unload(1);
   EVP_cleanup();
   CRYPTO_cleanup_all_ex_data();
   ERR_remove_state(0);
+  ERR_free_strings();
+
   mutexes.reset();
 }
 

lib/cpp/src/thrift/transport/TSSLSocket.h

@@ -126,11 +126,11 @@ protected:
    */
   TSSLSocket(boost::shared_ptr<SSLContext> ctx, std::string host, int port);
     /**
-	* Constructor with an interrupt signal.
-	*
-	* @param host  Remote host name
-	* @param port  Remote port number
-	*/
+  * Constructor with an interrupt signal.
+  *
+  * @param host  Remote host name
+  * @param port  Remote port number
+  */
     TSSLSocket(boost::shared_ptr<SSLContext> ctx, std::string host, int port, boost::shared_ptr<THRIFT_SOCKET> interruptListener);
   /**
    * Authorize peer access after SSL handshake completes.
@@ -159,6 +159,20 @@ protected:
 
 /**
  * SSL socket factory. SSL sockets should be created via SSL factory.
+ * The factory will automatically initialize and cleanup openssl as long as
+ * there is a TSSLSocketFactory instantiated, and as long as the static
+ * boolean manualOpenSSLInitialization_ is set to false, the default.
+ *
+ * If you would like to initialize and cleanup openssl yourself, set
+ * manualOpenSSLInitialization_ to true and TSSLSocketFactory will no
+ * longer be responsible for openssl initialization and teardown.
+ *
+ * It is the responsibility of the code using TSSLSocketFactory to
+ * ensure that the factory lifetime exceeds the lifetime of any sockets
+ * it might create.  If this is not guaranteed, a socket may call into
+ * openssl after the socket factory has cleaned up openssl!  This
+ * guarantee is unnecessary if manualOpenSSLInitialization_ is true,
+ * however, since it would be up to the consuming application instead.
  */
 class TSSLSocketFactory {
 public:

test/cpp/src/TestClient.cpp

@@ -228,12 +228,12 @@ int main(int argc, char** argv) {
     noinsane = true;
   }
 
+  // THRIFT-4164: The factory MUST outlive any sockets it creates for correct behavior!
+  boost::shared_ptr<TSSLSocketFactory> factory;
+  boost::shared_ptr<TSocket> socket;
   boost::shared_ptr<TTransport> transport;
   boost::shared_ptr<TProtocol> protocol;
 
-  boost::shared_ptr<TSocket> socket;
-  boost::shared_ptr<TSSLSocketFactory> factory;
-
   if (ssl) {
     cout << "Client Certificate File: " << certPath << endl;
     cout << "Client Key         File: " << keyPath << endl;