wbrawner/openssl
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Normalize SNI hostname handling for SSL and SSL_SESSION

Browse source 
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
9fb6cb810b 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 <matt@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/6378)
This commit is contained in:
Benjamin Kaduk 2018-05-30 09:49:29 -05:00 committed by Benjamin Kaduk
parent 4cc968df40
commit 1c4aa31d79
4 changed files with 43 additions and 20 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

10
ssl/ssl_lib.c
View file

@ -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;
}

9
ssl/ssl_sess.c
View file

@ -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;
}

25
ssl/statem/extensions.c
View file

@ -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);
}
}
}
/*

19
ssl/statem/extensions_srvr.c
View file

@ -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)