Skip to content

Commit

Permalink
Don't signal SSL_CB_HANDSHAKE_START for TLSv1.3 post-handshake messages
Browse files Browse the repository at this point in the history 
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 #8096)
  • Loading branch information
@mattcaswell
mattcaswell committed Feb 14, 2019
1 parent 3c83c5b commit 4af5836
Show file tree
Hide file tree
Showing 6 changed files with 51 additions and 61 deletions.
13 changes: 13 additions & 0 deletions CHANGES
View file
Expand Up @@ -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
Expand Down
14 changes: 5 additions & 9 deletions doc/man3/SSL_CTX_set_info_callback.pod
View file
Expand Up @@ -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

Expand Down
6 changes: 4 additions & 2 deletions ssl/statem/statem.c
View file
Expand Up @@ -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
Expand Down
11 changes: 8 additions & 3 deletions ssl/statem/statem_lib.c
View file
Expand Up @@ -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)) {
Expand All @@ -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;
Expand Down Expand Up @@ -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 */
Expand Down
19 changes: 0 additions & 19 deletions ssl/statem/statem_srvr.c
View file
Expand Up @@ -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 */
Expand All @@ -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.
Expand Down
49 changes: 21 additions & 28 deletions test/sslapitest.c
View file
Expand Up @@ -4919,39 +4919,31 @@ 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 "},
{SSL_CB_LOOP, "TWCH"}, {SSL_CB_EXIT, NULL}, {SSL_CB_LOOP, "TWCH"},
{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 "},
Expand All @@ -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 */
Expand All @@ -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},
}
Expand Down Expand Up @@ -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;
}
Expand Down

0 comments on commit 4af5836

Please sign in to comment.