From 1c4aa31d79821dee9be98e915159d52cc30d8403 Mon Sep 17 00:00:00 2001 From: Benjamin Kaduk Date: Wed, 30 May 2018 09:49:29 -0500 Subject: [PATCH] Normalize SNI hostname handling for SSL and SSL_SESSION In particular, adhere to the rule that we must not modify any property of an SSL_SESSION object once it is (or might be) in a session cache. Such modifications are thread-unsafe and have been observed to cause crashes at runtime. To effect this change, standardize on the property that SSL_SESSION->ext.hostname is set only when that SNI value has been negotiated by both parties for use with that session. For session resumption this is trivially the case, so only new handshakes are affected. On the client, the new semantics are that the SSL->ext.hostname is for storing the value configured by the caller, and this value is used when constructing the ClientHello. On the server, SSL->ext.hostname is used to hold the value received from the client. Only if the SNI negotiation is successful will the hostname be stored into the session object; the server can do this after it sends the ServerHello, and the client after it has received and processed the ServerHello. This obviates the need to remove the hostname from the session object in case of failed negotiation (a change that was introduced in commit 9fb6cb810b769abbd60f11ef6e936a4e4456b19d in order to allow TLS 1.3 early data when SNI was present in the ClientHello but not the session being resumed), which was modifying cached sessions in certain cases. (In TLS 1.3 we always produce a new SSL_SESSION object for new connections, even in the case of resumption, so no TLS 1.3 handshakes were affected.) Reviewed-by: Matt Caswell (Merged from https://github.com/openssl/openssl/pull/6378) --- ssl/ssl_lib.c | 10 +++++++++- ssl/ssl_sess.c | 9 --------- ssl/statem/extensions.c | 25 ++++++++++++++++++++++--- ssl/statem/extensions_srvr.c | 19 ++++++++++++------- 4 files changed, 43 insertions(+), 20 deletions(-) diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c index 38391fd2c0..10a76940dd 100644 --- a/ssl/ssl_lib.c +++ b/ssl/ssl_lib.c @@ -2612,7 +2612,15 @@ const char *SSL_get_servername(const SSL *s, const int type) if (type != TLSEXT_NAMETYPE_host_name) return NULL; - return s->session && !s->ext.hostname ? + /* + * TODO(OpenSSL1.2) clean up this compat mess. This API is + * currently a mix of "what did I configure" and "what did the + * peer send" and "what was actually negotiated"; we should have + * a clear distinction amongst those three. + */ + if (SSL_in_init(s)) + return s->ext.hostname; + return (s->session != NULL && s->ext.hostname == NULL) ? s->session->ext.hostname : s->ext.hostname; } diff --git a/ssl/ssl_sess.c b/ssl/ssl_sess.c index 628b9f060b..d4a4808f19 100644 --- a/ssl/ssl_sess.c +++ b/ssl/ssl_sess.c @@ -421,15 +421,6 @@ int ssl_get_new_session(SSL *s, int session) return 0; } - if (s->ext.hostname) { - ss->ext.hostname = OPENSSL_strdup(s->ext.hostname); - if (ss->ext.hostname == NULL) { - SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_SSL_GET_NEW_SESSION, - ERR_R_INTERNAL_ERROR); - SSL_SESSION_free(ss); - return 0; - } - } } else { ss->session_id_length = 0; } diff --git a/ssl/statem/extensions.c b/ssl/statem/extensions.c index 5309b12703..85945acc0d 100644 --- a/ssl/statem/extensions.c +++ b/ssl/statem/extensions.c @@ -929,9 +929,28 @@ 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; + /* + * For servers, propagate the SNI hostname from the temporary + * storage in the SSL to the persistent SSL_SESSION, now that we + * know we accepted it. + * Clients make this copy when parsing the server's response to + * the extension, which is when they find out that the negotiation + * was successful. + */ + if (s->server) { + if (!sent) { + /* Nothing from the client this handshake; cleanup stale value */ + OPENSSL_free(s->ext.hostname); + s->ext.hostname = NULL; + } else if (ret == SSL_TLSEXT_ERR_OK && (!s->hit || SSL_IS_TLS13(s))) { + /* 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); + if (s->session->ext.hostname == NULL && s->ext.hostname != NULL) { + SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_FINAL_SERVER_NAME, + ERR_R_INTERNAL_ERROR); + } + } } /* diff --git a/ssl/statem/extensions_srvr.c b/ssl/statem/extensions_srvr.c index f5ab5bb840..00c0ec9c09 100644 --- a/ssl/statem/extensions_srvr.c +++ b/ssl/statem/extensions_srvr.c @@ -127,7 +127,7 @@ int tls_parse_ctos_server_name(SSL *s, PACKET *pkt, unsigned int context, return 0; } - if (!s->hit) { + if (!s->hit || SSL_IS_TLS13(s)) { if (PACKET_remaining(&hostname) > TLSEXT_MAXLEN_host_name) { SSLfatal(s, SSL_AD_UNRECOGNIZED_NAME, SSL_F_TLS_PARSE_CTOS_SERVER_NAME, @@ -142,21 +142,26 @@ int tls_parse_ctos_server_name(SSL *s, PACKET *pkt, unsigned int context, return 0; } - OPENSSL_free(s->session->ext.hostname); - s->session->ext.hostname = NULL; - if (!PACKET_strndup(&hostname, &s->session->ext.hostname)) { + /* + * Store the requested SNI in the SSL as temporary storage. + * If we accept it, it will get stored in the SSL_SESSION as well. + */ + OPENSSL_free(s->ext.hostname); + s->ext.hostname = NULL; + if (!PACKET_strndup(&hostname, &s->ext.hostname)) { SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS_PARSE_CTOS_SERVER_NAME, ERR_R_INTERNAL_ERROR); return 0; } s->servername_done = 1; - } else { + } + if (s->hit) { /* * TODO(openssl-team): if the SNI doesn't match, we MUST * fall back to a full handshake. */ - s->servername_done = s->session->ext.hostname + s->servername_done = (s->session->ext.hostname != NULL) && PACKET_equal(&hostname, s->session->ext.hostname, strlen(s->session->ext.hostname)); @@ -1325,7 +1330,7 @@ EXT_RETURN tls_construct_stoc_server_name(SSL *s, WPACKET *pkt, size_t chainidx) { if (s->hit || s->servername_done != 1 - || s->session->ext.hostname == NULL) + || s->ext.hostname == NULL) return EXT_RETURN_NOT_SENT; if (!WPACKET_put_bytes_u16(pkt, TLSEXT_TYPE_server_name)