From 4ff1a5266685f4a687a9f91b531c2f979b96db22 Mon Sep 17 00:00:00 2001 From: Matt Caswell Date: Fri, 1 Jun 2018 16:52:34 +0100 Subject: [PATCH] Fix TLSv1.3 ticket nonces All tickets on a connection need to have a unique nonce. When this was originally implemented we only ever sent one ticket on the conneciton so this didn't matter. We were just using the value 0. Now we can get multiple tickets to we need to start doing the ticket nonce properly. Fixes #6387 Reviewed-by: Andy Polyakov (Merged from https://github.com/openssl/openssl/pull/6415) --- include/openssl/ssl.h | 1 - ssl/ssl_asn1.c | 2 +- ssl/ssl_locl.h | 12 ++++++---- ssl/statem/extensions.c | 35 ++++++++-------------------- ssl/statem/statem_clnt.c | 27 ++++++++++++++++++++++ ssl/statem/statem_srvr.c | 50 ++++++++++++++++++++++++++++++++++------ ssl/tls13_enc.c | 3 +-- 7 files changed, 90 insertions(+), 40 deletions(-) diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index db0a2d5d82..280250ad83 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h @@ -48,7 +48,6 @@ extern "C" { # define SSL_MIN_RSA_MODULUS_LENGTH_IN_BYTES (512/8) # define SSL_MAX_KEY_ARG_LENGTH 8 # define SSL_MAX_MASTER_KEY_LENGTH 48 -# define TLS13_MAX_RESUMPTION_MASTER_LENGTH 64 /* The maximum number of encrypt/decrypt pipelines we can support */ # define SSL_MAX_PIPELINES 32 diff --git a/ssl/ssl_asn1.c b/ssl/ssl_asn1.c index 1da1c828a0..8c2afbe6af 100644 --- a/ssl/ssl_asn1.c +++ b/ssl/ssl_asn1.c @@ -299,7 +299,7 @@ SSL_SESSION *d2i_SSL_SESSION(SSL_SESSION **a, const unsigned char **pp, goto err; if (!ssl_session_memcpy(ret->master_key, &tmpl, - as->master_key, TLS13_MAX_RESUMPTION_MASTER_LENGTH)) + as->master_key, TLS13_MAX_RESUMPTION_PSK_LENGTH)) goto err; ret->master_key_length = tmpl; diff --git a/ssl/ssl_locl.h b/ssl/ssl_locl.h index 31e5cc8aa9..23608561ac 100644 --- a/ssl/ssl_locl.h +++ b/ssl/ssl_locl.h @@ -470,6 +470,8 @@ struct ssl_method_st { long (*ssl_ctx_callback_ctrl) (SSL_CTX *s, int cb_id, void (*fp) (void)); }; +# define TLS13_MAX_RESUMPTION_PSK_LENGTH 64 + /*- * Lets make this into an ASN.1 type structure as follows * SSL_SESSION_ID ::= SEQUENCE { @@ -505,9 +507,9 @@ struct ssl_session_st { unsigned char early_secret[EVP_MAX_MD_SIZE]; /* * For <=TLS1.2 this is the master_key. For TLS1.3 this is the resumption - * master secret + * PSK */ - unsigned char master_key[TLS13_MAX_RESUMPTION_MASTER_LENGTH]; + unsigned char master_key[TLS13_MAX_RESUMPTION_PSK_LENGTH]; /* session_id - valid? */ size_t session_id_length; unsigned char session_id[SSL_MAX_SSL_SESSION_ID_LENGTH]; @@ -1125,12 +1127,12 @@ struct ssl_st { */ uint32_t mac_flags; /* - * The TLS1.3 secrets. The resumption master secret is stored in the - * session. + * The TLS1.3 secrets. */ unsigned char early_secret[EVP_MAX_MD_SIZE]; unsigned char handshake_secret[EVP_MAX_MD_SIZE]; unsigned char master_secret[EVP_MAX_MD_SIZE]; + unsigned char resumption_master_secret[EVP_MAX_MD_SIZE]; unsigned char client_finished_secret[EVP_MAX_MD_SIZE]; unsigned char server_finished_secret[EVP_MAX_MD_SIZE]; unsigned char server_finished_hash[EVP_MAX_MD_SIZE]; @@ -1422,6 +1424,8 @@ struct ssl_st { size_t num_tickets; /* The number of TLS1.3 tickets actually sent so far */ size_t sent_tickets; + /* The next nonce value to use when we send a ticket on this connection */ + uint64_t next_ticket_nonce; }; /* diff --git a/ssl/statem/extensions.c b/ssl/statem/extensions.c index 209b4df782..8885e5e0d7 100644 --- a/ssl/statem/extensions.c +++ b/ssl/statem/extensions.c @@ -1421,13 +1421,11 @@ int tls_psk_do_binder(SSL *s, const EVP_MD *md, const unsigned char *msgstart, EVP_MD_CTX *mctx = NULL; unsigned char hash[EVP_MAX_MD_SIZE], binderkey[EVP_MAX_MD_SIZE]; unsigned char finishedkey[EVP_MAX_MD_SIZE], tmpbinder[EVP_MAX_MD_SIZE]; - unsigned char tmppsk[EVP_MAX_MD_SIZE]; - unsigned char *early_secret, *psk; - const char resumption_label[] = "res binder"; - const char external_label[] = "ext binder"; - const char nonce_label[] = "resumption"; - const char *label; - size_t bindersize, labelsize, psklen, hashsize; + unsigned char *early_secret; + static const unsigned char resumption_label[] = "res binder"; + static const unsigned char external_label[] = "ext binder"; + const unsigned char *label; + size_t bindersize, labelsize, hashsize; int hashsizei = EVP_MD_size(md); int ret = -1; int usepskfored = 0; @@ -1454,21 +1452,6 @@ int tls_psk_do_binder(SSL *s, const EVP_MD *md, const unsigned char *msgstart, labelsize = sizeof(resumption_label) - 1; } - if (external) { - psk = sess->master_key; - psklen = sess->master_key_length; - } else { - psk = tmppsk; - psklen = hashsize; - if (!tls13_hkdf_expand(s, md, sess->master_key, - (const unsigned char *)nonce_label, - sizeof(nonce_label) - 1, sess->ext.tick_nonce, - sess->ext.tick_nonce_len, psk, hashsize)) { - /* SSLfatal() already called */ - goto err; - } - } - /* * Generate the early_secret. On the server side we've selected a PSK to * resume with (internal or external) so we always do this. On the client @@ -1481,7 +1464,9 @@ int tls_psk_do_binder(SSL *s, const EVP_MD *md, const unsigned char *msgstart, early_secret = (unsigned char *)s->early_secret; else early_secret = (unsigned char *)sess->early_secret; - if (!tls13_generate_secret(s, md, NULL, psk, psklen, early_secret)) { + + if (!tls13_generate_secret(s, md, NULL, sess->master_key, + sess->master_key_length, early_secret)) { /* SSLfatal() already called */ goto err; } @@ -1500,8 +1485,8 @@ int tls_psk_do_binder(SSL *s, const EVP_MD *md, const unsigned char *msgstart, } /* Generate the binder key */ - if (!tls13_hkdf_expand(s, md, early_secret, (unsigned char *)label, - labelsize, hash, hashsize, binderkey, hashsize)) { + if (!tls13_hkdf_expand(s, md, early_secret, label, labelsize, hash, + hashsize, binderkey, hashsize)) { /* SSLfatal() already called */ goto err; } diff --git a/ssl/statem/statem_clnt.c b/ssl/statem/statem_clnt.c index 6c0f8be564..99445a6564 100644 --- a/ssl/statem/statem_clnt.c +++ b/ssl/statem/statem_clnt.c @@ -22,6 +22,7 @@ #include #include #include +#include static MSG_PROCESS_RETURN tls_process_as_hello_retry_request(SSL *s, PACKET *pkt); static MSG_PROCESS_RETURN tls_process_encrypted_extensions(SSL *s, PACKET *pkt); @@ -2674,6 +2675,32 @@ MSG_PROCESS_RETURN tls_process_new_session_ticket(SSL *s, PACKET *pkt) /* This is a standalone message in TLSv1.3, so there is no more to read */ if (SSL_IS_TLS13(s)) { + const EVP_MD *md = ssl_handshake_md(s); + int hashleni = EVP_MD_size(md); + size_t hashlen; + static const unsigned char nonce_label[] = "resumption"; + + /* Ensure cast to size_t is safe */ + if (!ossl_assert(hashleni >= 0)) { + SSLfatal(s, SSL_AD_INTERNAL_ERROR, + SSL_F_TLS_PROCESS_NEW_SESSION_TICKET, + ERR_R_INTERNAL_ERROR); + goto err; + } + hashlen = (size_t)hashleni; + + if (!tls13_hkdf_expand(s, md, s->resumption_master_secret, + nonce_label, + sizeof(nonce_label) - 1, + s->session->ext.tick_nonce, + s->session->ext.tick_nonce_len, + s->session->master_key, + hashlen)) { + /* SSLfatal() already called */ + goto err; + } + s->session->master_key_length = hashlen; + OPENSSL_free(exts); ssl_update_cache(s, SSL_SESS_CACHE_CLIENT); return MSG_PROCESS_FINISHED_READING; diff --git a/ssl/statem/statem_srvr.c b/ssl/statem/statem_srvr.c index ce8cec185a..c690cf0191 100644 --- a/ssl/statem/statem_srvr.c +++ b/ssl/statem/statem_srvr.c @@ -24,6 +24,8 @@ #include #include +#define TICKET_NONCE_SIZE 8 + static int tls_construct_encrypted_extensions(SSL *s, WPACKET *pkt); /* @@ -3758,7 +3760,21 @@ int tls_construct_new_session_ticket(SSL *s, WPACKET *pkt) } age_add_u; if (SSL_IS_TLS13(s)) { + size_t i, hashlen; + uint64_t nonce; + const char nonce_label[] = "resumption"; + const EVP_MD *md = ssl_handshake_md(s); void (*cb) (const SSL *ssl, int type, int val) = NULL; + int hashleni = EVP_MD_size(md); + + /* Ensure cast to size_t is safe */ + if (!ossl_assert(hashleni >= 0)) { + SSLfatal(s, SSL_AD_INTERNAL_ERROR, + SSL_F_TLS_CONSTRUCT_NEW_SESSION_TICKET, + ERR_R_INTERNAL_ERROR); + goto err; + } + hashlen = (size_t)hashleni; if (s->info_callback != NULL) cb = s->info_callback; @@ -3806,20 +3822,34 @@ int tls_construct_new_session_ticket(SSL *s, WPACKET *pkt) goto err; } s->session->ext.tick_age_add = age_add_u.age_add; - /* - * ticket_nonce is set to a single 0 byte because we only ever send a - * single ticket per connection. IMPORTANT: If we ever support multiple - * tickets per connection then this will need to be changed. - */ + OPENSSL_free(s->session->ext.tick_nonce); - s->session->ext.tick_nonce = OPENSSL_zalloc(sizeof(char)); + s->session->ext.tick_nonce = OPENSSL_zalloc(TICKET_NONCE_SIZE); if (s->session->ext.tick_nonce == NULL) { SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS_CONSTRUCT_NEW_SESSION_TICKET, ERR_R_MALLOC_FAILURE); goto err; } - s->session->ext.tick_nonce_len = 1; + nonce = s->next_ticket_nonce; + for (i = TICKET_NONCE_SIZE; nonce > 0 && i > 0; i--) { + s->session->ext.tick_nonce[i - 1] = nonce & 0xff; + nonce >>= 8; + } + s->session->ext.tick_nonce_len = TICKET_NONCE_SIZE; + + if (!tls13_hkdf_expand(s, md, s->resumption_master_secret, + (const unsigned char *)nonce_label, + sizeof(nonce_label) - 1, + s->session->ext.tick_nonce, + s->session->ext.tick_nonce_len, + s->session->master_key, + hashlen)) { + /* SSLfatal() already called */ + goto err; + } + s->session->master_key_length = hashlen; + s->session->time = (long)time(NULL); if (s->s3->alpn_selected != NULL) { OPENSSL_free(s->session->ext.alpn_selected); @@ -4002,7 +4032,13 @@ int tls_construct_new_session_ticket(SSL *s, WPACKET *pkt) /* SSLfatal() already called */ goto err; } + /* + * Increment both |sent_tickets| and |next_ticket_nonce|. |sent_tickets| + * gets reset to 0 if we send more tickets following a post-handshake + * auth, but |next_ticket_nonce| does not. + */ s->sent_tickets++; + s->next_ticket_nonce++; ssl_update_cache(s, SSL_SESS_CACHE_SERVER); } EVP_CIPHER_CTX_free(ctx); diff --git a/ssl/tls13_enc.c b/ssl/tls13_enc.c index 3fc8e96a89..264381bd00 100644 --- a/ssl/tls13_enc.c +++ b/ssl/tls13_enc.c @@ -602,12 +602,11 @@ int tls13_change_cipher_state(SSL *s, int which) if (!tls13_hkdf_expand(s, ssl_handshake_md(s), insecret, resumption_master_secret, sizeof(resumption_master_secret) - 1, - hashval, hashlen, s->session->master_key, + hashval, hashlen, s->resumption_master_secret, hashlen)) { /* SSLfatal() already called */ goto err; } - s->session->master_key_length = hashlen; } if (!derive_secret_key_and_iv(s, which & SSL3_CC_WRITE, md, cipher,