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: Rich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/2157)

diff --git a/ssl/ssl_locl.h b/ssl/ssl_locl.h
index 06c1a09f52d852383d7e7885e0667295b108966e..8186a7f5ba55a8e8484536ad23612cc90ebde3bd 100644 (file)
--- a/ssl/ssl_locl.h
+++ b/ssl/ssl_locl.h
@@ -955,6 +955,7 @@ struct ssl_st {
     unsigned char handshake_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_hash[EVP_MAX_MD_SIZE];
     EVP_CIPHER_CTX *enc_read_ctx; /* cryptographic state */
     unsigned char read_iv[EVP_MAX_IV_LENGTH]; /* TLSv1.3 static read IV */
     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,
                                      unsigned char *p);
 __owur int tls13_change_cipher_state(SSL *s, int which);
-__owur int tls13_derive_secret(SSL *s, const unsigned char *insecret,
-                               const unsigned char *label, size_t labellen,
-                               unsigned char *secret);
+__owur int tls13_hkdf_expand(SSL *s, const unsigned char *secret,
+                             const unsigned char *label, size_t labellen,
+                             const unsigned char *hash,
+                             unsigned char *out, size_t outlen);
 __owur int tls13_derive_key(SSL *s, const unsigned char *secret,
                             unsigned char *key, size_t keylen);
 __owur int tls13_derive_iv(SSL *s, const unsigned char *secret,

diff --git a/ssl/tls13_enc.c b/ssl/tls13_enc.c
index cbe989c6a2eb62a0c95027e3723a72e16391db34..7ee9bb8ca351c9e3cee0668ac161c2283c74ef73 100644 (file)
--- a/ssl/tls13_enc.c
+++ b/ssl/tls13_enc.c
@@ -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
  * 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 *hash,
                              unsigned char *out, size_t outlen)
@@ -74,29 +74,6 @@ static int tls13_hkdf_expand(SSL *s, const unsigned char *secret,
     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
  * 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 *iv;
     unsigned char secret[EVP_MAX_MD_SIZE];
+    unsigned char hashval[EVP_MAX_MD_SIZE];
+    unsigned char *hash = hashval;
     unsigned char *insecret;
     unsigned char *finsecret = NULL;
     EVP_CIPHER_CTX *ciph_ctx;
     const EVP_CIPHER *ciph = s->s3->tmp.new_sym_enc;
     size_t ivlen, keylen, finsecretlen = 0;
     const unsigned char *label;
-    size_t labellen;
+    size_t labellen, hashlen = 0;
     int ret = 0;
 
     if (which & SSL3_CC_READ) {
@@ -334,9 +313,24 @@ int tls13_change_cipher_state(SSL *s, int which)
             label = client_handshake_traffic;
             labellen = sizeof(client_handshake_traffic) - 1;
         } else {
+            int hashleni;
+
             insecret = s->session->master_key;
             label = client_application_traffic;
             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 {
         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);
         goto err;
     }

diff --git a/test/tls13secretstest.c b/test/tls13secretstest.c
index 8734f2ae03dde45c916f842645f70c9d504c82e0..93b6e442578ae46ad4e1cbb701f726f33f758b62 100644 (file)
--- a/test/tls13secretstest.c
+++ b/test/tls13secretstest.c
@@ -186,12 +186,19 @@ static int test_secret(SSL *s, unsigned char *prk,
                        const unsigned char *ref_secret,
                        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 hash[EVP_MAX_MD_SIZE];
     unsigned char key[KEYLEN];
     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");
         return 0;
     }