Skip to content

Commit 0b821b8

Browse files
sfrostsfrost
committed
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.
1 parent 1346f40 commit 0b821b8

File tree

1 file changed

+53
-3
lines changed

1 file changed

+53
-3
lines changed

‎src/interfaces/libpq/fe-secure.c

Lines changed: 53 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -93,6 +93,12 @@ static void SSLerrfree(char *buf);
9393

9494
static bool pq_init_ssl_lib = true;
9595
static bool pq_init_crypto_lib = true;
96+
97+
/*
98+
* SSL_context is currently shared between threads and therefore we need to be
99+
* careful to lock around any usage of it when providing thread safety.
100+
* ssl_config_mutex is the mutex that we use to protect it.
101+
*/
96102
static SSL_CTX *SSL_context = NULL;
97103

98104
#ifdef ENABLE_THREAD_SAFETY
@@ -254,6 +260,10 @@ pqsecure_open_client(PGconn *conn)
254260
/* We cannot use MSG_NOSIGNAL to block SIGPIPE when using SSL */
255261
conn->sigpipe_flag = false;
256262

263+
#ifdef ENABLE_THREAD_SAFETY
264+
if (pthread_mutex_lock(&ssl_config_mutex))
265+
return -1;
266+
#endif
257267
/* Create a connection-specific SSL object */
258268
if (!(conn->ssl = SSL_new(SSL_context)) ||
259269
!SSL_set_app_data(conn->ssl, conn) ||
@@ -266,9 +276,14 @@ pqsecure_open_client(PGconn *conn)
266276
err);
267277
SSLerrfree(err);
268278
close_SSL(conn);
279+
#ifdef ENABLE_THREAD_SAFETY
280+
pthread_mutex_unlock(&ssl_config_mutex);
281+
#endif
269282
return PGRES_POLLING_FAILED;
270283
}
271-
284+
#ifdef ENABLE_THREAD_SAFETY
285+
pthread_mutex_unlock(&ssl_config_mutex);
286+
#endif
272287
/*
273288
* Load client certificate, private key, and trusted CA certs.
274289
*/
@@ -998,8 +1013,9 @@ destroy_ssl_system(void)
9981013
CRYPTO_set_id_callback(NULL);
9991014

10001015
/*
1001-
* We don't free the lock array. If we get another connection in this
1002-
* process, we will just re-use it with the existing mutexes.
1016+
* We don't free the lock array or the SSL_context. If we get another
1017+
* connection in this process, we will just re-use them with the
1018+
* existing mutexes.
10031019
*
10041020
* This means we leak a little memory on repeated load/unload of the
10051021
* library.
@@ -1088,7 +1104,15 @@ initialize_SSL(PGconn *conn)
10881104
* understands which subject cert to present, in case different
10891105
* sslcert settings are used for different connections in the same
10901106
* process.
1107+
*
1108+
* NOTE: This function may also modify our SSL_context and therefore
1109+
* we have to lock around this call and any places where we use the
1110+
* SSL_context struct.
10911111
*/
1112+
#ifdef ENABLE_THREAD_SAFETY
1113+
if (pthread_mutex_lock(&ssl_config_mutex))
1114+
return -1;
1115+
#endif
10921116
if (SSL_CTX_use_certificate_chain_file(SSL_context, fnbuf) != 1)
10931117
{
10941118
char *err = SSLerrmessage();
@@ -1097,8 +1121,13 @@ initialize_SSL(PGconn *conn)
10971121
libpq_gettext("could not read certificate file \"%s\": %s\n"),
10981122
fnbuf, err);
10991123
SSLerrfree(err);
1124+
1125+
#ifdef ENABLE_THREAD_SAFETY
1126+
pthread_mutex_unlock(&ssl_config_mutex);
1127+
#endif
11001128
return -1;
11011129
}
1130+
11021131
if (SSL_use_certificate_file(conn->ssl, fnbuf, SSL_FILETYPE_PEM) != 1)
11031132
{
11041133
char *err = SSLerrmessage();
@@ -1107,10 +1136,18 @@ initialize_SSL(PGconn *conn)
11071136
libpq_gettext("could not read certificate file \"%s\": %s\n"),
11081137
fnbuf, err);
11091138
SSLerrfree(err);
1139+
#ifdef ENABLE_THREAD_SAFETY
1140+
pthread_mutex_unlock(&ssl_config_mutex);
1141+
#endif
11101142
return -1;
11111143
}
1144+
11121145
/* need to load the associated private key, too */
11131146
have_cert = true;
1147+
1148+
#ifdef ENABLE_THREAD_SAFETY
1149+
pthread_mutex_unlock(&ssl_config_mutex);
1150+
#endif
11141151
}
11151152

11161153
/*
@@ -1286,6 +1323,10 @@ initialize_SSL(PGconn *conn)
12861323
{
12871324
X509_STORE *cvstore;
12881325

1326+
#ifdef ENABLE_THREAD_SAFETY
1327+
if (pthread_mutex_lock(&ssl_config_mutex))
1328+
return -1;
1329+
#endif
12891330
if (SSL_CTX_load_verify_locations(SSL_context, fnbuf, NULL) != 1)
12901331
{
12911332
char *err = SSLerrmessage();
@@ -1294,6 +1335,9 @@ initialize_SSL(PGconn *conn)
12941335
libpq_gettext("could not read root certificate file \"%s\": %s\n"),
12951336
fnbuf, err);
12961337
SSLerrfree(err);
1338+
#ifdef ENABLE_THREAD_SAFETY
1339+
pthread_mutex_unlock(&ssl_config_mutex);
1340+
#endif
12971341
return -1;
12981342
}
12991343

@@ -1321,11 +1365,17 @@ initialize_SSL(PGconn *conn)
13211365
libpq_gettext("SSL library does not support CRL certificates (file \"%s\")\n"),
13221366
fnbuf);
13231367
SSLerrfree(err);
1368+
#ifdef ENABLE_THREAD_SAFETY
1369+
pthread_mutex_unlock(&ssl_config_mutex);
1370+
#endif
13241371
return -1;
13251372
#endif
13261373
}
13271374
/* if not found, silently ignore; we do not require CRL */
13281375
}
1376+
#ifdef ENABLE_THREAD_SAFETY
1377+
pthread_mutex_unlock(&ssl_config_mutex);
1378+
#endif
13291379

13301380
SSL_set_verify(conn->ssl, SSL_VERIFY_PEER, verify_cb);
13311381
}

0 commit comments

Comments
 (0)