From da14edfc9956c710db083ff9668c596212e781b9 Mon Sep 17 00:00:00 2001 From: Daniel Gustafsson Date: Fri, 27 Jun 2025 19:16:11 +0200 Subject: [PATCH v2] libpq: Make SSL errorhandling in backend threadsafe In order to make the errorhandling code in backend libpq be thread- safe the global variable used by the certificate verification call- back need to be replaced with passing private data, and the call to strerror need to be replaced with strerror_r which use an allocated buffer passed back instead of the static buffer. * The callback use a new member in the Port struct for passing the error detail string, and the Port struct is in turn passed as private data in the SSL object * The strerror call is replaced with a strerror_r and the static errbuf buffer is palloced with the caller being responsible for cleanup. Author: Daniel Gustafsson Reviewed-by: Peter Eisentraut Discussion: https://postgr.es/m/353226C7-97A1-4507-A380-36AA92983AE6@yesql.se fixup --- src/backend/libpq/be-secure-openssl.c | 142 ++++++++++++++++++-------- src/include/libpq/libpq-be.h | 6 ++ 2 files changed, 104 insertions(+), 44 deletions(-) diff --git a/src/backend/libpq/be-secure-openssl.c b/src/backend/libpq/be-secure-openssl.c index c8b63ef8249..c68cf438a96 100644 --- a/src/backend/libpq/be-secure-openssl.c +++ b/src/backend/libpq/be-secure-openssl.c @@ -75,8 +75,8 @@ static int alpn_cb(SSL *ssl, void *userdata); static bool initialize_dh(SSL_CTX *context, bool isServerStart); static bool initialize_ecdh(SSL_CTX *context, bool isServerStart); -static const char *SSLerrmessageExt(unsigned long ecode, const char *replacement); -static const char *SSLerrmessage(unsigned long ecode); +static char *SSLerrmessageExt(unsigned long ecode, const char *replacement); +static char *SSLerrmessage(unsigned long ecode); static char *X509_NAME_to_cstring(X509_NAME *name); @@ -87,9 +87,6 @@ static bool ssl_is_server_start; static int ssl_protocol_version_to_openssl(int v); static const char *ssl_protocol_version_to_string(int v); -/* for passing data back from verify_cb() */ -static const char *cert_errdetail; - /* ------------------------------------------------------------ */ /* Public interface */ /* ------------------------------------------------------------ */ @@ -100,6 +97,7 @@ be_tls_init(bool isServerStart) SSL_CTX *context; int ssl_ver_min = -1; int ssl_ver_max = -1; + char *errbuf = NULL; /* * Create a new SSL context into which we'll load all the configuration @@ -114,9 +112,9 @@ be_tls_init(bool isServerStart) context = SSL_CTX_new(SSLv23_method()); if (!context) { + errbuf = SSLerrmessage(ERR_get_error()); ereport(isServerStart ? FATAL : LOG, - (errmsg("could not create SSL context: %s", - SSLerrmessage(ERR_get_error())))); + (errmsg("could not create SSL context: %s", errbuf))); goto error; } @@ -139,10 +137,11 @@ be_tls_init(bool isServerStart) */ if (SSL_CTX_use_certificate_chain_file(context, ssl_cert_file) != 1) { + errbuf = SSLerrmessage(ERR_get_error()); ereport(isServerStart ? FATAL : LOG, (errcode(ERRCODE_CONFIG_FILE_ERROR), errmsg("could not load server certificate file \"%s\": %s", - ssl_cert_file, SSLerrmessage(ERR_get_error())))); + ssl_cert_file, errbuf))); goto error; } @@ -159,24 +158,29 @@ be_tls_init(bool isServerStart) SSL_FILETYPE_PEM) != 1) { if (dummy_ssl_passwd_cb_called) + { ereport(isServerStart ? FATAL : LOG, (errcode(ERRCODE_CONFIG_FILE_ERROR), errmsg("private key file \"%s\" cannot be reloaded because it requires a passphrase", ssl_key_file))); + } else + { + errbuf = SSLerrmessage(ERR_get_error()); ereport(isServerStart ? FATAL : LOG, (errcode(ERRCODE_CONFIG_FILE_ERROR), errmsg("could not load private key file \"%s\": %s", - ssl_key_file, SSLerrmessage(ERR_get_error())))); + ssl_key_file, errbuf))); + } goto error; } if (SSL_CTX_check_private_key(context) != 1) { + errbuf = SSLerrmessage(ERR_get_error()); ereport(isServerStart ? FATAL : LOG, (errcode(ERRCODE_CONFIG_FILE_ERROR), - errmsg("check of private key failed: %s", - SSLerrmessage(ERR_get_error())))); + errmsg("check of private key failed: %s", errbuf))); goto error; } @@ -326,10 +330,11 @@ be_tls_init(bool isServerStart) if (SSL_CTX_load_verify_locations(context, ssl_ca_file, NULL) != 1 || (root_cert_list = SSL_load_client_CA_file(ssl_ca_file)) == NULL) { + errbuf = SSLerrmessage(ERR_get_error()); ereport(isServerStart ? FATAL : LOG, (errcode(ERRCODE_CONFIG_FILE_ERROR), errmsg("could not load root certificate file \"%s\": %s", - ssl_ca_file, SSLerrmessage(ERR_get_error())))); + ssl_ca_file, errbuf))); goto error; } @@ -375,27 +380,29 @@ be_tls_init(bool isServerStart) } else if (ssl_crl_dir[0] == 0) { + errbuf = SSLerrmessage(ERR_get_error()); ereport(isServerStart ? FATAL : LOG, (errcode(ERRCODE_CONFIG_FILE_ERROR), errmsg("could not load SSL certificate revocation list file \"%s\": %s", - ssl_crl_file, SSLerrmessage(ERR_get_error())))); + ssl_crl_file, errbuf))); goto error; } else if (ssl_crl_file[0] == 0) { + errbuf = SSLerrmessage(ERR_get_error()); ereport(isServerStart ? FATAL : LOG, (errcode(ERRCODE_CONFIG_FILE_ERROR), errmsg("could not load SSL certificate revocation list directory \"%s\": %s", - ssl_crl_dir, SSLerrmessage(ERR_get_error())))); + ssl_crl_dir, errbuf))); goto error; } else { + errbuf = SSLerrmessage(ERR_get_error()); ereport(isServerStart ? FATAL : LOG, (errcode(ERRCODE_CONFIG_FILE_ERROR), errmsg("could not load SSL certificate revocation list file \"%s\" or directory \"%s\": %s", - ssl_crl_file, ssl_crl_dir, - SSLerrmessage(ERR_get_error())))); + ssl_crl_file, ssl_crl_dir, errbuf))); goto error; } } @@ -419,8 +426,10 @@ be_tls_init(bool isServerStart) return 0; - /* Clean up by releasing working context. */ + /* Clean up by releasing working context and potential error message */ error: + if (errbuf) + pfree(errbuf); if (context) SSL_CTX_free(context); return -1; @@ -461,6 +470,10 @@ be_tls_open_server(Port *port) /* enable ALPN */ SSL_CTX_set_alpn_select_cb(SSL_context, alpn_cb, port); + /* + * This, and later, calls to SSLerrmessage intentionally leak the palloced + * errormessage in the errorpaths as they will lead to a proc_exit. + */ if (!(port->ssl = SSL_new(SSL_context))) { ereport(COMMERROR, @@ -477,6 +490,14 @@ be_tls_open_server(Port *port) SSLerrmessage(ERR_get_error())))); return -1; } + + /* + * This is a bit of a circle motion, but we want to be able to access the + * Port object in callbacks for passing data from callback to the main + * process. + */ + SSL_set_ex_data(port->ssl, 0, port); + port->ssl_in_use = true; aloop: @@ -576,7 +597,7 @@ aloop: (errcode(ERRCODE_PROTOCOL_VIOLATION), errmsg("could not accept SSL connection: %s", SSLerrmessage(ecode)), - cert_errdetail ? errdetail_internal("%s", cert_errdetail) : 0, + port->cert_errdetail ? errdetail_internal("%s", port->cert_errdetail) : 0, give_proto_hint ? errhint("This may indicate that the client does not support any SSL protocol version between %s and %s.", ssl_min_protocol_version ? @@ -585,7 +606,6 @@ aloop: ssl_max_protocol_version ? ssl_protocol_version_to_string(ssl_max_protocol_version) : MAX_OPENSSL_TLS_VERSION) : 0)); - cert_errdetail = NULL; break; case SSL_ERROR_ZERO_RETURN: ereport(COMMERROR, @@ -767,6 +787,7 @@ be_tls_read(Port *port, void *ptr, size_t len, int *waitfor) ssize_t n; int err; unsigned long ecode; + char *errbuf; errno = 0; ERR_clear_error(); @@ -797,9 +818,11 @@ be_tls_read(Port *port, void *ptr, size_t len, int *waitfor) } break; case SSL_ERROR_SSL: + errbuf = SSLerrmessage(ecode); ereport(COMMERROR, (errcode(ERRCODE_PROTOCOL_VIOLATION), - errmsg("SSL error: %s", SSLerrmessage(ecode)))); + errmsg("SSL error: %s", errbuf))); + pfree(errbuf); errno = ECONNRESET; n = -1; break; @@ -826,6 +849,7 @@ be_tls_write(Port *port, const void *ptr, size_t len, int *waitfor) ssize_t n; int err; unsigned long ecode; + char *errbuf; errno = 0; ERR_clear_error(); @@ -862,9 +886,11 @@ be_tls_write(Port *port, const void *ptr, size_t len, int *waitfor) } break; case SSL_ERROR_SSL: + errbuf = SSLerrmessage(ecode); ereport(COMMERROR, (errcode(ERRCODE_PROTOCOL_VIOLATION), - errmsg("SSL error: %s", SSLerrmessage(ecode)))); + errmsg("SSL error: %s", errbuf))); + pfree(errbuf); errno = ECONNRESET; n = -1; break; @@ -1041,6 +1067,7 @@ load_dh_file(char *filename, bool isServerStart) FILE *fp; DH *dh = NULL; int codes; + char *errbuf = NULL; /* attempt to open file. It's not an error if it doesn't exist. */ if ((fp = AllocateFile(filename, "r")) == NULL) @@ -1057,30 +1084,28 @@ load_dh_file(char *filename, bool isServerStart) if (dh == NULL) { + errbuf = SSLerrmessage(ERR_get_error()); ereport(isServerStart ? FATAL : LOG, (errcode(ERRCODE_CONFIG_FILE_ERROR), - errmsg("could not load DH parameters file: %s", - SSLerrmessage(ERR_get_error())))); - return NULL; + errmsg("could not load DH parameters file: %s", errbuf))); + goto error; } /* make sure the DH parameters are usable */ if (DH_check(dh, &codes) == 0) { + errbuf = SSLerrmessage(ERR_get_error()); ereport(isServerStart ? FATAL : LOG, (errcode(ERRCODE_CONFIG_FILE_ERROR), - errmsg("invalid DH parameters: %s", - SSLerrmessage(ERR_get_error())))); - DH_free(dh); - return NULL; + errmsg("invalid DH parameters: %s", errbuf))); + goto error; } if (codes & DH_CHECK_P_NOT_PRIME) { ereport(isServerStart ? FATAL : LOG, (errcode(ERRCODE_CONFIG_FILE_ERROR), errmsg("invalid DH parameters: p is not prime"))); - DH_free(dh); - return NULL; + goto error; } if ((codes & DH_NOT_SUITABLE_GENERATOR) && (codes & DH_CHECK_P_NOT_SAFE_PRIME)) @@ -1088,11 +1113,16 @@ load_dh_file(char *filename, bool isServerStart) ereport(isServerStart ? FATAL : LOG, (errcode(ERRCODE_CONFIG_FILE_ERROR), errmsg("invalid DH parameters: neither suitable generator or safe prime"))); - DH_free(dh); - return NULL; + goto error; } return dh; +error: + if (errbuf) + pfree(errbuf); + if (dh) + DH_free(dh); + return NULL; } /* @@ -1107,15 +1137,19 @@ load_dh_buffer(const char *buffer, size_t len) { BIO *bio; DH *dh = NULL; + char *errbuf; bio = BIO_new_mem_buf(buffer, len); if (bio == NULL) return NULL; dh = PEM_read_bio_DHparams(bio, NULL, NULL, NULL); if (dh == NULL) + { + errbuf = SSLerrmessage(ERR_get_error()); ereport(DEBUG2, - (errmsg_internal("DH load buffer: %s", - SSLerrmessage(ERR_get_error())))); + (errmsg_internal("DH load buffer: %s", errbuf))); + pfree(errbuf); + } BIO_free(bio); return dh; @@ -1209,6 +1243,8 @@ verify_cb(int ok, X509_STORE_CTX *ctx) const char *errstring; StringInfoData str; X509 *cert; + SSL *ssl; + Port *port; if (ok) { @@ -1221,6 +1257,13 @@ verify_cb(int ok, X509_STORE_CTX *ctx) errcode = X509_STORE_CTX_get_error(ctx); errstring = X509_verify_cert_error_string(errcode); + /* + * Extract the current SSL and Port object to use for passing error detail + * back from the callback. + */ + ssl = X509_STORE_CTX_get_ex_data(ctx, SSL_get_ex_data_X509_STORE_CTX_idx()); + port = (Port *) SSL_get_ex_data(ssl, 0); + initStringInfo(&str); appendStringInfo(&str, _("Client certificate verification failed at depth %d: %s."), @@ -1271,7 +1314,7 @@ verify_cb(int ok, X509_STORE_CTX *ctx) } /* Store our detail message to be logged later. */ - cert_errdetail = str.data; + port->cert_errdetail = str.data; return ok; } @@ -1387,6 +1430,7 @@ static bool initialize_dh(SSL_CTX *context, bool isServerStart) { DH *dh = NULL; + char *errbuf; SSL_CTX_set_options(context, SSL_OP_SINGLE_DH_USE); @@ -1404,10 +1448,11 @@ initialize_dh(SSL_CTX *context, bool isServerStart) if (SSL_CTX_set_tmp_dh(context, dh) != 1) { + errbuf = SSLerrmessage(ERR_get_error()); ereport(isServerStart ? FATAL : LOG, (errcode(ERRCODE_CONFIG_FILE_ERROR), - errmsg("DH: could not set DH parameters: %s", - SSLerrmessage(ERR_get_error())))); + errmsg("DH: could not set DH parameters: %s", errbuf))); + pfree(errbuf); DH_free(dh); return false; } @@ -1459,12 +1504,14 @@ initialize_ecdh(SSL_CTX *context, bool isServerStart) * function can be used to ensure that a proper error message is displayed for * versions reporting no error, while using the OpenSSL error via SSLerrmessage * for versions where there is one. + * + * The caller is responsible for freeing the returned string. */ -static const char * +static char * SSLerrmessageExt(unsigned long ecode, const char *replacement) { if (ecode == 0) - return replacement; + return pstrdup(replacement); else return SSLerrmessage(ecode); } @@ -1477,18 +1524,22 @@ SSLerrmessageExt(unsigned long ecode, const char *replacement) * Some caution is needed here since ERR_reason_error_string will return NULL * if it doesn't recognize the error code, or (in OpenSSL >= 3) if the code * represents a system errno value. We don't want to return NULL ever. + * + * The caller is responsible for freeing the returned string. */ -static const char * +static char * SSLerrmessage(unsigned long ecode) { const char *errreason; - static char errbuf[36]; + char *errbuf; if (ecode == 0) - return _("no SSL error reported"); + return pstrdup(_("no SSL error reported")); errreason = ERR_reason_error_string(ecode); if (errreason != NULL) - return errreason; + return pstrdup(errreason); + + errbuf = palloc(PG_STRERROR_R_BUFLEN); /* * In OpenSSL 3.0.0 and later, ERR_reason_error_string does not map system @@ -1499,7 +1550,10 @@ SSLerrmessage(unsigned long ecode) */ #ifdef ERR_SYSTEM_ERROR if (ERR_SYSTEM_ERROR(ecode)) - return strerror(ERR_GET_REASON(ecode)); + { + strerror_r(ERR_GET_REASON(ecode), errbuf, sizeof(errbuf)); + return errbuf; + } #endif /* No choice but to report the numeric ecode */ diff --git a/src/include/libpq/libpq-be.h b/src/include/libpq/libpq-be.h index d6e671a6382..2d25cd3aaa5 100644 --- a/src/include/libpq/libpq-be.h +++ b/src/include/libpq/libpq-be.h @@ -159,6 +159,12 @@ typedef struct Port */ char *application_name; + /* + * Storage for passing certificate verification error logging from the + * callback. + */ + char *cert_errdetail; + /* * Information that needs to be held during the authentication cycle. */ -- 2.39.3 (Apple Git-146)