From 7257bd0c838240768cfcb021179d50976c845d05 Mon Sep 17 00:00:00 2001 From: Daniel Gustafsson Date: Mon, 3 Mar 2025 16:17:14 +0100 Subject: [PATCH v8 2/2] Review fixups --- configure.ac | 2 +- doc/src/sgml/installation.sgml | 2 +- doc/src/sgml/libpq.sgml | 19 +++++++--- meson.build | 1 + src/include/pg_config.h.in | 3 ++ src/interfaces/libpq/fe-secure-openssl.c | 46 ++++++++++++++++-------- src/test/ssl/t/001_ssltests.pl | 35 +++++++++--------- 7 files changed, 68 insertions(+), 40 deletions(-) diff --git a/configure.ac b/configure.ac index b6d02f5ecc7..c6f61f7e3a8 100644 --- a/configure.ac +++ b/configure.ac @@ -1370,7 +1370,7 @@ if test "$with_ssl" = openssl ; then # Function introduced in OpenSSL 1.0.2, not in LibreSSL. AC_CHECK_FUNCS([SSL_CTX_set_cert_cb]) # Function introduced in OpenSSL 1.1.1, not in LibreSSL. - AC_CHECK_FUNCS([X509_get_signature_info SSL_CTX_set_num_tickets]) + AC_CHECK_FUNCS([X509_get_signature_info SSL_CTX_set_num_tickets SSL_CTX_set_keylog_callback]) AC_DEFINE([USE_OPENSSL], 1, [Define to 1 to build with OpenSSL support. (--with-ssl=openssl)]) elif test "$with_ssl" != no ; then AC_MSG_ERROR([--with-ssl must specify openssl]) diff --git a/doc/src/sgml/installation.sgml b/doc/src/sgml/installation.sgml index 39fd16651b9..e076cefa3b9 100644 --- a/doc/src/sgml/installation.sgml +++ b/doc/src/sgml/installation.sgml @@ -301,7 +301,7 @@ Additionally, LibreSSL is supported using the OpenSSL compatibility layer. The minimum required version is 3.4 (from OpenBSD - version 7.5). + version 7.0). diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml index 31b711004aa..cea8acd3ccf 100644 --- a/doc/src/sgml/libpq.sgml +++ b/doc/src/sgml/libpq.sgml @@ -1922,10 +1922,21 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname sslkeylogfile - This parameter specifies the location where libpq will log keys - used in this SSL context. This is useful for debugging postgres - protocol using tools like wireshark. This parameter is ignored if an - SSL connection is not made. Keys are logged in the NSS format. + This parameter specifies the location where libpq + will log keys used in this SSL context. This is useful for debugging + PostgreSQL protocol interations or client + connections using network inspection tools like + Wireshark. This parameter is ignored if an + SSL connection is not made, or if LibreSSL + is used. Keys are logged in the NSS format. + + + + Key logging will expose potentially sensitive information in the + keylog file and it should be handled with the same caution as + files. + + diff --git a/meson.build b/meson.build index 13c13748e5d..07c51e7db58 100644 --- a/meson.build +++ b/meson.build @@ -1472,6 +1472,7 @@ if sslopt in ['auto', 'openssl'] # Function introduced in OpenSSL 1.1.1, not in LibreSSL. ['X509_get_signature_info'], ['SSL_CTX_set_num_tickets'], + ['SSL_CTX_set_keylog_callback'], ] are_openssl_funcs_complete = true diff --git a/src/include/pg_config.h.in b/src/include/pg_config.h.in index db6454090d2..64351a3adaa 100644 --- a/src/include/pg_config.h.in +++ b/src/include/pg_config.h.in @@ -361,6 +361,9 @@ /* Define to 1 if you have the `SSL_CTX_set_ciphersuites' function. */ #undef HAVE_SSL_CTX_SET_CIPHERSUITES +/* Define to 1 if you have the `SSL_CTX_set_keylog_callback' function. */ +#undef HAVE_SSL_CTX_SET_KEYLOG_CALLBACK + /* Define to 1 if you have the `SSL_CTX_set_num_tickets' function. */ #undef HAVE_SSL_CTX_SET_NUM_TICKETS diff --git a/src/interfaces/libpq/fe-secure-openssl.c b/src/interfaces/libpq/fe-secure-openssl.c index b0ca09b6700..fb6c99f4200 100644 --- a/src/interfaces/libpq/fe-secure-openssl.c +++ b/src/interfaces/libpq/fe-secure-openssl.c @@ -87,7 +87,6 @@ static pthread_mutex_t ssl_config_mutex = PTHREAD_MUTEX_INITIALIZER; static PQsslKeyPassHook_OpenSSL_type PQsslKeyPassHook = NULL; static int ssl_protocol_version_to_openssl(const char *protocol); -static void SSL_CTX_keylog_cb(const SSL *ssl, const char *line); /* ------------------------------------------------------------ */ /* Procedures common to all secure sessions */ @@ -686,12 +685,22 @@ pgtls_verify_peer_name_matches_certificate_guts(PGconn *conn, /* See pqcomm.h comments on OpenSSL implementation of ALPN (RFC 7301) */ static unsigned char alpn_protos[] = PG_ALPN_PROTOCOL_VECTOR; -/* This is a callback that writes to a given ssl key log file */ -static void SSL_CTX_keylog_cb(const SSL *ssl, const char *line) { - int fd; - mode_t old_umask; - ssize_t bytes_written; - PGconn *conn = SSL_get_app_data(ssl); +#ifdef HAVE_SSL_CTX_SET_KEYLOG_CALLBACK +/* + * SSL Key Logging callback + * + * This callback lets the user store all key material to a file for debugging + * purposes. The file will be written using the NSS keylog format. LibreSSL + * 3.5 introduce stub function to set the callback for OpenSSL compatibility, + * but the callback is never invoked. + */ +static void +SSL_CTX_keylog_cb(const SSL *ssl, const char *line) +{ + int fd; + mode_t old_umask; + ssize_t rc; + PGconn *conn = SSL_get_app_data(ssl); if (conn == NULL) return; @@ -700,19 +709,24 @@ static void SSL_CTX_keylog_cb(const SSL *ssl, const char *line) { fd = open(conn->sslkeylogfile, O_WRONLY | O_APPEND | O_CREAT, 0600); umask(old_umask); - if (fd == -1) { - libpq_append_conn_error(conn, "could not open ssl key log file %s: %s", conn->sslkeylogfile, pg_strerror(errno)); + if (fd == -1) + { + libpq_append_conn_error(conn, "could not open ssl key log file %s: %s", + conn->sslkeylogfile, pg_strerror(errno)); return; } - bytes_written = dprintf(fd, "%s\n", line); - if (bytes_written < 0) { - libpq_append_conn_error(conn, "could not write to ssl key log file %s: %s", conn->sslkeylogfile, pg_strerror(errno)); - close(fd); - return; - } + /* line is guaranteed by OpenSSL to be NUL terminated */ + rc = write(fd, line, strlen(line)); + if (rc < 0) + libpq_append_conn_error(conn, "could not write to ssl key log file %s: %s", + conn->sslkeylogfile, pg_strerror(errno)); + else + rc = write(fd, "\n", 1); + (void) rc; /* silence compiler warnings */ close(fd); } +#endif /* * Create per-connection SSL object, and load the client certificate, @@ -1030,8 +1044,10 @@ initialize_SSL(PGconn *conn) } conn->ssl_in_use = true; +#ifdef HAVE_SSL_CTX_SET_KEYLOG_CALLBACK if (conn->sslkeylogfile && strlen(conn->sslkeylogfile) > 0) SSL_CTX_set_keylog_callback(SSL_context, SSL_CTX_keylog_cb); +#endif /* * SSL contexts are reference counted by OpenSSL. We can free it as soon diff --git a/src/test/ssl/t/001_ssltests.pl b/src/test/ssl/t/001_ssltests.pl index ecf9bf7dd93..b1fc40b9e05 100644 --- a/src/test/ssl/t/001_ssltests.pl +++ b/src/test/ssl/t/001_ssltests.pl @@ -147,30 +147,27 @@ my $default_ssl_connstr = $common_connstr = "$default_ssl_connstr user=ssltestuser dbname=trustdb hostaddr=$SERVERHOSTADDR host=common-name.pg-ssltest.test"; -my $tempdir = PostgreSQL::Test::Utils::tempdir; - -# Connect should work with a given sslkeylogfile -$node->connect_ok( - "$common_connstr sslrootcert=ssl/root+server_ca.crt sslkeylogfile=$tempdir/key.txt sslmode=require", - "connect with server root cert and sslkeylogfile=$tempdir/key.txt"); - -# Verify the key file exists -ok(-f "$tempdir/key.txt", "key log file exists"); - -# Skip permission checks on Windows/Cygwin SKIP: { - skip "Permissions check not enforced on Windows", 1 - if ($windows_os || $Config::Config{osname} eq 'cygwin'); + skip "Keylogging is not supported with LibreSSL", 5 if $libressl; + + my $tempdir = PostgreSQL::Test::Utils::tempdir; + my @status; + + # Connect should work with a given sslkeylogfile + $node->connect_ok( + "$common_connstr sslrootcert=ssl/root+server_ca.crt sslkeylogfile=$tempdir/key.txt sslmode=require", + "connect with server root cert and sslkeylogfile=$tempdir/key.txt"); - my $mode = (stat("$tempdir/key.txt"))[2]; - my $permissions_ok = 1; + # Verify the key file exists + ok(-f "$tempdir/key.txt", "key log file exists"); - if ($mode & 0006) { - $permissions_ok = 0; - } + # Skip permission checks on Windows/Cygwin + skip "Permissions check not enforced on Windows", 2 + if ($windows_os || $Config::Config{osname} eq 'cygwin'); - ok($permissions_ok, "key log file is not world readble"); + ok((@status = stat("$tempdir/key.txt")), "keylog file exists and returned status"); + ok(@status && !($status[2] & 0006), "key log file is not world readable"); } # The server should not accept non-SSL connections. -- 2.39.3 (Apple Git-146)