From 6a11d5c5ededa1543c2eeb2f9edcbe39bc58bb70 Mon Sep 17 00:00:00 2001 From: Matt Caswell Date: Wed, 13 Jun 2018 11:59:43 +0100 Subject: [PATCH] Restructure the ticket construction code Separate out as a new function the code to write out data which is specific to a stateless ticket. Reviewed-by: Rich Salz Reviewed-by: Viktor Dukhovni (Merged from https://github.com/openssl/openssl/pull/6563) --- include/openssl/sslerr.h | 1 + ssl/ssl_err.c | 2 + ssl/statem/statem_srvr.c | 313 ++++++++++++++++++++++----------------- 3 files changed, 183 insertions(+), 133 deletions(-) diff --git a/include/openssl/sslerr.h b/include/openssl/sslerr.h index 8e395cdd2d..d7d58e5eef 100644 --- a/include/openssl/sslerr.h +++ b/include/openssl/sslerr.h @@ -27,6 +27,7 @@ int ERR_load_SSL_strings(void); # define SSL_F_CONSTRUCT_CA_NAMES 552 # define SSL_F_CONSTRUCT_KEY_EXCHANGE_TBS 553 # define SSL_F_CREATE_SYNTHETIC_MESSAGE_HASH 539 +# define SSL_F_CREATE_TICKET_PREQUEL 636 # define SSL_F_CT_MOVE_SCTS 345 # define SSL_F_CT_STRICT 349 # define SSL_F_CUSTOM_EXT_ADD 554 diff --git a/ssl/ssl_err.c b/ssl/ssl_err.c index bce203654a..6f6430e337 100644 --- a/ssl/ssl_err.c +++ b/ssl/ssl_err.c @@ -26,6 +26,8 @@ static const ERR_STRING_DATA SSL_str_functs[] = { "construct_key_exchange_tbs"}, {ERR_PACK(ERR_LIB_SSL, SSL_F_CREATE_SYNTHETIC_MESSAGE_HASH, 0), "create_synthetic_message_hash"}, + {ERR_PACK(ERR_LIB_SSL, SSL_F_CREATE_TICKET_PREQUEL, 0), + "create_ticket_prequel"}, {ERR_PACK(ERR_LIB_SSL, SSL_F_CT_MOVE_SCTS, 0), "ct_move_scts"}, {ERR_PACK(ERR_LIB_SSL, SSL_F_CT_STRICT, 0), "ct_strict"}, {ERR_PACK(ERR_LIB_SSL, SSL_F_CUSTOM_EXT_ADD, 0), "custom_ext_add"}, diff --git a/ssl/statem/statem_srvr.c b/ssl/statem/statem_srvr.c index df3f15a789..a4a0ea2438 100644 --- a/ssl/statem/statem_srvr.c +++ b/ssl/statem/statem_srvr.c @@ -3739,7 +3739,44 @@ int tls_construct_server_certificate(SSL *s, WPACKET *pkt) return 1; } -int tls_construct_new_session_ticket(SSL *s, WPACKET *pkt) +static int create_ticket_prequel(SSL *s, WPACKET *pkt, uint32_t age_add, + unsigned char *tick_nonce) +{ + /* + * Ticket lifetime hint: For TLSv1.2 this is advisory only and we leave this + * unspecified for resumed session (for simplicity). + * In TLSv1.3 we reset the "time" field above, and always specify the + * timeout. + */ + if (!WPACKET_put_bytes_u32(pkt, + (s->hit && !SSL_IS_TLS13(s)) + ? 0 : s->session->timeout)) { + SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_CREATE_TICKET_PREQUEL, + ERR_R_INTERNAL_ERROR); + return 0; + } + + if (SSL_IS_TLS13(s)) { + if (!WPACKET_put_bytes_u32(pkt, age_add) + || !WPACKET_sub_memcpy_u8(pkt, tick_nonce, TICKET_NONCE_SIZE)) { + SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_CREATE_TICKET_PREQUEL, + ERR_R_INTERNAL_ERROR); + return 0; + } + } + + /* Start the sub-packet for the actual ticket data */ + if (!WPACKET_start_sub_packet_u16(pkt)) { + SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_CREATE_TICKET_PREQUEL, + ERR_R_INTERNAL_ERROR); + return 0; + } + + return 1; +} + +static int construct_stateless_ticket(SSL *s, WPACKET *pkt, uint32_t age_add, + unsigned char *tick_nonce) { unsigned char *senc = NULL; EVP_CIPHER_CTX *ctx = NULL; @@ -3752,115 +3789,8 @@ int tls_construct_new_session_ticket(SSL *s, WPACKET *pkt) SSL_CTX *tctx = s->session_ctx; unsigned char iv[EVP_MAX_IV_LENGTH]; unsigned char key_name[TLSEXT_KEYNAME_LENGTH]; - int iv_len; - unsigned char tick_nonce[TICKET_NONCE_SIZE]; + int iv_len, ok = 0; size_t macoffset, macendoffset; - union { - unsigned char age_add_c[sizeof(uint32_t)]; - uint32_t age_add; - } age_add_u; - - if (SSL_IS_TLS13(s)) { - size_t i, hashlen; - uint64_t nonce; - static const unsigned 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; - else if (s->ctx->info_callback != NULL) - cb = s->ctx->info_callback; - - if (cb != NULL) { - /* - * We don't start and stop the handshake in between each ticket when - * sending more than one - but it should appear that way to the info - * callback. - */ - if (s->sent_tickets != 0) { - ossl_statem_set_in_init(s, 0); - cb(s, SSL_CB_HANDSHAKE_DONE, 1); - ossl_statem_set_in_init(s, 1); - } - cb(s, SSL_CB_HANDSHAKE_START, 1); - } - /* - * If we already sent one NewSessionTicket, or we resumed then - * s->session may already be in a cache and so we must not modify it. - * Instead we need to take a copy of it and modify that. - */ - if (s->sent_tickets != 0 || s->hit) { - SSL_SESSION *new_sess = ssl_session_dup(s->session, 0); - - if (new_sess == NULL) { - /* SSLfatal already called */ - goto err; - } - - SSL_SESSION_free(s->session); - s->session = new_sess; - } - - if (!ssl_generate_session_id(s, s->session)) { - /* SSLfatal() already called */ - goto err; - } - if (RAND_bytes(age_add_u.age_add_c, sizeof(age_add_u)) <= 0) { - SSLfatal(s, SSL_AD_INTERNAL_ERROR, - SSL_F_TLS_CONSTRUCT_NEW_SESSION_TICKET, - ERR_R_INTERNAL_ERROR); - goto err; - } - s->session->ext.tick_age_add = age_add_u.age_add; - - nonce = s->next_ticket_nonce; - for (i = TICKET_NONCE_SIZE; i > 0; i--) { - tick_nonce[i - 1] = (unsigned char)(nonce & 0xff); - nonce >>= 8; - } - - if (!tls13_hkdf_expand(s, md, s->resumption_master_secret, - nonce_label, - sizeof(nonce_label) - 1, - tick_nonce, - TICKET_NONCE_SIZE, - 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); - s->session->ext.alpn_selected = - OPENSSL_memdup(s->s3->alpn_selected, s->s3->alpn_selected_len); - if (s->session->ext.alpn_selected == NULL) { - SSLfatal(s, SSL_AD_INTERNAL_ERROR, - SSL_F_TLS_CONSTRUCT_NEW_SESSION_TICKET, - ERR_R_MALLOC_FAILURE); - goto err; - } - s->session->ext.alpn_selected_len = s->s3->alpn_selected_len; - } - s->session->ext.max_early_data = s->max_early_data; - } - - if (tctx->generate_ticket_cb != NULL && - tctx->generate_ticket_cb(s, tctx->ticket_cb_data) == 0) - goto err; /* get session encoding length */ slen_full = i2d_SSL_SESSION(s->session, NULL); @@ -3973,22 +3903,12 @@ int tls_construct_new_session_ticket(SSL *s, WPACKET *pkt) sizeof(tctx->ext.tick_key_name)); } - /* - * Ticket lifetime hint: For TLSv1.2 this is advisory only and we leave this - * unspecified for resumed session (for simplicity). - * In TLSv1.3 we reset the "time" field above, and always specify the - * timeout. - */ - if (!WPACKET_put_bytes_u32(pkt, - (s->hit && !SSL_IS_TLS13(s)) - ? 0 : s->session->timeout) - || (SSL_IS_TLS13(s) - && (!WPACKET_put_bytes_u32(pkt, age_add_u.age_add) - || !WPACKET_sub_memcpy_u8(pkt, tick_nonce, - TICKET_NONCE_SIZE))) - /* Now the actual ticket data */ - || !WPACKET_start_sub_packet_u16(pkt) - || !WPACKET_get_total_written(pkt, &macoffset) + if (!create_ticket_prequel(s, pkt, age_add, tick_nonce)) { + /* SSLfatal() already called */ + goto err; + } + + if (!WPACKET_get_total_written(pkt, &macoffset) /* Output key name */ || !WPACKET_memcpy(pkt, key_name, sizeof(key_name)) /* output IV */ @@ -4011,12 +3931,145 @@ int tls_construct_new_session_ticket(SSL *s, WPACKET *pkt) || !HMAC_Final(hctx, macdata1, &hlen) || hlen > EVP_MAX_MD_SIZE || !WPACKET_allocate_bytes(pkt, hlen, &macdata2) - || macdata1 != macdata2 - || !WPACKET_close(pkt)) { + || macdata1 != macdata2) { + SSLfatal(s, SSL_AD_INTERNAL_ERROR, + SSL_F_TLS_CONSTRUCT_NEW_SESSION_TICKET, ERR_R_INTERNAL_ERROR); + goto err; + } + + /* Close the sub-packet created by create_ticket_prequel() */ + if (!WPACKET_close(pkt)) { SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS_CONSTRUCT_NEW_SESSION_TICKET, ERR_R_INTERNAL_ERROR); goto err; } + + ok = 1; + err: + OPENSSL_free(senc); + EVP_CIPHER_CTX_free(ctx); + HMAC_CTX_free(hctx); + return ok; +} + +int tls_construct_new_session_ticket(SSL *s, WPACKET *pkt) +{ + SSL_CTX *tctx = s->session_ctx; + unsigned char tick_nonce[TICKET_NONCE_SIZE]; + union { + unsigned char age_add_c[sizeof(uint32_t)]; + uint32_t age_add; + } age_add_u; + + age_add_u.age_add = 0; + + if (SSL_IS_TLS13(s)) { + size_t i, hashlen; + uint64_t nonce; + static const unsigned 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; + else if (s->ctx->info_callback != NULL) + cb = s->ctx->info_callback; + + if (cb != NULL) { + /* + * We don't start and stop the handshake in between each ticket when + * sending more than one - but it should appear that way to the info + * callback. + */ + if (s->sent_tickets != 0) { + ossl_statem_set_in_init(s, 0); + cb(s, SSL_CB_HANDSHAKE_DONE, 1); + ossl_statem_set_in_init(s, 1); + } + cb(s, SSL_CB_HANDSHAKE_START, 1); + } + /* + * If we already sent one NewSessionTicket, or we resumed then + * s->session may already be in a cache and so we must not modify it. + * Instead we need to take a copy of it and modify that. + */ + if (s->sent_tickets != 0 || s->hit) { + SSL_SESSION *new_sess = ssl_session_dup(s->session, 0); + + if (new_sess == NULL) { + /* SSLfatal already called */ + goto err; + } + + SSL_SESSION_free(s->session); + s->session = new_sess; + } + + if (!ssl_generate_session_id(s, s->session)) { + /* SSLfatal() already called */ + goto err; + } + if (RAND_bytes(age_add_u.age_add_c, sizeof(age_add_u)) <= 0) { + SSLfatal(s, SSL_AD_INTERNAL_ERROR, + SSL_F_TLS_CONSTRUCT_NEW_SESSION_TICKET, + ERR_R_INTERNAL_ERROR); + goto err; + } + s->session->ext.tick_age_add = age_add_u.age_add; + + nonce = s->next_ticket_nonce; + for (i = TICKET_NONCE_SIZE; i > 0; i--) { + tick_nonce[i - 1] = (unsigned char)(nonce & 0xff); + nonce >>= 8; + } + + if (!tls13_hkdf_expand(s, md, s->resumption_master_secret, + nonce_label, + sizeof(nonce_label) - 1, + tick_nonce, + TICKET_NONCE_SIZE, + 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); + s->session->ext.alpn_selected = + OPENSSL_memdup(s->s3->alpn_selected, s->s3->alpn_selected_len); + if (s->session->ext.alpn_selected == NULL) { + SSLfatal(s, SSL_AD_INTERNAL_ERROR, + SSL_F_TLS_CONSTRUCT_NEW_SESSION_TICKET, + ERR_R_MALLOC_FAILURE); + goto err; + } + s->session->ext.alpn_selected_len = s->s3->alpn_selected_len; + } + s->session->ext.max_early_data = s->max_early_data; + } + + if (tctx->generate_ticket_cb != NULL && + tctx->generate_ticket_cb(s, tctx->ticket_cb_data) == 0) + goto err; + + if (!construct_stateless_ticket(s, pkt, age_add_u.age_add, tick_nonce)) { + /* SSLfatal() already called */ + goto err; + } + if (SSL_IS_TLS13(s)) { if (!tls_construct_extensions(s, pkt, SSL_EXT_TLS1_3_NEW_SESSION_TICKET, @@ -4033,15 +4086,9 @@ int tls_construct_new_session_ticket(SSL *s, WPACKET *pkt) s->next_ticket_nonce++; ssl_update_cache(s, SSL_SESS_CACHE_SERVER); } - EVP_CIPHER_CTX_free(ctx); - HMAC_CTX_free(hctx); - OPENSSL_free(senc); return 1; err: - OPENSSL_free(senc); - EVP_CIPHER_CTX_free(ctx); - HMAC_CTX_free(hctx); return 0; } -- 2.34.1