From 55754380f36d098551bd55dd49e27f64dd1c8d2f Mon Sep 17 00:00:00 2001
From: Stephen Frost <sfrost@snowman.net>
Date: Thu, 1 Aug 2013 01:15:45 -0400
Subject: [PATCH] Add locking around SSL_context usage in libpq

I've been working with Nick Phillips on an issue he ran into when
trying to use threads with SSL client certificates.  As it turns out,
the call in initialize_SSL() to SSL_CTX_use_certificate_chain_file()
will modify our SSL_context without any protection from other threads
also calling that function or being at some other point and trying to
read from SSL_context.

To protect against this, I've written up the attached (based on an
initial patch from Nick and much subsequent discussion) which puts
locks around SSL_CTX_use_certificate_chain_file() and all of the other
users of SSL_context which weren't already protected.

Nick Phillips, much reworked by Stephen Frost

Back-patch to 9.0 where we started loading the cert directly instead of
using a callback.
---
 src/interfaces/libpq/fe-secure.c | 56 ++++++++++++++++++++++++++++++--
 1 file changed, 53 insertions(+), 3 deletions(-)

diff --git a/src/interfaces/libpq/fe-secure.c b/src/interfaces/libpq/fe-secure.c
index a6a09cd1ab..e9a32ded15 100644
--- a/src/interfaces/libpq/fe-secure.c
+++ b/src/interfaces/libpq/fe-secure.c
@@ -92,6 +92,12 @@ static void SSLerrfree(char *buf);
 
 static bool pq_init_ssl_lib = true;
 static bool pq_init_crypto_lib = true;
+
+/*
+ * SSL_context is currently shared between threads and therefore we need to be
+ * careful to lock around any usage of it when providing thread safety.
+ * ssl_config_mutex is the mutex that we use to protect it.
+ */
 static SSL_CTX *SSL_context = NULL;
 
 #ifdef ENABLE_THREAD_SAFETY
@@ -253,6 +259,10 @@ pqsecure_open_client(PGconn *conn)
 		/* We cannot use MSG_NOSIGNAL to block SIGPIPE when using SSL */
 		conn->sigpipe_flag = false;
 
+#ifdef ENABLE_THREAD_SAFETY
+		if (pthread_mutex_lock(&ssl_config_mutex))
+			return -1;
+#endif
 		/* Create a connection-specific SSL object */
 		if (!(conn->ssl = SSL_new(SSL_context)) ||
 			!SSL_set_app_data(conn->ssl, conn) ||
@@ -265,9 +275,14 @@ pqsecure_open_client(PGconn *conn)
 							  err);
 			SSLerrfree(err);
 			close_SSL(conn);
+#ifdef ENABLE_THREAD_SAFETY
+			pthread_mutex_unlock(&ssl_config_mutex);
+#endif
 			return PGRES_POLLING_FAILED;
 		}
-
+#ifdef ENABLE_THREAD_SAFETY
+		pthread_mutex_unlock(&ssl_config_mutex);
+#endif
 		/*
 		 * Load client certificate, private key, and trusted CA certs.
 		 */
@@ -999,8 +1014,9 @@ destroy_ssl_system(void)
 		CRYPTO_set_id_callback(NULL);
 
 		/*
-		 * We don't free the lock array. If we get another connection in this
-		 * process, we will just re-use it with the existing mutexes.
+		 * We don't free the lock array or the SSL_context. If we get another
+		 * connection in this process, we will just re-use them with the
+		 * existing mutexes.
 		 *
 		 * This means we leak a little memory on repeated load/unload of the
 		 * library.
@@ -1089,7 +1105,15 @@ initialize_SSL(PGconn *conn)
 		 * understands which subject cert to present, in case different
 		 * sslcert settings are used for different connections in the same
 		 * process.
+		 *
+		 * NOTE: This function may also modify our SSL_context and therefore
+		 * we have to lock around this call and any places where we use the
+		 * SSL_context struct.
 		 */
+#ifdef ENABLE_THREAD_SAFETY
+		if (pthread_mutex_lock(&ssl_config_mutex))
+			return -1;
+#endif
 		if (SSL_CTX_use_certificate_chain_file(SSL_context, fnbuf) != 1)
 		{
 			char	   *err = SSLerrmessage();
@@ -1098,8 +1122,13 @@ initialize_SSL(PGconn *conn)
 			   libpq_gettext("could not read certificate file \"%s\": %s\n"),
 							  fnbuf, err);
 			SSLerrfree(err);
+
+#ifdef ENABLE_THREAD_SAFETY
+			pthread_mutex_unlock(&ssl_config_mutex);
+#endif
 			return -1;
 		}
+
 		if (SSL_use_certificate_file(conn->ssl, fnbuf, SSL_FILETYPE_PEM) != 1)
 		{
 			char	   *err = SSLerrmessage();
@@ -1108,10 +1137,18 @@ initialize_SSL(PGconn *conn)
 			   libpq_gettext("could not read certificate file \"%s\": %s\n"),
 							  fnbuf, err);
 			SSLerrfree(err);
+#ifdef ENABLE_THREAD_SAFETY
+			pthread_mutex_unlock(&ssl_config_mutex);
+#endif
 			return -1;
 		}
+
 		/* need to load the associated private key, too */
 		have_cert = true;
+
+#ifdef ENABLE_THREAD_SAFETY
+		pthread_mutex_unlock(&ssl_config_mutex);
+#endif
 	}
 
 	/*
@@ -1287,6 +1324,10 @@ initialize_SSL(PGconn *conn)
 	{
 		X509_STORE *cvstore;
 
+#ifdef ENABLE_THREAD_SAFETY
+		if (pthread_mutex_lock(&ssl_config_mutex))
+			return -1;
+#endif
 		if (SSL_CTX_load_verify_locations(SSL_context, fnbuf, NULL) != 1)
 		{
 			char	   *err = SSLerrmessage();
@@ -1295,6 +1336,9 @@ initialize_SSL(PGconn *conn)
 							  libpq_gettext("could not read root certificate file \"%s\": %s\n"),
 							  fnbuf, err);
 			SSLerrfree(err);
+#ifdef ENABLE_THREAD_SAFETY
+			pthread_mutex_unlock(&ssl_config_mutex);
+#endif
 			return -1;
 		}
 
@@ -1322,11 +1366,17 @@ initialize_SSL(PGconn *conn)
 								  libpq_gettext("SSL library does not support CRL certificates (file \"%s\")\n"),
 								  fnbuf);
 				SSLerrfree(err);
+#ifdef ENABLE_THREAD_SAFETY
+				pthread_mutex_unlock(&ssl_config_mutex);
+#endif
 				return -1;
 #endif
 			}
 			/* if not found, silently ignore;  we do not require CRL */
 		}
+#ifdef ENABLE_THREAD_SAFETY
+		pthread_mutex_unlock(&ssl_config_mutex);
+#endif
 
 		SSL_set_verify(conn->ssl, SSL_VERIFY_PEER, verify_cb);
 	}
-- 
2.40.0