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

Fix a race condition in ciphers handling

Browse source 
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 <tmraz@fedoraproject.org>
(Merged from https://github.com/openssl/openssl/pull/9176)
This commit is contained in:
Matt Caswell 2019-06-14 14:06:55 +01:00
parent 2813852d71
commit 860fed97aa
4 changed files with 14 additions and 22 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

10
ssl/ssl_lib.c
View file

@ -1160,6 +1160,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) {
@ -2438,9 +2439,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)
@ -2579,13 +2580,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;

2
ssl/ssl_locl.h
View file

@ -552,7 +552,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
@ -1135,6 +1134,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 */

8
ssl/ssl_sess.c
View file

@ -121,7 +121,6 @@ SSL_SESSION *ssl_session_dup(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(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

16
ssl/statem/statem_srvr.c
View file

@ -1921,14 +1921,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,
@ -1939,9 +1939,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);
}
}
@ -2041,12 +2041,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,
@ -2253,7 +2253,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,