From 3e93c5fe1eab677500448e18e4274b26e4b246ae Mon Sep 17 00:00:00 2001 From: Matt Caswell Date: Fri, 12 Aug 2022 13:24:19 +0100 Subject: [PATCH] If a ticket key callback returns 0 in TLSv1.3 don't send a ticket If we can't construct the ticket don't send one. This requires a change to the TLS state machine to be able to a handle a construction function deciding not to send a message after all. Fixes #18977 Reviewed-by: Viktor Dukhovni Reviewed-by: Tomas Mraz Reviewed-by: Hugo Landau (Merged from https://github.com/openssl/openssl/pull/18990) --- ssl/statem/statem.c | 22 +++++++++++--- ssl/statem/statem_srvr.c | 64 +++++++++++++++++++++++++++++----------- 2 files changed, 64 insertions(+), 22 deletions(-) diff --git a/ssl/statem/statem.c b/ssl/statem/statem.c index 7daa220443..ad227d2a9f 100644 --- a/ssl/statem/statem.c +++ b/ssl/statem/statem.c @@ -888,10 +888,24 @@ static SUB_STATE_RETURN write_state_machine(SSL_CONNECTION *s) SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR); return SUB_STATE_ERROR; } - if (confunc != NULL && !confunc(s, &pkt)) { - WPACKET_cleanup(&pkt); - check_fatal(s); - return SUB_STATE_ERROR; + if (confunc != NULL) { + int tmpret; + + tmpret = confunc(s, &pkt); + if (tmpret <= 0) { + WPACKET_cleanup(&pkt); + check_fatal(s); + return SUB_STATE_ERROR; + } else if (tmpret == 2) { + /* + * The construction function decided not to construct the + * message after all and continue. Skip sending. + */ + WPACKET_cleanup(&pkt); + st->write_state = WRITE_STATE_POST_WORK; + st->write_state_work = WORK_MORE_A; + break; + } /* else success */ } if (!ssl_close_construct_packet(s, &pkt, mt) || !WPACKET_finish(&pkt)) { diff --git a/ssl/statem/statem_srvr.c b/ssl/statem/statem_srvr.c index ac7cdda9da..cafc18ef54 100644 --- a/ssl/statem/statem_srvr.c +++ b/ssl/statem/statem_srvr.c @@ -3696,6 +3696,10 @@ static int create_ticket_prequel(SSL_CONNECTION *s, WPACKET *pkt, return 1; } +/* + * Returns 1 on success, 0 to abort construction of the ticket (non-fatal), or + * -1 on fatal error + */ static int construct_stateless_ticket(SSL_CONNECTION *s, WPACKET *pkt, uint32_t age_add, unsigned char *tick_nonce) @@ -3711,7 +3715,7 @@ static int construct_stateless_ticket(SSL_CONNECTION *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, ok = 0; + int iv_len, ok = -1; size_t macoffset, macendoffset; SSL *ssl = SSL_CONNECTION_GET_SSL(s); SSL_CTX *sctx = SSL_CONNECTION_GET_CTX(s); @@ -3794,7 +3798,15 @@ static int construct_stateless_ticket(SSL_CONNECTION *s, WPACKET *pkt, #endif if (ret == 0) { - + /* + * In TLSv1.2 we construct a 0 length ticket. In TLSv1.3 a 0 + * length ticket is not allowed so we abort construction of the + * ticket + */ + if (SSL_CONNECTION_IS_TLS13(s)) { + ok = 0; + goto err; + } /* Put timeout and length */ if (!WPACKET_put_bytes_u32(pkt, 0) || !WPACKET_put_bytes_u16(pkt, 0)) { @@ -3908,6 +3920,20 @@ static int construct_stateful_ticket(SSL_CONNECTION *s, WPACKET *pkt, return 1; } +static void tls_update_ticket_counts(SSL_CONNECTION *s) +{ + /* + * 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. If we're sending extra + * tickets, decrement the count of pending extra tickets. + */ + s->sent_tickets++; + s->next_ticket_nonce++; + if (s->ext.extra_tickets_expected > 0) + s->ext.extra_tickets_expected--; +} + int tls_construct_new_session_ticket(SSL_CONNECTION *s, WPACKET *pkt) { SSL_CTX *tctx = s->session_ctx; @@ -3916,6 +3942,7 @@ int tls_construct_new_session_ticket(SSL_CONNECTION *s, WPACKET *pkt) unsigned char age_add_c[sizeof(uint32_t)]; uint32_t age_add; } age_add_u; + int ret = 0; age_add_u.age_add = 0; @@ -4014,10 +4041,20 @@ int tls_construct_new_session_ticket(SSL_CONNECTION *s, WPACKET *pkt) /* SSLfatal() already called */ goto err; } - } else if (!construct_stateless_ticket(s, pkt, age_add_u.age_add, - tick_nonce)) { - /* SSLfatal() already called */ - goto err; + } else { + int tmpret; + + tmpret = construct_stateless_ticket(s, pkt, age_add_u.age_add, + tick_nonce); + if (tmpret != 1) { + if (tmpret == 0) { + ret = 2; /* Non-fatal. Abort construction but continue */ + /* We count this as a success so update the counts anwyay */ + tls_update_ticket_counts(s); + } + /* else SSLfatal() already called */ + goto err; + } } if (SSL_CONNECTION_IS_TLS13(s)) { @@ -4027,22 +4064,13 @@ int tls_construct_new_session_ticket(SSL_CONNECTION *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. If we're sending extra - * tickets, decrement the count of pending extra tickets. - */ - s->sent_tickets++; - s->next_ticket_nonce++; - if (s->ext.extra_tickets_expected > 0) - s->ext.extra_tickets_expected--; + tls_update_ticket_counts(s); ssl_update_cache(s, SSL_SESS_CACHE_SERVER); } - return 1; + ret = 1; err: - return 0; + return ret; } /* -- 2.34.1