From 2c0cefcd18161549e9e8b103f46c0f65fca84d99 Mon Sep 17 00:00:00 2001 From: Michael Paquier Date: Thu, 11 Mar 2021 17:14:25 +0900 Subject: [PATCH] Set libcrypto callbacks for all connection threads in libpq Based on an analysis of the OpenSSL code with Jacob, moving to EVP for the cryptohash computations makes necessary the setup of the libcrypto callbacks that were getting set only for SSL connections, but not for connections without SSL. Not setting the callbacks makes the use of threads potentially unsafe for connections calling cryptohashes during authentication, like MD5 or SCRAM, if a failure happens during a cryptohash computation. The logic setting the libssl and libcrypto states is then split into two parts, both using the same locking, with libcrypto being set up for SSL and non-SSL connections, while SSL connections set any libssl state afterwards as needed. Prior to this commit, only SSL connections would have set libcrypto callbacks that are necessary to ensure a proper thread locking when using multiple concurrent threads in libpq (ENABLE_THREAD_SAFETY). Note that this is only required for OpenSSL 1.0.2 and 1.0.1 (oldest version supported on HEAD), as 1.1.0 has its own internal locking and it has dropped support for CRYPTO_set_locking_callback(). Tests with up to 300 threads with OpenSSL 1.0.1 and 1.0.2, mixing SSL and non-SSL connection threads did not show any performance impact after some micro-benchmarking. pgbench can be used here with -C and a mostly-empty script (with one \set meta-command for example) to stress authentication requests, and we have mixed that with some custom programs for testing. Reported-by: Jacob Champion Author: Michael Paquier Reviewed-by: Jacob Champion Discussion: https://postgr.es/m/fd3ba610085f1ff54623478cf2f7adf5af193cbb.camel@vmware.com --- src/interfaces/libpq/fe-connect.c | 20 ++++- src/interfaces/libpq/fe-secure-openssl.c | 109 ++++++++++++++--------- src/interfaces/libpq/fe-secure.c | 7 +- src/interfaces/libpq/libpq-int.h | 13 ++- 4 files changed, 97 insertions(+), 52 deletions(-) diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c index 29054bad7b..4e21057d0f 100644 --- a/src/interfaces/libpq/fe-connect.c +++ b/src/interfaces/libpq/fe-connect.c @@ -2887,6 +2887,16 @@ keep_going: /* We will come back to here until there is #ifdef USE_SSL + /* + * Enable the libcrypto callbacks before checking if SSL needs + * to be done. This is done before sending the startup packet + * as depending on the type of authentication done, like MD5 + * or SCRAM that use cryptohashes, the callbacks would be + * required even without a SSL connection + */ + if (pqsecure_initialize(conn, false, true) < 0) + goto error_return; + /* * If SSL is enabled and we haven't already got encryption of * some sort running, request SSL instead of sending the @@ -2998,8 +3008,14 @@ keep_going: /* We will come back to here until there is { /* mark byte consumed */ conn->inStart = conn->inCursor; - /* Set up global SSL state if required */ - if (pqsecure_initialize(conn) != 0) + + /* + * Set up global SSL state if required. The crypto + * state has already been set if libpq took care of + * doing that, so there is no need to make that happen + * again. + */ + if (pqsecure_initialize(conn, true, false) != 0) goto error_return; } else if (SSLok == 'N') diff --git a/src/interfaces/libpq/fe-secure-openssl.c b/src/interfaces/libpq/fe-secure-openssl.c index 0fa10a23b4..9c2222c1d1 100644 --- a/src/interfaces/libpq/fe-secure-openssl.c +++ b/src/interfaces/libpq/fe-secure-openssl.c @@ -85,7 +85,7 @@ static bool pq_init_crypto_lib = true; static bool ssl_lib_initialized = false; #ifdef ENABLE_THREAD_SAFETY -static long ssl_open_connections = 0; +static long crypto_open_connections = 0; #ifndef WIN32 static pthread_mutex_t ssl_config_mutex = PTHREAD_MUTEX_INITIALIZER; @@ -111,7 +111,7 @@ pgtls_init_library(bool do_ssl, int do_crypto) * Disallow changing the flags while we have open connections, else we'd * get completely confused. */ - if (ssl_open_connections != 0) + if (crypto_open_connections != 0) return; #endif @@ -635,7 +635,7 @@ pq_lockingcallback(int mode, int n, const char *file, int line) * override it. */ int -pgtls_init(PGconn *conn) +pgtls_init(PGconn *conn, bool do_ssl, bool do_crypto) { #ifdef ENABLE_THREAD_SAFETY #ifdef WIN32 @@ -684,22 +684,28 @@ pgtls_init(PGconn *conn) } } - if (ssl_open_connections++ == 0) + if (do_crypto && !conn->crypto_loaded) { - /* - * These are only required for threaded libcrypto applications, - * but make sure we don't stomp on them if they're already set. - */ - if (CRYPTO_get_id_callback() == NULL) - CRYPTO_set_id_callback(pq_threadidcallback); - if (CRYPTO_get_locking_callback() == NULL) - CRYPTO_set_locking_callback(pq_lockingcallback); + if (crypto_open_connections++ == 0) + { + /* + * These are only required for threaded libcrypto + * applications, but make sure we don't stomp on them if + * they're already set. + */ + if (CRYPTO_get_id_callback() == NULL) + CRYPTO_set_id_callback(pq_threadidcallback); + if (CRYPTO_get_locking_callback() == NULL) + CRYPTO_set_locking_callback(pq_lockingcallback); + } + + conn->crypto_loaded = true; } } #endif /* HAVE_CRYPTO_LOCK */ #endif /* ENABLE_THREAD_SAFETY */ - if (!ssl_lib_initialized) + if (!ssl_lib_initialized && do_ssl) { if (pq_init_ssl_lib) { @@ -740,10 +746,10 @@ destroy_ssl_system(void) if (pthread_mutex_lock(&ssl_config_mutex)) return; - if (pq_init_crypto_lib && ssl_open_connections > 0) - --ssl_open_connections; + if (pq_init_crypto_lib && crypto_open_connections > 0) + --crypto_open_connections; - if (pq_init_crypto_lib && ssl_open_connections == 0) + if (pq_init_crypto_lib && crypto_open_connections == 0) { /* * No connections left, unregister libcrypto callbacks, if no one @@ -1402,46 +1408,63 @@ pgtls_close(PGconn *conn) { bool destroy_needed = false; - if (conn->ssl) + if (conn->ssl_in_use) { - /* - * We can't destroy everything SSL-related here due to the possible - * later calls to OpenSSL routines which may need our thread - * callbacks, so set a flag here and check at the end. - */ - destroy_needed = true; + if (conn->ssl) + { + /* + * We can't destroy everything SSL-related here due to the + * possible later calls to OpenSSL routines which may need our + * thread callbacks, so set a flag here and check at the end. + */ - SSL_shutdown(conn->ssl); - SSL_free(conn->ssl); - conn->ssl = NULL; - conn->ssl_in_use = false; - } + SSL_shutdown(conn->ssl); + SSL_free(conn->ssl); + conn->ssl = NULL; + conn->ssl_in_use = false; - if (conn->peer) - { - X509_free(conn->peer); - conn->peer = NULL; - } + destroy_needed = true; + } + + if (conn->peer) + { + X509_free(conn->peer); + conn->peer = NULL; + } #ifdef USE_SSL_ENGINE - if (conn->engine) - { - ENGINE_finish(conn->engine); - ENGINE_free(conn->engine); - conn->engine = NULL; - } + if (conn->engine) + { + ENGINE_finish(conn->engine); + ENGINE_free(conn->engine); + conn->engine = NULL; + } #endif + } + else + { + /* + * In the non-SSL case, just remove the crypto callbacks if the + * connection has then loaded. This code path has no dependency + * on any pending SSL calls. + */ + if (conn->crypto_loaded) + destroy_needed = true; + } /* - * This will remove our SSL locking hooks, if this is the last SSL - * connection, which means we must wait to call it until after all SSL - * calls have been made, otherwise we can end up with a race condition and - * possible deadlocks. + * This will remove our crypto locking hooks if this is the last + * connection using libcrypto which means we must wait to call it until + * after all the potential SSL calls have been made, otherwise we can end + * up with a race condition and possible deadlocks. * * See comments above destroy_ssl_system(). */ if (destroy_needed) + { destroy_ssl_system(); + conn->crypto_loaded = false; + } } diff --git a/src/interfaces/libpq/fe-secure.c b/src/interfaces/libpq/fe-secure.c index c601071838..b15d8d137c 100644 --- a/src/interfaces/libpq/fe-secure.c +++ b/src/interfaces/libpq/fe-secure.c @@ -159,12 +159,12 @@ PQinitOpenSSL(int do_ssl, int do_crypto) * Initialize global SSL context */ int -pqsecure_initialize(PGconn *conn) +pqsecure_initialize(PGconn *conn, bool do_ssl, bool do_crypto) { int r = 0; #ifdef USE_SSL - r = pgtls_init(conn); + r = pgtls_init(conn, do_ssl, do_crypto); #endif return r; @@ -191,8 +191,7 @@ void pqsecure_close(PGconn *conn) { #ifdef USE_SSL - if (conn->ssl_in_use) - pgtls_close(conn); + pgtls_close(conn); #endif } diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h index adf149a76f..2f052f61f8 100644 --- a/src/interfaces/libpq/libpq-int.h +++ b/src/interfaces/libpq/libpq-int.h @@ -486,6 +486,11 @@ struct pg_conn void *engine; /* dummy field to keep struct the same if * OpenSSL version changes */ #endif + bool crypto_loaded; /* Track if libcrypto locking callbacks have + * been done for this connection. This can be + * removed once support for OpenSSL 1.0.2 is + * removed as this locking is handled + * internally in OpenSSL >= 1.1.0. */ #endif /* USE_OPENSSL */ #endif /* USE_SSL */ @@ -667,7 +672,7 @@ extern int pqWriteReady(PGconn *conn); /* === in fe-secure.c === */ -extern int pqsecure_initialize(PGconn *); +extern int pqsecure_initialize(PGconn *, bool, bool); extern PostgresPollingStatusType pqsecure_open_client(PGconn *); extern void pqsecure_close(PGconn *); extern ssize_t pqsecure_read(PGconn *, void *ptr, size_t len); @@ -696,11 +701,13 @@ extern void pgtls_init_library(bool do_ssl, int do_crypto); * Initialize SSL library. * * The conn parameter is only used to be able to pass back an error - * message - no connection-local setup is made here. + * message - no connection-local setup is made here. do_ssl controls + * if SSL is initialized, and do_crypto does the same for the crypto + * part. * * Returns 0 if OK, -1 on failure (adding a message to conn->errorMessage). */ -extern int pgtls_init(PGconn *conn); +extern int pgtls_init(PGconn *conn, bool do_ssl, bool do_crypto); /* * Begin or continue negotiating a secure session.