Fix bug where early_data does not work if no SNI callback is present #4519
Conversation
…data Test for the bug where early_data is not accepted by the server when it does not have an SNI callback set up, but the client sent a servername in the initial ClientHello establishing the session.
|
Pushed. Thanks. |
…data Test for the bug where early_data is not accepted by the server when it does not have an SNI callback set up, but the client sent a servername in the initial ClientHello establishing the session. Reviewed-by: Rich Salz <rsalz@openssl.org> Reviewed-by: Ben Kaduk <kaduk@mit.edu> (Merged from #4519)
| @@ -834,6 +834,11 @@ static int final_server_name(SSL *s, unsigned int context, int sent, | |||
| ret = s->session_ctx->ext.servername_cb(s, &altmp, | |||
| s->session_ctx->ext.servername_arg); | |||
|
|
|||
| if (!sent) { | |||
| OPENSSL_free(s->session->ext.hostname); | |||
| s->session->ext.hostname = NULL; | |||
There was a problem hiding this comment.
This change is intended to fix a bug with early data, a TLS 1.3-only feature.
However, if I am reading things correctly, we can also run this code for pre-1.3 handshakes, and since we do not send SNI in the ServerHello in some cases (e.g., when resuming), we end up freeing a field in the SSL_SESSION object for a resumed session, i.e., one that is still in cache (for non-ticket cases). This is, or should be, forbidden, because cached sessions can be in use by multiple threads simultaneously -- for the TLS 1.3 case we fix this by always creating a new SSL_SESSION object for each connection, but TLS 1.2 and older still have the old behavior.
One could perhaps argue that we should not be storing the "intermediate-state" (i.e., during handshake processing) hostname in the SSL_SESSION at all, and should instead be using the SSL's ext.hostname field. We would then only populate the session once we know that we have negotiated an SNI value and are establishing a successful session, so the SSL_SESSION would remain read-only after (logical) creation.
There was a problem hiding this comment.
We are getting a double-free condition at this particular condition. We can get it to happen fairly readily, but don't know the exact cause (i.e. modifying a cached session).
There was a problem hiding this comment.
Hi,
https://tools.ietf.org/html/draft-ietf-tls-tls13-28#section-4.2.11 says :
"
In TLS 1.3, the SNI value is always explicitly specified in
the resumption handshake, and there is no need for the server to
associate an SNI value with the ticket.
"
So it seems that there is no need to zero ext.hostname.
Instead there is a need to check the existence of ext.hostname.
I also think that the draft should mention that SNI is required also at initial handshake.
There was a problem hiding this comment.
We are getting a double-free condition at this particular condition. We can get it to happen fairly readily, but don't know the exact cause (i.e. modifying a cached session).
Is this a multi-threaded server? This function does indeed appear to violate the threading rules around s->session, so that's probably what's going on. With the caveat that I'm not familiar with the 1.1.0 state machine, this function is run in both resumption and full handshakes, right? s->session is immutable in resumption handshakes.
We split s->session into a bunch of different session pointers on our end, largely to help avoid this sort of mixup. This is especially helpful starting TLS 1.3, where, at least in our implementation, we found it was simplest to say that 1.3 resumption stamps out a new SSL_SESSION using the previous one as a template. Having the mutability encoded in which field you're accessing rather than runtime state makes it easier to get things right.
There was a problem hiding this comment.
It is multi-threaded, and the current leading hypothesis is that this code's violation of the threading rules for s->session is causing the problems.
It is perhaps debatable whether we should be writing a hostname into the session during handshake processing but before we have decided to accept SNI, though, which would be a somewhat more invasive patch.
There was a problem hiding this comment.
Is this a multi-threaded server?
Yes, this is the path we are currently going down; and agree with the SSL_SESSION write-access violation.
There was a problem hiding this comment.
Oops, somehow missed that @kaduk already said basically the same thing. :-)
One could perhaps argue that we should not be storing the "intermediate-state" (i.e., during handshake processing) hostname in the SSL_SESSION at all, and should instead be using the SSL's ext.hostname field. We would then only populate the session once we know that we have negotiated an SNI value and are establishing a successful session, so the SSL_SESSION would remain read-only after (logical) creation.
This is what we do. Or rather, it's what we did when we stored the hostname in the SSL_SESSION at all. We've since stopped doing that. But note there is some weirdness here around being consistent whether, in a TLS 1.2 resumption, SSL_get_servername returns the value stored in the session or the value received on the connection, if you don't enforce that they match. That API is sometimes queried during and sometimes after the handshake.
There was a problem hiding this comment.
Indeed, there'd probably need to be some awful hack where SSL_get_servername can return the hostname from the SSL during the handshake and the one from the session otherwise. Maybe that's more messiness than it's worth, and we should just go for the minimal change of only doing this free for non-cached session objects (i.e., non-resumptions and TLS 1.3 connections).
There was a problem hiding this comment.
Okay, I made #6378 to show what this approach looks like.
Fixes #4496
Checklist