From c33884464f5d8b5f7e336be5357fc5c8114a4b34 Mon Sep 17 00:00:00 2001 From: Daniel Gustafsson Date: Tue, 3 Sep 2024 14:13:05 +0200 Subject: [PATCH v4] Avoid mixing custom and OpenSSL BIO functions This addresses the root cause of the BIO_set_data conflict that c82207a548db47623a2bfa2447babdaa630302b9 attempted to address. That fix was really just a workaround. The real root cause was that postgres was mixing up two BIO implementations, each of which expected to be driving the BIO. Mixing them up did not actually do any good. The Port and PGconn structures already maintained the file descriptors. The socket BIO's copy, configured via BIO_set_fd, wasn't even being used because my_BIO_s_socket replaced read and write anyway. We've been essentially keeping extra state around, and relying on it being unused: - gets - Not implemented by sockets and not used by libssl. - puts - Not used by libssl. If it were, it would break the special SIGPIPE and interrupt handling postgres aiming for. - ctrl - (More on this later) - create - This is just setting up state that we don't use. - destroy - This is a no-op because we use BIO_NOCLOSE. In fact it's important that it's a no-op because otherwise OpenSSL would close the socket under postgres' feet! - callback_ctrl - Not implemented by sockets. That leaves ctrl. ctrl is a bunch of operations (it's ioctl). The only operation that matters is BIO_CTRL_FLUSH, which is implemented as a no-op. All other operations are unused. It's once again good that they're unused because otherwise OpenSSL might mess with postgres's socket, break the assumptions around interrupt handling, etc. Instead, simply implement a very basic ctrl ourselves and drop the other functions. This avoids the risk that future OpenSSL upgrades add some feature to BIO_s_socket's ctrl which conflicts with postgres. Once we've done that, we're free to use BIO_set_data. While BIO_set_app_data works fine, I've reverted back to BIO_set_data because it's more commonly used. app_data depends on OpenSSL's "ex_data" mechanism, which is a tad heavier under the hood. As this is no longer related to BIO_s_socket or calling SSL_set_fd, I've renamed the methods to reference the PGconn and Port types instead. --- src/backend/libpq/be-secure-openssl.c | 94 +++++++++++++++--------- src/include/libpq/libpq-be.h | 1 + src/interfaces/libpq/fe-secure-openssl.c | 81 ++++++++++++-------- src/interfaces/libpq/libpq-int.h | 1 + 4 files changed, 113 insertions(+), 64 deletions(-) diff --git a/src/backend/libpq/be-secure-openssl.c b/src/backend/libpq/be-secure-openssl.c index 1ebd3f2e6d..1f86b0cd98 100644 --- a/src/backend/libpq/be-secure-openssl.c +++ b/src/backend/libpq/be-secure-openssl.c @@ -57,10 +57,10 @@ static void default_openssl_tls_init(SSL_CTX *context, bool isServerStart); openssl_tls_init_hook_typ openssl_tls_init_hook = default_openssl_tls_init; -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 int my_SSL_set_fd(Port *port, int fd); +static int port_bio_read(BIO *h, char *buf, int size); +static int port_bio_write(BIO *h, const char *buf, int size); +static BIO_METHOD *port_bio_method(void); +static int ssl_set_port_bio(Port *port); static DH *load_dh_file(char *filename, bool isServerStart); static DH *load_dh_buffer(const char *buffer, size_t len); @@ -453,7 +453,7 @@ be_tls_open_server(Port *port) SSLerrmessage(ERR_get_error())))); return -1; } - if (!my_SSL_set_fd(port, port->sock)) + if (!ssl_set_port_bio(port)) { ereport(COMMERROR, (errcode(ERRCODE_PROTOCOL_VIOLATION), @@ -890,17 +890,19 @@ be_tls_write(Port *port, void *ptr, size_t len, int *waitfor) * see sock_read() and sock_write() in OpenSSL's crypto/bio/bss_sock.c. */ -static BIO_METHOD *my_bio_methods = NULL; +static BIO_METHOD *port_bio_method_ptr = NULL; static int -my_sock_read(BIO *h, char *buf, int size) +port_bio_read(BIO *h, char *buf, int size) { int res = 0; + Port *port = (Port *) BIO_get_data(h); if (buf != NULL) { - res = secure_raw_read(((Port *) BIO_get_app_data(h)), buf, size); + res = secure_raw_read(port, buf, size); BIO_clear_retry_flags(h); + port->last_read_was_eof = res == 0; if (res <= 0) { /* If we were interrupted, tell caller to retry */ @@ -915,11 +917,11 @@ my_sock_read(BIO *h, char *buf, int size) } static int -my_sock_write(BIO *h, const char *buf, int size) +port_bio_write(BIO *h, const char *buf, int size) { int res = 0; - res = secure_raw_write(((Port *) BIO_get_app_data(h)), buf, size); + res = secure_raw_write(((Port *) BIO_get_data(h)), buf, size); BIO_clear_retry_flags(h); if (res <= 0) { @@ -933,47 +935,69 @@ my_sock_write(BIO *h, const char *buf, int size) return res; } +static long +port_bio_ctrl(BIO *h, int cmd, long num, void *ptr) +{ + long res; + Port *port = (Port *) BIO_get_data(h); + + switch (cmd) + { + case BIO_CTRL_EOF: + /* + * This should not be needed. port_bio_read already has a way to + * signal EOF to OpenSSL. However, OpenSSL made an undocumented, + * backwards-incompatible change and now expects EOF via BIO_ctrl. + * See https://github.com/openssl/openssl/issues/8208 + */ + res = port->last_read_was_eof; + break; + case BIO_CTRL_FLUSH: + /* libssl expects all BIOs to support BIO_flush. */ + res = 1; + break; + default: + res = 0; + break; + } + + return res; +} + static BIO_METHOD * -my_BIO_s_socket(void) +port_bio_method(void) { - if (!my_bio_methods) + if (!port_bio_method_ptr) { - BIO_METHOD *biom = (BIO_METHOD *) BIO_s_socket(); int my_bio_index; - my_bio_index = BIO_get_new_index(); - if (my_bio_index == -1) - return NULL; - my_bio_index |= (BIO_TYPE_DESCRIPTOR | BIO_TYPE_SOURCE_SINK); - my_bio_methods = BIO_meth_new(my_bio_index, "PostgreSQL backend socket"); - if (!my_bio_methods) + my_bio_index = BIO_get_new_index(); + if (my_bio_index == -1) + return NULL; + my_bio_index |= BIO_TYPE_SOURCE_SINK; + port_bio_method_ptr = BIO_meth_new(my_bio_index, "PostgreSQL backend socket"); + if (!port_bio_method_ptr) return NULL; - 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(port_bio_method_ptr, port_bio_write) || + !BIO_meth_set_read(port_bio_method_ptr, port_bio_read) || + !BIO_meth_set_ctrl(port_bio_method_ptr, port_bio_ctrl)) { - BIO_meth_free(my_bio_methods); - my_bio_methods = NULL; + BIO_meth_free(port_bio_method_ptr); + port_bio_method_ptr = NULL; return NULL; } } - return my_bio_methods; + return port_bio_method_ptr; } -/* This should exactly match OpenSSL's SSL_set_fd except for using my BIO */ static int -my_SSL_set_fd(Port *port, int fd) +ssl_set_port_bio(Port *port) { int ret = 0; BIO *bio; BIO_METHOD *bio_method; - bio_method = my_BIO_s_socket(); + bio_method = port_bio_method(); if (bio_method == NULL) { SSLerr(SSL_F_SSL_SET_FD, ERR_R_BUF_LIB); @@ -986,9 +1010,9 @@ my_SSL_set_fd(Port *port, int fd) SSLerr(SSL_F_SSL_SET_FD, ERR_R_BUF_LIB); goto err; } - BIO_set_app_data(bio, port); + BIO_set_data(bio, port); + BIO_set_init(bio, 1); - BIO_set_fd(bio, fd, BIO_NOCLOSE); SSL_set_bio(port->ssl, bio, bio); ret = 1; err: diff --git a/src/include/libpq/libpq-be.h b/src/include/libpq/libpq-be.h index 05cb1874c5..d97d1e5f6b 100644 --- a/src/include/libpq/libpq-be.h +++ b/src/include/libpq/libpq-be.h @@ -204,6 +204,7 @@ typedef struct Port char *peer_dn; bool peer_cert_valid; bool alpn_used; + bool last_read_was_eof; /* * OpenSSL structures. (Keep these last so that the locations of other diff --git a/src/interfaces/libpq/fe-secure-openssl.c b/src/interfaces/libpq/fe-secure-openssl.c index b5749d0292..420443a031 100644 --- a/src/interfaces/libpq/fe-secure-openssl.c +++ b/src/interfaces/libpq/fe-secure-openssl.c @@ -77,10 +77,10 @@ static char *SSLerrmessage(unsigned long ecode); static void SSLerrfree(char *buf); 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 int my_SSL_set_fd(PGconn *conn, int fd); +static int pgconn_bio_read(BIO *h, char *buf, int size); +static int pgconn_bio_write(BIO *h, const char *buf, int size); +static BIO_METHOD *pgconn_bio_method(void); +static int ssl_set_pgconn_bio(PGconn *conn); static pthread_mutex_t ssl_config_mutex = PTHREAD_MUTEX_INITIALIZER; @@ -989,7 +989,7 @@ initialize_SSL(PGconn *conn) */ if (!(conn->ssl = SSL_new(SSL_context)) || !SSL_set_app_data(conn->ssl, conn) || - !my_SSL_set_fd(conn, conn->sock)) + !ssl_set_pgconn_bio(conn)) { char *err = SSLerrmessage(ERR_get_error()); @@ -1670,16 +1670,17 @@ PQsslAttribute(PGconn *conn, const char *attribute_name) */ /* protected by ssl_config_mutex */ -static BIO_METHOD *my_bio_methods; +static BIO_METHOD *pgconn_bio_method_ptr; static int -my_sock_read(BIO *h, char *buf, int size) +pgconn_bio_read(BIO *h, char *buf, int size) { - PGconn *conn = (PGconn *) BIO_get_app_data(h); + PGconn *conn = (PGconn *) BIO_get_data(h); int res; res = pqsecure_raw_read(conn, buf, size); BIO_clear_retry_flags(h); + conn->last_read_was_eof = res == 0; if (res < 0) { /* If we were interrupted, tell caller to retry */ @@ -1707,11 +1708,11 @@ my_sock_read(BIO *h, char *buf, int size) } static int -my_sock_write(BIO *h, const char *buf, int size) +pgconn_bio_write(BIO *h, const char *buf, int size) { int res; - res = pqsecure_raw_write((PGconn *) BIO_get_app_data(h), buf, size); + res = pqsecure_raw_write((PGconn *) BIO_get_data(h), buf, size); BIO_clear_retry_flags(h); if (res < 0) { @@ -1736,25 +1737,53 @@ my_sock_write(BIO *h, const char *buf, int size) return res; } +static long +pgconn_bio_ctrl(BIO *h, int cmd, long num, void *ptr) +{ + long res; + PGconn *conn = (PGconn *) BIO_get_data(h); + + switch (cmd) + { + case BIO_CTRL_EOF: + /* + * This should not be needed. pgconn_bio_read already has a way to + * signal EOF to OpenSSL. However, OpenSSL made an undocumented, + * backwards-incompatible change and now expects EOF via BIO_ctrl. + * See https://github.com/openssl/openssl/issues/8208 + */ + res = conn->last_read_was_eof; + break; + case BIO_CTRL_FLUSH: + /* libssl expects all BIOs to support BIO_flush. */ + res = 1; + break; + default: + res = 0; + break; + } + + return res; +} + static BIO_METHOD * -my_BIO_s_socket(void) +pgconn_bio_method(void) { BIO_METHOD *res; if (pthread_mutex_lock(&ssl_config_mutex)) return NULL; - res = my_bio_methods; + res = pgconn_bio_method_ptr; - if (!my_bio_methods) + if (!pgconn_bio_method_ptr) { - BIO_METHOD *biom = (BIO_METHOD *) BIO_s_socket(); int my_bio_index; my_bio_index = BIO_get_new_index(); if (my_bio_index == -1) goto err; - my_bio_index |= (BIO_TYPE_DESCRIPTOR | BIO_TYPE_SOURCE_SINK); + my_bio_index |= BIO_TYPE_SOURCE_SINK; res = BIO_meth_new(my_bio_index, "libpq socket"); if (!res) goto err; @@ -1763,20 +1792,15 @@ my_BIO_s_socket(void) * As of this writing, these functions never fail. But check anyway, * like OpenSSL's own examples do. */ - 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))) + if (!BIO_meth_set_write(res, pgconn_bio_write) || + !BIO_meth_set_read(res, pgconn_bio_read) || + !BIO_meth_set_ctrl(res, pgconn_bio_ctrl)) { goto err; } } - my_bio_methods = res; + pgconn_bio_method_ptr = res; pthread_mutex_unlock(&ssl_config_mutex); return res; @@ -1787,15 +1811,14 @@ err: return NULL; } -/* This should exactly match OpenSSL's SSL_set_fd except for using my BIO */ static int -my_SSL_set_fd(PGconn *conn, int fd) +ssl_set_pgconn_bio(PGconn *conn) { int ret = 0; BIO *bio; BIO_METHOD *bio_method; - bio_method = my_BIO_s_socket(); + bio_method = pgconn_bio_method(); if (bio_method == NULL) { SSLerr(SSL_F_SSL_SET_FD, ERR_R_BUF_LIB); @@ -1807,10 +1830,10 @@ my_SSL_set_fd(PGconn *conn, int fd) SSLerr(SSL_F_SSL_SET_FD, ERR_R_BUF_LIB); goto err; } - BIO_set_app_data(bio, conn); + BIO_set_data(bio, conn); + BIO_set_init(bio, 1); SSL_set_bio(conn->ssl, bio, bio); - BIO_set_fd(bio, fd, BIO_NOCLOSE); ret = 1; err: return ret; diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h index 9579f80353..08cc391cbd 100644 --- a/src/interfaces/libpq/libpq-int.h +++ b/src/interfaces/libpq/libpq-int.h @@ -581,6 +581,7 @@ struct pg_conn bool ssl_handshake_started; bool ssl_cert_requested; /* Did the server ask us for a cert? */ bool ssl_cert_sent; /* Did we send one in reply? */ + bool last_read_was_eof; #ifdef USE_SSL #ifdef USE_OPENSSL -- 2.39.3 (Apple Git-146)