Fix a race condition in SNI handling

As was done for ciphers, supported groups, and EC point formats in
https://github.com/openssl/openssl/pull/9162, only write the negotiated
SNI hostname value to the session object when not resuming, even for
TLS 1.3 resumptions.  Otherwise, when using a stateful session cache
(as is done by default when 0-RTT data is enabled), we can have multiple
SSLs active using the same in-memory session object, which leads to
double-frees and similar race conditions in the SNI handler prior
to this commit.

Fortunately, since draft-ietf-tls-tls13-22, there is no requirement
that the SNI hostname be preserved across TLS 1.3 resumption, and thus
not a need to continually update the session object with the "current"
value (to be used when producing session tickets, so that the subsequent
resumption can be checked against the current value).  So we can just
relax the logic and only write to the session object for initial handshakes.
This still leaves us in a somewhat inconsistent state, since if the SNI value
does change across handshakes, the session object will continue to record
the initial handshake's value, even if that bears no relation to the
current handshake.  The current SSL_get_servername() implementation
prefers the value from the session if s->hit, but a more complete fix
for that and related issues is underway in
https://github.com/openssl/openssl/pull/10018; there is no need to wait
for the complete fix for SNI name handling in order to close the
race condition and avoid runtime crashes.

Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/10441)

diff --git a/ssl/statem/extensions.c b/ssl/statem/extensions.c
index d5f6e1afbab1cfc715ec83f9c06358e05b58244b..e2e704543eabe3944cb93ccff9d73ab09a6399f1 100644 (file)
--- a/ssl/statem/extensions.c
+++ b/ssl/statem/extensions.c
@@ -949,7 +949,7 @@ static int final_server_name(SSL *s, unsigned int context, int sent)
      */
     if (s->server) {
         /* TODO(OpenSSL1.2) revisit !sent case */
-        if (sent && ret == SSL_TLSEXT_ERR_OK && (!s->hit || SSL_IS_TLS13(s))) {
+        if (sent && ret == SSL_TLSEXT_ERR_OK && !s->hit) {
             /* Only store the hostname in the session if we accepted it. */
             OPENSSL_free(s->session->ext.hostname);
             s->session->ext.hostname = OPENSSL_strdup(s->ext.hostname);