Split TLSv1.3 record protection from record layer.

This makes the TLSv1.2 and TLSv1.3 record layers more consistent and while
it is not currently necessary from a functionality perspective, it makes
for more readable and simpler code.

ok inoguchi@ tb@

1 files changed, 72 insertions, 46 deletions

diff --git a/lib/libssl/tls13_record_layer.c b/lib/libssl/tls13_record_layer.c
index bbecc60674d..4be4bad8602 100644
--- a/lib/libssl/tls13_record_layer.c
+++ b/lib/libssl/tls13_record_layer.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: tls13_record_layer.c,v 1.58 2021/01/05 17:49:04 tb Exp $ */
+/* $OpenBSD: tls13_record_layer.c,v 1.59 2021/03/21 17:25:17 jsing Exp $ */
 /*
  * Copyright (c) 2018, 2019 Joel Sing <jsing@openbsd.org>
  *
@@ -25,6 +25,41 @@ static ssize_t tls13_record_layer_write_chunk(struct tls13_record_layer *rl,
 static ssize_t tls13_record_layer_write_record(struct tls13_record_layer *rl,
     uint8_t content_type, const uint8_t *content, size_t content_len);
 
+struct tls13_record_protection {
+	EVP_AEAD_CTX aead_ctx;
+	struct tls13_secret iv;
+	struct tls13_secret nonce;
+	uint8_t seq_num[TLS13_RECORD_SEQ_NUM_LEN];
+};
+
+struct tls13_record_protection *
+tls13_record_protection_new(void)
+{
+	return calloc(1, sizeof(struct tls13_record_protection));
+}
+
+void
+tls13_record_protection_clear(struct tls13_record_protection *rp)
+{
+	EVP_AEAD_CTX_cleanup(&rp->aead_ctx);
+
+	tls13_secret_cleanup(&rp->iv);
+	tls13_secret_cleanup(&rp->nonce);
+
+	memset(rp->seq_num, 0, sizeof(rp->seq_num));
+}
+
+void
+tls13_record_protection_free(struct tls13_record_protection *rp)
+{
+	if (rp == NULL)
+		return;
+
+	tls13_record_protection_clear(rp);
+
+	freezero(rp, sizeof(struct tls13_record_protection));
+}
+
 struct tls13_record_layer {
 	uint16_t legacy_version;
 
@@ -75,14 +110,8 @@ struct tls13_record_layer {
 	/* Record protection. */
 	const EVP_MD *hash;
 	const EVP_AEAD *aead;
-	EVP_AEAD_CTX read_aead_ctx;
-	EVP_AEAD_CTX write_aead_ctx;
-	struct tls13_secret read_iv;
-	struct tls13_secret write_iv;
-	struct tls13_secret read_nonce;
-	struct tls13_secret write_nonce;
-	uint8_t read_seq_num[TLS13_RECORD_SEQ_NUM_LEN];
-	uint8_t write_seq_num[TLS13_RECORD_SEQ_NUM_LEN];
+	struct tls13_record_protection *read;
+	struct tls13_record_protection *write;
 
 	/* Callbacks. */
 	struct tls13_record_layer_callbacks cb;
@@ -120,13 +149,23 @@ tls13_record_layer_new(const struct tls13_record_layer_callbacks *callbacks,
 	struct tls13_record_layer *rl;
 
 	if ((rl = calloc(1, sizeof(struct tls13_record_layer))) == NULL)
-		return NULL;
+		goto err;
+
+	if ((rl->read = tls13_record_protection_new()) == NULL)
+		goto err;
+	if ((rl->write = tls13_record_protection_new()) == NULL)
+		goto err;
 
 	rl->legacy_version = TLS1_2_VERSION;
 	rl->cb = *callbacks;
 	rl->cb_arg = cb_arg;
 
 	return rl;
+
+ err:
+	tls13_record_layer_free(rl);
+
+	return NULL;
 }
 
 void
@@ -143,13 +182,8 @@ tls13_record_layer_free(struct tls13_record_layer *rl)
 
 	tls13_record_layer_rbuf_free(rl);
 
-	EVP_AEAD_CTX_cleanup(&rl->read_aead_ctx);
-	EVP_AEAD_CTX_cleanup(&rl->write_aead_ctx);
-
-	tls13_secret_cleanup(&rl->read_iv);
-	tls13_secret_cleanup(&rl->write_iv);
-	tls13_secret_cleanup(&rl->read_nonce);
-	tls13_secret_cleanup(&rl->write_nonce);
+	tls13_record_protection_free(rl->read);
+	tls13_record_protection_free(rl->write);
 
 	freezero(rl, sizeof(struct tls13_record_layer));
 }
@@ -430,32 +464,28 @@ tls13_record_layer_phh(struct tls13_record_layer *rl, CBS *cbs)
 }
 
 static int
-tls13_record_layer_set_traffic_key(const EVP_AEAD *aead, EVP_AEAD_CTX *aead_ctx,
-    const EVP_MD *hash, struct tls13_secret *iv, struct tls13_secret *nonce,
-    struct tls13_secret *traffic_key)
+tls13_record_layer_set_traffic_key(const EVP_AEAD *aead, const EVP_MD *hash,
+    struct tls13_record_protection *rp, struct tls13_secret *traffic_key)
 {
 	struct tls13_secret context = { .data = "", .len = 0 };
 	struct tls13_secret key = { .data = NULL, .len = 0 };
 	int ret = 0;
 
-	EVP_AEAD_CTX_cleanup(aead_ctx);
+	tls13_record_protection_clear(rp);
 
-	tls13_secret_cleanup(iv);
-	tls13_secret_cleanup(nonce);
-
-	if (!tls13_secret_init(iv, EVP_AEAD_nonce_length(aead)))
+	if (!tls13_secret_init(&rp->iv, EVP_AEAD_nonce_length(aead)))
 		goto err;
-	if (!tls13_secret_init(nonce, EVP_AEAD_nonce_length(aead)))
+	if (!tls13_secret_init(&rp->nonce, EVP_AEAD_nonce_length(aead)))
 		goto err;
 	if (!tls13_secret_init(&key, EVP_AEAD_key_length(aead)))
 		goto err;
 
-	if (!tls13_hkdf_expand_label(iv, hash, traffic_key, "iv", &context))
+	if (!tls13_hkdf_expand_label(&rp->iv, hash, traffic_key, "iv", &context))
 		goto err;
 	if (!tls13_hkdf_expand_label(&key, hash, traffic_key, "key", &context))
 		goto err;
 
-	if (!EVP_AEAD_CTX_init(aead_ctx, aead, key.data, key.len,
+	if (!EVP_AEAD_CTX_init(&rp->aead_ctx, aead, key.data, key.len,
 	    EVP_AEAD_DEFAULT_TAG_LENGTH, NULL))
 		goto err;
 
@@ -471,20 +501,16 @@ int
 tls13_record_layer_set_read_traffic_key(struct tls13_record_layer *rl,
     struct tls13_secret *read_key)
 {
-	memset(rl->read_seq_num, 0, TLS13_RECORD_SEQ_NUM_LEN);
-
-	return tls13_record_layer_set_traffic_key(rl->aead, &rl->read_aead_ctx,
-	    rl->hash, &rl->read_iv, &rl->read_nonce, read_key);
+	return tls13_record_layer_set_traffic_key(rl->aead, rl->hash,
+	    rl->read, read_key);
 }
 
 int
 tls13_record_layer_set_write_traffic_key(struct tls13_record_layer *rl,
     struct tls13_secret *write_key)
 {
-	memset(rl->write_seq_num, 0, TLS13_RECORD_SEQ_NUM_LEN);
-
-	return tls13_record_layer_set_traffic_key(rl->aead, &rl->write_aead_ctx,
-	    rl->hash, &rl->write_iv, &rl->write_nonce, write_key);
+	return tls13_record_layer_set_traffic_key(rl->aead, rl->hash,
+	    rl->write, write_key);
 }
 
 static int
@@ -541,13 +567,13 @@ tls13_record_layer_open_record_protected(struct tls13_record_layer *rl)
 		goto err;
 	content_len = CBS_len(&enc_record);
 
-	if (!tls13_record_layer_update_nonce(&rl->read_nonce, &rl->read_iv,
-	    rl->read_seq_num))
+	if (!tls13_record_layer_update_nonce(&rl->read->nonce, &rl->read->iv,
+	    rl->read->seq_num))
 		goto err;
 
-	if (!EVP_AEAD_CTX_open(&rl->read_aead_ctx,
+	if (!EVP_AEAD_CTX_open(&rl->read->aead_ctx,
 	    content, &out_len, content_len,
-	    rl->read_nonce.data, rl->read_nonce.len,
+	    rl->read->nonce.data, rl->read->nonce.len,
 	    CBS_data(&enc_record), CBS_len(&enc_record),
 	    CBS_data(&header), CBS_len(&header)))
 		goto err;
@@ -557,7 +583,7 @@ tls13_record_layer_open_record_protected(struct tls13_record_layer *rl)
 		goto err;
 	}
 
-	if (!tls13_record_layer_inc_seq_num(rl->read_seq_num))
+	if (!tls13_record_layer_inc_seq_num(rl->read->seq_num))
 		goto err;
 
 	/*
@@ -718,8 +744,8 @@ tls13_record_layer_seal_record_protected(struct tls13_record_layer *rl,
 	if (!CBB_finish(&cbb, &data, &data_len))
 		goto err;
 
-	if (!tls13_record_layer_update_nonce(&rl->write_nonce,
-	    &rl->write_iv, rl->write_seq_num))
+	if (!tls13_record_layer_update_nonce(&rl->write->nonce,
+	    &rl->write->iv, rl->write->seq_num))
 		goto err;
 
 	/*
@@ -727,16 +753,16 @@ tls13_record_layer_seal_record_protected(struct tls13_record_layer *rl,
 	 * this would avoid a copy since the inner would be passed as two
 	 * separate pieces.
 	 */
-	if (!EVP_AEAD_CTX_seal(&rl->write_aead_ctx,
+	if (!EVP_AEAD_CTX_seal(&rl->write->aead_ctx,
 	    enc_record, &out_len, enc_record_len,
-	    rl->write_nonce.data, rl->write_nonce.len,
+	    rl->write->nonce.data, rl->write->nonce.len,
 	    inner, inner_len, header, header_len))
 		goto err;
 
 	if (out_len != enc_record_len)
 		goto err;
 
-	if (!tls13_record_layer_inc_seq_num(rl->write_seq_num))
+	if (!tls13_record_layer_inc_seq_num(rl->write->seq_num))
 		goto err;
 
 	if (!tls13_record_set_data(rl->wrec, data, data_len))