[openssl.org #4572] SSL_set_bio and friends

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

[openssl.org #4572] SSL_set_bio and friends

Rich Salz via RT
On Tue Jun 14 20:30:09 2016, [hidden email] 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

--
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Reply | Threaded
Open this post in threaded view
|

Re: [openssl.org #4572] SSL_set_bio and friends

David Benjamin
On Fri, Jul 29, 2016 at 9:21 AM Matt Caswell via RT <[hidden email]> wrote:
On Tue Jun 14 20:30:09 2016, [hidden email] 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.

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 

--
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Reply | Threaded
Open this post in threaded view
|

Re: [openssl.org #4572] SSL_set_bio and friends

Rich Salz via RT
On Fri, Jul 29, 2016 at 9:21 AM Matt Caswell via RT <[hidden email]> wrote:

> On Tue Jun 14 20:30:09 2016, [hidden email] 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.


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

--
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
Reply | Threaded
Open this post in threaded view
|

Re: [openssl.org #4572] SSL_set_bio and friends

Rich Salz 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


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