[openssl-commits] [openssl] master update
Matt Caswell
matt at openssl.org
Thu Feb 14 16:31:14 UTC 2019
The branch master has been updated
via 4af5836b55442f31795eff6c8c81ea7a1b8cf94b (commit)
from 3c83c5ba4f6502c708b7a5f55c98a10e312668da (commit)
- Log -----------------------------------------------------------------
commit 4af5836b55442f31795eff6c8c81ea7a1b8cf94b
Author: Matt Caswell <matt at openssl.org>
Date: Sun Jan 27 11:00:16 2019 +0000
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 at openssl.org>
(Merged from https://github.com/openssl/openssl/pull/8096)
-----------------------------------------------------------------------
Summary of changes:
CHANGES | 13 +++++++++
doc/man3/SSL_CTX_set_info_callback.pod | 14 ++++------
ssl/statem/statem.c | 6 +++--
ssl/statem/statem_lib.c | 11 +++++---
ssl/statem/statem_srvr.c | 19 -------------
test/sslapitest.c | 49 +++++++++++++++-------------------
6 files changed, 51 insertions(+), 61 deletions(-)
diff --git a/CHANGES b/CHANGES
index d9a2e1b..2fbe89f 100644
--- a/CHANGES
+++ b/CHANGES
@@ -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 cb8f996..3248e10 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 ebe471b..24c7e94 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 2f78a3f..8a7ada8 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 f76568c..bf1819d 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 6b44c16..02eb1fe 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;
}
More information about the openssl-commits
mailing list