From 1bf4cb0fe3b00e1c501a04ace4e3e3145314cb20 Mon Sep 17 00:00:00 2001 From: Matt Caswell Date: Tue, 4 Sep 2018 13:36:55 +0100 Subject: [PATCH] Process KeyUpdate and NewSessionTicket messages after a close_notify If we've sent a close_notify then we are restricted about what we can do in response to handshake messages that we receive. However we can sensibly process NewSessionTicket messages. We can also process a KeyUpdate message as long as we also ignore any request for us to update our sending keys. Reviewed-by: Tim Hudson (Merged from https://github.com/openssl/openssl/pull/7114) --- ssl/record/rec_layer_s3.c | 42 ++++++++++++++++++++------------------- ssl/statem/statem_clnt.c | 27 ++++++++++++++++++++----- ssl/statem/statem_lib.c | 7 +++++-- 3 files changed, 49 insertions(+), 27 deletions(-) diff --git a/ssl/record/rec_layer_s3.c b/ssl/record/rec_layer_s3.c index d208695b16..6d495715b2 100644 --- a/ssl/record/rec_layer_s3.c +++ b/ssl/record/rec_layer_s3.c @@ -1554,30 +1554,30 @@ int ssl3_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf, return -1; } - /* - * If we've sent a close_notify but not yet received one back then ditch - * anything we read. - */ if ((s->shutdown & SSL_SENT_SHUTDOWN) != 0) { - /* - * In TLSv1.3 this could get problematic if we receive a KeyUpdate - * message after we sent a close_notify because we're about to ditch it, - * so we won't be able to read a close_notify sent afterwards! We don't - * support that. - */ - SSL3_RECORD_set_length(rr, 0); - SSL3_RECORD_set_read(rr); - if (SSL3_RECORD_get_type(rr) == SSL3_RT_HANDSHAKE) { BIO *rbio; - if ((s->mode & SSL_MODE_AUTO_RETRY) != 0) - goto start; + /* + * We ignore any handshake messages sent to us unless they are + * TLSv1.3 in which case we want to process them. For all other + * handshake messages we can't do anything reasonable with them + * because we are unable to write any response due to having already + * sent close_notify. + */ + if (!SSL_IS_TLS13(s)) { + SSL3_RECORD_set_length(rr, 0); + SSL3_RECORD_set_read(rr); - s->rwstate = SSL_READING; - rbio = SSL_get_rbio(s); - BIO_clear_retry_flags(rbio); - BIO_set_retry_read(rbio); + if ((s->mode & SSL_MODE_AUTO_RETRY) != 0) + goto start; + + s->rwstate = SSL_READING; + rbio = SSL_get_rbio(s); + BIO_clear_retry_flags(rbio); + BIO_set_retry_read(rbio); + return -1; + } } else { /* * The peer is continuing to send application data, but we have @@ -1586,10 +1586,12 @@ int ssl3_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf, * above. * No alert sent because we already sent close_notify */ + SSL3_RECORD_set_length(rr, 0); + SSL3_RECORD_set_read(rr); SSLfatal(s, SSL_AD_NO_ALERT, SSL_F_SSL3_READ_BYTES, SSL_R_APPLICATION_DATA_AFTER_CLOSE_NOTIFY); + return -1; } - return -1; } /* diff --git a/ssl/statem/statem_clnt.c b/ssl/statem/statem_clnt.c index 3dc29cc757..8c658da899 100644 --- a/ssl/statem/statem_clnt.c +++ b/ssl/statem/statem_clnt.c @@ -423,11 +423,19 @@ static WRITE_TRAN ossl_statem_client13_write_transition(SSL *s) st->hand_state = TLS_ST_CW_CERT; return WRITE_TRAN_CONTINUE; } - /* Shouldn't happen - same as default case */ - SSLfatal(s, SSL_AD_INTERNAL_ERROR, - SSL_F_OSSL_STATEM_CLIENT13_WRITE_TRANSITION, - ERR_R_INTERNAL_ERROR); - return WRITE_TRAN_ERROR; + /* + * We should only get here if we received a CertificateRequest after + * we already sent close_notify + */ + if (!ossl_assert((s->shutdown & SSL_SENT_SHUTDOWN) != 0)) { + /* Shouldn't happen - same as default case */ + SSLfatal(s, SSL_AD_INTERNAL_ERROR, + SSL_F_OSSL_STATEM_CLIENT13_WRITE_TRANSITION, + ERR_R_INTERNAL_ERROR); + return WRITE_TRAN_ERROR; + } + st->hand_state = TLS_ST_OK; + return WRITE_TRAN_CONTINUE; case TLS_ST_CR_FINISHED: if (s->early_data_state == SSL_EARLY_DATA_WRITE_RETRY @@ -2446,6 +2454,15 @@ MSG_PROCESS_RETURN tls_process_certificate_request(SSL *s, PACKET *pkt) PACKET reqctx, extensions; RAW_EXTENSION *rawexts = NULL; + if ((s->shutdown & SSL_SENT_SHUTDOWN) != 0) { + /* + * We already sent close_notify. This can only happen in TLSv1.3 + * post-handshake messages. We can't reasonably respond to this, so + * we just ignore it + */ + return MSG_PROCESS_FINISHED_READING; + } + /* Free and zero certificate types: it is not present in TLS 1.3 */ OPENSSL_free(s->s3->tmp.ctype); s->s3->tmp.ctype = NULL; diff --git a/ssl/statem/statem_lib.c b/ssl/statem/statem_lib.c index 3961c14719..adc8b98144 100644 --- a/ssl/statem/statem_lib.c +++ b/ssl/statem/statem_lib.c @@ -638,9 +638,12 @@ MSG_PROCESS_RETURN tls_process_key_update(SSL *s, PACKET *pkt) /* * If we get a request for us to update our sending keys too then, we need * to additionally send a KeyUpdate message. However that message should - * not also request an update (otherwise we get into an infinite loop). + * not also request an update (otherwise we get into an infinite loop). We + * ignore a request for us to update our sending keys too if we already + * sent close_notify. */ - if (updatetype == SSL_KEY_UPDATE_REQUESTED) + if (updatetype == SSL_KEY_UPDATE_REQUESTED + && (s->shutdown & SSL_SENT_SHUTDOWN) == 0) s->key_update = SSL_KEY_UPDATE_NOT_REQUESTED; if (!tls13_update_key(s, 0)) { -- 2.25.1