From: jsing <jsing@openbsd.org>
Date: Mon, 29 Mar 2021 16:19:15 +0000 (+0000)
Subject: Move the TLSv1.2 record number increment into the new record layer.
X-Git-Url: http://artulab.com/gitweb/?a=commitdiff_plain;h=e3ae3cd6c9c09e2489c5d8b6149d643e1ead7c07;p=openbsd

Move the TLSv1.2 record number increment into the new record layer.

This adds checks (based on the TLSv1.3 implementation) to ensure that the
TLS/DTLS sequence numbers do not wrap, as required by the respective RFCs.

ok inoguchi@ tb@
---

diff --git a/lib/libssl/ssl_locl.h b/lib/libssl/ssl_locl.h
index 7f197bbcdff..4b2f98f84d2 100644
--- a/lib/libssl/ssl_locl.h
+++ b/lib/libssl/ssl_locl.h
@@ -1,4 +1,4 @@
-/* $OpenBSD: ssl_locl.h,v 1.331 2021/03/27 17:56:28 tb Exp $ */
+/* $OpenBSD: ssl_locl.h,v 1.332 2021/03/29 16:19:15 jsing Exp $ */
 /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
  * All rights reserved.
  *
@@ -1261,7 +1261,6 @@ int ssl3_handshake_msg_finish(SSL *s, CBB *handshake);
 int ssl3_handshake_write(SSL *s);
 int ssl3_record_write(SSL *s, int type);
 
-void tls1_record_sequence_increment(unsigned char *seq);
 int ssl3_do_change_cipher_spec(SSL *ssl);
 
 int dtls1_do_write(SSL *s, int type);
diff --git a/lib/libssl/t1_enc.c b/lib/libssl/t1_enc.c
index b9dcbac6614..0ddd52b5301 100644
--- a/lib/libssl/t1_enc.c
+++ b/lib/libssl/t1_enc.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: t1_enc.c,v 1.135 2021/03/24 18:44:00 jsing Exp $ */
+/* $OpenBSD: t1_enc.c,v 1.136 2021/03/29 16:19:15 jsing Exp $ */
 /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
  * All rights reserved.
  *
@@ -157,17 +157,6 @@ tls1_cleanup_key_block(SSL *s)
 	S3I(s)->hs.tls12.key_block_len = 0;
 }
 
-void
-tls1_record_sequence_increment(unsigned char *seq)
-{
-	int i;
-
-	for (i = SSL3_SEQUENCE_SIZE - 1; i >= 0; i--) {
-		if (++seq[i] != 0)
-			break;
-	}
-}
-
 /*
  * TLS P_hash() data expansion function - see RFC 5246, section 5.
  */
diff --git a/lib/libssl/tls12_record_layer.c b/lib/libssl/tls12_record_layer.c
index ba3c3dfb2bd..6cf8b31c63e 100644
--- a/lib/libssl/tls12_record_layer.c
+++ b/lib/libssl/tls12_record_layer.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: tls12_record_layer.c,v 1.24 2021/03/21 19:08:22 tb Exp $ */
+/* $OpenBSD: tls12_record_layer.c,v 1.25 2021/03/29 16:19:15 jsing Exp $ */
 /*
  * Copyright (c) 2020 Joel Sing <jsing@openbsd.org>
  *
@@ -22,9 +22,11 @@
 
 #include "ssl_locl.h"
 
+#define TLS12_RECORD_SEQ_NUM_LEN 8
+
 struct tls12_record_protection {
 	uint16_t epoch;
-	uint8_t seq_num[SSL3_SEQUENCE_SIZE];
+	uint8_t seq_num[TLS12_RECORD_SEQ_NUM_LEN];
 
 	SSL_AEAD_CTX *aead_ctx;
 
@@ -342,6 +344,38 @@ tls12_record_layer_reflect_seq_num(struct tls12_record_layer *rl)
 	    sizeof(rl->write->seq_num));
 }
 
+static const uint8_t tls12_max_seq_num[TLS12_RECORD_SEQ_NUM_LEN] = {
+	0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+};
+
+int
+tls12_record_layer_inc_seq_num(struct tls12_record_layer *rl, uint8_t *seq_num)
+{
+	CBS max_seq_num;
+	int i;
+
+	/*
+	 * RFC 5246 section 6.1 and RFC 6347 section 4.1 - both TLS and DTLS
+	 * sequence numbers must not wrap. Note that for DTLS the first two
+	 * bytes are used as an "epoch" and not part of the sequence number.
+	 */
+	CBS_init(&max_seq_num, seq_num, TLS12_RECORD_SEQ_NUM_LEN);
+	if (rl->dtls) {
+		if (!CBS_skip(&max_seq_num, 2))
+			return 0;
+	}
+	if (CBS_mem_equal(&max_seq_num, tls12_max_seq_num,
+	    CBS_len(&max_seq_num)))
+		return 0;
+
+	for (i = TLS12_RECORD_SEQ_NUM_LEN - 1; i >= 0; i--) {
+		if (++seq_num[i] != 0)
+			break;
+	}
+
+	return 1;
+}
+
 static int
 tls12_record_layer_set_mac_key(struct tls12_record_protection *rp,
     const uint8_t *mac_key, size_t mac_key_len)
@@ -1074,8 +1108,10 @@ tls12_record_layer_open_record(struct tls12_record_layer *rl, uint8_t *buf,
 			return 0;
 	}
 
-	if (!rl->dtls)
-		tls1_record_sequence_increment(rl->read->seq_num);
+	if (!rl->dtls) {
+		if (!tls12_record_layer_inc_seq_num(rl, rl->read->seq_num))
+			return 0;
+	}
 
 	return 1;
 }
@@ -1274,7 +1310,8 @@ tls12_record_layer_seal_record(struct tls12_record_layer *rl,
 	if (!CBB_flush(cbb))
 		goto err;
 
-	tls1_record_sequence_increment(rl->write->seq_num);
+	if (!tls12_record_layer_inc_seq_num(rl, rl->write->seq_num))
+		goto err;
 
 	ret = 1;