From dc16713c5afc952f4a68e9a3fae56aa6b2e9bd74 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Sun, 11 Feb 2024 10:42:25 -0500 Subject: [PATCH] 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. --- configure | 2 +- configure.ac | 2 +- meson.build | 1 + src/backend/libpq/be-secure-openssl.c | 48 ++++++++++++++++-------- src/include/pg_config.h.in | 3 ++ src/interfaces/libpq/fe-secure-openssl.c | 48 ++++++++++++++++-------- 6 files changed, 72 insertions(+), 32 deletions(-) diff --git a/configure b/configure index 2a1ee251f2..2b1f83b8b1 100755 --- a/configure +++ b/configure @@ -12870,7 +12870,7 @@ done # defines OPENSSL_VERSION_NUMBER to claim version 2.0.0, even though it # doesn't have these OpenSSL 1.1.0 functions. So check for individual # functions. - for ac_func in OPENSSL_init_ssl BIO_meth_new ASN1_STRING_get0_data HMAC_CTX_new HMAC_CTX_free + for ac_func in OPENSSL_init_ssl BIO_get_data BIO_meth_new ASN1_STRING_get0_data HMAC_CTX_new HMAC_CTX_free do : as_ac_var=`$as_echo "ac_cv_func_$ac_func" | $as_tr_sh` ac_fn_c_check_func "$LINENO" "$ac_func" "$as_ac_var" diff --git a/configure.ac b/configure.ac index 52fd7af446..be46d9a6fc 100644 --- a/configure.ac +++ b/configure.ac @@ -1374,7 +1374,7 @@ if test "$with_ssl" = openssl ; then # defines OPENSSL_VERSION_NUMBER to claim version 2.0.0, even though it # doesn't have these OpenSSL 1.1.0 functions. So check for individual # functions. - AC_CHECK_FUNCS([OPENSSL_init_ssl BIO_meth_new ASN1_STRING_get0_data HMAC_CTX_new HMAC_CTX_free]) + AC_CHECK_FUNCS([OPENSSL_init_ssl BIO_get_data BIO_meth_new ASN1_STRING_get0_data HMAC_CTX_new HMAC_CTX_free]) # OpenSSL versions before 1.1.0 required setting callback functions, for # thread-safety. In 1.1.0, it's no longer required, and CRYPTO_lock() # function was removed. diff --git a/meson.build b/meson.build index 8ed51b6aae..ba133b6e11 100644 --- a/meson.build +++ b/meson.build @@ -1290,6 +1290,7 @@ if sslopt in ['auto', 'openssl'] # doesn't have these OpenSSL 1.1.0 functions. So check for individual # functions. ['OPENSSL_init_ssl'], + ['BIO_get_data'], ['BIO_meth_new'], ['ASN1_STRING_get0_data'], ['HMAC_CTX_new'], diff --git a/src/backend/libpq/be-secure-openssl.c b/src/backend/libpq/be-secure-openssl.c index e12b1cc9e3..68697eb4ce 100644 --- a/src/backend/libpq/be-secure-openssl.c +++ b/src/backend/libpq/be-secure-openssl.c @@ -59,7 +59,7 @@ 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 my_SSL_set_fd(Port *port); static DH *load_dh_file(char *filename, bool isServerStart); static DH *load_dh_buffer(const char *buffer, size_t len); @@ -440,7 +440,7 @@ be_tls_open_server(Port *port) SSLerrmessage(ERR_get_error())))); return -1; } - if (!my_SSL_set_fd(port, port->sock)) + if (!my_SSL_set_fd(port)) { ereport(COMMERROR, (errcode(ERRCODE_PROTOCOL_VIOLATION), @@ -849,6 +849,11 @@ 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. */ +#ifndef HAVE_BIO_GET_DATA +#define BIO_get_data(bio) (bio->ptr) +#define BIO_set_data(bio, data) (bio->ptr = data) +#endif + static BIO_METHOD *my_bio_methods = NULL; static int @@ -858,7 +863,7 @@ my_sock_read(BIO *h, char *buf, int size) if (buf != NULL) { - res = secure_raw_read(((Port *) BIO_get_app_data(h)), buf, size); + res = secure_raw_read(((Port *) BIO_get_data(h)), buf, size); BIO_clear_retry_flags(h); if (res <= 0) { @@ -878,7 +883,7 @@ my_sock_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) { @@ -892,12 +897,27 @@ my_sock_write(BIO *h, const char *buf, int size) return res; } +static long +my_sock_ctrl(BIO *h, int cmd, long num, void *ptr) +{ + long res = 0; + + switch (cmd) + { + case BIO_CTRL_FLUSH: + /* libssl expects all BIOs to support BIO_flush. */ + res = 1; + break; + } + + return res; +} + static BIO_METHOD * my_BIO_s_socket(void) { if (!my_bio_methods) { - BIO_METHOD *biom = (BIO_METHOD *) BIO_s_socket(); #ifdef HAVE_BIO_METH_NEW int my_bio_index; @@ -910,12 +930,7 @@ my_BIO_s_socket(void) 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))) + !BIO_meth_set_ctrl(my_bio_methods, my_sock_ctrl)) { BIO_meth_free(my_bio_methods); my_bio_methods = NULL; @@ -925,9 +940,12 @@ my_BIO_s_socket(void) my_bio_methods = malloc(sizeof(BIO_METHOD)); if (!my_bio_methods) return NULL; - memcpy(my_bio_methods, biom, sizeof(BIO_METHOD)); + memset(my_bio_methods, 0, sizeof(BIO_METHOD)); + my_bio_methods->type = BIO_TYPE_SOCKET; + my_bio_methods->name = "libpq socket"; my_bio_methods->bread = my_sock_read; my_bio_methods->bwrite = my_sock_write; + my_bio_methods->ctrl = my_sock_ctrl; #endif } return my_bio_methods; @@ -935,7 +953,7 @@ my_BIO_s_socket(void) /* This should exactly match OpenSSL's SSL_set_fd except for using my BIO */ static int -my_SSL_set_fd(Port *port, int fd) +my_SSL_set_fd(Port *port) { int ret = 0; BIO *bio; @@ -954,9 +972,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/pg_config.h.in b/src/include/pg_config.h.in index 07e73567dc..c7b88945fa 100644 --- a/src/include/pg_config.h.in +++ b/src/include/pg_config.h.in @@ -66,6 +66,9 @@ /* Define to 1 if you have the `backtrace_symbols' function. */ #undef HAVE_BACKTRACE_SYMBOLS +/* Define to 1 if you have the `BIO_get_data' function. */ +#undef HAVE_BIO_GET_DATA + /* Define to 1 if you have the `BIO_meth_new' function. */ #undef HAVE_BIO_METH_NEW diff --git a/src/interfaces/libpq/fe-secure-openssl.c b/src/interfaces/libpq/fe-secure-openssl.c index 8110882262..52b23638f0 100644 --- a/src/interfaces/libpq/fe-secure-openssl.c +++ b/src/interfaces/libpq/fe-secure-openssl.c @@ -81,7 +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 int my_SSL_set_fd(PGconn *conn, int fd); +static int my_SSL_set_fd(PGconn *conn); static bool pq_init_ssl_lib = true; @@ -1191,7 +1191,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)) + !my_SSL_set_fd(conn)) { char *err = SSLerrmessage(ERR_get_error()); @@ -1802,6 +1802,11 @@ PQsslAttribute(PGconn *conn, const char *attribute_name) * see sock_read() and sock_write() in OpenSSL's crypto/bio/bss_sock.c. */ +#ifndef HAVE_BIO_GET_DATA +#define BIO_get_data(bio) (bio->ptr) +#define BIO_set_data(bio, data) (bio->ptr = data) +#endif + /* protected by ssl_config_mutex */ static BIO_METHOD *my_bio_methods; @@ -1810,7 +1815,7 @@ my_sock_read(BIO *h, char *buf, int size) { int res; - res = pqsecure_raw_read((PGconn *) BIO_get_app_data(h), buf, size); + res = pqsecure_raw_read((PGconn *) BIO_get_data(h), buf, size); BIO_clear_retry_flags(h); if (res < 0) { @@ -1840,7 +1845,7 @@ my_sock_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) { @@ -1865,6 +1870,22 @@ my_sock_write(BIO *h, const char *buf, int size) return res; } +static long +my_sock_ctrl(BIO *h, int cmd, long num, void *ptr) +{ + long res = 0; + + switch (cmd) + { + case BIO_CTRL_FLUSH: + /* libssl expects all BIOs to support BIO_flush. */ + res = 1; + break; + } + + return res; +} + static BIO_METHOD * my_BIO_s_socket(void) { @@ -1877,7 +1898,6 @@ my_BIO_s_socket(void) if (!my_bio_methods) { - BIO_METHOD *biom = (BIO_METHOD *) BIO_s_socket(); #ifdef HAVE_BIO_METH_NEW int my_bio_index; @@ -1895,12 +1915,7 @@ my_BIO_s_socket(void) */ 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_set_ctrl(res, my_sock_ctrl)) { goto err; } @@ -1908,9 +1923,12 @@ my_BIO_s_socket(void) res = malloc(sizeof(BIO_METHOD)); if (!res) goto err; - memcpy(res, biom, sizeof(BIO_METHOD)); + memset(res, 0, sizeof(BIO_METHOD)); + res->type = BIO_TYPE_SOCKET; + res->name = "libpq socket"; res->bread = my_sock_read; res->bwrite = my_sock_write; + res->ctrl = my_sock_ctrl; #endif } @@ -1932,7 +1950,7 @@ err: /* This should exactly match OpenSSL's SSL_set_fd except for using my BIO */ static int -my_SSL_set_fd(PGconn *conn, int fd) +my_SSL_set_fd(PGconn *conn) { int ret = 0; BIO *bio; @@ -1950,10 +1968,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; -- 2.39.2