[openssl-dev] [openssl.org #4572] SSL_set_bio and friends

[David Benjamin via RT]

I recently made some changes around BoringSSL's SSL_set_bio, etc. which you
all might be interested in. The BIO management has two weird behaviors
right now:

1. The existence of bbio is leaked in the public API when it should be an
implementation detail. (Otherwise you're stuck with it for DTLS where it's
really messy.) SSL_get_wbio will return it, and SSL_set_bio messes up when
the bbio is active.

2. SSL_set_bio's object ownership story is a mess. It also doesn't quite
work. This crashes:
SSL_set_fd(ssl, 1);
SSL_set_rfd(ssl, 2);
But this does not:
SSL_set_fd(ssl, 1);
SSL_set_wfd(ssl, 2);
Not that anyone would do such a thing, but the asymmetry is off.

For 1, I made this change:
https://boringssl.googlesource.com/boringssl/+/2f87112b963fe9dee6a75b23a8dae45000001063%5E%21/
SSL_get_wbio now always returns the true wbio. BIO_set_bio is aware of bbio
and behaves accordingly.

Then for 2, I wrote this test:
https://boringssl.googlesource.com/boringssl/+/5c0fb889a1348ecaa5691f6139f9d60a610f2129%5E%21/
And then made this change:
https://boringssl.googlesource.com/boringssl/+/f715c423224a292d79ba0e3df373c828fbae29f7%5E%21/
[Plus
comment typo fix]
Releasing ssl->{rbio,wbio} is now much more straight-forward. All the
ownership quirks are left in SSL_set_bio. It's messy, but it's the best
option I found which preserves the existing calling patterns. The different
cases reflect the desired behavior inherently having a lot of cases.

For OpenSSL master, I believe it'd also work to add an s->rbio != s->wbio
check to SSL_set_rbio, but I think those are worse semantics for
SSL_set_{rbio,wbio}. They are new APIs, so, before it's too late, give them
clear semantics like "SSL_set_rbio takes ownership of its argument",
consistent with "set0" functions, rather than a mix of "set0" and "set1".

David

--
Ticket here: http://rt.openssl.org/Ticket/Display.html?id=4572
Please log in as guest with password guest if prompted

--
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

[Matt Caswell via RT]

On 14/06/16 21:30, David Benjamin via RT wrote:
> For OpenSSL master, I believe it'd also work to add an s->rbio != s->wbio
> check to SSL_set_rbio, but I think those are worse semantics for
> SSL_set_{rbio,wbio}. They are new APIs, so, before it's too late, give them
> clear semantics like "SSL_set_rbio takes ownership of its argument",
> consistent with "set0" functions, rather than a mix of "set0" and "set1".

These look like good changes. I'm wondering whether we should actually
rename SSL_set_rbio() and SSL_set_wbio() to SSL_set0_rbio() and
SSL_set0_wbio() - especially since they are new to 1.1.0 so not released
yet.

*Possibly* we could also rename SSL_set_bio() to SSL_set0_bio() with a
deprecated compatibility macro.

Matt

[David Benjamin via RT]

On Fri, Jun 17, 2016 at 8:48 AM Matt Caswell via RT <r...@openssl.org> wrote:

>
>
> On 14/06/16 21:30, David Benjamin via RT wrote:
> > For OpenSSL master, I believe it'd also work to add an s->rbio != s->wbio
> > check to SSL_set_rbio, but I think those are worse semantics for
> > SSL_set_{rbio,wbio}. They are new APIs, so, before it's too late, give
> them
> > clear semantics like "SSL_set_rbio takes ownership of its argument",
> > consistent with "set0" functions, rather than a mix of "set0" and "set1".
>
> These look like good changes. I'm wondering whether we should actually
> rename SSL_set_rbio() and SSL_set_wbio() to SSL_set0_rbio() and
> SSL_set0_wbio() - especially since they are new to 1.1.0 so not released
> yet.
>

Sounds good to me.

> *Possibly* we could also rename SSL_set_bio() to SSL_set0_bio() with a
> deprecated compatibility macro.
>

I dunno, SSL_set_bio is kind of weird all around. :-) I suppose it is
closer to a set0 than a set1, but set0 doesn't typically have all these
no-op cases around taking ownership and such.

[Matt Caswell via RT]

On Tue Jun 14 20:30:09 2016, davi...@google.com wrote:
> I recently made some changes around BoringSSL's SSL_set_bio, etc.
> which you
> all might be interested in. The BIO management has two weird behaviors
> right now:
>
> 1. The existence of bbio is leaked in the public API when it should be
> an
> implementation detail. (Otherwise you're stuck with it for DTLS where
> it's
> really messy.) SSL_get_wbio will return it, and SSL_set_bio messes up
> when
> the bbio is active.

Fixed by 2e7dc7cd688.

> 2. SSL_set_bio's object ownership story is a mess. It also doesn't
> quite
> work. This crashes:
> SSL_set_fd(ssl, 1);
> SSL_set_rfd(ssl, 2);
> But this does not:
> SSL_set_fd(ssl, 1);
> SSL_set_wfd(ssl, 2);
> Not that anyone would do such a thing, but the asymmetry is off.

Fixed by 2e7dc7cd688 and in the docs by e040a42e44.

I also added a test, which I verified against the original 1.0.2 implementation
of SSL_set_bio(), in 7fb4c82035.

I found I needed to make some tweaks to the implementation of SSL_set_bio()
from your version in order to preserve the behaviour between 1.0.2 and master.
Possibly your version was a deliberate simplification.

Anyway, marking this as resolved.

Matt

--
Ticket here: http://rt.openssl.org/Ticket/Display.html?id=4572
Please log in as guest with password guest if prompted

--

[David Benjamin]

Hrm. It's been a while, but I believe it was a deliberate simplification to fix the SSL_set_rfd crash above.

My interpretation was that SSL_set_rfd and SSL_set_wfd's calling pattern, insane as it is, is definitively correct. Thus, the asymmetry in SSL_set_bio is a bug and was a deliberate change on my part.

It seems your interpretation was that SSL_set_bio's behavior, insane as it is, is definitively correct. Thus the bug lies in SSL_set_[rw]fd using SSL_set_bio wrong, so you changed those functions and kept SSL_set_bio's behavior as-is. (I'm not sure SSL_set_bio actually lets you implement SSL_set_rfd, but you have the new side-specific setters and just used that.)

I like my interpretation slightly better if only because it makes SSL_set_bio's behavior a little more sane. Judging by SSL_set_[rw]fd, it also seems to have been the original intent. It is a behavior change, but one I'm sure will break no one.

(If you still prefer yours, I will make BoringSSL match since I see no reason for us to diverge here.)

David 

[David Benjamin via RT]

[Matt Caswell via RT]

On 30/07/16 23:45, David Benjamin via RT wrote:
> It is a behavior change, but
> one I'm sure will break no one.

Unfortunately I don't share your optimism that it won't break any one :-(

Matt

[Matt Caswell via RT]

Closing this ticket.