mirrors/postgres
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Avoid mixing custom and OpenSSL BIO functions

Browse Source 
PostgreSQL has for a long time mixed two BIO implementations, which can
lead to subtle bugs and inconsistencies. This cleans up our BIO by just
just setting up the methods we need. This patch does not introduce any
functionality changes.

The following methods are no longer defined due to not being needed:

  - gets: Not used by libssl
  - puts: Not used by libssl
  - create: Sets up state not used by libpq
  - destroy: Not used since libpq use BIO_NOCLOSE, if it was used it close
             the socket from underneath libpq
  - callback_ctrl: Not implemented by sockets

The following methods are defined for our BIO:

  - read: Used for reading arbitrary length data from the BIO. No change
          in functionality from the previous implementation.
  - write: Used for writing arbitrary length data to the BIO. No change
           in functionality from the previous implementation.
  - ctrl: Used for processing ctrl messages in the BIO (similar to ioctl).
          The only ctrl message which matters is BIO_CTRL_FLUSH used for
          writing out buffered data (or signal EOF and that no more data
          will be written). BIO_CTRL_FLUSH is mandatory to implement and
          is implemented as a no-op since there is no intermediate buffer
          to flush.
          BIO_CTRL_EOF is the out-of-band method for signalling EOF to
          read_ex based BIO's. Our BIO is not read_ex based but someone
          could accidentally call BIO_CTRL_EOF on us so implement mainly
          for completeness sake.

As the implementation is no longer related to BIO_s_socket or calling
SSL_set_fd, methods have been renamed to reference the PGconn and Port
types instead.

This also reverts back to using BIO_set_data, with our fallback, as a small
optimization as BIO_set_app_data require the ex_data mechanism in OpenSSL.

Author: David Benjamin <davidben@google.com>
Reviewed-by: Andres Freund <andres@anarazel.de>
Reviewed-by: Daniel Gustafsson <daniel@yesql.se>
Discussion: https://postgr.es/m/CAF8qwaCZ97AZWXtg_y359SpOHe+HdJ+p0poLCpJYSUxL-8Eo8A@mail.gmail.com
This commit is contained in:
Daniel Gustafsson 2024-10-11 21:58:58 +02:00
parent 4e1fad3787
commit 6f782a2a17
4 changed files with 123 additions and 87 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

109
src/backend/libpq/be-secure-openssl.c
View File

@ -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,66 +935,81 @@ my_sock_write(BIO *h, const char *buf, int size)
return res;
}
static BIO_METHOD *
my_BIO_s_socket(void)
static long
port_bio_ctrl(BIO *h, int cmd, long num, void *ptr)
{
if (!my_bio_methods)
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 *
port_bio_method(void)
{
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_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);
goto err;
}
return 0;
bio = BIO_new(bio_method);
if (bio == NULL)
{
SSLerr(SSL_F_SSL_SET_FD, ERR_R_BUF_LIB);
goto err;
}
BIO_set_app_data(bio, port);
return 0;
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:
return ret;
return 1;
}
/*

1
src/include/libpq/libpq-be.h
View File

@ -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

99
src/interfaces/libpq/fe-secure-openssl.c
View File

@ -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,54 @@ 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 +1793,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,33 +1812,25 @@ 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);
goto err;
}
return 0;
bio = BIO_new(bio_method);
if (bio == NULL)
{
SSLerr(SSL_F_SSL_SET_FD, ERR_R_BUF_LIB);
goto err;
}
BIO_set_app_data(bio, conn);
return 0;
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;
return 1;
}
/*

1
src/interfaces/libpq/libpq-int.h
View File

@ -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