Commit 622ddb57 authored by Matt Caswell's avatar Matt Caswell
Browse files

Don't calculate the Finished MAC twice 



In <= TLSv1.2 a Finished message always comes immediately after a CCS
except in the case of NPN where there is an additional message between
the CCS and Finished. Historically we always calculated the Finished MAC
when we processed the CCS. However to deal with NPN we also calculated it
when we receive the Finished message. Really this should only have been
done if we hand negotiated NPN.

This simplifies the code to only calculate the MAC when we receive the
Finished. In 1.1.1 we need to do it this way anyway because there is no
CCS (except in middlebox compat mode) in TLSv1.3.

Reviewed-by: default avatarAndy Polyakov <appro@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/5286)
parent 30562caa

include/openssl/ssl.h

+1 −0
Original line number Diff line number Diff line
@@ -2131,6 +2131,7 @@ int ERR_load_SSL_strings(void); 
# define SSL_F_SSL3_SETUP_KEY_BLOCK                       157

# define SSL_F_SSL3_SETUP_READ_BUFFER                     156

# define SSL_F_SSL3_SETUP_WRITE_BUFFER                    291

# define SSL_F_SSL3_TAKE_MAC                              425

# define SSL_F_SSL3_WRITE_BYTES                           158

# define SSL_F_SSL3_WRITE_PENDING                         159

# define SSL_F_SSL_ADD_CERT_CHAIN                         316

ssl/s3_msg.c

+1 −24
Original line number Diff line number Diff line
@@ -13,8 +13,6 @@ 
int ssl3_do_change_cipher_spec(SSL *s)

{

    int i;

    const char *sender;

    int slen;



    if (s->server)

        i = SSL3_CHANGE_CIPHER_SERVER_READ;
@@ -36,28 +34,7 @@ int ssl3_do_change_cipher_spec(SSL *s) 
    if (!s->method->ssl3_enc->change_cipher_state(s, i))

        return (0);



    /*

     * we have to record the message digest at this point so we can get it

     * before we read the finished message

     */

    if (!s->server) {

        sender = s->method->ssl3_enc->server_finished_label;

        slen = s->method->ssl3_enc->server_finished_label_len;

    } else {

        sender = s->method->ssl3_enc->client_finished_label;

        slen = s->method->ssl3_enc->client_finished_label_len;

    }



    i = s->method->ssl3_enc->final_finish_mac(s,

                                              sender, slen,

                                              s->s3->tmp.peer_finish_md);

    if (i == 0) {

        SSLerr(SSL_F_SSL3_DO_CHANGE_CIPHER_SPEC, ERR_R_INTERNAL_ERROR);

        return 0;

    }

    s->s3->tmp.peer_finish_md_len = i;



    return (1);

    return 1;

}



int ssl3_send_alert(SSL *s, int level, int desc)

ssl/ssl_err.c

+2 −1
Original line number Diff line number Diff line 
/*

 * Generated by util/mkerr.pl DO NOT EDIT

 * Copyright 1995-2016 The OpenSSL Project Authors. All Rights Reserved.

 * Copyright 1995-2018 The OpenSSL Project Authors. All Rights Reserved.

 *

 * Licensed under the OpenSSL license (the "License").  You may not use

 * this file except in compliance with the License.  You can obtain a copy
@@ -75,6 +75,7 @@ static ERR_STRING_DATA SSL_str_functs[] = { 
    {ERR_FUNC(SSL_F_SSL3_SETUP_KEY_BLOCK), "ssl3_setup_key_block"},

    {ERR_FUNC(SSL_F_SSL3_SETUP_READ_BUFFER), "ssl3_setup_read_buffer"},

    {ERR_FUNC(SSL_F_SSL3_SETUP_WRITE_BUFFER), "ssl3_setup_write_buffer"},

    {ERR_FUNC(SSL_F_SSL3_TAKE_MAC), "ssl3_take_mac"},

    {ERR_FUNC(SSL_F_SSL3_WRITE_BYTES), "ssl3_write_bytes"},

    {ERR_FUNC(SSL_F_SSL3_WRITE_PENDING), "ssl3_write_pending"},

    {ERR_FUNC(SSL_F_SSL_ADD_CERT_CHAIN), "ssl_add_cert_chain"},

ssl/statem/statem_dtls.c

+9 −0
Original line number Diff line number Diff line
@@ -371,6 +371,15 @@ int dtls_get_message(SSL *s, int *mt, unsigned long *len) 
        msg_len += DTLS1_HM_HEADER_LENGTH;

    }



    /*

     * If receiving Finished, record MAC of prior handshake messages for

     * Finished verification.

     */

    if (*mt == SSL3_MT_FINISHED && !ssl3_take_mac(s)) {

        /* SSLfatal() already called */

        return 0;

    }



    if (!ssl3_finish_mac(s, p, msg_len))

        return 0;

    if (s->msg_callback)

ssl/statem/statem_lib.c

+17 −17
Original line number Diff line number Diff line
@@ -95,21 +95,15 @@ int tls_construct_finished(SSL *s, const char *sender, int slen) 
    return 1;

}



#ifndef OPENSSL_NO_NEXTPROTONEG

/*

 * ssl3_take_mac calculates the Finished MAC for the handshakes messages seen

 * to far.

 */

static void ssl3_take_mac(SSL *s)

int ssl3_take_mac(SSL *s)

{

    const char *sender;

    int slen;

    /*

     * If no new cipher setup return immediately: other functions will set

     * the appropriate error.

     */

    if (s->s3->tmp.new_cipher == NULL)

        return;



    if (!s->server) {

        sender = s->method->ssl3_enc->server_finished_label;

        slen = s->method->ssl3_enc->server_finished_label_len;
@@ -118,12 +112,17 @@ static void ssl3_take_mac(SSL *s) 
        slen = s->method->ssl3_enc->client_finished_label_len;

    }



    s->s3->tmp.peer_finish_md_len = s->method->ssl3_enc->final_finish_mac(s,

                                                                          sender,

                                                                          slen,

    s->s3->tmp.peer_finish_md_len =

        s->method->ssl3_enc->final_finish_mac(s, sender, slen,

                                              s->s3->tmp.peer_finish_md);



    if (s->s3->tmp.peer_finish_md_len == 0) {

        SSLerr(SSL_F_SSL3_TAKE_MAC, ERR_R_INTERNAL_ERROR);

        return 0;

    }



    return 1;

}

#endif



MSG_PROCESS_RETURN tls_process_change_cipher_spec(SSL *s, PACKET *pkt)

{
@@ -465,14 +464,15 @@ int tls_get_message_body(SSL *s, unsigned long *len) 
        n -= i;

    }



#ifndef OPENSSL_NO_NEXTPROTONEG

    /*

     * If receiving Finished, record MAC of prior handshake messages for

     * Finished verification.

     */

    if (*s->init_buf->data == SSL3_MT_FINISHED)

        ssl3_take_mac(s);

#endif

    if (*(s->init_buf->data) == SSL3_MT_FINISHED && !ssl3_take_mac(s)) {

        /* SSLfatal() already called */

        *len = 0;

        return 0;

    }



    /* Feed this message into MAC computation. */

    if (RECORD_LAYER_is_sslv2_record(&s->rlayer)) {