Skip to content

Commit 4af5836

Browse files
mattcaswellmattcaswell
committed
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 <[email protected]> (Merged from #8096)
1 parent 3c83c5b commit 4af5836

6 files changed

Lines changed: 51 additions & 61 deletions

File tree

‎CHANGES‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -119,6 +119,19 @@
119119
applications with zero-copy system calls such as sendfile and splice.
120120
[Boris Pismenny]
121121

122+
Changes between 1.1.1a and 1.1.1b [xx XXX xxxx]
123+
124+
*) Change the info callback signals for the start and end of a post-handshake
125+
message exchange in TLSv1.3. In 1.1.1/1.1.1a we used SSL_CB_HANDSHAKE_START
126+
and SSL_CB_HANDSHAKE_DONE. Experience has shown that many applications get
127+
confused by this and assume that a TLSv1.2 renegotiation has started. This
128+
can break KeyUpdate handling. Instead we no longer signal the start and end
129+
of a post handshake message exchange (although the messages themselves are
130+
still signalled). This could break some applications that were expecting
131+
the old signals. However without this KeyUpdate is not usable for many
132+
applications.
133+
[Matt Caswell]
134+
122135
Changes between 1.1.1 and 1.1.1a [20 Nov 2018]
123136

124137
*) Timing vulnerability in DSA signature generation

‎doc/man3/SSL_CTX_set_info_callback.pod‎

Lines changed: 5 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -92,17 +92,13 @@ Callback has been called due to an alert being sent or received.
9292

9393
=item SSL_CB_HANDSHAKE_START
9494

95-
Callback has been called because a new handshake is started. In TLSv1.3 this is
96-
also used for the start of post-handshake message exchanges such as for the
97-
exchange of session tickets, or for key updates. It also occurs when resuming a
98-
handshake following a pause to handle early data.
95+
Callback has been called because a new handshake is started. It also occurs when
96+
resuming a handshake following a pause to handle early data.
9997

100-
=item SSL_CB_HANDSHAKE_DONE 0x20
98+
=item SSL_CB_HANDSHAKE_DONE
10199

102-
Callback has been called because a handshake is finished. In TLSv1.3 this is
103-
also used at the end of an exchange of post-handshake messages such as for
104-
session tickets or key updates. It also occurs if the handshake is paused to
105-
allow the exchange of early data.
100+
Callback has been called because a handshake is finished. It also occurs if the
101+
handshake is paused to allow the exchange of early data.
106102

107103
=back
108104

‎ssl/statem/statem.c‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -342,8 +342,10 @@ static int state_machine(SSL *s, int server)
342342
}
343343

344344
s->server = server;
345-
if (cb != NULL)
346-
cb(s, SSL_CB_HANDSHAKE_START, 1);
345+
if (cb != NULL) {
346+
if (SSL_IS_FIRST_HANDSHAKE(s) || !SSL_IS_TLS13(s))
347+
cb(s, SSL_CB_HANDSHAKE_START, 1);
348+
}
347349

348350
/*
349351
* Fatal errors in this block don't send an alert because we have

‎ssl/statem/statem_lib.c‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1030,6 +1030,7 @@ unsigned long ssl3_output_cert_chain(SSL *s, WPACKET *pkt, CERT_PKEY *cpk)
10301030
WORK_STATE tls_finish_handshake(SSL *s, WORK_STATE wst, int clearbufs, int stop)
10311031
{
10321032
void (*cb) (const SSL *ssl, int type, int val) = NULL;
1033+
int cleanuphand = s->statem.cleanuphand;
10331034

10341035
if (clearbufs) {
10351036
if (!SSL_IS_DTLS(s)) {
@@ -1056,7 +1057,7 @@ WORK_STATE tls_finish_handshake(SSL *s, WORK_STATE wst, int clearbufs, int stop)
10561057
* Only set if there was a Finished message and this isn't after a TLSv1.3
10571058
* post handshake exchange
10581059
*/
1059-
if (s->statem.cleanuphand) {
1060+
if (cleanuphand) {
10601061
/* skipped if we just sent a HelloRequest */
10611062
s->renegotiate = 0;
10621063
s->new_session = 0;
@@ -1116,8 +1117,12 @@ WORK_STATE tls_finish_handshake(SSL *s, WORK_STATE wst, int clearbufs, int stop)
11161117
/* The callback may expect us to not be in init at handshake done */
11171118
ossl_statem_set_in_init(s, 0);
11181119

1119-
if (cb != NULL)
1120-
cb(s, SSL_CB_HANDSHAKE_DONE, 1);
1120+
if (cb != NULL) {
1121+
if (cleanuphand
1122+
|| !SSL_IS_TLS13(s)
1123+
|| SSL_IS_FIRST_HANDSHAKE(s))
1124+
cb(s, SSL_CB_HANDSHAKE_DONE, 1);
1125+
}
11211126

11221127
if (!stop) {
11231128
/* If we've got more work to do we go back into init */

‎ssl/statem/statem_srvr.c‎

Lines changed: 0 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -4040,7 +4040,6 @@ int tls_construct_new_session_ticket(SSL *s, WPACKET *pkt)
40404040
uint64_t nonce;
40414041
static const unsigned char nonce_label[] = "resumption";
40424042
const EVP_MD *md = ssl_handshake_md(s);
4043-
void (*cb) (const SSL *ssl, int type, int val) = NULL;
40444043
int hashleni = EVP_MD_size(md);
40454044

40464045
/* Ensure cast to size_t is safe */
@@ -4052,24 +4051,6 @@ int tls_construct_new_session_ticket(SSL *s, WPACKET *pkt)
40524051
}
40534052
hashlen = (size_t)hashleni;
40544053

4055-
if (s->info_callback != NULL)
4056-
cb = s->info_callback;
4057-
else if (s->ctx->info_callback != NULL)
4058-
cb = s->ctx->info_callback;
4059-
4060-
if (cb != NULL) {
4061-
/*
4062-
* We don't start and stop the handshake in between each ticket when
4063-
* sending more than one - but it should appear that way to the info
4064-
* callback.
4065-
*/
4066-
if (s->sent_tickets != 0) {
4067-
ossl_statem_set_in_init(s, 0);
4068-
cb(s, SSL_CB_HANDSHAKE_DONE, 1);
4069-
ossl_statem_set_in_init(s, 1);
4070-
}
4071-
cb(s, SSL_CB_HANDSHAKE_START, 1);
4072-
}
40734054
/*
40744055
* If we already sent one NewSessionTicket, or we resumed then
40754056
* s->session may already be in a cache and so we must not modify it.

‎test/sslapitest.c‎

Lines changed: 21 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -4919,39 +4919,31 @@ static struct info_cb_states_st {
49194919
{SSL_CB_LOOP, "TWCCS"}, {SSL_CB_LOOP, "TWEE"}, {SSL_CB_LOOP, "TWSC"},
49204920
{SSL_CB_LOOP, "TRSCV"}, {SSL_CB_LOOP, "TWFIN"}, {SSL_CB_LOOP, "TED"},
49214921
{SSL_CB_EXIT, NULL}, {SSL_CB_LOOP, "TED"}, {SSL_CB_LOOP, "TRFIN"},
4922-
{SSL_CB_HANDSHAKE_DONE, NULL}, {SSL_CB_HANDSHAKE_START, NULL},
4923-
{SSL_CB_LOOP, "TWST"}, {SSL_CB_HANDSHAKE_DONE, NULL},
4924-
{SSL_CB_HANDSHAKE_START, NULL}, {SSL_CB_LOOP, "TWST"},
4925-
{SSL_CB_HANDSHAKE_DONE, NULL}, {SSL_CB_EXIT, NULL},
4926-
{SSL_CB_ALERT, NULL}, {SSL_CB_HANDSHAKE_START, NULL},
4927-
{SSL_CB_LOOP, "PINIT "}, {SSL_CB_LOOP, "PINIT "}, {SSL_CB_LOOP, "TRCH"},
4928-
{SSL_CB_LOOP, "TWSH"}, {SSL_CB_LOOP, "TWCCS"}, {SSL_CB_LOOP, "TWEE"},
4929-
{SSL_CB_LOOP, "TWFIN"}, {SSL_CB_LOOP, "TED"}, {SSL_CB_EXIT, NULL},
4930-
{SSL_CB_LOOP, "TED"}, {SSL_CB_LOOP, "TRFIN"},
4931-
{SSL_CB_HANDSHAKE_DONE, NULL}, {SSL_CB_HANDSHAKE_START, NULL},
4932-
{SSL_CB_LOOP, "TWST"}, {SSL_CB_HANDSHAKE_DONE, NULL},
4933-
{SSL_CB_EXIT, NULL}, {0, NULL},
4922+
{SSL_CB_HANDSHAKE_DONE, NULL}, {SSL_CB_LOOP, "TWST"},
4923+
{SSL_CB_LOOP, "TWST"}, {SSL_CB_EXIT, NULL}, {SSL_CB_ALERT, NULL},
4924+
{SSL_CB_HANDSHAKE_START, NULL}, {SSL_CB_LOOP, "PINIT "},
4925+
{SSL_CB_LOOP, "PINIT "}, {SSL_CB_LOOP, "TRCH"}, {SSL_CB_LOOP, "TWSH"},
4926+
{SSL_CB_LOOP, "TWCCS"}, {SSL_CB_LOOP, "TWEE"}, {SSL_CB_LOOP, "TWFIN"},
4927+
{SSL_CB_LOOP, "TED"}, {SSL_CB_EXIT, NULL}, {SSL_CB_LOOP, "TED"},
4928+
{SSL_CB_LOOP, "TRFIN"}, {SSL_CB_HANDSHAKE_DONE, NULL},
4929+
{SSL_CB_LOOP, "TWST"}, {SSL_CB_EXIT, NULL}, {0, NULL},
49344930
}, {
49354931
/* TLSv1.3 client followed by resumption */
49364932
{SSL_CB_HANDSHAKE_START, NULL}, {SSL_CB_LOOP, "PINIT "},
49374933
{SSL_CB_LOOP, "TWCH"}, {SSL_CB_EXIT, NULL}, {SSL_CB_LOOP, "TWCH"},
49384934
{SSL_CB_LOOP, "TRSH"}, {SSL_CB_LOOP, "TREE"}, {SSL_CB_LOOP, "TRSC"},
49394935
{SSL_CB_LOOP, "TRSCV"}, {SSL_CB_LOOP, "TRFIN"}, {SSL_CB_LOOP, "TWCCS"},
49404936
{SSL_CB_LOOP, "TWFIN"}, {SSL_CB_HANDSHAKE_DONE, NULL},
4941-
{SSL_CB_EXIT, NULL}, {SSL_CB_HANDSHAKE_START, NULL},
4942-
{SSL_CB_LOOP, "SSLOK "}, {SSL_CB_LOOP, "SSLOK "}, {SSL_CB_LOOP, "TRST"},
4943-
{SSL_CB_HANDSHAKE_DONE, NULL}, {SSL_CB_EXIT, NULL},
4944-
{SSL_CB_HANDSHAKE_START, NULL}, {SSL_CB_LOOP, "SSLOK "},
4945-
{SSL_CB_LOOP, "SSLOK "}, {SSL_CB_LOOP, "TRST"},
4946-
{SSL_CB_HANDSHAKE_DONE, NULL}, {SSL_CB_EXIT, NULL},
4937+
{SSL_CB_EXIT, NULL}, {SSL_CB_LOOP, "SSLOK "}, {SSL_CB_LOOP, "SSLOK "},
4938+
{SSL_CB_LOOP, "TRST"}, {SSL_CB_EXIT, NULL}, {SSL_CB_LOOP, "SSLOK "},
4939+
{SSL_CB_LOOP, "SSLOK "}, {SSL_CB_LOOP, "TRST"}, {SSL_CB_EXIT, NULL},
49474940
{SSL_CB_ALERT, NULL}, {SSL_CB_HANDSHAKE_START, NULL},
49484941
{SSL_CB_LOOP, "PINIT "}, {SSL_CB_LOOP, "TWCH"}, {SSL_CB_EXIT, NULL},
49494942
{SSL_CB_LOOP, "TWCH"}, {SSL_CB_LOOP, "TRSH"}, {SSL_CB_LOOP, "TREE"},
49504943
{SSL_CB_LOOP, "TRFIN"}, {SSL_CB_LOOP, "TWCCS"}, {SSL_CB_LOOP, "TWFIN"},
49514944
{SSL_CB_HANDSHAKE_DONE, NULL}, {SSL_CB_EXIT, NULL},
4952-
{SSL_CB_HANDSHAKE_START, NULL}, {SSL_CB_LOOP, "SSLOK "},
4953-
{SSL_CB_LOOP, "SSLOK "}, {SSL_CB_LOOP, "TRST"},
4954-
{SSL_CB_HANDSHAKE_DONE, NULL}, {SSL_CB_EXIT, NULL}, {0, NULL},
4945+
{SSL_CB_LOOP, "SSLOK "}, {SSL_CB_LOOP, "SSLOK "}, {SSL_CB_LOOP, "TRST"},
4946+
{SSL_CB_EXIT, NULL}, {0, NULL},
49554947
}, {
49564948
/* TLSv1.3 server, early_data */
49574949
{SSL_CB_HANDSHAKE_START, NULL}, {SSL_CB_LOOP, "PINIT "},
@@ -4960,8 +4952,7 @@ static struct info_cb_states_st {
49604952
{SSL_CB_HANDSHAKE_DONE, NULL}, {SSL_CB_EXIT, NULL},
49614953
{SSL_CB_HANDSHAKE_START, NULL}, {SSL_CB_LOOP, "TED"},
49624954
{SSL_CB_LOOP, "TED"}, {SSL_CB_LOOP, "TWEOED"}, {SSL_CB_LOOP, "TRFIN"},
4963-
{SSL_CB_HANDSHAKE_DONE, NULL}, {SSL_CB_HANDSHAKE_START, NULL},
4964-
{SSL_CB_LOOP, "TWST"}, {SSL_CB_HANDSHAKE_DONE, NULL},
4955+
{SSL_CB_HANDSHAKE_DONE, NULL}, {SSL_CB_LOOP, "TWST"},
49654956
{SSL_CB_EXIT, NULL}, {0, NULL},
49664957
}, {
49674958
/* TLSv1.3 client, early_data */
@@ -4972,9 +4963,8 @@ static struct info_cb_states_st {
49724963
{SSL_CB_LOOP, "TED"}, {SSL_CB_LOOP, "TRSH"}, {SSL_CB_LOOP, "TREE"},
49734964
{SSL_CB_LOOP, "TRFIN"}, {SSL_CB_LOOP, "TPEDE"}, {SSL_CB_LOOP, "TWEOED"},
49744965
{SSL_CB_LOOP, "TWFIN"}, {SSL_CB_HANDSHAKE_DONE, NULL},
4975-
{SSL_CB_EXIT, NULL}, {SSL_CB_HANDSHAKE_START, NULL},
4976-
{SSL_CB_LOOP, "SSLOK "}, {SSL_CB_LOOP, "SSLOK "}, {SSL_CB_LOOP, "TRST"},
4977-
{SSL_CB_HANDSHAKE_DONE, NULL}, {SSL_CB_EXIT, NULL}, {0, NULL},
4966+
{SSL_CB_EXIT, NULL}, {SSL_CB_LOOP, "SSLOK "}, {SSL_CB_LOOP, "SSLOK "},
4967+
{SSL_CB_LOOP, "TRST"}, {SSL_CB_EXIT, NULL}, {0, NULL},
49784968
}, {
49794969
{0, NULL},
49804970
}
@@ -5013,8 +5003,11 @@ static void sslapi_info_callback(const SSL *s, int where, int ret)
50135003
return;
50145004
}
50155005

5016-
/* Check that, if we've got SSL_CB_HANDSHAKE_DONE we are not in init */
5017-
if ((where & SSL_CB_HANDSHAKE_DONE) && SSL_in_init((SSL *)s) != 0) {
5006+
/*
5007+
* Check that, if we've got SSL_CB_HANDSHAKE_DONE we are not in init
5008+
*/
5009+
if ((where & SSL_CB_HANDSHAKE_DONE)
5010+
&& SSL_in_init((SSL *)s) != 0) {
50185011
info_cb_failed = 1;
50195012
return;
50205013
}

0 commit comments

Comments
 (0)