Commit 37857e9b authored by Matt Caswell's avatar Matt Caswell
Browse files

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: default avatarRichard Levitte <levitte@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/8096)

(cherry picked from commit 4af5836b)
parent 1c31fe7e

CHANGES

+11 −0
Original line number Diff line number Diff line
@@ -9,6 +9,17 @@ 














 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]



























  *) Fix a bug in the computation of the endpoint-pair shared secret used















     by DTLS over SCTP. This breaks interoperability with older versions















     of OpenSSL like OpenSSL 1.1.0 and OpenSSL 1.0.2. There is a runtime

































doc/man3/SSL_CTX_set_info_callback.pod



+5
−9






























Original line number
Diff line number
Diff line








@@ -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

















































ssl/statem/statem.c



+4
−2






























Original line number
Diff line number
Diff line








@@ -342,8 +342,10 @@ static int state_machine(SSL *s, int server)













        }































        s->server = server;













        if (cb != NULL)













        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

































ssl/statem/statem_lib.c



+8
−3






























Original line number
Diff line number
Diff line








@@ -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)













    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 */

































ssl/statem/statem_srvr.c



+0
−19






























Original line number
Diff line number
Diff line








@@ -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.