Use CBB more correctly when writing SSL3/DTLS records.

Previously we used CBB to build the record headers, but not the entire
record. Use CBB_init_fixed() upfront, then build the record header and
add space for the record content. However, in order to do this we need
to determine the length of the record upfront.

This simplifies the code, removes a number of manual bounds checks and
makes way for further improvements.

ok inoguchi@ tb@

2 files changed, 92 insertions, 66 deletions

diff --git a/lib/libssl/d1_pkt.c b/lib/libssl/d1_pkt.c
index 5bbdf5f7384..dd7d9aa76c6 100644
--- a/lib/libssl/d1_pkt.c
+++ b/lib/libssl/d1_pkt.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: d1_pkt.c,v 1.77 2020/08/09 15:46:28 jsing Exp $ */
+/* $OpenBSD: d1_pkt.c,v 1.78 2020/08/09 16:02:58 jsing Exp $ */
 /*
  * DTLS implementation written by Nagendra Modadugu
  * (nagendra@cs.stanford.edu) for the OpenSSL project 2005.
@@ -1177,10 +1177,12 @@ do_dtls1_write(SSL *s, int type, const unsigned char *buf, unsigned int len)
 	SSL3_RECORD_INTERNAL *wr = &(S3I(s)->wrec);
 	SSL3_BUFFER_INTERNAL *wb = &(S3I(s)->wbuf);
 	SSL_SESSION *sess = s->session;
-	int eivlen = 0, mac_size = 0;
-	unsigned char *p;
+	int block_size = 0, eivlen = 0, mac_size = 0;
+	size_t pad_len, record_len;
+	CBB cbb, fragment;
+	size_t out_len;
+	uint8_t *p;
 	int ret;
-	CBB cbb;
 
 	memset(&cbb, 0, sizeof(cbb));
 
@@ -1209,12 +1211,38 @@ do_dtls1_write(SSL *s, int type, const unsigned char *buf, unsigned int len)
 			goto err;
 	}
 
+	/* Explicit IV length. */
+	if (s->internal->enc_write_ctx && SSL_USE_EXPLICIT_IV(s)) {
+		int mode = EVP_CIPHER_CTX_mode(s->internal->enc_write_ctx);
+		if (mode == EVP_CIPH_CBC_MODE) {
+			eivlen = EVP_CIPHER_CTX_iv_length(s->internal->enc_write_ctx);
+			if (eivlen <= 1)
+				eivlen = 0;
+		}
+	} else if (s->internal->aead_write_ctx != NULL &&
+	    s->internal->aead_write_ctx->variable_nonce_in_record) {
+		eivlen = s->internal->aead_write_ctx->variable_nonce_len;
+	}
+
+	/* Determine length of record fragment. */
+	record_len = eivlen + len + mac_size;
+	if (s->internal->enc_write_ctx != NULL) {
+		block_size = EVP_CIPHER_CTX_block_size(s->internal->enc_write_ctx);
+		if (block_size <= 0 || block_size > EVP_MAX_BLOCK_LENGTH)
+			goto err;
+		if (block_size > 1) {
+			pad_len = block_size - (record_len % block_size);
+			record_len += pad_len;
+		}
+	} else if (s->internal->aead_write_ctx != NULL) {
+		record_len += s->internal->aead_write_ctx->tag_len;
+	}
+
 	/* DTLS implements explicit IV, so no need for empty fragments. */
 
-	p = wb->buf;
 	wb->offset = 0;
 
-	if (!CBB_init_fixed(&cbb, p, DTLS1_RT_HEADER_LENGTH))
+	if (!CBB_init_fixed(&cbb, wb->buf, wb->len))
 		goto err;
 
 	/* Write the header. */
@@ -1226,21 +1254,10 @@ do_dtls1_write(SSL *s, int type, const unsigned char *buf, unsigned int len)
 		goto err;
 	if (!CBB_add_bytes(&cbb, &(S3I(s)->write_sequence[2]), 6))
 		goto err;
-
-	p += DTLS1_RT_HEADER_LENGTH;
-
-	/* Explicit IV length. */
-	if (s->internal->enc_write_ctx && SSL_USE_EXPLICIT_IV(s)) {
-		int mode = EVP_CIPHER_CTX_mode(s->internal->enc_write_ctx);
-		if (mode == EVP_CIPH_CBC_MODE) {
-			eivlen = EVP_CIPHER_CTX_iv_length(s->internal->enc_write_ctx);
-			if (eivlen <= 1)
-				eivlen = 0;
-		}
-	} else if (s->internal->aead_write_ctx != NULL &&
-	    s->internal->aead_write_ctx->variable_nonce_in_record) {
-		eivlen = s->internal->aead_write_ctx->variable_nonce_len;
-	}
+	if (!CBB_add_u16_length_prefixed(&cbb, &fragment))
+		goto err;
+	if (!CBB_add_space(&fragment, &p, record_len))
+		goto err;
 
 	wr->type = type;
 	wr->data = p + eivlen;
@@ -1262,11 +1279,14 @@ do_dtls1_write(SSL *s, int type, const unsigned char *buf, unsigned int len)
 	if (tls1_enc(s, 1) != 1)
 		goto err;
 
-	if (!CBB_add_u16(&cbb, wr->length))
+	if (wr->length != record_len)
 		goto err;
-	if (!CBB_finish(&cbb, NULL, NULL))
+
+	if (!CBB_finish(&cbb, NULL, &out_len))
 		goto err;
 
+	wb->left = out_len;
+
 	/*
 	 * We should now have wr->data pointing to the encrypted data,
 	 * which is wr->length long.
@@ -1276,8 +1296,6 @@ do_dtls1_write(SSL *s, int type, const unsigned char *buf, unsigned int len)
 
 	tls1_record_sequence_increment(S3I(s)->write_sequence);
 
-	wb->left = wr->length;
-
 	/*
 	 * Memorize arguments so that ssl3_write_pending can detect
 	 * bad write retries later.
diff --git a/lib/libssl/ssl_pkt.c b/lib/libssl/ssl_pkt.c
index 39ce46381df..da059915f20 100644
--- a/lib/libssl/ssl_pkt.c
+++ b/lib/libssl/ssl_pkt.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: ssl_pkt.c,v 1.28 2020/08/02 07:33:15 jsing Exp $ */
+/* $OpenBSD: ssl_pkt.c,v 1.29 2020/08/09 16:02:58 jsing Exp $ */
 /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
  * All rights reserved.
  *
@@ -617,15 +617,15 @@ ssl3_write_bytes(SSL *s, int type, const void *buf_, int len)
 }
 
 static int
-ssl3_create_record(SSL *s, unsigned char *p, uint16_t version, uint8_t type,
+ssl3_create_record(SSL *s, CBB *cbb, uint16_t version, uint8_t type,
     const unsigned char *buf, unsigned int len)
 {
 	SSL3_RECORD_INTERNAL *wr = &(S3I(s)->wrec);
 	SSL_SESSION *sess = s->session;
-	int eivlen = 0, mac_size = 0;
-	CBB cbb;
-
-	memset(&cbb, 0, sizeof(cbb));
+	int block_size = 0, eivlen = 0, mac_size = 0;
+	size_t pad_len, record_len;
+	CBB fragment;
+	uint8_t *p;
 
 	if (sess != NULL && s->internal->enc_write_ctx != NULL &&
 	    EVP_MD_CTX_md(s->internal->write_hash) != NULL) {
@@ -633,17 +633,6 @@ ssl3_create_record(SSL *s, unsigned char *p, uint16_t version, uint8_t type,
 			goto err;
 	}
 
-	if (!CBB_init_fixed(&cbb, p, SSL3_RT_HEADER_LENGTH))
-		goto err;
-
-	/* Write the header. */
-	if (!CBB_add_u8(&cbb, type))
-		goto err;
-	if (!CBB_add_u16(&cbb, version))
-		goto err;
-
-	p += SSL3_RT_HEADER_LENGTH;
-
 	/* Explicit IV length. */
 	if (s->internal->enc_write_ctx && SSL_USE_EXPLICIT_IV(s)) {
 		int mode = EVP_CIPHER_CTX_mode(s->internal->enc_write_ctx);
@@ -657,6 +646,31 @@ ssl3_create_record(SSL *s, unsigned char *p, uint16_t version, uint8_t type,
 		eivlen = s->internal->aead_write_ctx->variable_nonce_len;
 	}
 
+	/* Determine length of record fragment. */
+	record_len = eivlen + len + mac_size;
+	if (s->internal->enc_write_ctx != NULL) {
+		block_size = EVP_CIPHER_CTX_block_size(s->internal->enc_write_ctx);
+		if (block_size <= 0 || block_size > EVP_MAX_BLOCK_LENGTH)
+			goto err;
+		if (block_size > 1) {
+			pad_len = block_size - (record_len % block_size);
+			record_len += pad_len;
+		}
+	} else if (s->internal->aead_write_ctx != NULL) {
+		record_len += s->internal->aead_write_ctx->tag_len;
+	}
+
+	/* Write the header. */
+	if (!CBB_add_u8(cbb, type))
+		goto err;
+	if (!CBB_add_u16(cbb, version))
+		goto err;
+	if (!CBB_add_u16_length_prefixed(cbb, &fragment))
+		goto err;
+	if (!CBB_add_space(&fragment, &p, record_len))
+		goto err;
+
+	/* Set up the record. */
 	wr->type = type;
 	wr->data = p + eivlen;
 	wr->length = (int)len;
@@ -677,10 +691,10 @@ ssl3_create_record(SSL *s, unsigned char *p, uint16_t version, uint8_t type,
 	if (tls1_enc(s, 1) != 1)
 		goto err;
 
-	/* record length after mac and block padding */
-	if (!CBB_add_u16(&cbb, wr->length))
+	if (wr->length != record_len)
 		goto err;
-	if (!CBB_finish(&cbb, NULL, NULL))
+
+	if (!CBB_flush(cbb))
 		goto err;
 
 	/*
@@ -693,24 +707,22 @@ ssl3_create_record(SSL *s, unsigned char *p, uint16_t version, uint8_t type,
 	return 1;
 
  err:
-	CBB_cleanup(&cbb);
-
 	return 0;
 }
 
 static int
 do_ssl3_write(SSL *s, int type, const unsigned char *buf, unsigned int len)
 {
-	SSL3_RECORD_INTERNAL *wr = &(S3I(s)->wrec);
 	SSL3_BUFFER_INTERNAL *wb = &(S3I(s)->wbuf);
 	SSL_SESSION *sess = s->session;
-	unsigned char *p;
 	int need_empty_fragment = 0;
-	int prefix_len = 0;
+	size_t align, out_len;
 	uint16_t version;
-	size_t align;
+	CBB cbb;
 	int ret;
 
+	memset(&cbb, 0, sizeof(cbb));
+
 	if (wb->buf == NULL)
 		if (!ssl3_setup_write_buffer(s))
 			return -1;
@@ -768,30 +780,24 @@ do_ssl3_write(SSL *s, int type, const unsigned char *buf, unsigned int len)
 	if (need_empty_fragment)
 		align += SSL3_RT_HEADER_LENGTH;
 	align = (-align) & (SSL3_ALIGN_PAYLOAD - 1);
-
-	p = wb->buf + align;
 	wb->offset = align;
 
-	if (need_empty_fragment) {
-		if (!ssl3_create_record(s, p, version, type, buf, 0))
-			goto err;
+	if (!CBB_init_fixed(&cbb, wb->buf + align, wb->len - align))
+		goto err;
 
-		prefix_len = wr->length;
-		if (prefix_len > (SSL3_RT_HEADER_LENGTH +
-		    SSL3_RT_SEND_MAX_ENCRYPTED_OVERHEAD)) {
-			/* insufficient space */
-			SSLerror(s, ERR_R_INTERNAL_ERROR);
+	if (need_empty_fragment) {
+		if (!ssl3_create_record(s, &cbb, version, type, buf, 0))
 			goto err;
-		}
-		p = wb->buf + wb->offset + prefix_len;
-
 		S3I(s)->empty_fragment_done = 1;
 	}
 
-	if (!ssl3_create_record(s, p, version, type, buf, len))
+	if (!ssl3_create_record(s, &cbb, version, type, buf, len))
 		goto err;
 
-	wb->left = prefix_len + wr->length;
+	if (!CBB_finish(&cbb, NULL, &out_len))
+		goto err;
+
+	wb->left = out_len;
 
 	/*
 	 * Memorize arguments so that ssl3_write_pending can detect
@@ -806,6 +812,8 @@ do_ssl3_write(SSL *s, int type, const unsigned char *buf, unsigned int len)
 	return ssl3_write_pending(s, type, buf, len);
 
  err:
+	CBB_cleanup(&cbb);
+
 	return -1;
 }