Commit ace081c1 authored by Matt Caswell's avatar Matt Caswell
Browse files

Fix client application traffic secret 



A misreading of the TLS1.3 spec meant we were using the handshake hashes
up to and including the Client Finished to calculate the client
application traffic secret. We should be only use up until the Server
Finished.

Reviewed-by: default avatarRich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/2157)
parent 4954fd13

ssl/ssl_locl.h

+5 −3
Original line number Original line Diff line number Diff line
@@ -955,6 +955,7 @@ struct ssl_st { 
    unsigned char handshake_secret[EVP_MAX_MD_SIZE];

    unsigned char handshake_secret[EVP_MAX_MD_SIZE];

    unsigned char client_finished_secret[EVP_MAX_MD_SIZE];

    unsigned char client_finished_secret[EVP_MAX_MD_SIZE];

    unsigned char server_finished_secret[EVP_MAX_MD_SIZE];

    unsigned char server_finished_secret[EVP_MAX_MD_SIZE];

    unsigned char server_finished_hash[EVP_MAX_MD_SIZE];

    EVP_CIPHER_CTX *enc_read_ctx; /* cryptographic state */

    EVP_CIPHER_CTX *enc_read_ctx; /* cryptographic state */

    unsigned char read_iv[EVP_MAX_IV_LENGTH]; /* TLSv1.3 static read IV */

    unsigned char read_iv[EVP_MAX_IV_LENGTH]; /* TLSv1.3 static read IV */

    EVP_MD_CTX *read_hash;      /* used for mac generation */

    EVP_MD_CTX *read_hash;      /* used for mac generation */
@@ -2080,9 +2081,10 @@ __owur int tls13_setup_key_block(SSL *s); 
__owur size_t tls13_final_finish_mac(SSL *s, const char *str, size_t slen,

__owur size_t tls13_final_finish_mac(SSL *s, const char *str, size_t slen,

                                     unsigned char *p);

                                     unsigned char *p);

__owur int tls13_change_cipher_state(SSL *s, int which);

__owur int tls13_change_cipher_state(SSL *s, int which);

__owur int tls13_derive_secret(SSL *s, const unsigned char *insecret,

__owur int tls13_hkdf_expand(SSL *s, const unsigned char *secret,

                             const unsigned char *label, size_t labellen,

                             const unsigned char *label, size_t labellen,

                               unsigned char *secret);

                             const unsigned char *hash,

                             unsigned char *out, size_t outlen);

__owur int tls13_derive_key(SSL *s, const unsigned char *secret,

__owur int tls13_derive_key(SSL *s, const unsigned char *secret,

                            unsigned char *key, size_t keylen);

                            unsigned char *key, size_t keylen);

__owur int tls13_derive_iv(SSL *s, const unsigned char *secret,

__owur int tls13_derive_iv(SSL *s, const unsigned char *secret,

ssl/tls13_enc.c

+36 −26
Original line number Original line Diff line number Diff line
@@ -23,7 +23,7 @@ static const unsigned char default_zeros[EVP_MAX_MD_SIZE]; 
 * the location pointed to be |out|. The |hash| value may be NULL. Returns 1 on

 * the location pointed to be |out|. The |hash| value may be NULL. Returns 1 on

 * success  0 on failure.

 * success  0 on failure.

 */

 */

static int tls13_hkdf_expand(SSL *s, const unsigned char *secret,

int tls13_hkdf_expand(SSL *s, const unsigned char *secret,

                             const unsigned char *label, size_t labellen,

                             const unsigned char *label, size_t labellen,

                             const unsigned char *hash,

                             const unsigned char *hash,

                             unsigned char *out, size_t outlen)

                             unsigned char *out, size_t outlen)
@@ -74,29 +74,6 @@ static int tls13_hkdf_expand(SSL *s, const unsigned char *secret, 
    return ret == 0;

    return ret == 0;

}

}





/*

 * Given a input secret |insecret| and a |label| of length |labellen|, derive a

 * new |secret|. This will be the length of the current hash output size and

 * will be based on the current state of the handshake hashes. Returns 1 on

 * success  0 on failure.

 */

int tls13_derive_secret(SSL *s, const unsigned char *insecret,

                        const unsigned char *label, size_t labellen,

                        unsigned char *secret)

{

    unsigned char hash[EVP_MAX_MD_SIZE];

    size_t hashlen;



    if (!ssl3_digest_cached_records(s, 1))

        return 0;



    if (!ssl_handshake_hash(s, hash, sizeof(hash), &hashlen))

        return 0;



    return tls13_hkdf_expand(s, insecret, label, labellen, hash, secret,

                             hashlen);

}



/*

/*

 * Given a |secret| generate a |key| of length |keylen| bytes. Returns 1 on

 * Given a |secret| generate a |key| of length |keylen| bytes. Returns 1 on

 * success  0 on failure.

 * success  0 on failure.
@@ -286,13 +263,15 @@ int tls13_change_cipher_state(SSL *s, int which) 
    unsigned char key[EVP_MAX_KEY_LENGTH];

    unsigned char key[EVP_MAX_KEY_LENGTH];

    unsigned char *iv;

    unsigned char *iv;

    unsigned char secret[EVP_MAX_MD_SIZE];

    unsigned char secret[EVP_MAX_MD_SIZE];

    unsigned char hashval[EVP_MAX_MD_SIZE];

    unsigned char *hash = hashval;

    unsigned char *insecret;

    unsigned char *insecret;

    unsigned char *finsecret = NULL;

    unsigned char *finsecret = NULL;

    EVP_CIPHER_CTX *ciph_ctx;

    EVP_CIPHER_CTX *ciph_ctx;

    const EVP_CIPHER *ciph = s->s3->tmp.new_sym_enc;

    const EVP_CIPHER *ciph = s->s3->tmp.new_sym_enc;

    size_t ivlen, keylen, finsecretlen = 0;

    size_t ivlen, keylen, finsecretlen = 0;

    const unsigned char *label;

    const unsigned char *label;

    size_t labellen;

    size_t labellen, hashlen = 0;

    int ret = 0;

    int ret = 0;





    if (which & SSL3_CC_READ) {

    if (which & SSL3_CC_READ) {
@@ -334,9 +313,24 @@ int tls13_change_cipher_state(SSL *s, int which) 
            label = client_handshake_traffic;

            label = client_handshake_traffic;

            labellen = sizeof(client_handshake_traffic) - 1;

            labellen = sizeof(client_handshake_traffic) - 1;

        } else {

        } else {

            int hashleni;



            insecret = s->session->master_key;

            insecret = s->session->master_key;

            label = client_application_traffic;

            label = client_application_traffic;

            labellen = sizeof(client_application_traffic) - 1;

            labellen = sizeof(client_application_traffic) - 1;

            /*

             * For this we only use the handshake hashes up until the server

             * Finished hash. We do not include the client's Finished, which is

             * what ssl_handshake_hash() would give us. Instead we use the

             * previously saved value.

             */

            hash = s->server_finished_hash;

            hashleni = EVP_MD_CTX_size(s->s3->handshake_dgst);

            if (hashleni < 0) {

                SSLerr(SSL_F_TLS13_CHANGE_CIPHER_STATE, ERR_R_INTERNAL_ERROR);

                goto err;

            }

            hashlen = (size_t)hashleni;

        }

        }

    } else {

    } else {

        if (which & SSL3_CC_HANDSHAKE) {

        if (which & SSL3_CC_HANDSHAKE) {
@@ -352,7 +346,23 @@ int tls13_change_cipher_state(SSL *s, int which) 
        }

        }

    }

    }





    if (!tls13_derive_secret(s, insecret, label, labellen, secret)) {

    if (label != client_application_traffic) {

        if (!ssl3_digest_cached_records(s, 1)

                || !ssl_handshake_hash(s, hash, sizeof(hashval), &hashlen)) {

            SSLerr(SSL_F_TLS13_CHANGE_CIPHER_STATE, ERR_R_INTERNAL_ERROR);

            goto err;

        }



        /*

         * Save the hash of handshakes up to now for use when we calculate the

         * client application traffic secret

         */

        if (label == server_application_traffic)

            memcpy(s->server_finished_hash, hash, hashlen);

    }



    if (!tls13_hkdf_expand(s, insecret, label, labellen, hash, secret,

                           hashlen)) {

        SSLerr(SSL_F_TLS13_CHANGE_CIPHER_STATE, ERR_R_INTERNAL_ERROR);

        SSLerr(SSL_F_TLS13_CHANGE_CIPHER_STATE, ERR_R_INTERNAL_ERROR);

        goto err;

        goto err;

    }

    }

test/tls13secretstest.c

+9 −2
Original line number Original line Diff line number Diff line
@@ -186,12 +186,19 @@ static int test_secret(SSL *s, unsigned char *prk, 
                       const unsigned char *ref_secret,

                       const unsigned char *ref_secret,

                       const unsigned char *ref_key, const unsigned char *ref_iv)

                       const unsigned char *ref_key, const unsigned char *ref_iv)

{

{

    size_t hashsize = EVP_MD_size(ssl_handshake_md(s));

    size_t hashsize;

    unsigned char gensecret[EVP_MAX_MD_SIZE];

    unsigned char gensecret[EVP_MAX_MD_SIZE];

    unsigned char hash[EVP_MAX_MD_SIZE];

    unsigned char key[KEYLEN];

    unsigned char key[KEYLEN];

    unsigned char iv[IVLEN];

    unsigned char iv[IVLEN];





    if (!tls13_derive_secret(s, prk, label, labellen, gensecret)) {

    if (!ssl_handshake_hash(s, hash, sizeof(hash), &hashsize)) {

        fprintf(stderr, "Failed to get hash\n");

        return 0;

    }



    if (!tls13_hkdf_expand(s, prk, label, labellen, hash, gensecret,

                           hashsize)) {

        fprintf(stderr, "Secret generation failed\n");

        fprintf(stderr, "Secret generation failed\n");

        return 0;

        return 0;

    }

    }