mirror/openssl
2
0
Fork 0
mirror of https://github.com/openssl/openssl.git synced 2025-01-30 14:01:55 +08:00
Code Issues Packages Projects Releases Wiki Activity

Don't signal SSL_CB_HANDSHAKE_START for TLSv1.3 post-handshake messages

Browse Source 
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)
This commit is contained in:
Matt Caswell 2019-01-27 11:00:16 +00:00
parent 3c83c5ba4f
commit 4af5836b55
6 changed files with 51 additions and 61 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

13
CHANGES
View File

@ -119,6 +119,19 @@
applications with zero-copy system calls such as sendfile and splice. applications with zero-copy system calls such as sendfile and splice.
[Boris Pismenny] [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] Changes between 1.1.1 and 1.1.1a [20 Nov 2018]
*) Timing vulnerability in DSA signature generation *) Timing vulnerability in DSA signature generation

14
doc/man3/SSL_CTX_set_info_callback.pod
View File

@ -92,17 +92,13 @@ Callback has been called due to an alert being sent or received.
=item SSL_CB_HANDSHAKE_START =item SSL_CB_HANDSHAKE_START
Callback has been called because a new handshake is started. In TLSv1.3 this is Callback has been called because a new handshake is started. It also occurs when
also used for the start of post-handshake message exchanges such as for the resuming a handshake following a pause to handle early data.
exchange of session tickets, or for key updates. 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 Callback has been called because a handshake is finished. It also occurs if the
also used at the end of an exchange of post-handshake messages such as for handshake is paused to allow the exchange of early data.
session tickets or key updates. It also occurs if the handshake is paused to
allow the exchange of early data.
=back =back

6
ssl/statem/statem.c
View File

@ -342,8 +342,10 @@ static int state_machine(SSL *s, int server)
} }
s->server = server; s->server = server;
if (cb != NULL) if (cb != NULL) {
cb(s, SSL_CB_HANDSHAKE_START, 1); 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 * Fatal errors in this block don't send an alert because we have

11
ssl/statem/statem_lib.c
View File

@ -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) WORK_STATE tls_finish_handshake(SSL *s, WORK_STATE wst, int clearbufs, int stop)
{ {
void (*cb) (const SSL *ssl, int type, int val) = NULL; void (*cb) (const SSL *ssl, int type, int val) = NULL;
int cleanuphand = s->statem.cleanuphand;
if (clearbufs) { if (clearbufs) {
if (!SSL_IS_DTLS(s)) { 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 * Only set if there was a Finished message and this isn't after a TLSv1.3
* post handshake exchange * post handshake exchange
*/ */
if (s->statem.cleanuphand) { if (cleanuphand) {
/* skipped if we just sent a HelloRequest */ /* skipped if we just sent a HelloRequest */
s->renegotiate = 0; s->renegotiate = 0;
s->new_session = 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 */ /* The callback may expect us to not be in init at handshake done */
ossl_statem_set_in_init(s, 0); ossl_statem_set_in_init(s, 0);
if (cb != NULL) if (cb != NULL) {
cb(s, SSL_CB_HANDSHAKE_DONE, 1); if (cleanuphand
|| !SSL_IS_TLS13(s)
|| SSL_IS_FIRST_HANDSHAKE(s))
cb(s, SSL_CB_HANDSHAKE_DONE, 1);
}
if (!stop) { if (!stop) {
/* If we've got more work to do we go back into init */ /* If we've got more work to do we go back into init */

19
ssl/statem/statem_srvr.c
View File

@ -4040,7 +4040,6 @@ int tls_construct_new_session_ticket(SSL *s, WPACKET *pkt)
uint64_t nonce; uint64_t nonce;
static const unsigned char nonce_label[] = "resumption"; static const unsigned char nonce_label[] = "resumption";
const EVP_MD *md = ssl_handshake_md(s); const EVP_MD *md = ssl_handshake_md(s);
void (*cb) (const SSL *ssl, int type, int val) = NULL;
int hashleni = EVP_MD_size(md); int hashleni = EVP_MD_size(md);
/* Ensure cast to size_t is safe */ /* 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; 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 * 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. * s->session may already be in a cache and so we must not modify it.

49
test/sslapitest.c
View File

@ -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, "TWCCS"}, {SSL_CB_LOOP, "TWEE"}, {SSL_CB_LOOP, "TWSC"},
{SSL_CB_LOOP, "TRSCV"}, {SSL_CB_LOOP, "TWFIN"}, {SSL_CB_LOOP, "TED"}, {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_EXIT, NULL}, {SSL_CB_LOOP, "TED"}, {SSL_CB_LOOP, "TRFIN"},
{SSL_CB_HANDSHAKE_DONE, NULL}, {SSL_CB_HANDSHAKE_START, NULL}, {SSL_CB_HANDSHAKE_DONE, NULL}, {SSL_CB_LOOP, "TWST"},
{SSL_CB_LOOP, "TWST"}, {SSL_CB_HANDSHAKE_DONE, NULL}, {SSL_CB_LOOP, "TWST"}, {SSL_CB_EXIT, NULL}, {SSL_CB_ALERT, NULL},
{SSL_CB_HANDSHAKE_START, NULL}, {SSL_CB_LOOP, "TWST"}, {SSL_CB_HANDSHAKE_START, NULL}, {SSL_CB_LOOP, "PINIT "},
{SSL_CB_HANDSHAKE_DONE, NULL}, {SSL_CB_EXIT, NULL}, {SSL_CB_LOOP, "PINIT "}, {SSL_CB_LOOP, "TRCH"}, {SSL_CB_LOOP, "TWSH"},
{SSL_CB_ALERT, NULL}, {SSL_CB_HANDSHAKE_START, NULL}, {SSL_CB_LOOP, "TWCCS"}, {SSL_CB_LOOP, "TWEE"}, {SSL_CB_LOOP, "TWFIN"},
{SSL_CB_LOOP, "PINIT "}, {SSL_CB_LOOP, "PINIT "}, {SSL_CB_LOOP, "TRCH"}, {SSL_CB_LOOP, "TED"}, {SSL_CB_EXIT, NULL}, {SSL_CB_LOOP, "TED"},
{SSL_CB_LOOP, "TWSH"}, {SSL_CB_LOOP, "TWCCS"}, {SSL_CB_LOOP, "TWEE"}, {SSL_CB_LOOP, "TRFIN"}, {SSL_CB_HANDSHAKE_DONE, NULL},
{SSL_CB_LOOP, "TWFIN"}, {SSL_CB_LOOP, "TED"}, {SSL_CB_EXIT, NULL}, {SSL_CB_LOOP, "TWST"}, {SSL_CB_EXIT, NULL}, {0, 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},
}, { }, {
/* TLSv1.3 client followed by resumption */ /* TLSv1.3 client followed by resumption */
{SSL_CB_HANDSHAKE_START, NULL}, {SSL_CB_LOOP, "PINIT "}, {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, "TRSH"}, {SSL_CB_LOOP, "TREE"}, {SSL_CB_LOOP, "TRSC"},
{SSL_CB_LOOP, "TRSCV"}, {SSL_CB_LOOP, "TRFIN"}, {SSL_CB_LOOP, "TWCCS"}, {SSL_CB_LOOP, "TRSCV"}, {SSL_CB_LOOP, "TRFIN"}, {SSL_CB_LOOP, "TWCCS"},
{SSL_CB_LOOP, "TWFIN"}, {SSL_CB_HANDSHAKE_DONE, NULL}, {SSL_CB_LOOP, "TWFIN"}, {SSL_CB_HANDSHAKE_DONE, NULL},
{SSL_CB_EXIT, NULL}, {SSL_CB_HANDSHAKE_START, NULL}, {SSL_CB_EXIT, NULL}, {SSL_CB_LOOP, "SSLOK "}, {SSL_CB_LOOP, "SSLOK "},
{SSL_CB_LOOP, "SSLOK "}, {SSL_CB_LOOP, "SSLOK "}, {SSL_CB_LOOP, "TRST"}, {SSL_CB_LOOP, "TRST"}, {SSL_CB_EXIT, NULL}, {SSL_CB_LOOP, "SSLOK "},
{SSL_CB_HANDSHAKE_DONE, NULL}, {SSL_CB_EXIT, NULL}, {SSL_CB_LOOP, "SSLOK "}, {SSL_CB_LOOP, "TRST"}, {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_ALERT, NULL}, {SSL_CB_HANDSHAKE_START, 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, "PINIT "}, {SSL_CB_LOOP, "TWCH"}, {SSL_CB_EXIT, NULL},
{SSL_CB_LOOP, "TWCH"}, {SSL_CB_LOOP, "TRSH"}, {SSL_CB_LOOP, "TREE"}, {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_LOOP, "TRFIN"}, {SSL_CB_LOOP, "TWCCS"}, {SSL_CB_LOOP, "TWFIN"},
{SSL_CB_HANDSHAKE_DONE, NULL}, {SSL_CB_EXIT, NULL}, {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, "SSLOK "}, {SSL_CB_LOOP, "TRST"},
{SSL_CB_LOOP, "SSLOK "}, {SSL_CB_LOOP, "TRST"}, {SSL_CB_EXIT, NULL}, {0, NULL},
{SSL_CB_HANDSHAKE_DONE, NULL}, {SSL_CB_EXIT, NULL}, {0, NULL},
}, { }, {
/* TLSv1.3 server, early_data */ /* TLSv1.3 server, early_data */
{SSL_CB_HANDSHAKE_START, NULL}, {SSL_CB_LOOP, "PINIT "}, {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_DONE, NULL}, {SSL_CB_EXIT, NULL},
{SSL_CB_HANDSHAKE_START, NULL}, {SSL_CB_LOOP, "TED"}, {SSL_CB_HANDSHAKE_START, NULL}, {SSL_CB_LOOP, "TED"},
{SSL_CB_LOOP, "TED"}, {SSL_CB_LOOP, "TWEOED"}, {SSL_CB_LOOP, "TRFIN"}, {SSL_CB_LOOP, "TED"}, {SSL_CB_LOOP, "TWEOED"}, {SSL_CB_LOOP, "TRFIN"},
{SSL_CB_HANDSHAKE_DONE, NULL}, {SSL_CB_HANDSHAKE_START, NULL}, {SSL_CB_HANDSHAKE_DONE, NULL}, {SSL_CB_LOOP, "TWST"},
{SSL_CB_LOOP, "TWST"}, {SSL_CB_HANDSHAKE_DONE, NULL},
{SSL_CB_EXIT, NULL}, {0, NULL}, {SSL_CB_EXIT, NULL}, {0, NULL},
}, { }, {
/* TLSv1.3 client, early_data */ /* 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, "TED"}, {SSL_CB_LOOP, "TRSH"}, {SSL_CB_LOOP, "TREE"},
{SSL_CB_LOOP, "TRFIN"}, {SSL_CB_LOOP, "TPEDE"}, {SSL_CB_LOOP, "TWEOED"}, {SSL_CB_LOOP, "TRFIN"}, {SSL_CB_LOOP, "TPEDE"}, {SSL_CB_LOOP, "TWEOED"},
{SSL_CB_LOOP, "TWFIN"}, {SSL_CB_HANDSHAKE_DONE, NULL}, {SSL_CB_LOOP, "TWFIN"}, {SSL_CB_HANDSHAKE_DONE, NULL},
{SSL_CB_EXIT, NULL}, {SSL_CB_HANDSHAKE_START, NULL}, {SSL_CB_EXIT, NULL}, {SSL_CB_LOOP, "SSLOK "}, {SSL_CB_LOOP, "SSLOK "},
{SSL_CB_LOOP, "SSLOK "}, {SSL_CB_LOOP, "SSLOK "}, {SSL_CB_LOOP, "TRST"}, {SSL_CB_LOOP, "TRST"}, {SSL_CB_EXIT, NULL}, {0, NULL},
{SSL_CB_HANDSHAKE_DONE, NULL}, {SSL_CB_EXIT, NULL}, {0, NULL},
}, { }, {
{0, NULL}, {0, NULL},
} }
@ -5013,8 +5003,11 @@ static void sslapi_info_callback(const SSL *s, int where, int ret)
return; 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; info_cb_failed = 1;
return; return;
} }