[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

[Matt Caswell via RT]

Post by David Benjamin via RT
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]

Post by Matt Caswell via RT

Post by David Benjamin via RT
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

Post by David Benjamin via RT
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.

Post by Matt Caswell via RT
*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.

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

[Matt Caswell via RT]

Post by 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
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.

Post by David Benjamin via RT
2. SSL_set_bio's object ownership story is a mess. It also doesn't
quite
SSL_set_fd(ssl, 1);
SSL_set_rfd(ssl, 2);
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

[David Benjamin]

Post by Matt Caswell via RT

Post by 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
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.

Post by David Benjamin via RT
2. SSL_set_bio's object ownership story is a mess. It also doesn't
quite
SSL_set_fd(ssl, 1);
SSL_set_rfd(ssl, 2);
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.

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]

Post by Matt Caswell via RT

Post by 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
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.

Post by David Benjamin via RT
2. SSL_set_bio's object ownership story is a mess. It also doesn't
quite
SSL_set_fd(ssl, 1);
SSL_set_rfd(ssl, 2);
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.

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

[Matt Caswell via RT]

Post by David Benjamin
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.

Matt