From 2a5385511051d33be8d2b20d7669d8b1862fe510 Mon Sep 17 00:00:00 2001 From: Benjamin Kaduk Date: Wed, 13 Nov 2019 09:42:19 -0800 Subject: [PATCH] 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 (Merged from https://github.com/openssl/openssl/pull/10441) --- ssl/statem/extensions.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ssl/statem/extensions.c b/ssl/statem/extensions.c index d5f6e1afba..e2e704543e 100644 --- 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); -- 2.25.1