From eee2a6a718151336534d15a61d8d11209d4dfb1e Mon Sep 17 00:00:00 2001 From: Matt Caswell Date: Fri, 14 Jun 2019 14:06:55 +0100 Subject: [PATCH] Fix a race condition in ciphers handling Similarly to the previous commit we were storing the peer offered list of ciphers in the session. In practice there is no need for this information to be avilable from one resumption to the next since this list is specific to a particular handshake. Since the session object is supposed to be immutable we should not be updating it once we have decided to resume. The solution is to remove the session list out of the session object. Reviewed-by: Tomas Mraz (Merged from https://github.com/openssl/openssl/pull/9162) --- ssl/ssl_lib.c | 10 +++++----- ssl/ssl_locl.h | 2 +- ssl/ssl_sess.c | 8 -------- ssl/statem/statem_srvr.c | 16 ++++++++-------- 4 files changed, 14 insertions(+), 22 deletions(-) diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c index 43a1099ba8..220168a148 100644 --- a/ssl/ssl_lib.c +++ b/ssl/ssl_lib.c @@ -1169,6 +1169,7 @@ void SSL_free(SSL *s) sk_SSL_CIPHER_free(s->cipher_list); sk_SSL_CIPHER_free(s->cipher_list_by_id); sk_SSL_CIPHER_free(s->tls13_ciphersuites); + sk_SSL_CIPHER_free(s->peer_ciphers); /* Make the next call work :-) */ if (s->session != NULL) { @@ -2571,9 +2572,9 @@ STACK_OF(SSL_CIPHER) *SSL_get_ciphers(const SSL *s) STACK_OF(SSL_CIPHER) *SSL_get_client_ciphers(const SSL *s) { - if ((s == NULL) || (s->session == NULL) || !s->server) + if ((s == NULL) || !s->server) return NULL; - return s->session->ciphers; + return s->peer_ciphers; } STACK_OF(SSL_CIPHER) *SSL_get1_supported_ciphers(SSL *s) @@ -2712,13 +2713,12 @@ char *SSL_get_shared_ciphers(const SSL *s, char *buf, int size) int i; if (!s->server - || s->session == NULL - || s->session->ciphers == NULL + || s->peer_ciphers == NULL || size < 2) return NULL; p = buf; - clntsk = s->session->ciphers; + clntsk = s->peer_ciphers; srvrsk = SSL_get_ciphers(s); if (clntsk == NULL || srvrsk == NULL) return NULL; diff --git a/ssl/ssl_locl.h b/ssl/ssl_locl.h index 2f76b66352..9663c7c3bb 100644 --- a/ssl/ssl_locl.h +++ b/ssl/ssl_locl.h @@ -556,7 +556,6 @@ struct ssl_session_st { const SSL_CIPHER *cipher; unsigned long cipher_id; /* when ASN.1 loaded, this needs to be used to * load the 'cipher' structure */ - STACK_OF(SSL_CIPHER) *ciphers; /* ciphers offered by the client */ CRYPTO_EX_DATA ex_data; /* application specific data */ /* * These are used to make removal of session-ids more efficient and to @@ -1318,6 +1317,7 @@ struct ssl_st { /* Per connection DANE state */ SSL_DANE dane; /* crypto */ + STACK_OF(SSL_CIPHER) *peer_ciphers; STACK_OF(SSL_CIPHER) *cipher_list; STACK_OF(SSL_CIPHER) *cipher_list_by_id; /* TLSv1.3 specific ciphersuites */ diff --git a/ssl/ssl_sess.c b/ssl/ssl_sess.c index 9809fcc882..f13c909da7 100644 --- a/ssl/ssl_sess.c +++ b/ssl/ssl_sess.c @@ -121,7 +121,6 @@ SSL_SESSION *ssl_session_dup(const SSL_SESSION *src, int ticket) dest->psk_identity_hint = NULL; dest->psk_identity = NULL; #endif - dest->ciphers = NULL; dest->ext.hostname = NULL; #ifndef OPENSSL_NO_EC dest->ext.ecpointformats = NULL; @@ -175,12 +174,6 @@ SSL_SESSION *ssl_session_dup(const SSL_SESSION *src, int ticket) } #endif - if (src->ciphers != NULL) { - dest->ciphers = sk_SSL_CIPHER_dup(src->ciphers); - if (dest->ciphers == NULL) - goto err; - } - if (!CRYPTO_dup_ex_data(CRYPTO_EX_INDEX_SSL_SESSION, &dest->ex_data, &src->ex_data)) { goto err; @@ -781,7 +774,6 @@ void SSL_SESSION_free(SSL_SESSION *ss) OPENSSL_cleanse(ss->session_id, sizeof(ss->session_id)); X509_free(ss->peer); sk_X509_pop_free(ss->peer_chain, X509_free); - sk_SSL_CIPHER_free(ss->ciphers); OPENSSL_free(ss->ext.hostname); OPENSSL_free(ss->ext.tick); #ifndef OPENSSL_NO_EC diff --git a/ssl/statem/statem_srvr.c b/ssl/statem/statem_srvr.c index 6504f4f74e..79c2aa0ede 100644 --- a/ssl/statem/statem_srvr.c +++ b/ssl/statem/statem_srvr.c @@ -1924,14 +1924,14 @@ static int tls_early_post_process_client_hello(SSL *s) && master_key_length > 0) { s->session->master_key_length = master_key_length; s->hit = 1; - s->session->ciphers = ciphers; + s->peer_ciphers = ciphers; s->session->verify_result = X509_V_OK; ciphers = NULL; /* check if some cipher was preferred by call back */ if (pref_cipher == NULL) - pref_cipher = ssl3_choose_cipher(s, s->session->ciphers, + pref_cipher = ssl3_choose_cipher(s, s->peer_ciphers, SSL_get_ciphers(s)); if (pref_cipher == NULL) { SSLfatal(s, SSL_AD_HANDSHAKE_FAILURE, @@ -1942,9 +1942,9 @@ static int tls_early_post_process_client_hello(SSL *s) s->session->cipher = pref_cipher; sk_SSL_CIPHER_free(s->cipher_list); - s->cipher_list = sk_SSL_CIPHER_dup(s->session->ciphers); + s->cipher_list = sk_SSL_CIPHER_dup(s->peer_ciphers); sk_SSL_CIPHER_free(s->cipher_list_by_id); - s->cipher_list_by_id = sk_SSL_CIPHER_dup(s->session->ciphers); + s->cipher_list_by_id = sk_SSL_CIPHER_dup(s->peer_ciphers); } } @@ -2044,12 +2044,12 @@ static int tls_early_post_process_client_hello(SSL *s) #endif /* - * Given s->session->ciphers and SSL_get_ciphers, we must pick a cipher + * Given s->peer_ciphers and SSL_get_ciphers, we must pick a cipher */ if (!s->hit || SSL_IS_TLS13(s)) { - sk_SSL_CIPHER_free(s->session->ciphers); - s->session->ciphers = ciphers; + sk_SSL_CIPHER_free(s->peer_ciphers); + s->peer_ciphers = ciphers; if (ciphers == NULL) { SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS_EARLY_POST_PROCESS_CLIENT_HELLO, @@ -2256,7 +2256,7 @@ WORK_STATE tls_post_process_client_hello(SSL *s, WORK_STATE wst) /* In TLSv1.3 we selected the ciphersuite before resumption */ if (!SSL_IS_TLS13(s)) { cipher = - ssl3_choose_cipher(s, s->session->ciphers, SSL_get_ciphers(s)); + ssl3_choose_cipher(s, s->peer_ciphers, SSL_get_ciphers(s)); if (cipher == NULL) { SSLfatal(s, SSL_AD_HANDSHAKE_FAILURE, -- 2.39.2