Actually update the keys when a KeyUpdate message is sent or received

Reviewed-by: NRich Salz <rsalz@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/2609)

include/openssl/ssl.h

@@ -2096,6 +2096,7 @@ int ERR_load_SSL_strings(void);
 # define SSL_F_DANE_CTX_ENABLE                            347
 # define SSL_F_DANE_MTYPE_SET                             393
 # define SSL_F_DANE_TLSA_ADD                              394
+# define SSL_F_DERIVE_SECRET_KEY_AND_IV                   514
 # define SSL_F_DO_DTLS1_WRITE                             245
 # define SSL_F_DO_SSL3_WRITE                              104
 # define SSL_F_DTLS1_BUFFER_RECORD                        247

ssl/ssl_err.c

@@ -28,6 +28,7 @@ static ERR_STRING_DATA SSL_str_functs[] = {
     {ERR_FUNC(SSL_F_DANE_CTX_ENABLE), "dane_ctx_enable"},
     {ERR_FUNC(SSL_F_DANE_MTYPE_SET), "dane_mtype_set"},
     {ERR_FUNC(SSL_F_DANE_TLSA_ADD), "dane_tlsa_add"},
+    {ERR_FUNC(SSL_F_DERIVE_SECRET_KEY_AND_IV), "derive_secret_key_and_iv"},
     {ERR_FUNC(SSL_F_DO_DTLS1_WRITE), "do_dtls1_write"},
     {ERR_FUNC(SSL_F_DO_SSL3_WRITE), "do_ssl3_write"},
     {ERR_FUNC(SSL_F_DTLS1_BUFFER_RECORD), "dtls1_buffer_record"},

ssl/ssl_locl.h

@@ -986,6 +986,8 @@ struct ssl_st {
     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];
+    unsigned char client_app_traffic_secret[EVP_MAX_MD_SIZE];
+    unsigned char server_app_traffic_secret[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 */
@@ -2163,6 +2165,7 @@ __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_update_key(SSL *s, int write);
 __owur int tls13_hkdf_expand(SSL *s, const EVP_MD *md,
                              const unsigned char *secret,
                              const unsigned char *label, size_t labellen,

ssl/statem/statem_clnt.c

@@ -735,6 +735,9 @@ WORK_STATE ossl_statem_client_post_work(SSL *s, WORK_STATE wst)
     case TLS_ST_CW_KEY_UPDATE:
         if (statem_flush(s) != 1)
             return WORK_MORE_A;
+
+        if (!tls13_update_key(s, 1))
+            return WORK_ERROR;
         break;
     }
 

ssl/statem/statem_lib.c

@@ -513,16 +513,16 @@ int tls_construct_key_update(SSL *s, WPACKET *pkt)
 
 MSG_PROCESS_RETURN tls_process_key_update(SSL *s, PACKET *pkt)
 {
+    int al;
     unsigned int updatetype;
 
     if (!PACKET_get_1(pkt, &updatetype)
             || PACKET_remaining(pkt) != 0
             || (updatetype != SSL_KEY_UPDATE_NOT_REQUESTED
                 && updatetype != SSL_KEY_UPDATE_REQUESTED)) {
-        ssl3_send_alert(s, SSL3_AL_FATAL, SSL_AD_DECODE_ERROR);
+        al = SSL_AD_DECODE_ERROR;
         SSLerr(SSL_F_TLS_PROCESS_KEY_UPDATE, SSL_R_BAD_KEY_UPDATE);
-        ossl_statem_set_error(s);
-        return MSG_PROCESS_ERROR;
+        goto err;
     }
 
     /*
@@ -533,7 +533,17 @@ MSG_PROCESS_RETURN tls_process_key_update(SSL *s, PACKET *pkt)
     if (updatetype == SSL_KEY_UPDATE_REQUESTED)
         s->key_update = SSL_KEY_UPDATE_NOT_REQUESTED;
 
+    if (!tls13_update_key(s, 0)) {
+        al = SSL_AD_INTERNAL_ERROR;
+        SSLerr(SSL_F_TLS_PROCESS_KEY_UPDATE, ERR_R_INTERNAL_ERROR);
+        goto err;
+    }
+
     return MSG_PROCESS_FINISHED_READING;
+ err:
+    ssl3_send_alert(s, SSL3_AL_FATAL, al);
+    ossl_statem_set_error(s);
+    return MSG_PROCESS_ERROR;
 }
 
 #ifndef OPENSSL_NO_NEXTPROTONEG

ssl/statem/statem_srvr.c

@@ -841,6 +841,13 @@ WORK_STATE ossl_statem_server_post_work(SSL *s, WORK_STATE wst)
         break;
 
     case TLS_ST_SW_KEY_UPDATE:
+        if (statem_flush(s) != 1)
+            return WORK_MORE_A;
+
+        if (!tls13_update_key(s, 1))
+            return WORK_ERROR;
+        break;
+
     case TLS_ST_SW_SESSION_TICKET:
         if (SSL_IS_TLS13(s) && statem_flush(s) != 1)
             return WORK_MORE_A;

ssl/tls13_enc.c

@@ -242,6 +242,74 @@ int tls13_setup_key_block(SSL *s)
     return 1;
 }
 
+static int derive_secret_key_and_iv(SSL *s, int write,
+                                    const unsigned char *insecret,
+                                    const unsigned char *hash,
+                                    const unsigned char *label,
+                                    size_t labellen, unsigned char *secret,
+                                    unsigned char *iv, EVP_CIPHER_CTX *ciph_ctx)
+{
+    unsigned char key[EVP_MAX_KEY_LENGTH];
+    size_t ivlen, keylen, taglen;
+    const EVP_MD *md = ssl_handshake_md(s);
+    size_t hashlen = EVP_MD_size(md);
+    const EVP_CIPHER *ciph = s->s3->tmp.new_sym_enc;
+
+    if (!tls13_hkdf_expand(s, md, insecret, label, labellen, hash, secret,
+                           hashlen)) {
+        SSLerr(SSL_F_DERIVE_SECRET_KEY_AND_IV, ERR_R_INTERNAL_ERROR);
+        goto err;
+    }
+
+    /* TODO(size_t): convert me */
+    keylen = EVP_CIPHER_key_length(ciph);
+    if (EVP_CIPHER_mode(ciph) == EVP_CIPH_CCM_MODE) {
+        ivlen = EVP_CCM_TLS_IV_LEN;
+        if (s->s3->tmp.new_cipher->algorithm_enc
+                & (SSL_AES128CCM8 | SSL_AES256CCM8))
+            taglen = EVP_CCM8_TLS_TAG_LEN;
+         else
+            taglen = EVP_CCM_TLS_TAG_LEN;
+    } else {
+        ivlen = EVP_CIPHER_iv_length(ciph);
+        taglen = 0;
+    }
+
+    if (!tls13_derive_key(s, secret, key, keylen)
+            || !tls13_derive_iv(s, secret, iv, ivlen)) {
+        SSLerr(SSL_F_DERIVE_SECRET_KEY_AND_IV, ERR_R_INTERNAL_ERROR);
+        goto err;
+    }
+
+    if (EVP_CipherInit_ex(ciph_ctx, ciph, NULL, NULL, NULL, write) <= 0
+        || !EVP_CIPHER_CTX_ctrl(ciph_ctx, EVP_CTRL_AEAD_SET_IVLEN, ivlen, NULL)
+        || (taglen != 0 && !EVP_CIPHER_CTX_ctrl(ciph_ctx, EVP_CTRL_AEAD_SET_TAG,
+                                                taglen, NULL))
+        || EVP_CipherInit_ex(ciph_ctx, NULL, NULL, key, NULL, -1) <= 0) {
+        SSLerr(SSL_F_DERIVE_SECRET_KEY_AND_IV, ERR_R_EVP_LIB);
+        goto err;
+    }
+
+#ifdef OPENSSL_SSL_TRACE_CRYPTO
+    if (s->msg_callback) {
+        int wh = write ? TLS1_RT_CRYPTO_WRITE : 0;
+
+        if (ciph->key_len)
+            s->msg_callback(2, s->version, wh | TLS1_RT_CRYPTO_KEY,
+                            key, ciph->key_len, s, s->msg_callback_arg);
+
+        wh |= TLS1_RT_CRYPTO_IV;
+        s->msg_callback(2, s->version, wh, iv, ivlen, s,
+                        s->msg_callback_arg);
+    }
+#endif
+
+    return 1;
+ err:
+    OPENSSL_cleanse(key, sizeof(key));
+    return 0;
+}
+
 int tls13_change_cipher_state(SSL *s, int which)
 {
     static const unsigned char client_handshake_traffic[] =
@@ -254,7 +322,6 @@ int tls13_change_cipher_state(SSL *s, int which)
         "server application traffic secret";
     static const unsigned char resumption_master_secret[] =
         "resumption master secret";
-    unsigned char key[EVP_MAX_KEY_LENGTH];
     unsigned char *iv;
     unsigned char secret[EVP_MAX_MD_SIZE];
     unsigned char hashval[EVP_MAX_MD_SIZE];
@@ -263,8 +330,7 @@ int tls13_change_cipher_state(SSL *s, int which)
     unsigned char *finsecret = NULL;
     const char *log_label = NULL;
     EVP_CIPHER_CTX *ciph_ctx;
-    const EVP_CIPHER *ciph = s->s3->tmp.new_sym_enc;
-    size_t ivlen, keylen, taglen, finsecretlen = 0;
+    size_t finsecretlen = 0;
     const unsigned char *label;
     size_t labellen, hashlen = 0;
     int ret = 0;
@@ -350,12 +416,6 @@ int tls13_change_cipher_state(SSL *s, int which)
     if (label == server_application_traffic)
         memcpy(s->server_finished_hash, hashval, hashlen);
 
-    if (!tls13_hkdf_expand(s, ssl_handshake_md(s), insecret, label, labellen,
-                           hash, secret, hashlen)) {
-        SSLerr(SSL_F_TLS13_CHANGE_CIPHER_STATE, ERR_R_INTERNAL_ERROR);
-        goto err;
-    }
-
     if (label == client_application_traffic) {
         /*
          * We also create the resumption master secret, but this time use the
@@ -371,64 +431,70 @@ int tls13_change_cipher_state(SSL *s, int which)
         s->session->master_key_length = hashlen;
     }
 
-    /* TODO(size_t): convert me */
-    keylen = EVP_CIPHER_key_length(ciph);
-    if (EVP_CIPHER_mode(ciph) == EVP_CIPH_CCM_MODE) {
-        ivlen = EVP_CCM_TLS_IV_LEN;
-        if (s->s3->tmp.new_cipher->algorithm_enc
-                & (SSL_AES128CCM8 | SSL_AES256CCM8))
-            taglen = EVP_CCM8_TLS_TAG_LEN;
-         else
-            taglen = EVP_CCM_TLS_TAG_LEN;
-    } else {
-        ivlen = EVP_CIPHER_iv_length(ciph);
-        taglen = 0;
+    if (!derive_secret_key_and_iv(s, which & SSL3_CC_WRITE, insecret, hash,
+                                  label, labellen, secret, iv, ciph_ctx)) {
+        goto err;
     }
 
+    if (label == server_application_traffic)
+        memcpy(s->server_app_traffic_secret, secret, hashlen);
+    else if (label == client_application_traffic)
+        memcpy(s->client_app_traffic_secret, secret, hashlen);
+
     if (!ssl_log_secret(s, log_label, secret, hashlen)) {
         SSLerr(SSL_F_TLS13_CHANGE_CIPHER_STATE, ERR_R_INTERNAL_ERROR);
         goto err;
     }
 
-    if (!tls13_derive_key(s, secret, key, keylen)
-            || !tls13_derive_iv(s, secret, iv, ivlen)
-            || (finsecret != NULL && !tls13_derive_finishedkey(s,
-                                                           ssl_handshake_md(s),
-                                                           secret,
-                                                           finsecret,
-                                                           finsecretlen))) {
+    if (finsecret != NULL
+            && !tls13_derive_finishedkey(s, ssl_handshake_md(s), secret,
+                                         finsecret, finsecretlen)) {
         SSLerr(SSL_F_TLS13_CHANGE_CIPHER_STATE, ERR_R_INTERNAL_ERROR);
         goto err;
     }
 
-    if (EVP_CipherInit_ex(ciph_ctx, ciph, NULL, NULL, NULL,
-                          (which & SSL3_CC_WRITE)) <= 0
-        || !EVP_CIPHER_CTX_ctrl(ciph_ctx, EVP_CTRL_AEAD_SET_IVLEN, ivlen, NULL)
-        || (taglen != 0 && !EVP_CIPHER_CTX_ctrl(ciph_ctx, EVP_CTRL_AEAD_SET_TAG,
-                                                taglen, NULL))
-        || EVP_CipherInit_ex(ciph_ctx, NULL, NULL, key, NULL, -1) <= 0) {
-        SSLerr(SSL_F_TLS13_CHANGE_CIPHER_STATE, ERR_R_EVP_LIB);
-        goto err;
-    }
+    ret = 1;
+ err:
+    OPENSSL_cleanse(secret, sizeof(secret));
+    return ret;
+}
 
-#ifdef OPENSSL_SSL_TRACE_CRYPTO
-    if (s->msg_callback) {
-        int wh = which & SSL3_CC_WRITE ? TLS1_RT_CRYPTO_WRITE : 0;
+int tls13_update_key(SSL *s, int write)
+{
+    static const unsigned char application_traffic[] =
+        "application traffic secret";
+    const EVP_MD *md = ssl_handshake_md(s);
+    size_t hashlen = EVP_MD_size(md);
+    unsigned char *insecret, *iv;
+    unsigned char secret[EVP_MAX_MD_SIZE];
+    EVP_CIPHER_CTX *ciph_ctx;
+    int ret = 0;
 
-        if (ciph->key_len)
-            s->msg_callback(2, s->version, wh | TLS1_RT_CRYPTO_KEY,
-                            key, ciph->key_len, s, s->msg_callback_arg);
+    if (s->server == write)
+        insecret = s->server_app_traffic_secret;
+    else
+        insecret = s->client_app_traffic_secret;
 
-        wh |= TLS1_RT_CRYPTO_IV;
-        s->msg_callback(2, s->version, wh, iv, ivlen, s,
-                        s->msg_callback_arg);
+    if (write) {
+        iv = s->write_iv;
+        ciph_ctx = s->enc_write_ctx;
+        RECORD_LAYER_reset_write_sequence(&s->rlayer);
+    } else {
+        iv = s->read_iv;
+        ciph_ctx = s->enc_read_ctx;
+        RECORD_LAYER_reset_read_sequence(&s->rlayer);
     }
-#endif
+
+    if (!derive_secret_key_and_iv(s, write, insecret, NULL, application_traffic,
+                                  sizeof(application_traffic) - 1, secret, iv,
+                                  ciph_ctx))
+        goto err;
+
+    memcpy(insecret, secret, hashlen);
 
     ret = 1;
  err:
     OPENSSL_cleanse(secret, sizeof(secret));
-    OPENSSL_cleanse(key, sizeof(key));
     return ret;
 }