From 09f680d114b5e1372404d0c1e17851a786c6c615 Mon Sep 17 00:00:00 2001 From: Michael Paquier Date: Mon, 27 Nov 2023 09:40:55 +0900 Subject: [PATCH] Fix race condition with BIO methods initialization in libpq with threads The libpq code in charge of creating per-connection SSL objects was prone to a race condition when loading the custom BIO methods needed by my_SSL_set_fd(). As BIO methods are stored as a static variable, the initialization of a connection could fail because it could be possible to have one thread refer to my_bio_methods while it is being manipulated by a second concurrent thread. This error has been introduced by 8bb14cdd33de, that has removed ssl_config_mutex around the call of my_SSL_set_fd(), that itself sets the custom BIO methods used in libpq. Like previously, the BIO method initialization is now protected by the existing ssl_config_mutex, itself initialized earlier for WIN32. While on it, document that my_bio_methods is protected by ssl_config_mutex, as this can be easy to miss. Reported-by: Willi Mann Author: Willi Mann, Michael Paquier Discussion: https://postgr.es/m/e77abc4c-4d03-4058-a9d7-ef0035657e04@celonis.com Backpatch-through: 12 --- src/interfaces/libpq/fe-secure-openssl.c | 73 +++++++++++++++++------- 1 file changed, 51 insertions(+), 22 deletions(-) diff --git a/src/interfaces/libpq/fe-secure-openssl.c b/src/interfaces/libpq/fe-secure-openssl.c index 07d5daf4d9..05fa05aa3e 100644 --- a/src/interfaces/libpq/fe-secure-openssl.c +++ b/src/interfaces/libpq/fe-secure-openssl.c @@ -1607,6 +1607,7 @@ PQsslAttribute(PGconn *conn, const char *attribute_name) #define BIO_set_data(bio, data) (bio->ptr = data) #endif +/* protected by ssl_config_mutex */ static BIO_METHOD *my_bio_methods; static int @@ -1672,6 +1673,15 @@ my_sock_write(BIO *h, const char *buf, int size) static BIO_METHOD * my_BIO_s_socket(void) { + BIO_METHOD *res; + +#ifdef ENABLE_THREAD_SAFETY + if (pthread_mutex_lock(&ssl_config_mutex)) + return NULL; +#endif + + res = my_bio_methods; + if (!my_bio_methods) { BIO_METHOD *biom = (BIO_METHOD *) BIO_s_socket(); @@ -1680,39 +1690,58 @@ my_BIO_s_socket(void) my_bio_index = BIO_get_new_index(); if (my_bio_index == -1) - return NULL; + goto err; my_bio_index |= (BIO_TYPE_DESCRIPTOR | BIO_TYPE_SOURCE_SINK); - my_bio_methods = BIO_meth_new(my_bio_index, "libpq socket"); - if (!my_bio_methods) - return NULL; + res = BIO_meth_new(my_bio_index, "libpq socket"); + if (!res) + goto err; /* * As of this writing, these functions never fail. But check anyway, * like OpenSSL's own examples do. */ - if (!BIO_meth_set_write(my_bio_methods, my_sock_write) || - !BIO_meth_set_read(my_bio_methods, my_sock_read) || - !BIO_meth_set_gets(my_bio_methods, BIO_meth_get_gets(biom)) || - !BIO_meth_set_puts(my_bio_methods, BIO_meth_get_puts(biom)) || - !BIO_meth_set_ctrl(my_bio_methods, BIO_meth_get_ctrl(biom)) || - !BIO_meth_set_create(my_bio_methods, BIO_meth_get_create(biom)) || - !BIO_meth_set_destroy(my_bio_methods, BIO_meth_get_destroy(biom)) || - !BIO_meth_set_callback_ctrl(my_bio_methods, BIO_meth_get_callback_ctrl(biom))) + if (!BIO_meth_set_write(res, my_sock_write) || + !BIO_meth_set_read(res, my_sock_read) || + !BIO_meth_set_gets(res, BIO_meth_get_gets(biom)) || + !BIO_meth_set_puts(res, BIO_meth_get_puts(biom)) || + !BIO_meth_set_ctrl(res, BIO_meth_get_ctrl(biom)) || + !BIO_meth_set_create(res, BIO_meth_get_create(biom)) || + !BIO_meth_set_destroy(res, BIO_meth_get_destroy(biom)) || + !BIO_meth_set_callback_ctrl(res, BIO_meth_get_callback_ctrl(biom))) { - BIO_meth_free(my_bio_methods); - my_bio_methods = NULL; - return NULL; + goto err; } #else - my_bio_methods = malloc(sizeof(BIO_METHOD)); - if (!my_bio_methods) - return NULL; - memcpy(my_bio_methods, biom, sizeof(BIO_METHOD)); - my_bio_methods->bread = my_sock_read; - my_bio_methods->bwrite = my_sock_write; + res = malloc(sizeof(BIO_METHOD)); + if (!res) + goto err; + memcpy(res, biom, sizeof(BIO_METHOD)); + res->bread = my_sock_read; + res->bwrite = my_sock_write; #endif } - return my_bio_methods; + + my_bio_methods = res; + +#ifdef ENABLE_THREAD_SAFETY + pthread_mutex_unlock(&ssl_config_mutex); +#endif + + return res; + +err: +#ifdef HAVE_BIO_METH_NEW + if (res) + BIO_meth_free(res); +#else + if (res) + free(res); +#endif + +#ifdef ENABLE_THREAD_SAFETY + pthread_mutex_unlock(&ssl_config_mutex); +#endif + return NULL; } /* This should exactly match OpenSSL's SSL_set_fd except for using my BIO */