diff options
author | Matt Caswell <matt@openssl.org> | 2019-01-27 11:00:16 +0000 |
---|---|---|
committer | Matt Caswell <matt@openssl.org> | 2019-02-14 16:17:34 +0000 |
commit | 4af5836b55442f31795eff6c8c81ea7a1b8cf94b (patch) | |
tree | 9c0e2318753afbc715e71ad91dbf557205a2e4a5 | |
parent | 3c83c5ba4f6502c708b7a5f55c98a10e312668da (diff) |
Don't signal SSL_CB_HANDSHAKE_START for TLSv1.3 post-handshake messages
The original 1.1.1 design was to use SSL_CB_HANDSHAKE_START and
SSL_CB_HANDSHAKE_DONE to signal start/end of a post-handshake message
exchange in TLSv1.3. Unfortunately experience has shown that this confuses
some applications who mistake it for a TLSv1.2 renegotiation. This means
that KeyUpdate messages are not handled properly.
This commit removes the use of SSL_CB_HANDSHAKE_START and
SSL_CB_HANDSHAKE_DONE to signal the start/end of a post-handshake
message exchange. Individual post-handshake messages are still signalled in
the normal way.
This is a potentially breaking change if there are any applications already
written that expect to see these TLSv1.3 events. However, without it,
KeyUpdate is not currently usable for many applications.
Fixes #8069
Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/8096)
-rw-r--r-- | CHANGES | 13 | ||||
-rw-r--r-- | doc/man3/SSL_CTX_set_info_callback.pod | 14 | ||||
-rw-r--r-- | ssl/statem/statem.c | 6 | ||||
-rw-r--r-- | ssl/statem/statem_lib.c | 11 | ||||
-rw-r--r-- | ssl/statem/statem_srvr.c | 19 | ||||
-rw-r--r-- | test/sslapitest.c | 49 |
6 files changed, 51 insertions, 61 deletions
@@ -119,6 +119,19 @@ applications with zero-copy system calls such as sendfile and splice. [Boris Pismenny] + Changes between 1.1.1a and 1.1.1b [xx XXX xxxx] + + *) Change the info callback signals for the start and end of a post-handshake + message exchange in TLSv1.3. In 1.1.1/1.1.1a we used SSL_CB_HANDSHAKE_START + and SSL_CB_HANDSHAKE_DONE. Experience has shown that many applications get + confused by this and assume that a TLSv1.2 renegotiation has started. This + can break KeyUpdate handling. Instead we no longer signal the start and end + of a post handshake message exchange (although the messages themselves are + still signalled). This could break some applications that were expecting + the old signals. However without this KeyUpdate is not usable for many + applications. + [Matt Caswell] + Changes between 1.1.1 and 1.1.1a [20 Nov 2018] *) Timing vulnerability in DSA signature generation diff --git a/doc/man3/SSL_CTX_set_info_callback.pod b/doc/man3/SSL_CTX_set_info_callback.pod index cb8f9965b1..3248e10c09 100644 --- a/doc/man3/SSL_CTX_set_info_callback.pod +++ b/doc/man3/SSL_CTX_set_info_callback.pod @@ -92,17 +92,13 @@ Callback has been called due to an alert being sent or received. =item SSL_CB_HANDSHAKE_START -Callback has been called because a new handshake is started. In TLSv1.3 this is -also used for the start of post-handshake message exchanges such as for the -exchange of session tickets, or for key updates. It also occurs when resuming a -handshake following a pause to handle early data. +Callback has been called because a new handshake is started. It also occurs when +resuming a handshake following a pause to handle early data. -=item SSL_CB_HANDSHAKE_DONE 0x20 +=item SSL_CB_HANDSHAKE_DONE -Callback has been called because a handshake is finished. In TLSv1.3 this is -also used at the end of an exchange of post-handshake messages such as for -session tickets or key updates. It also occurs if the handshake is paused to -allow the exchange of early data. +Callback has been called because a handshake is finished. It also occurs if the +handshake is paused to allow the exchange of early data. =back diff --git a/ssl/statem/statem.c b/ssl/statem/statem.c index ebe471b65c..24c7e94ef1 100644 --- a/ssl/statem/statem.c +++ b/ssl/statem/statem.c @@ -342,8 +342,10 @@ static int state_machine(SSL *s, int server) } s->server = server; - if (cb != NULL) - cb(s, SSL_CB_HANDSHAKE_START, 1); + if (cb != NULL) { + if (SSL_IS_FIRST_HANDSHAKE(s) || !SSL_IS_TLS13(s)) + cb(s, SSL_CB_HANDSHAKE_START, 1); + } /* * Fatal errors in this block don't send an alert because we have diff --git a/ssl/statem/statem_lib.c b/ssl/statem/statem_lib.c index 2f78a3f602..8a7ada8f51 100644 --- a/ssl/statem/statem_lib.c +++ b/ssl/statem/statem_lib.c @@ -1030,6 +1030,7 @@ unsigned long ssl3_output_cert_chain(SSL *s, WPACKET *pkt, CERT_PKEY *cpk) WORK_STATE tls_finish_handshake(SSL *s, WORK_STATE wst, int clearbufs, int stop) { void (*cb) (const SSL *ssl, int type, int val) = NULL; + int cleanuphand = s->statem.cleanuphand; if (clearbufs) { if (!SSL_IS_DTLS(s)) { @@ -1056,7 +1057,7 @@ WORK_STATE tls_finish_handshake(SSL *s, WORK_STATE wst, int clearbufs, int stop) * Only set if there was a Finished message and this isn't after a TLSv1.3 * post handshake exchange */ - if (s->statem.cleanuphand) { + if (cleanuphand) { /* skipped if we just sent a HelloRequest */ s->renegotiate = 0; s->new_session = 0; @@ -1116,8 +1117,12 @@ WORK_STATE tls_finish_handshake(SSL *s, WORK_STATE wst, int clearbufs, int stop) /* The callback may expect us to not be in init at handshake done */ ossl_statem_set_in_init(s, 0); - if (cb != NULL) - cb(s, SSL_CB_HANDSHAKE_DONE, 1); + if (cb != NULL) { + if (cleanuphand + || !SSL_IS_TLS13(s) + || SSL_IS_FIRST_HANDSHAKE(s)) + cb(s, SSL_CB_HANDSHAKE_DONE, 1); + } if (!stop) { /* If we've got more work to do we go back into init */ diff --git a/ssl/statem/statem_srvr.c b/ssl/statem/statem_srvr.c index f76568cb0e..bf1819d356 100644 --- a/ssl/statem/statem_srvr.c +++ b/ssl/statem/statem_srvr.c @@ -4040,7 +4040,6 @@ int tls_construct_new_session_ticket(SSL *s, WPACKET *pkt) 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 */ @@ -4052,24 +4051,6 @@ int tls_construct_new_session_ticket(SSL *s, WPACKET *pkt) } 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. diff --git a/test/sslapitest.c b/test/sslapitest.c index 6b44c160b1..02eb1fef7e 100644 --- a/test/sslapitest.c +++ b/test/sslapitest.c @@ -4919,18 +4919,14 @@ static struct info_cb_states_st { {SSL_CB_LOOP, "TWCCS"}, {SSL_CB_LOOP, "TWEE"}, {SSL_CB_LOOP, "TWSC"}, {SSL_CB_LOOP, "TRSCV"}, {SSL_CB_LOOP, "TWFIN"}, {SSL_CB_LOOP, "TED"}, {SSL_CB_EXIT, NULL}, {SSL_CB_LOOP, "TED"}, {SSL_CB_LOOP, "TRFIN"}, - {SSL_CB_HANDSHAKE_DONE, NULL}, {SSL_CB_HANDSHAKE_START, NULL}, - {SSL_CB_LOOP, "TWST"}, {SSL_CB_HANDSHAKE_DONE, NULL}, - {SSL_CB_HANDSHAKE_START, NULL}, {SSL_CB_LOOP, "TWST"}, - {SSL_CB_HANDSHAKE_DONE, NULL}, {SSL_CB_EXIT, NULL}, - {SSL_CB_ALERT, NULL}, {SSL_CB_HANDSHAKE_START, NULL}, - {SSL_CB_LOOP, "PINIT "}, {SSL_CB_LOOP, "PINIT "}, {SSL_CB_LOOP, "TRCH"}, - {SSL_CB_LOOP, "TWSH"}, {SSL_CB_LOOP, "TWCCS"}, {SSL_CB_LOOP, "TWEE"}, - {SSL_CB_LOOP, "TWFIN"}, {SSL_CB_LOOP, "TED"}, {SSL_CB_EXIT, NULL}, - {SSL_CB_LOOP, "TED"}, {SSL_CB_LOOP, "TRFIN"}, - {SSL_CB_HANDSHAKE_DONE, NULL}, {SSL_CB_HANDSHAKE_START, NULL}, - {SSL_CB_LOOP, "TWST"}, {SSL_CB_HANDSHAKE_DONE, NULL}, - {SSL_CB_EXIT, NULL}, {0, NULL}, + {SSL_CB_HANDSHAKE_DONE, NULL}, {SSL_CB_LOOP, "TWST"}, + {SSL_CB_LOOP, "TWST"}, {SSL_CB_EXIT, NULL}, {SSL_CB_ALERT, NULL}, + {SSL_CB_HANDSHAKE_START, NULL}, {SSL_CB_LOOP, "PINIT "}, + {SSL_CB_LOOP, "PINIT "}, {SSL_CB_LOOP, "TRCH"}, {SSL_CB_LOOP, "TWSH"}, + {SSL_CB_LOOP, "TWCCS"}, {SSL_CB_LOOP, "TWEE"}, {SSL_CB_LOOP, "TWFIN"}, + {SSL_CB_LOOP, "TED"}, {SSL_CB_EXIT, NULL}, {SSL_CB_LOOP, "TED"}, + {SSL_CB_LOOP, "TRFIN"}, {SSL_CB_HANDSHAKE_DONE, NULL}, + {SSL_CB_LOOP, "TWST"}, {SSL_CB_EXIT, NULL}, {0, NULL}, }, { /* TLSv1.3 client followed by resumption */ {SSL_CB_HANDSHAKE_START, NULL}, {SSL_CB_LOOP, "PINIT "}, @@ -4938,20 +4934,16 @@ static struct info_cb_states_st { {SSL_CB_LOOP, "TRSH"}, {SSL_CB_LOOP, "TREE"}, {SSL_CB_LOOP, "TRSC"}, {SSL_CB_LOOP, "TRSCV"}, {SSL_CB_LOOP, "TRFIN"}, {SSL_CB_LOOP, "TWCCS"}, {SSL_CB_LOOP, "TWFIN"}, {SSL_CB_HANDSHAKE_DONE, NULL}, - {SSL_CB_EXIT, NULL}, {SSL_CB_HANDSHAKE_START, NULL}, - {SSL_CB_LOOP, "SSLOK "}, {SSL_CB_LOOP, "SSLOK "}, {SSL_CB_LOOP, "TRST"}, - {SSL_CB_HANDSHAKE_DONE, NULL}, {SSL_CB_EXIT, NULL}, - {SSL_CB_HANDSHAKE_START, NULL}, {SSL_CB_LOOP, "SSLOK "}, - {SSL_CB_LOOP, "SSLOK "}, {SSL_CB_LOOP, "TRST"}, - {SSL_CB_HANDSHAKE_DONE, NULL}, {SSL_CB_EXIT, NULL}, + {SSL_CB_EXIT, NULL}, {SSL_CB_LOOP, "SSLOK "}, {SSL_CB_LOOP, "SSLOK "}, + {SSL_CB_LOOP, "TRST"}, {SSL_CB_EXIT, NULL}, {SSL_CB_LOOP, "SSLOK "}, + {SSL_CB_LOOP, "SSLOK "}, {SSL_CB_LOOP, "TRST"}, {SSL_CB_EXIT, NULL}, {SSL_CB_ALERT, NULL}, {SSL_CB_HANDSHAKE_START, NULL}, {SSL_CB_LOOP, "PINIT "}, {SSL_CB_LOOP, "TWCH"}, {SSL_CB_EXIT, NULL}, {SSL_CB_LOOP, "TWCH"}, {SSL_CB_LOOP, "TRSH"}, {SSL_CB_LOOP, "TREE"}, {SSL_CB_LOOP, "TRFIN"}, {SSL_CB_LOOP, "TWCCS"}, {SSL_CB_LOOP, "TWFIN"}, {SSL_CB_HANDSHAKE_DONE, NULL}, {SSL_CB_EXIT, NULL}, - {SSL_CB_HANDSHAKE_START, NULL}, {SSL_CB_LOOP, "SSLOK "}, - {SSL_CB_LOOP, "SSLOK "}, {SSL_CB_LOOP, "TRST"}, - {SSL_CB_HANDSHAKE_DONE, NULL}, {SSL_CB_EXIT, NULL}, {0, NULL}, + {SSL_CB_LOOP, "SSLOK "}, {SSL_CB_LOOP, "SSLOK "}, {SSL_CB_LOOP, "TRST"}, + {SSL_CB_EXIT, NULL}, {0, NULL}, }, { /* TLSv1.3 server, early_data */ {SSL_CB_HANDSHAKE_START, NULL}, {SSL_CB_LOOP, "PINIT "}, @@ -4960,8 +4952,7 @@ static struct info_cb_states_st { {SSL_CB_HANDSHAKE_DONE, NULL}, {SSL_CB_EXIT, NULL}, {SSL_CB_HANDSHAKE_START, NULL}, {SSL_CB_LOOP, "TED"}, {SSL_CB_LOOP, "TED"}, {SSL_CB_LOOP, "TWEOED"}, {SSL_CB_LOOP, "TRFIN"}, - {SSL_CB_HANDSHAKE_DONE, NULL}, {SSL_CB_HANDSHAKE_START, NULL}, - {SSL_CB_LOOP, "TWST"}, {SSL_CB_HANDSHAKE_DONE, NULL}, + {SSL_CB_HANDSHAKE_DONE, NULL}, {SSL_CB_LOOP, "TWST"}, {SSL_CB_EXIT, NULL}, {0, NULL}, }, { /* TLSv1.3 client, early_data */ @@ -4972,9 +4963,8 @@ static struct info_cb_states_st { {SSL_CB_LOOP, "TED"}, {SSL_CB_LOOP, "TRSH"}, {SSL_CB_LOOP, "TREE"}, {SSL_CB_LOOP, "TRFIN"}, {SSL_CB_LOOP, "TPEDE"}, {SSL_CB_LOOP, "TWEOED"}, {SSL_CB_LOOP, "TWFIN"}, {SSL_CB_HANDSHAKE_DONE, NULL}, - {SSL_CB_EXIT, NULL}, {SSL_CB_HANDSHAKE_START, NULL}, - {SSL_CB_LOOP, "SSLOK "}, {SSL_CB_LOOP, "SSLOK "}, {SSL_CB_LOOP, "TRST"}, - {SSL_CB_HANDSHAKE_DONE, NULL}, {SSL_CB_EXIT, NULL}, {0, NULL}, + {SSL_CB_EXIT, NULL}, {SSL_CB_LOOP, "SSLOK "}, {SSL_CB_LOOP, "SSLOK "}, + {SSL_CB_LOOP, "TRST"}, {SSL_CB_EXIT, NULL}, {0, NULL}, }, { {0, NULL}, } @@ -5013,8 +5003,11 @@ static void sslapi_info_callback(const SSL *s, int where, int ret) return; } - /* Check that, if we've got SSL_CB_HANDSHAKE_DONE we are not in init */ - if ((where & SSL_CB_HANDSHAKE_DONE) && SSL_in_init((SSL *)s) != 0) { + /* + * Check that, if we've got SSL_CB_HANDSHAKE_DONE we are not in init + */ + if ((where & SSL_CB_HANDSHAKE_DONE) + && SSL_in_init((SSL *)s) != 0) { info_cb_failed = 1; return; } |