Repository: thrift Updated Branches: refs/heads/master 00d425239 -> 7f5a8c28b
THRIFT-4164: update openssl cleanup to match current requirements and document TSSLSocketFactory lifetime requirements Client: cpp This closes #1235 Project: http://git-wip-us.apache.org/repos/asf/thrift/repo Commit: http://git-wip-us.apache.org/repos/asf/thrift/commit/7f5a8c28 Tree: http://git-wip-us.apache.org/repos/asf/thrift/tree/7f5a8c28 Diff: http://git-wip-us.apache.org/repos/asf/thrift/diff/7f5a8c28 Branch: refs/heads/master Commit: 7f5a8c28bc58011abef0cede10915c2071efbe41 Parents: 00d4252 Author: James E. King, III <[email protected]> Authored: Tue Apr 4 09:36:38 2017 -0400 Committer: James E. King, III <[email protected]> Committed: Tue Apr 4 09:36:38 2017 -0400 ---------------------------------------------------------------------- lib/cpp/README.md | 10 ++++++ lib/cpp/src/thrift/transport/TSSLSocket.cpp | 40 ++++++++++++++---------- lib/cpp/src/thrift/transport/TSSLSocket.h | 24 +++++++++++--- test/cpp/src/TestClient.cpp | 6 ++-- 4 files changed, 56 insertions(+), 24 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/thrift/blob/7f5a8c28/lib/cpp/README.md ---------------------------------------------------------------------- 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 @@ 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) + http://git-wip-us.apache.org/repos/asf/thrift/blob/7f5a8c28/lib/cpp/src/thrift/transport/TSSLSocket.cpp ---------------------------------------------------------------------- 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 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(); } http://git-wip-us.apache.org/repos/asf/thrift/blob/7f5a8c28/lib/cpp/src/thrift/transport/TSSLSocket.h ---------------------------------------------------------------------- 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 @@ 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: http://git-wip-us.apache.org/repos/asf/thrift/blob/7f5a8c28/test/cpp/src/TestClient.cpp ---------------------------------------------------------------------- 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 @@ 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;
