Thanks for the very thorough comments! I've attached a new version of the
patch.
On Thu, Feb 15, 2024 at 4:17 PM Andres Freund <and...@anarazel.de> wrote:
> Hi,
>
> On 2024-02-11 13:19:00 -0500, David Benjamin wrote:
> > I've attached a patch for the master branch to fix up the custom BIOs
> used
> > by PostgreSQL, in light of the issues with the OpenSSL update recently.
> > While c82207a548db47623a2bfa2447babdaa630302b9 (switching from
> BIO_get_data
> > to BIO_get_app_data) resolved the immediate conflict, I don't think it
> > addressed the root cause, which is that PostgreSQL was mixing up two BIO
> > implementations, each of which expected to be driving the BIO.
>
> Yea, that's certainly not nice - and I think we've been somewhat lucky it
> hasn't caused more issues. There's some nasty possibilities, e.g.
> sock_ctrl()
> partially enabling ktls without our initialization having called
> ktls_enable(). Right now that just means ktls is unusable, but it's not
> hard
> to imagine accidentally ending up sending unencrypted data.
>
Yeah. Even if, say, the ktls bits work, given you all care enough about I/O
to have wanted to wrap the BIO, I assume you'd want to pick up those
features on your own terms, e.g. by implementing the BIO_CTRLs yourself.
> I've in the past looked into not using a custom BIO [1], but I have my
> doubts
> about that being a good idea. I think medium term we want to be able to do
> network IO asynchronously, which seems quite hard to do when using
> openssl's
> socket BIO.
> 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.
>
> At first I was a bit wary of that, because it requires us to bring back the
> fallback implementation. But you're right, it's noticeably heavier than
> BIO_get_data(), and we do call BIO_get_app_data() fairly frequently.
>
TBH, I doubt it makes any real difference perf-wise. But I think
BIO_get_data is a bit more common in this context.
> > 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.
>
> How did you determine that only FLUSH is required? I didn't even really
> find
> documentation about what the intended semantics actually are.
>
The unhelpful answer is that my day job is working on BoringSSL, so I've
spent a lot of time with this mess. :-) But, yeah, it's not well-documented
at all. OpenSSL ends up calling BIO_flush at the end of each batch of
writes in libssl. TBH, I suspect that was less intentional and more an
emergent property of them internally layering a buffer BIO at one point in
the process, but it's long been part of the (undocumented) API contract.
Conversely, I don't think OpenSSL can possibly make libssl *require* a new
BIO_CTRL because they'd break every custom BIO anyone has ever used with
the library.
> E.g. should we implement BIO_CTRL_EOF? Sure, it wasn't really supported so
> far, because we never set it, but is that right?
Ah hmm, BIO_CTRL_EOF is... a bit of a mess. OpenSSL kind of messed things
up. So, up until recently, I would have said that BIO_CTRL_EOF was not part
of the interface here. OpenSSL 1.0.x did not implement it for sockets, and
the BIO_read API *already* had a way to signal EOF: did you return zero or
-1?
Then OpenSSL 1.1.x introduced size_t-based BIO_read_ex APIs. However, in
the process, they *forgot that EOF and error are different things* and made
it impossible to detect EOFs if you use BIO_read_ex! They never noticed
this, because they didn't actually convert their own code to their new API.
See this discussion, which alas ended with OpenSSL deciding to ignore the
problem and not even document their current interface.
https://github.com/openssl/openssl/issues/8208
Though they never responded, they seem to have tacitly settled using the
out-of-band BIO_eof function (which is BIO_CTRL_EOF) as the way to signal
EOF for BIO_read_ex. This is kind of fiddly, but is at least a well-defined
option. But the problem is no one's BIO_METHODs, including their own, are
read_ex-based. They all implement the old read callback. But someone might
call BIO_read_ex on a read-based BIO_METHOD. IMO, BIO_read_ex should be
responsible for translating between the two EOF conventions. For example,
it could automatically set a flag when the read callback returns 0 and then
make BIO_ctrl check the flag and automatically implement BIO_CTRL_EOF for
BIOs that don't do it themselves. Alas, OpenSSL did not do this and instead
they just made their built-in BIOs implement BIO_CTRL_EOF, even though this
new expectation is a compatibility break. That leaves BIO_read, the one
everyone uses, in a pretty ambiguous state that they seem uninterested in
addressing.
https://github.com/openssl/openssl/pull/10882
https://github.com/openssl/openssl/pull/10907
Honestly, I suspect nothing in postgres actually cares about this, given
you all didn't notice things breaking around here in the early days of
1.1.x. Still, that is a good point. I've updated the patch to implement
BIO_CTRL_EOF for completeness' sake.
(For BoringSSL, we did not go down this route because, unlike OpenSSL
apparently, we do not think breaking backwards compatibility in the EOF
convention is OK. I do want to add the size_t APIs eventually, but I'm
thinking we'll do the automatic BIO_CTRL_EOF thing described above, to
avoid breaking compatibility with existing BIO_METHODs.)
Beyond BIO_CTRL_EOF, I assume what you're really asking here is "how do we
know OpenSSL won't change the interface again?". And, honestly, we don't.
They promise API and ABI stability, which in theory means they shouldn't.
But their track record with custom BIOs is not great. That said, if they do
break it, I think it will unambiguously be an API break on their part and
hopefully it'll be possible to get them to fix the issue. The EOF issue
snuck in because it mostly only impacted people who tried to migrate to
their new APIs. It broke existing APIs too, but the one project that
noticed (CPython) misdiagnosed the issue and worked around it.
(Incorrectly, but no one noticed. See
https://github.com/python/cpython/pull/95495.) So, I raised the issue,
they'd long since shipped it and evidently felt no burning need to fix
their regression or unclear APIs. :-(
But the alternative, picking up the real socket BIO's ctrl function, is
even more unsound, so I think this is the better place to be.
> What about
> BIO_CTRL_GET_CLOSE/BIO_CTRL_SET_CLOSE?
>
The close flag is pretty silly. All BIOs do slightly different things with
it, which means that libssl cannot possibly call it generically. So if your
BIO doesn't have any need for it, you don't need to bother. It's usually
used to indicate whether the BIO owns the underlying fd/socket/BUF_MEM/etc,
> Another issue is that 0 doesn't actually seem like the universal error
> return
> - e.g. BIO_C_GET_FD seems to return -1, because 0 is a valid fd.
>
Yeah, that is... also a mess. All of OpenSSL's ctrl functions return zero
for unrecognized commands. It seems they just don't have a convention for
distinguishing unimplemented commands from zero returns. As you note, this
is a problem for BIO_C_GET_FD. OpenSSL's other non-fd BIOs have the same
problem though:
https://github.com/openssl/openssl/blob/master/crypto/bio/bss_file.c#L340
Realistically, type-specific ioctls like that don't matter much because
you're really only going to call them if you already know you have an
applicable BIO. Hopefully, between that, and removing BIO_TYPE_DESCRIPTOR
(below), it's mostly okay? Also happy to add a `ret = -1` implementation
of BIO_C_GET_FD if you'd prefer. But, in the general case, I suspect we
just have to live with the zero thing, and hopefully OpenSSL will stop
introducing ctrls where zero is an unsafe return value.
Perhaps BIO_ctrl in OpenSSL should have some logic like...
```
if (!(type & BIO_TYPE_DESCRIPTOR) && cmd == BIO_C_GET_FD) {
return -1;
}
```
Although I suspect there are people who implement BIO_C_GET_FD without
setting BIO_TYPE_DESCRIPTOR because none of this was ever documented.
> As of your patch the bio doesn't actually have an FD anymore, should it
> still
> set BIO_TYPE_DESCRIPTOR?
>
Ah good call. Done. I don't think much code really cares, but it does mean
that, if you all call SSL_get_fd (why would you?), it won't get confused.
:-)
> +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;
> > +}
>
> I'd move the res = 0 into a default: block. That way the compiler can warn
> if
> some case doesn't set it in all branches.
>
Done.
> > static BIO_METHOD *
> > my_BIO_s_socket(void)
> > {
>
> Wonder if we should rename this. It's pretty odd that we still call it's
> not
> really related to s_socket anymore, and doesn't even really implement the
> same
> interface (e.g. get_fd doesn't work anymore). Similarly, my_SSL_set_fd()
> doesn't actually call set_fd() anymore, which sure seems odd.
>
Done. I wasn't sure what to name them, or even what the naming convention
was, but I opted to name them after the Port and PGconn objects they wrap.
Happy to switch to another name if you have a better idea though.
David
From e29b816dfb7ee5ed9f3389d7f06408f83783258a Mon Sep 17 00:00:00 2001
From: David Benjamin <davidben@google.com>
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.
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.
---
configure | 2 +-
configure.ac | 2 +-
meson.build | 1 +
src/backend/libpq/be-secure-openssl.c | 106 +++++++++++++++--------
src/include/libpq/libpq-be.h | 1 +
src/include/pg_config.h.in | 3 +
src/interfaces/libpq/fe-secure-openssl.c | 96 +++++++++++++-------
src/interfaces/libpq/libpq-int.h | 1 +
8 files changed, 141 insertions(+), 71 deletions(-)
diff --git a/configure b/configure
index 6b87e5c9a8..cacda708b0 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 6e64ece11d..083bbabfbf 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..05cfb8c5d3 100644
--- a/src/backend/libpq/be-secure-openssl.c
+++ b/src/backend/libpq/be-secure-openssl.c
@@ -56,10 +56,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);
@@ -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 (!ssl_set_port_bio(port))
{
ereport(COMMERROR,
(errcode(ERRCODE_PROTOCOL_VIOLATION),
@@ -849,17 +849,24 @@ 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;
+#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 *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 */
@@ -874,11 +881,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)
{
@@ -892,56 +899,81 @@ 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();
#ifdef HAVE_BIO_METH_NEW
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;
}
#else
- my_bio_methods = malloc(sizeof(BIO_METHOD));
- if (!my_bio_methods)
+ port_bio_method_ptr = malloc(sizeof(BIO_METHOD));
+ if (!port_bio_method_ptr)
return NULL;
- memcpy(my_bio_methods, biom, sizeof(BIO_METHOD));
- my_bio_methods->bread = my_sock_read;
- my_bio_methods->bwrite = my_sock_write;
+ memset(port_bio_method_ptr, 0, sizeof(BIO_METHOD));
+ port_bio_method_ptr->type = BIO_TYPE_SOURCE_SINK;
+ port_bio_method_ptr->name = "libpq socket";
+ port_bio_method_ptr->bread = port_bio_read;
+ port_bio_method_ptr->bwrite = port_bio_write;
+ port_bio_method_ptr->ctrl = port_bio_ctrl;
#endif
}
- 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);
@@ -954,9 +986,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 47d66d5524..014c067e81 100644
--- a/src/include/libpq/libpq-be.h
+++ b/src/include/libpq/libpq-be.h
@@ -218,6 +218,7 @@ typedef struct Port
char *peer_cn;
char *peer_dn;
bool peer_cert_valid;
+ bool last_read_was_eof;
/*
* OpenSSL structures. (Keep these last so that the locations of other
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..cadb5b32f4 100644
--- a/src/interfaces/libpq/fe-secure-openssl.c
+++ b/src/interfaces/libpq/fe-secure-openssl.c
@@ -78,10 +78,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 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))
+ !ssl_set_pgconn_bio(conn))
{
char *err = SSLerrmessage(ERR_get_error());
@@ -1802,16 +1802,23 @@ 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;
+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)
{
int res;
+ PGconn *conn = (PGconn *) BIO_get_data(h);
- res = pqsecure_raw_read((PGconn *) BIO_get_app_data(h), buf, size);
+ 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 */
@@ -1836,11 +1843,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)
{
@@ -1865,26 +1872,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();
#ifdef HAVE_BIO_METH_NEW
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;
@@ -1893,14 +1928,9 @@ 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;
}
@@ -1908,13 +1938,16 @@ my_BIO_s_socket(void)
res = malloc(sizeof(BIO_METHOD));
if (!res)
goto err;
- memcpy(res, biom, sizeof(BIO_METHOD));
- res->bread = my_sock_read;
- res->bwrite = my_sock_write;
+ memset(res, 0, sizeof(BIO_METHOD));
+ res->type = BIO_TYPE_SOURCE_SINK;
+ res->name = "libpq socket";
+ res->bread = pgconn_bio_read;
+ res->bwrite = pgconn_bio_write;
+ res->ctrl = pgconn_bio_ctrl;
#endif
}
- my_bio_methods = res;
+ pgconn_bio_method_ptr = res;
pthread_mutex_unlock(&ssl_config_mutex);
return res;
@@ -1930,15 +1963,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);
@@ -1950,10 +1982,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 82c18f870d..a6c36073be 100644
--- a/src/interfaces/libpq/libpq-int.h
+++ b/src/interfaces/libpq/libpq-int.h
@@ -547,6 +547,7 @@ struct pg_conn
bool ssl_in_use;
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
bool allow_ssl_try; /* Allowed to try SSL negotiation */
--
2.44.0.rc0.258.g7320e95886-goog
