On Mon, Feb 18, 2019 at 2:18 PM Jakob Bohm via openssl-users <openssl-users@xxxxxxxxxxx> wrote:
On 17/02/2019 14:26, Matt Caswell wrote:
> On 16/02/2019 05:04, Sam Roberts wrote:
>> On Fri, Feb 15, 2019 at 3:35 PM Matt Caswell <matt@xxxxxxxxxxx> wrote:
>>> On 15/02/2019 20:32, Viktor Dukhovni wrote:
>>>>> On Feb 15, 2019, at 12:11 PM, Sam Roberts <vieuxtech@xxxxxxxxx> wrote:
>>>> OpenSSL could delay the actual shutdown until we're about to return
>>>> from the SSL_accept() that invoked the callback. That is SSL_shutdown()
>>>> called from callbacks could be deferred until a more favourable time.
>> In this case, it's an SSL_read() that invoked the callback, though
>> probably not relevant.
>>
>> And actually, no deferal would be necessary, I looks to me that the
>> info callback for handshake done is coming too early. Particularly,
>> the writing of the NewSessionTickets into the BIO should occur before
>> the info callback. I'll check later, but I'm pretty sure with TLS1.2
>> the session tickets are written and then the HANDSHAKE_DONE info
>> callback occurs, so the timing here is incompatible with TLS1.2.
> In TLSv1.2 New session tickets are written as part of the handshake. In TLSv1.3
> session tickets are sent after the handshake has completed. It sounds to me like
> the info callback is doing the right thing.
That seems to be a major theme in many reported OpenSSL 1.1.1
problems. It seems that you guys have gotten too hung up on how
the TLS 1.3 RFC uses words like handshake differently than the
TLS 1.2 RFC, rather than by the higher level semantics of what
would be considered the API visible meta-operations.
From an API user perspective, the messages that are exchanged
right after the RFC-handshake in order to complete the connection
set up should be considered part of the API-handshake.
This made little difference for the "change cipher spec" TLS 1.2
record, but makes a lot more difference for TLS 1.3 where various
things like certificate checks and session tickets fall into that
gray area.
Any opportunity to send data earlier than that should be handled
in a way that doesn't break the API for applications that aren't
doing so.
>> Though the deferal mechanism might be there already. It looks like
>> doing an SSL_write(); SSL_shutdown() in the info callback works fine,
>> on the client side new ticket callbacks are fired by the SSL_read()
>> before the SSL_read() sees the close_notify and returns 0. I haven't
>> looked at the packet/API trace for this, because the tests all pass
>> for this case, but I do see that in the _javascript_ called from the
>> HANDSHAKE_DONE callback, that calling .end("x") (write + shutdown)
>> causes the client to get tickets, but .end() causes it to miss them
>> because they are after close_notify.
>>
>>> Oh - right. I missed this detail. Calling SSL_shutdown() from inside a callback
>>> is definitely a bad idea. Don't do that.
>> The info callback, or ANY callbacks? What about the new ticket
>> callback, for example? Is it expected that no SSL_ calls are made in
>> ANY callbacks?
> I wouldn't go that far. Callbacks occur during the processing of an IO
> operation. Callbacks are generally expected to be small and quick. I wouldn't
> call anything that might invoke a new IO operation from within a callback, so
> SSL_read, SSL_write, SSL_do_handshake, SSL_shutdown etc.
>
Callbacks are often an opportunity for applications to detect
violations of security policy. It thus makes a lot of sense for
callbacks to request that the connection is ended as soon as
allowed by the risk of creating an attack side channel.
Other OpenSSL callbacks represent the one place to do certain
complex tasks, such as choosing among different certificates,
checking against outside (networked!) revocation systems etc.>
All of that makes me question; so in migrating to 1.3, does the basic flow change?
https://github.com/d3x0r/SACK/blob/master/src/netlib/ssl_layer.c#L178 (handshake... hmm that's long tedious debug optioned code)
https://github.com/d3x0r/SACK/blob/master/src/netlib/ssl_layer.c#L178 (handshake... hmm that's long tedious debug optioned code)
summary is pretty short...
if (!SSL_is_init_finished(ses->ssl)) {r = SSL_do_handshake(ses->ssl); if( r == 0 )/*error/incomplete */ else /* handle errors; usually WANT_READ; read for any control data pending, and send data*/ } else return 2/1;
until is_init_finished which is handshake() returns 2 on the first is_init_finished... and 1 after that; so the first callback does certificate verification...
then kinda...
onread() { /* recv got data */
handshake();
-1 ; close
0 - return wait for more data
2 - verify handshaken certs
1 - continue as normal.
read data (if any) (post to app)
read if any control data/send control data to remote
}
and I could optionally? register verification callbacks and remove the == 2 check inline?
Enjoy
Jakob
--
Jakob Bohm, CIO, Partner, WiseMo A/S. https://www.wisemo.com
Transformervej 29, 2860 Søborg, Denmark. Direct +45 31 13 16 10
This public discussion message is non-binding and may contain errors.
WiseMo - Remote Service Management for PCs, Phones and Embedded
