On Mon, Mar 08, 2021 at 06:06:32PM +0000, Jacob Champion wrote: > I had to convince myself that this logic is correct -- we set > destroy_needed even if crypto is not enabled, but then check later to > make sure that crypto_loaded is true before doing anything. What would > you think about moving the conn->crypto_loaded check to the else > branch, so that destroy_needed is only set if we actually need it?
Do you mean something like the attached? If I recall my mood from the moment, I think that I did that to be more careful with the case where the client has its own set of callbacks set (pq_init_crypto_lib as false) but that does not matter as this is double-checked in destroy_ssl_system(). I have adjusted some comments after more review. -- Michael
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 29054bad7b..e4a85c37c4 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, 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..3de8f89264 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 they
+ * are 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.
signature.asc
Description: PGP signature
