Split the record protection from the TLSv1.2 record layer.

When changing cipher state, DTLS requires that the previous write
protection state remain available so that messages can be retransmitted.
Currently, this is done by DTLS saving and restoring various pointers,
along with special casing to not free the cipher and hash where it would
normally be freed for TLS (and requiring DTLS to free things at the
appropriate times).

This can be handled in a much cleaner manner by splitting the record
protection from the record layer. This allows for the previous write state
to be retained and restored by swapping a single pointer. Additionally,
it also results in more readable and manageable code.

This diff simply splits the record protection from the record layer -
future changes will add support for maintaining and switching between
write states.

ok inoguchi@ tb@

1 files changed, 101 insertions, 75 deletions

diff --git a/src/lib/libssl/tls12_record_layer.c b/src/lib/libssl/tls12_record_layer.c
index 600b73987e..2b331355be 100644
--- a/src/lib/libssl/tls12_record_layer.c
+++ b/src/lib/libssl/tls12_record_layer.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: tls12_record_layer.c,v 1.7 2021/01/07 15:37:19 jsing Exp $ */
+/* $OpenBSD: tls12_record_layer.c,v 1.8 2021/01/12 17:47:20 jsing Exp $ */
 /*
  * Copyright (c) 2020 Joel Sing <jsing@openbsd.org>
  *
@@ -21,35 +21,51 @@
 
 #include "ssl_locl.h"
 
-struct tls12_record_layer {
-        uint16_t version;
-        int dtls;
-
-        uint8_t alert_desc;
-
-        uint16_t read_epoch;
-        uint16_t write_epoch;
+struct tls12_record_protection {
+        uint16_t epoch;
 
-        int read_stream_mac;
-        int write_stream_mac;
+        int stream_mac;
 
-        uint8_t *read_mac_key;
-        size_t read_mac_key_len;
+        uint8_t *mac_key;
+        size_t mac_key_len;
 
         /*
          * XXX - for now these are just pointers to externally managed
          * structs/memory. These should eventually be owned by the record layer.
          */
-        SSL_AEAD_CTX *read_aead_ctx;
-        SSL_AEAD_CTX *write_aead_ctx;
+        SSL_AEAD_CTX *aead_ctx;
+
+        EVP_CIPHER_CTX *cipher_ctx;
+        EVP_MD_CTX *hash_ctx;
+
+        uint8_t *seq_num;
+};
+
+static struct tls12_record_protection *
+tls12_record_protection_new(void)
+{
+        return calloc(1, sizeof(struct tls12_record_protection));
+}
+
+static void
+tls12_record_protection_free(struct tls12_record_protection *rp)
+{
+        if (rp == NULL)
+                return;
+
+        freezero(rp->mac_key, rp->mac_key_len);
+
+        freezero(rp, sizeof(struct tls12_record_protection));
+}
 
-        EVP_CIPHER_CTX *read_cipher_ctx;
-        EVP_MD_CTX *read_hash_ctx;
-        EVP_CIPHER_CTX *write_cipher_ctx;
-        EVP_MD_CTX *write_hash_ctx;
+struct tls12_record_layer {
+        uint16_t version;
+        int dtls;
 
-        uint8_t *read_seq_num;
-        uint8_t *write_seq_num;
+        uint8_t alert_desc;
+
+        struct tls12_record_protection *read;
+        struct tls12_record_protection *write;
 };
 
 struct tls12_record_layer *
@@ -58,9 +74,18 @@ tls12_record_layer_new(void)
         struct tls12_record_layer *rl;
 
         if ((rl = calloc(1, sizeof(struct tls12_record_layer))) == NULL)
-                return NULL;
+                goto err;
+        if ((rl->read = tls12_record_protection_new()) == NULL)
+                goto err;
+        if ((rl->write = tls12_record_protection_new()) == NULL)
+                goto err;
 
         return rl;
+
+ err:
+        tls12_record_layer_free(rl);
+
+        return NULL;
 }
 
 void
@@ -69,7 +94,8 @@ tls12_record_layer_free(struct tls12_record_layer *rl)
         if (rl == NULL)
                 return;
 
-        freezero(rl->read_mac_key, rl->read_mac_key_len);
+        tls12_record_protection_free(rl->read);
+        tls12_record_protection_free(rl->write);
 
         freezero(rl, sizeof(struct tls12_record_layer));
 }
@@ -90,13 +116,13 @@ tls12_record_layer_set_version(struct tls12_record_layer *rl, uint16_t version)
 void
 tls12_record_layer_set_read_epoch(struct tls12_record_layer *rl, uint16_t epoch)
 {
-        rl->read_epoch = epoch;
+        rl->read->epoch = epoch;
 }
 
 void
 tls12_record_layer_set_write_epoch(struct tls12_record_layer *rl, uint16_t epoch)
 {
-        rl->write_epoch = epoch;
+        rl->write->epoch = epoch;
 }
 
 static void
@@ -104,11 +130,11 @@ tls12_record_layer_set_read_state(struct tls12_record_layer *rl,
     SSL_AEAD_CTX *aead_ctx, EVP_CIPHER_CTX *cipher_ctx, EVP_MD_CTX *hash_ctx,
     int stream_mac)
 {
-        rl->read_aead_ctx = aead_ctx;
+        rl->read->aead_ctx = aead_ctx;
 
-        rl->read_cipher_ctx = cipher_ctx;
-        rl->read_hash_ctx = hash_ctx;
-        rl->read_stream_mac = stream_mac;
+        rl->read->cipher_ctx = cipher_ctx;
+        rl->read->hash_ctx = hash_ctx;
+        rl->read->stream_mac = stream_mac;
 }
 
 static void
@@ -116,11 +142,11 @@ tls12_record_layer_set_write_state(struct tls12_record_layer *rl,
     SSL_AEAD_CTX *aead_ctx, EVP_CIPHER_CTX *cipher_ctx, EVP_MD_CTX *hash_ctx,
     int stream_mac)
 {
-        rl->write_aead_ctx = aead_ctx;
+        rl->write->aead_ctx = aead_ctx;
 
-        rl->write_cipher_ctx = cipher_ctx;
-        rl->write_hash_ctx = hash_ctx;
-        rl->write_stream_mac = stream_mac;
+        rl->write->cipher_ctx = cipher_ctx;
+        rl->write->hash_ctx = hash_ctx;
+        rl->write->stream_mac = stream_mac;
 }
 
 void
@@ -128,28 +154,28 @@ tls12_record_layer_clear_read_state(struct tls12_record_layer *rl)
 {
         tls12_record_layer_set_read_state(rl, NULL, NULL, NULL, 0);
         tls12_record_layer_set_read_mac_key(rl, NULL, 0);
-        rl->read_seq_num = NULL;
+        rl->read->seq_num = NULL;
 }
 
 void
 tls12_record_layer_clear_write_state(struct tls12_record_layer *rl)
 {
         tls12_record_layer_set_write_state(rl, NULL, NULL, NULL, 0);
-        rl->write_seq_num = NULL;
+        rl->write->seq_num = NULL;
 }
 
 void
 tls12_record_layer_set_read_seq_num(struct tls12_record_layer *rl,
     uint8_t *seq_num)
 {
-        rl->read_seq_num = seq_num;
+        rl->read->seq_num = seq_num;
 }
 
 void
 tls12_record_layer_set_write_seq_num(struct tls12_record_layer *rl,
     uint8_t *seq_num)
 {
-        rl->write_seq_num = seq_num;
+        rl->write->seq_num = seq_num;
 }
 
 int
@@ -194,18 +220,18 @@ int
 tls12_record_layer_set_read_mac_key(struct tls12_record_layer *rl,
     const uint8_t *mac_key, size_t mac_key_len)
 {
-        freezero(rl->read_mac_key, rl->read_mac_key_len);
-        rl->read_mac_key = NULL;
-        rl->read_mac_key_len = 0;
+        freezero(rl->read->mac_key, rl->read->mac_key_len);
+        rl->read->mac_key = NULL;
+        rl->read->mac_key_len = 0;
 
         if (mac_key == NULL || mac_key_len == 0)
                 return 1;
 
-        if ((rl->read_mac_key = calloc(1, mac_key_len)) == NULL)
+        if ((rl->read->mac_key = calloc(1, mac_key_len)) == NULL)
                 return 0;
 
-        memcpy(rl->read_mac_key, mac_key, mac_key_len);
-        rl->read_mac_key_len = mac_key_len;
+        memcpy(rl->read->mac_key, mac_key, mac_key_len);
+        rl->read->mac_key_len = mac_key_len;
 
         return 1;
 }
@@ -328,19 +354,19 @@ tls12_record_layer_read_mac_cbc(struct tls12_record_layer *rl, CBB *cbb,
          * Must be constant time to avoid leaking details about CBC padding.
          */
 
-        if (!ssl3_cbc_record_digest_supported(rl->read_hash_ctx))
+        if (!ssl3_cbc_record_digest_supported(rl->read->hash_ctx))
                 goto err;
 
         if (!tls12_record_layer_pseudo_header(rl, content_type, content_len,
-            rl->read_epoch, rl->read_seq_num, SSL3_SEQUENCE_SIZE,
+            rl->read->epoch, rl->read->seq_num, SSL3_SEQUENCE_SIZE,
             &header, &header_len))
                 goto err;
 
         if (!CBB_add_space(cbb, &mac, mac_len))
                 goto err;
-        if (!ssl3_cbc_digest_record(rl->read_hash_ctx, mac, &out_mac_len, header,
+        if (!ssl3_cbc_digest_record(rl->read->hash_ctx, mac, &out_mac_len, header,
             content, content_len + mac_len, content_len + mac_len + padding_len,
-            rl->read_mac_key, rl->read_mac_key_len))
+            rl->read->mac_key, rl->read->mac_key_len))
                 goto err;
         if (mac_len != out_mac_len)
                 goto err;
@@ -357,14 +383,14 @@ static int
 tls12_record_layer_read_mac(struct tls12_record_layer *rl, CBB *cbb,
     uint8_t content_type, const uint8_t *content, size_t content_len)
 {
-        EVP_CIPHER_CTX *enc = rl->read_cipher_ctx;
+        EVP_CIPHER_CTX *enc = rl->read->cipher_ctx;
         size_t out_len;
 
         if (EVP_CIPHER_CTX_mode(enc) == EVP_CIPH_CBC_MODE)
                 return 0;
 
-        return tls12_record_layer_mac(rl, cbb, rl->read_hash_ctx,
-            rl->read_stream_mac, rl->read_epoch, rl->read_seq_num,
+        return tls12_record_layer_mac(rl, cbb, rl->read->hash_ctx,
+            rl->read->stream_mac, rl->read->epoch, rl->read->seq_num,
             SSL3_SEQUENCE_SIZE, content_type, content, content_len, &out_len);
 }
 
@@ -373,8 +399,8 @@ tls12_record_layer_write_mac(struct tls12_record_layer *rl, CBB *cbb,
     uint8_t content_type, const uint8_t *content, size_t content_len,
     size_t *out_len)
 {
-        return tls12_record_layer_mac(rl, cbb, rl->write_hash_ctx,
-            rl->write_stream_mac, rl->write_epoch, rl->write_seq_num,
+        return tls12_record_layer_mac(rl, cbb, rl->write->hash_ctx,
+            rl->write->stream_mac, rl->write->epoch, rl->write->seq_num,
             SSL3_SEQUENCE_SIZE, content_type, content, content_len, out_len);
 }
 
@@ -456,7 +482,7 @@ static int
 tls12_record_layer_open_record_plaintext(struct tls12_record_layer *rl,
     uint8_t content_type, CBS *fragment, uint8_t **out, size_t *out_len)
 {
-        if (rl->read_aead_ctx != NULL || rl->read_cipher_ctx != NULL)
+        if (rl->read->aead_ctx != NULL || rl->read->cipher_ctx != NULL)
                 return 0;
 
         /* XXX - decrypt/process in place for now. */
@@ -470,7 +496,7 @@ static int
 tls12_record_layer_open_record_protected_aead(struct tls12_record_layer *rl,
     uint8_t content_type, CBS *fragment, uint8_t **out, size_t *out_len)
 {
-        const SSL_AEAD_CTX *aead = rl->read_aead_ctx;
+        const SSL_AEAD_CTX *aead = rl->read->aead_ctx;
         uint8_t *header = NULL, *nonce = NULL;
         size_t header_len = 0, nonce_len = 0;
         uint8_t *plain;
@@ -482,7 +508,7 @@ tls12_record_layer_open_record_protected_aead(struct tls12_record_layer *rl,
         /* XXX - move to nonce allocated in record layer, matching TLSv1.3 */
         if (aead->xor_fixed_nonce) {
                 if (!tls12_record_layer_aead_xored_nonce(rl, aead,
-                    rl->read_seq_num, &nonce, &nonce_len))
+                    rl->read->seq_num, &nonce, &nonce_len))
                         goto err;
         } else if (aead->variable_nonce_in_record) {
                 if (!CBS_get_bytes(fragment, &var_nonce,
@@ -493,7 +519,7 @@ tls12_record_layer_open_record_protected_aead(struct tls12_record_layer *rl,
                         goto err;
         } else {
                 if (!tls12_record_layer_aead_concat_nonce(rl, aead,
-                    rl->read_seq_num, &nonce, &nonce_len))
+                    rl->read->seq_num, &nonce, &nonce_len))
                         goto err;
         }
 
@@ -512,7 +538,7 @@ tls12_record_layer_open_record_protected_aead(struct tls12_record_layer *rl,
         plain_len = CBS_len(fragment) - aead->tag_len;
 
         if (!tls12_record_layer_pseudo_header(rl, content_type, plain_len,
-            epoch, rl->read_seq_num, SSL3_SEQUENCE_SIZE, &header, &header_len))
+            epoch, rl->read->seq_num, SSL3_SEQUENCE_SIZE, &header, &header_len))
                 goto err;
 
         if (!EVP_AEAD_CTX_open(&aead->ctx, plain, out_len, plain_len,
@@ -545,7 +571,7 @@ static int
 tls12_record_layer_open_record_protected_cipher(struct tls12_record_layer *rl,
     uint8_t content_type, CBS *fragment, uint8_t **out, size_t *out_len)
 {
-        EVP_CIPHER_CTX *enc = rl->read_cipher_ctx;
+        EVP_CIPHER_CTX *enc = rl->read->cipher_ctx;
         SSL3_RECORD_INTERNAL rrec;
         int block_size, eiv_len;
         uint8_t *mac = NULL;
@@ -573,8 +599,8 @@ tls12_record_layer_open_record_protected_cipher(struct tls12_record_layer *rl,
                 goto err;
 
         mac_len = 0;
-        if (rl->read_hash_ctx != NULL) {
-                mac_len = EVP_MD_CTX_size(rl->read_hash_ctx);
+        if (rl->read->hash_ctx != NULL) {
+                mac_len = EVP_MD_CTX_size(rl->read->hash_ctx);
                 if (mac_len <= 0 || mac_len > EVP_MAX_MD_SIZE)
                         goto err;
         }
@@ -689,11 +715,11 @@ tls12_record_layer_open_record(struct tls12_record_layer *rl, uint8_t *buf,
         if (!CBS_get_u16_length_prefixed(&cbs, &fragment))
                 return 0;
 
-        if (rl->read_aead_ctx != NULL) {
+        if (rl->read->aead_ctx != NULL) {
                 if (!tls12_record_layer_open_record_protected_aead(rl,
                     content_type, &fragment, out, out_len))
                         return 0;
-        } else if (rl->read_cipher_ctx != NULL) {
+        } else if (rl->read->cipher_ctx != NULL) {
                 if (!tls12_record_layer_open_record_protected_cipher(rl,
                     content_type, &fragment, out, out_len))
                         return 0;
@@ -704,7 +730,7 @@ tls12_record_layer_open_record(struct tls12_record_layer *rl, uint8_t *buf,
         }
 
         if (!rl->dtls)
-                tls1_record_sequence_increment(rl->read_seq_num);
+                tls1_record_sequence_increment(rl->read->seq_num);
 
         return 1;
 }
@@ -713,7 +739,7 @@ static int
 tls12_record_layer_seal_record_plaintext(struct tls12_record_layer *rl,
     uint8_t content_type, const uint8_t *content, size_t content_len, CBB *out)
 {
-        if (rl->write_aead_ctx != NULL || rl->write_cipher_ctx != NULL)
+        if (rl->write->aead_ctx != NULL || rl->write->cipher_ctx != NULL)
                 return 0;
 
         return CBB_add_bytes(out, content, content_len);
@@ -723,7 +749,7 @@ static int
 tls12_record_layer_seal_record_protected_aead(struct tls12_record_layer *rl,
     uint8_t content_type, const uint8_t *content, size_t content_len, CBB *out)
 {
-        const SSL_AEAD_CTX *aead = rl->write_aead_ctx;
+        const SSL_AEAD_CTX *aead = rl->write->aead_ctx;
         uint8_t *header = NULL, *nonce = NULL;
         size_t header_len = 0, nonce_len = 0;
         size_t enc_record_len, out_len;
@@ -734,22 +760,22 @@ tls12_record_layer_seal_record_protected_aead(struct tls12_record_layer *rl,
         /* XXX - move to nonce allocated in record layer, matching TLSv1.3 */
         if (aead->xor_fixed_nonce) {
                 if (!tls12_record_layer_aead_xored_nonce(rl, aead,
-                    rl->write_seq_num, &nonce, &nonce_len))
+                    rl->write->seq_num, &nonce, &nonce_len))
                         goto err;
         } else {
                 if (!tls12_record_layer_aead_concat_nonce(rl, aead,
-                    rl->write_seq_num, &nonce, &nonce_len))
+                    rl->write->seq_num, &nonce, &nonce_len))
                         goto err;
         }
 
         if (aead->variable_nonce_in_record) {
                 /* XXX - length check? */
-                if (!CBB_add_bytes(out, rl->write_seq_num, aead->variable_nonce_len))
+                if (!CBB_add_bytes(out, rl->write->seq_num, aead->variable_nonce_len))
                         goto err;
         }
 
         if (!tls12_record_layer_pseudo_header(rl, content_type, content_len,
-            epoch, rl->write_seq_num, SSL3_SEQUENCE_SIZE, &header, &header_len))
+            epoch, rl->write->seq_num, SSL3_SEQUENCE_SIZE, &header, &header_len))
                 goto err;
 
         /* XXX EVP_AEAD_max_tag_len vs EVP_AEAD_CTX_tag_len. */
@@ -779,7 +805,7 @@ static int
 tls12_record_layer_seal_record_protected_cipher(struct tls12_record_layer *rl,
     uint8_t content_type, const uint8_t *content, size_t content_len, CBB *out)
 {
-        EVP_CIPHER_CTX *enc = rl->write_cipher_ctx;
+        EVP_CIPHER_CTX *enc = rl->write->cipher_ctx;
         size_t mac_len, pad_len;
         int block_size, eiv_len;
         uint8_t *enc_data, *eiv, *pad, pad_val;
@@ -808,7 +834,7 @@ tls12_record_layer_seal_record_protected_cipher(struct tls12_record_layer *rl,
                 goto err;
 
         mac_len = 0;
-        if (rl->write_hash_ctx != NULL) {
+        if (rl->write->hash_ctx != NULL) {
                 if (!tls12_record_layer_write_mac(rl, &cbb, content_type,
                     content, content_len, &mac_len))
                         goto err;
@@ -865,18 +891,18 @@ tls12_record_layer_seal_record(struct tls12_record_layer *rl,
                 return 0;
         if (rl->dtls) {
                 if (!tls12_record_layer_build_seq_num(rl, cbb,
-                    rl->write_epoch, rl->write_seq_num,
+                    rl->write->epoch, rl->write->seq_num,
                     SSL3_SEQUENCE_SIZE))
                         return 0;
         }
         if (!CBB_add_u16_length_prefixed(cbb, &fragment))
                 return 0;
 
-        if (rl->write_aead_ctx != NULL) {
+        if (rl->write->aead_ctx != NULL) {
                 if (!tls12_record_layer_seal_record_protected_aead(rl,
                     content_type, content, content_len, &fragment))
                         return 0;
-        } else if (rl->write_cipher_ctx != NULL) {
+        } else if (rl->write->cipher_ctx != NULL) {
                 if (!tls12_record_layer_seal_record_protected_cipher(rl,
                     content_type, content, content_len, &fragment))
                         return 0;
@@ -889,7 +915,7 @@ tls12_record_layer_seal_record(struct tls12_record_layer *rl,
         if (!CBB_flush(cbb))
                 return 0;
 
-        tls1_record_sequence_increment(rl->write_seq_num);
+        tls1_record_sequence_increment(rl->write->seq_num);
 
         return 1;
 }