From 2813852d7111ad0a49a963bdc49d944d453e52e7 Mon Sep 17 00:00:00 2001 From: Matt Caswell Date: Fri, 14 Jun 2019 12:46:13 +0100 Subject: [PATCH] Fix a race condition in supported groups handling In TLSv1.3 the supported groups can be negotiated each time a handshake occurs, regardless of whether we are resuming or not. We should not store the supported groups information in the session because session objects can be shared between multiple threads and we can end up with race conditions. For most users this won't be seen because, by default, we use stateless tickets in TLSv1.3 which don't get shared. However if you use SSL_OP_NO_TICKET (to get stateful tickets in TLSv1.3) then this can happen. The answer is to move the supported the supported group information into the SSL object instead. Reviewed-by: Tomas Mraz (Merged from https://github.com/openssl/openssl/pull/9176) --- ssl/s3_lib.c | 4 ++-- ssl/ssl_lib.c | 1 + ssl/ssl_locl.h | 13 ++++++++----- ssl/ssl_sess.c | 12 ------------ ssl/statem/extensions_srvr.c | 10 +++++----- 5 files changed, 16 insertions(+), 24 deletions(-) diff --git a/ssl/s3_lib.c b/ssl/s3_lib.c index 99ae48199c..dc148bc2b1 100644 --- a/ssl/s3_lib.c +++ b/ssl/s3_lib.c @@ -3601,8 +3601,8 @@ long ssl3_ctrl(SSL *s, int cmd, long larg, void *parg) if (!s->session) return 0; - clist = s->session->ext.supportedgroups; - clistlen = s->session->ext.supportedgroups_len; + clist = s->ext.peer_supportedgroups; + clistlen = s->ext.peer_supportedgroups_len; if (parg) { size_t i; int *cptr = parg; diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c index f559bc10ef..5584a1b089 100644 --- a/ssl/ssl_lib.c +++ b/ssl/ssl_lib.c @@ -1179,6 +1179,7 @@ void SSL_free(SSL *s) #ifndef OPENSSL_NO_EC OPENSSL_free(s->ext.ecpointformats); OPENSSL_free(s->ext.supportedgroups); + OPENSSL_free(s->ext.peer_supportedgroups); #endif /* OPENSSL_NO_EC */ sk_X509_EXTENSION_pop_free(s->ext.ocsp.exts, X509_EXTENSION_free); #ifndef OPENSSL_NO_OCSP diff --git a/ssl/ssl_locl.h b/ssl/ssl_locl.h index 0cf3893e06..48c7eb0e53 100644 --- a/ssl/ssl_locl.h +++ b/ssl/ssl_locl.h @@ -566,9 +566,7 @@ struct ssl_session_st { size_t ecpointformats_len; unsigned char *ecpointformats; /* peer's list */ # endif /* OPENSSL_NO_EC */ - size_t supportedgroups_len; - uint16_t *supportedgroups; /* peer's list */ - /* RFC4507 info */ + /* RFC4507 info */ unsigned char *tick; /* Session ticket */ size_t ticklen; /* Session ticket length */ /* Session lifetime hint in seconds */ @@ -1304,6 +1302,11 @@ struct ssl_st { size_t supportedgroups_len; /* our list */ uint16_t *supportedgroups; + + size_t peer_supportedgroups_len; + /* peer's list */ + uint16_t *peer_supportedgroups; + /* TLS Session Ticket extension override */ TLS_SESSION_TICKET_EXT *session_ticket; /* TLS Session Ticket extension callback */ @@ -2240,8 +2243,8 @@ static ossl_inline int ssl_has_cert(const SSL *s, int idx) static ossl_inline void tls1_get_peer_groups(SSL *s, const uint16_t **pgroups, size_t *pgroupslen) { - *pgroups = s->session->ext.supportedgroups; - *pgroupslen = s->session->ext.supportedgroups_len; + *pgroups = s->ext.peer_supportedgroups; + *pgroupslen = s->ext.peer_supportedgroups_len; } # ifndef OPENSSL_UNIT_TEST diff --git a/ssl/ssl_sess.c b/ssl/ssl_sess.c index 5ad2792a1b..b7638780d0 100644 --- a/ssl/ssl_sess.c +++ b/ssl/ssl_sess.c @@ -125,7 +125,6 @@ SSL_SESSION *ssl_session_dup(SSL_SESSION *src, int ticket) dest->ext.hostname = NULL; #ifndef OPENSSL_NO_EC dest->ext.ecpointformats = NULL; - dest->ext.supportedgroups = NULL; #endif dest->ext.tick = NULL; dest->ext.alpn_selected = NULL; @@ -201,14 +200,6 @@ SSL_SESSION *ssl_session_dup(SSL_SESSION *src, int ticket) if (dest->ext.ecpointformats == NULL) goto err; } - if (src->ext.supportedgroups) { - dest->ext.supportedgroups = - OPENSSL_memdup(src->ext.supportedgroups, - src->ext.supportedgroups_len - * sizeof(*src->ext.supportedgroups)); - if (dest->ext.supportedgroups == NULL) - goto err; - } #endif if (ticket != 0 && src->ext.tick != NULL) { @@ -797,9 +788,6 @@ void SSL_SESSION_free(SSL_SESSION *ss) OPENSSL_free(ss->ext.ecpointformats); ss->ext.ecpointformats = NULL; ss->ext.ecpointformats_len = 0; - OPENSSL_free(ss->ext.supportedgroups); - ss->ext.supportedgroups = NULL; - ss->ext.supportedgroups_len = 0; #endif /* OPENSSL_NO_EC */ #ifndef OPENSSL_NO_PSK OPENSSL_free(ss->psk_identity_hint); diff --git a/ssl/statem/extensions_srvr.c b/ssl/statem/extensions_srvr.c index 6301b4e77c..5b83c26785 100644 --- a/ssl/statem/extensions_srvr.c +++ b/ssl/statem/extensions_srvr.c @@ -962,12 +962,12 @@ int tls_parse_ctos_supported_groups(SSL *s, PACKET *pkt, unsigned int context, } if (!s->hit || SSL_IS_TLS13(s)) { - OPENSSL_free(s->session->ext.supportedgroups); - s->session->ext.supportedgroups = NULL; - s->session->ext.supportedgroups_len = 0; + OPENSSL_free(s->ext.peer_supportedgroups); + s->ext.peer_supportedgroups = NULL; + s->ext.peer_supportedgroups_len = 0; if (!tls1_save_u16(&supported_groups_list, - &s->session->ext.supportedgroups, - &s->session->ext.supportedgroups_len)) { + &s->ext.peer_supportedgroups, + &s->ext.peer_supportedgroups_len)) { SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS_PARSE_CTOS_SUPPORTED_GROUPS, ERR_R_INTERNAL_ERROR);