Re: [PATCH] Avoid mixing custom and OpenSSL BIO functions

On Wed, Sep 4, 2024 at 9:22 AM Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:

> > On 3 Sep 2024, at 14:18, Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:
>
> > Attached is a v4 rebase over the recent OpenSSL 1.0.2 removal which made
> this
> > patch no longer apply. I've just started to dig into it so have no
> comments on
> > it right now, but wanted to get a cleaned up version into the CFBot.
>
> CFBot building green for this, I just have a few small questions/comments:
>
> + my_bio_index |= BIO_TYPE_SOURCE_SINK;
>
> According to the OpenSSL docs we should set BIO_TYPE_DESCRIPTOR as well as
> this
> BIO is socket based, but it's not entirely clear to me why. Is there a
> specific reason it was removed?
>

Looking around at what uses it, it seems BIO_TYPE_DESCRIPTOR is how OpenSSL
decides whether the BIO is expected to respond to BIO_get_fd
(BIO_C_GET_FD). Since the custom BIO is not directly backed by an fd and
doesn't implement that control, I think we don't want to include that bit.
https://github.com/openssl/openssl/blob/openssl-3.3.2/ssl/ssl_lib.c#L1621-L1643

The other place I saw that cares about this bit is this debug callback.
That one's kinda amusing because it assumes every fd-backed BIO stores its
fd in bio->num, but bio->num is not even accessible to external BIOs. I
assume this is an oversight because no one cares about this function.
Perhaps that should be sampled from BIO_get_fd.
https://github.com/openssl/openssl/blob/openssl-3.3.2/crypto/bio/bio_cb.c#L45-L62

Practically speaking, though, I don't think it makes any difference whether
BIO_TYPE_DESCRIPTOR or even BIO_TYPE_SOURCE_SINK is set or unset. I
couldn't find any code that's sensitive to BIO_TYPE_SOURCE_SINK and
presumably Postgres is not calling SSL_get_rfd on an SSL object that it
already knows is backed by a PGconn. TBH if you just passed 0 in for the
index, it would probably work just as well.

> + bio_method = port_bio_method();
> if (bio_method == NULL)
> {
> SSLerr(SSL_F_SSL_SET_FD, ERR_R_BUF_LIB);
>
> SSL_F_SSL_SET_FD is no longer the correct function context for this error
> reporting. In OpenSSL 3.x it means nothing since SSLerr throws away the
> function when calling ERR_raise_data, but we still support 1.1.0+. I
> think the
> correct error would be BIOerr(BIO_F_BIO_METH_NEW..) but I wonder if we
> should
> just remove it since BIO_meth_new and BIO_new already set errors for us to
> consume? It doesn't seem to make sense to add more errors on the queue
> here?
> The same goes for the frontend part.
>

Ah yeah, +1 to removing them. I've always found external code adding to the
error queue to be a little goofy. OpenSSL's error queue is weird enough
without external additions! :-)

> The attached v5 is a fresh rebase with my comments from above as 0002 to
> illustrate.
>

LGTM

David