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

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,

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;
     }

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;
     }