Skip to content

Commit 37857e9

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) (cherry picked from commit 4af5836)
1 parent 1c31fe7 commit 37857e9

6 files changed

Lines changed: 49 additions & 61 deletions

File tree

‎CHANGES‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,17 @@
99

1010
Changes between 1.1.1a and 1.1.1b [xx XXX xxxx]
1111

12+
*) Change the info callback signals for the start and end of a post-handshake
13+
message exchange in TLSv1.3. In 1.1.1/1.1.1a we used SSL_CB_HANDSHAKE_START
14+
and SSL_CB_HANDSHAKE_DONE. Experience has shown that many applications get
15+
confused by this and assume that a TLSv1.2 renegotiation has started. This
16+
can break KeyUpdate handling. Instead we no longer signal the start and end
17+
of a post handshake message exchange (although the messages themselves are
18+
still signalled). This could break some applications that were expecting
19+
the old signals. However without this KeyUpdate is not usable for many
20+
applications.
21+
[Matt Caswell]
22+
1223
*) Fix a bug in the computation of the endpoint-pair shared secret used
1324
by DTLS over SCTP. This breaks interoperability with older versions
1425
of OpenSSL like OpenSSL 1.1.0 and OpenSSL 1.0.2. There is a runtime

‎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
@@ -4734,39 +4734,31 @@ static struct info_cb_states_st {
47344734
{SSL_CB_LOOP, "TWCCS"}, {SSL_CB_LOOP, "TWEE"}, {SSL_CB_LOOP, "TWSC"},
47354735
{SSL_CB_LOOP, "TRSCV"}, {SSL_CB_LOOP, "TWFIN"}, {SSL_CB_LOOP, "TED"},
47364736
{SSL_CB_EXIT, NULL}, {SSL_CB_LOOP, "TED"}, {SSL_CB_LOOP, "TRFIN"},
4737-
{SSL_CB_HANDSHAKE_DONE, NULL}, {SSL_CB_HANDSHAKE_START, NULL},
4738-
{SSL_CB_LOOP, "TWST"}, {SSL_CB_HANDSHAKE_DONE, NULL},
4739-
{SSL_CB_HANDSHAKE_START, NULL}, {SSL_CB_LOOP, "TWST"},
4740-
{SSL_CB_HANDSHAKE_DONE, NULL}, {SSL_CB_EXIT, NULL},
4741-
{SSL_CB_ALERT, NULL}, {SSL_CB_HANDSHAKE_START, NULL},
4742-
{SSL_CB_LOOP, "PINIT "}, {SSL_CB_LOOP, "PINIT "}, {SSL_CB_LOOP, "TRCH"},
4743-
{SSL_CB_LOOP, "TWSH"}, {SSL_CB_LOOP, "TWCCS"}, {SSL_CB_LOOP, "TWEE"},
4744-
{SSL_CB_LOOP, "TWFIN"}, {SSL_CB_LOOP, "TED"}, {SSL_CB_EXIT, NULL},
4745-
{SSL_CB_LOOP, "TED"}, {SSL_CB_LOOP, "TRFIN"},
4746-
{SSL_CB_HANDSHAKE_DONE, NULL}, {SSL_CB_HANDSHAKE_START, NULL},
4747-
{SSL_CB_LOOP, "TWST"}, {SSL_CB_HANDSHAKE_DONE, NULL},
4748-
{SSL_CB_EXIT, NULL}, {0, NULL},
4737+
{SSL_CB_HANDSHAKE_DONE, NULL}, {SSL_CB_LOOP, "TWST"},
4738+
{SSL_CB_LOOP, "TWST"}, {SSL_CB_EXIT, NULL}, {SSL_CB_ALERT, NULL},
4739+
{SSL_CB_HANDSHAKE_START, NULL}, {SSL_CB_LOOP, "PINIT "},
4740+
{SSL_CB_LOOP, "PINIT "}, {SSL_CB_LOOP, "TRCH"}, {SSL_CB_LOOP, "TWSH"},
4741+
{SSL_CB_LOOP, "TWCCS"}, {SSL_CB_LOOP, "TWEE"}, {SSL_CB_LOOP, "TWFIN"},
4742+
{SSL_CB_LOOP, "TED"}, {SSL_CB_EXIT, NULL}, {SSL_CB_LOOP, "TED"},
4743+
{SSL_CB_LOOP, "TRFIN"}, {SSL_CB_HANDSHAKE_DONE, NULL},
4744+
{SSL_CB_LOOP, "TWST"}, {SSL_CB_EXIT, NULL}, {0, NULL},
47494745
}, {
47504746
/* TLSv1.3 client followed by resumption */
47514747
{SSL_CB_HANDSHAKE_START, NULL}, {SSL_CB_LOOP, "PINIT "},
47524748
{SSL_CB_LOOP, "TWCH"}, {SSL_CB_EXIT, NULL}, {SSL_CB_LOOP, "TWCH"},
47534749
{SSL_CB_LOOP, "TRSH"}, {SSL_CB_LOOP, "TREE"}, {SSL_CB_LOOP, "TRSC"},
47544750
{SSL_CB_LOOP, "TRSCV"}, {SSL_CB_LOOP, "TRFIN"}, {SSL_CB_LOOP, "TWCCS"},
47554751
{SSL_CB_LOOP, "TWFIN"}, {SSL_CB_HANDSHAKE_DONE, NULL},
4756-
{SSL_CB_EXIT, NULL}, {SSL_CB_HANDSHAKE_START, NULL},
4757-
{SSL_CB_LOOP, "SSLOK "}, {SSL_CB_LOOP, "SSLOK "}, {SSL_CB_LOOP, "TRST"},
4758-
{SSL_CB_HANDSHAKE_DONE, NULL}, {SSL_CB_EXIT, NULL},
4759-
{SSL_CB_HANDSHAKE_START, NULL}, {SSL_CB_LOOP, "SSLOK "},
4760-
{SSL_CB_LOOP, "SSLOK "}, {SSL_CB_LOOP, "TRST"},
4761-
{SSL_CB_HANDSHAKE_DONE, NULL}, {SSL_CB_EXIT, NULL},
4752+
{SSL_CB_EXIT, NULL}, {SSL_CB_LOOP, "SSLOK "}, {SSL_CB_LOOP, "SSLOK "},
4753+
{SSL_CB_LOOP, "TRST"}, {SSL_CB_EXIT, NULL}, {SSL_CB_LOOP, "SSLOK "},
4754+
{SSL_CB_LOOP, "SSLOK "}, {SSL_CB_LOOP, "TRST"}, {SSL_CB_EXIT, NULL},
47624755
{SSL_CB_ALERT, NULL}, {SSL_CB_HANDSHAKE_START, NULL},
47634756
{SSL_CB_LOOP, "PINIT "}, {SSL_CB_LOOP, "TWCH"}, {SSL_CB_EXIT, NULL},
47644757
{SSL_CB_LOOP, "TWCH"}, {SSL_CB_LOOP, "TRSH"}, {SSL_CB_LOOP, "TREE"},
47654758
{SSL_CB_LOOP, "TRFIN"}, {SSL_CB_LOOP, "TWCCS"}, {SSL_CB_LOOP, "TWFIN"},
47664759
{SSL_CB_HANDSHAKE_DONE, NULL}, {SSL_CB_EXIT, NULL},
4767-
{SSL_CB_HANDSHAKE_START, NULL}, {SSL_CB_LOOP, "SSLOK "},
4768-
{SSL_CB_LOOP, "SSLOK "}, {SSL_CB_LOOP, "TRST"},
4769-
{SSL_CB_HANDSHAKE_DONE, NULL}, {SSL_CB_EXIT, NULL}, {0, NULL},
4760+
{SSL_CB_LOOP, "SSLOK "}, {SSL_CB_LOOP, "SSLOK "}, {SSL_CB_LOOP, "TRST"},
4761+
{SSL_CB_EXIT, NULL}, {0, NULL},
47704762
}, {
47714763
/* TLSv1.3 server, early_data */
47724764
{SSL_CB_HANDSHAKE_START, NULL}, {SSL_CB_LOOP, "PINIT "},
@@ -4775,8 +4767,7 @@ static struct info_cb_states_st {
47754767
{SSL_CB_HANDSHAKE_DONE, NULL}, {SSL_CB_EXIT, NULL},
47764768
{SSL_CB_HANDSHAKE_START, NULL}, {SSL_CB_LOOP, "TED"},
47774769
{SSL_CB_LOOP, "TED"}, {SSL_CB_LOOP, "TWEOED"}, {SSL_CB_LOOP, "TRFIN"},
4778-
{SSL_CB_HANDSHAKE_DONE, NULL}, {SSL_CB_HANDSHAKE_START, NULL},
4779-
{SSL_CB_LOOP, "TWST"}, {SSL_CB_HANDSHAKE_DONE, NULL},
4770+
{SSL_CB_HANDSHAKE_DONE, NULL}, {SSL_CB_LOOP, "TWST"},
47804771
{SSL_CB_EXIT, NULL}, {0, NULL},
47814772
}, {
47824773
/* TLSv1.3 client, early_data */
@@ -4787,9 +4778,8 @@ static struct info_cb_states_st {
47874778
{SSL_CB_LOOP, "TED"}, {SSL_CB_LOOP, "TRSH"}, {SSL_CB_LOOP, "TREE"},
47884779
{SSL_CB_LOOP, "TRFIN"}, {SSL_CB_LOOP, "TPEDE"}, {SSL_CB_LOOP, "TWEOED"},
47894780
{SSL_CB_LOOP, "TWFIN"}, {SSL_CB_HANDSHAKE_DONE, NULL},
4790-
{SSL_CB_EXIT, NULL}, {SSL_CB_HANDSHAKE_START, NULL},
4791-
{SSL_CB_LOOP, "SSLOK "}, {SSL_CB_LOOP, "SSLOK "}, {SSL_CB_LOOP, "TRST"},
4792-
{SSL_CB_HANDSHAKE_DONE, NULL}, {SSL_CB_EXIT, NULL}, {0, NULL},
4781+
{SSL_CB_EXIT, NULL}, {SSL_CB_LOOP, "SSLOK "}, {SSL_CB_LOOP, "SSLOK "},
4782+
{SSL_CB_LOOP, "TRST"}, {SSL_CB_EXIT, NULL}, {0, NULL},
47934783
}, {
47944784
{0, NULL},
47954785
}
@@ -4828,8 +4818,11 @@ static void sslapi_info_callback(const SSL *s, int where, int ret)
48284818
return;
48294819
}
48304820

4831-
/* Check that, if we've got SSL_CB_HANDSHAKE_DONE we are not in init */
4832-
if ((where & SSL_CB_HANDSHAKE_DONE) && SSL_in_init((SSL *)s) != 0) {
4821+
/*
4822+
* Check that, if we've got SSL_CB_HANDSHAKE_DONE we are not in init
4823+
*/
4824+
if ((where & SSL_CB_HANDSHAKE_DONE)
4825+
&& SSL_in_init((SSL *)s) != 0) {
48334826
info_cb_failed = 1;
48344827
return;
48354828
}

0 commit comments

Comments
 (0)