diff --git a/lib/cpp/README.md b/lib/cpp/README.md index 05aef9507..a7f7e792c 100755 --- a/lib/cpp/README.md +++ b/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) + diff --git a/lib/cpp/src/thrift/transport/TSSLSocket.cpp b/lib/cpp/src/thrift/transport/TSSLSocket.cpp index b0f9166f1..926a58f82 100644 --- a/lib/cpp/src/thrift/transport/TSSLSocket.cpp +++ b/lib/cpp/src/thrift/transport/TSSLSocket.cpp @@ -35,8 +35,14 @@ #include #endif +#define OPENSSL_VERSION_NO_THREAD_ID_BEFORE 0x10000000L +#define OPENSSL_ENGINE_CLEANUP_REQUIRED_BEFORE 0x10100000L #include +#include +#if (OPENSSL_VERSION_NUMBER < OPENSSL_ENGINE_CLEANUP_REQUIRED_BEFORE) +#include +#endif #include #include #include @@ -46,8 +52,6 @@ #include #include -#define OPENSSL_VERSION_NO_THREAD_ID 0x10000000L - using namespace std; using namespace apache::thrift::concurrency; @@ -66,13 +70,16 @@ static boost::shared_array 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(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(); } diff --git a/lib/cpp/src/thrift/transport/TSSLSocket.h b/lib/cpp/src/thrift/transport/TSSLSocket.h index 03157d7d1..0462a20be 100644 --- a/lib/cpp/src/thrift/transport/TSSLSocket.h +++ b/lib/cpp/src/thrift/transport/TSSLSocket.h @@ -126,11 +126,11 @@ protected: */ TSSLSocket(boost::shared_ptr 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 ctx, std::string host, int port, boost::shared_ptr 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: diff --git a/test/cpp/src/TestClient.cpp b/test/cpp/src/TestClient.cpp index a918bfba4..6ecf5404d 100644 --- a/test/cpp/src/TestClient.cpp +++ b/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 factory; + boost::shared_ptr socket; boost::shared_ptr transport; boost::shared_ptr protocol; - boost::shared_ptr socket; - boost::shared_ptr factory; - if (ssl) { cout << "Client Certificate File: " << certPath << endl; cout << "Client Key File: " << keyPath << endl;