THRIFT-4164: update openssl cleanup to match current requirements and document TSSLSocketFactory lifetime requirements
Client: cpp
This closes #1235
diff --git a/lib/cpp/README.md b/lib/cpp/README.md
index 05aef95..a7f7e79 100755
--- a/lib/cpp/README.md
+++ b/lib/cpp/README.md
@@ -279,3 +279,13 @@
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)
+
diff --git a/lib/cpp/src/thrift/transport/TSSLSocket.cpp b/lib/cpp/src/thrift/transport/TSSLSocket.cpp
index b0f9166..926a58f 100644
--- a/lib/cpp/src/thrift/transport/TSSLSocket.cpp
+++ b/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 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 @@
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 @@
#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 @@
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();
}
diff --git a/lib/cpp/src/thrift/transport/TSSLSocket.h b/lib/cpp/src/thrift/transport/TSSLSocket.h
index 03157d7..0462a20 100644
--- a/lib/cpp/src/thrift/transport/TSSLSocket.h
+++ b/lib/cpp/src/thrift/transport/TSSLSocket.h
@@ -126,11 +126,11 @@
*/
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 @@
/**
* 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:
diff --git a/test/cpp/src/TestClient.cpp b/test/cpp/src/TestClient.cpp
index a918bfb..6ecf540 100644
--- a/test/cpp/src/TestClient.cpp
+++ b/test/cpp/src/TestClient.cpp
@@ -228,12 +228,12 @@
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;