[PATCH] fix race condition in libpq (related to ssl connections)

[Willi Mann]

Hi,

I've found a race condition in libpq. It is about the initialization of
the my_bio_methods static variable in fe-secure-openssl.c, which is not
protected by any lock. The race condition may make the initialization of
the connection fail, and as an additional weird consequence, it might
cause openssl call close(0), so stdin of the client application gets
closed.

I've prepared a patch to protect the initialization of my_bio_methods
from the race. This is my first patch submission to the postgresql
project, so I hope I didn't miss anything. Any comments and suggestions
are of course very welcome.

I also prepared a testcase. In the testcase tarball, there is a patch
that adds sleeps at the right positions to make the close(0) problem
occur practically always. It also includes comments to explain how the
race can end up calling close(0).

Concerning the patch, it is only tested on Linux. I'm unsure about
whether the simple initialization of the mutex would work nowadays also
on Windows or whether the more complicated initialization also to be
found for the ssl_config_mutex in the same source file needs to be used.
Let me know whether I should adapt that.



Regards,
Willi

--
___________________________________________________

Dr. Willi Mann | Principal Software Engineer, Tech Lead PQL

Celonis SE | Theresienstrasse 6 | 80333 Munich, Germany
F: +4989416159679
w.m...@celonis.com | www.celonis.com | LinkedIn | Twitter | Xing


From 721be8d6ea26d78fc086a2e85413858585ca9300 Mon Sep 17 00:00:00 2001
From: Willi Mann <w.m...@celonis.com>
Date: Thu, 16 Nov 2023 11:50:27 +0100
Subject: [PATCH] libpq: Fix race condition in initialization of my_bio_methods
 static variable

---
 src/interfaces/libpq/fe-secure-openssl.c | 26 ++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

diff --git a/src/interfaces/libpq/fe-secure-openssl.c b/src/interfaces/libpq/fe-secure-openssl.c
index f1192d28f2..e8fa3eb403 100644
--- a/src/interfaces/libpq/fe-secure-openssl.c
+++ b/src/interfaces/libpq/fe-secure-openssl.c
@@ -81,6 +81,7 @@ static int	PQssl_passwd_cb(char *buf, int size, int rwflag, void *userdata);
 static int	my_sock_read(BIO *h, char *buf, int size);
 static int	my_sock_write(BIO *h, const char *buf, int size);
 static BIO_METHOD *my_BIO_s_socket(void);
+static void my_BIO_methods_init(void);
 static int	my_SSL_set_fd(PGconn *conn, int fd);
 
 
@@ -1882,8 +1883,8 @@ my_sock_write(BIO *h, const char *buf, int size)
 	return res;
 }
 
-static BIO_METHOD *
-my_BIO_s_socket(void)
+static void
+my_BIO_methods_init(void)
 {
 	if (!my_bio_methods)
 	{
@@ -1893,11 +1894,11 @@ my_BIO_s_socket(void)
 
 		my_bio_index = BIO_get_new_index();
 		if (my_bio_index == -1)
-			return NULL;
+			return;
 		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;
+			return;
 
 		/*
 		 * As of this writing, these functions never fail. But check anyway,
@@ -1914,17 +1915,30 @@ my_BIO_s_socket(void)
 		{
 			BIO_meth_free(my_bio_methods);
 			my_bio_methods = NULL;
-			return NULL;
+			return;
 		}
 #else
 		my_bio_methods = malloc(sizeof(BIO_METHOD));
 		if (!my_bio_methods)
-			return NULL;
+			return;
 		memcpy(my_bio_methods, biom, sizeof(BIO_METHOD));
 		my_bio_methods->bread = my_sock_read;
 		my_bio_methods->bwrite = my_sock_write;
 #endif
 	}
+}
+
+static pthread_mutex_t my_BIO_methods_init_mutex = PTHREAD_MUTEX_INITIALIZER;
+
+static BIO_METHOD *
+my_BIO_s_socket()
+{
+	if (pthread_mutex_lock(&my_BIO_methods_init_mutex) != 0)
+	{
+		return NULL;
+	}
+	my_BIO_methods_init();
+	pthread_mutex_unlock(&my_BIO_methods_init_mutex);
 	return my_bio_methods;
 }
 
-- 
2.39.2

We discovered the problem with release 11.5, but the patch and the testcase are against the master branch. AG Munich HRB 225439 | Management: Martin Klenk, Bastian Nominacher, Alexander Rinke

testcase.tar.gz
Description: application/gzip