Simplify nonce handling in the TLSv1.2 record layer.

Pass the CBS for the sequence number through, which also allows us to do
more sensible length checks. Also, add a missing length check while here.

ok inoguchi@ tb@

1 files changed, 16 insertions, 13 deletions

diff --git a/src/lib/libssl/tls12_record_layer.c b/src/lib/libssl/tls12_record_layer.c
index 06d1af9def..481680d9cc 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.30 2021/05/16 15:49:01 jsing Exp $ */
+/* $OpenBSD: tls12_record_layer.c,v 1.31 2021/06/14 14:22:52 jsing Exp $ */
 /*
  * Copyright (c) 2020 Joel Sing <jsing@openbsd.org>
  *
@@ -780,12 +780,12 @@ tls12_record_layer_write_mac(struct tls12_record_layer *rl, CBB *cbb,
 
 static int
 tls12_record_layer_aead_concat_nonce(struct tls12_record_layer *rl,
-    struct tls12_record_protection *rp, const uint8_t *seq_num,
+    struct tls12_record_protection *rp, CBS *seq_num,
     uint8_t **out, size_t *out_len)
 {
         CBB cbb;
 
-        if (rp->aead_variable_nonce_len > SSL3_SEQUENCE_SIZE)
+        if (rp->aead_variable_nonce_len > CBS_len(seq_num))
                 return 0;
 
         /* Fixed nonce and variable nonce (sequence number) are concatenated. */
@@ -794,7 +794,8 @@ tls12_record_layer_aead_concat_nonce(struct tls12_record_layer *rl,
         if (!CBB_add_bytes(&cbb, rp->aead_fixed_nonce,
             rp->aead_fixed_nonce_len))
                 goto err;
-        if (!CBB_add_bytes(&cbb, seq_num, rp->aead_variable_nonce_len))
+        if (!CBB_add_bytes(&cbb, CBS_data(seq_num),
+            rp->aead_variable_nonce_len))
                 goto err;
         if (!CBB_finish(&cbb, out, out_len))
                 goto err;
@@ -809,7 +810,7 @@ tls12_record_layer_aead_concat_nonce(struct tls12_record_layer *rl,
 
 static int
 tls12_record_layer_aead_xored_nonce(struct tls12_record_layer *rl,
-    struct tls12_record_protection *rp, const uint8_t *seq_num,
+    struct tls12_record_protection *rp, CBS *seq_num,
     uint8_t **out, size_t *out_len)
 {
         uint8_t *nonce = NULL;
@@ -818,7 +819,7 @@ tls12_record_layer_aead_xored_nonce(struct tls12_record_layer *rl,
         CBB cbb;
         int i;
 
-        if (rp->aead_variable_nonce_len > SSL3_SEQUENCE_SIZE)
+        if (rp->aead_variable_nonce_len > CBS_len(seq_num))
                 return 0;
         if (rp->aead_fixed_nonce_len < rp->aead_variable_nonce_len)
                 return 0;
@@ -832,7 +833,8 @@ tls12_record_layer_aead_xored_nonce(struct tls12_record_layer *rl,
         if (!CBB_add_space(&cbb, &pad,
             rp->aead_fixed_nonce_len - rp->aead_variable_nonce_len))
                 goto err;
-        if (!CBB_add_bytes(&cbb, seq_num, rp->aead_variable_nonce_len))
+        if (!CBB_add_bytes(&cbb, CBS_data(seq_num),
+            rp->aead_variable_nonce_len))
                 goto err;
         if (!CBB_finish(&cbb, &nonce, &nonce_len))
                 goto err;
@@ -882,18 +884,18 @@ tls12_record_layer_open_record_protected_aead(struct tls12_record_layer *rl,
         /* XXX - move to nonce allocated in record layer, matching TLSv1.3 */
         if (rp->aead_xor_nonces) {
                 if (!tls12_record_layer_aead_xored_nonce(rl, rp,
-                    CBS_data(seq_num), &nonce, &nonce_len))
+                    seq_num, &nonce, &nonce_len))
                         goto err;
         } else if (rp->aead_variable_nonce_in_record) {
                 if (!CBS_get_bytes(fragment, &var_nonce,
                     rp->aead_variable_nonce_len))
                         goto err;
                 if (!tls12_record_layer_aead_concat_nonce(rl, rp,
-                    CBS_data(&var_nonce), &nonce, &nonce_len))
+                    &var_nonce, &nonce, &nonce_len))
                         goto err;
         } else {
                 if (!tls12_record_layer_aead_concat_nonce(rl, rp,
-                    CBS_data(seq_num), &nonce, &nonce_len))
+                    seq_num, &nonce, &nonce_len))
                         goto err;
         }
 
@@ -1145,16 +1147,17 @@ tls12_record_layer_seal_record_protected_aead(struct tls12_record_layer *rl,
         /* XXX - move to nonce allocated in record layer, matching TLSv1.3 */
         if (rp->aead_xor_nonces) {
                 if (!tls12_record_layer_aead_xored_nonce(rl, rp,
-                    CBS_data(seq_num), &nonce, &nonce_len))
+                    seq_num, &nonce, &nonce_len))
                         goto err;
         } else {
                 if (!tls12_record_layer_aead_concat_nonce(rl, rp,
-                    CBS_data(seq_num), &nonce, &nonce_len))
+                    seq_num, &nonce, &nonce_len))
                         goto err;
         }
 
         if (rp->aead_variable_nonce_in_record) {
-                /* XXX - length check? */
+                if (rp->aead_variable_nonce_len > CBS_len(seq_num))
+                        goto err;
                 if (!CBB_add_bytes(out, CBS_data(seq_num),
                     rp->aead_variable_nonce_len))
                         goto err;