Reimplement the TLSv1.2 record handling for the read side.

This is the next step in replacing the TLSv1.2 record layer.

The existing record handling code does decryption and processing in
place, which is not ideal for various reasons, however it is retained
for now as other code depends on this behaviour. Additionally, CBC
requires special handling to avoid timing oracles - for now the
existing timing safe code is largely retained.

ok beck@ inoguchi@ tb@

1 files changed, 33 insertions, 133 deletions

diff --git a/lib/libssl/ssl_pkt.c b/lib/libssl/ssl_pkt.c
index c9c86471d3c..02a476ea82b 100644
--- a/lib/libssl/ssl_pkt.c
+++ b/lib/libssl/ssl_pkt.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: ssl_pkt.c,v 1.31 2020/08/30 15:40:20 jsing Exp $ */
+/* $OpenBSD: ssl_pkt.c,v 1.32 2020/10/03 17:35:16 jsing Exp $ */
 /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
  * All rights reserved.
  *
@@ -149,15 +149,14 @@ ssl_force_want_read(SSL *s)
 static int
 ssl3_read_n(SSL *s, int n, int max, int extend)
 {
+	SSL3_BUFFER_INTERNAL *rb = &(S3I(s)->rbuf);
 	int i, len, left;
 	size_t align;
 	unsigned char *pkt;
-	SSL3_BUFFER_INTERNAL *rb;
 
 	if (n <= 0)
 		return n;
 
-	rb = &(S3I(s)->rbuf);
 	if (rb->buf == NULL)
 		if (!ssl3_setup_read_buffer(s))
 			return -1;
@@ -327,15 +326,13 @@ ssl3_packet_extend(SSL *s, int plen)
 static int
 ssl3_get_record(SSL *s)
 {
-	int al;
-	int enc_err, n, i, ret = -1;
-	SSL3_RECORD_INTERNAL *rr;
-	SSL_SESSION *sess;
-	unsigned char md[EVP_MAX_MD_SIZE];
-	unsigned int mac_size, orig_len;
-
-	rr = &(S3I(s)->rrec);
-	sess = s->session;
+	SSL3_BUFFER_INTERNAL *rb = &(S3I(s)->rbuf);
+	SSL3_RECORD_INTERNAL *rr = &(S3I(s)->rrec);
+	uint8_t alert_desc;
+	uint8_t *out;
+	size_t out_len;
+	int al, n;
+	int ret = -1;
 
  again:
 	/* check if we have the header */
@@ -387,17 +384,13 @@ ssl3_get_record(SSL *s)
 			goto err;
 		}
 
-		if (rr->length > S3I(s)->rbuf.len - SSL3_RT_HEADER_LENGTH) {
+		if (rr->length > rb->len - SSL3_RT_HEADER_LENGTH) {
 			al = SSL_AD_RECORD_OVERFLOW;
 			SSLerror(s, SSL_R_PACKET_LENGTH_TOO_LONG);
 			goto f_err;
 		}
-
-		/* now s->internal->rstate == SSL_ST_READ_BODY */
 	}
 
-	/* s->internal->rstate == SSL_ST_READ_BODY, get and decode the data */
-
 	n = ssl3_packet_extend(s, SSL3_RT_HEADER_LENGTH + rr->length);
 	if (n <= 0)
 		return (n);
@@ -406,133 +399,40 @@ ssl3_get_record(SSL *s)
 
 	s->internal->rstate = SSL_ST_READ_HEADER; /* set state for later operations */
 
-	/* At this point, s->internal->packet_length == SSL3_RT_HEADER_LNGTH + rr->length,
-	 * and we have that many bytes in s->internal->packet
+	/*
+	 * A full record has now been read from the wire, which now needs
+	 * to be processed.
 	 */
-	rr->input = &(s->internal->packet[SSL3_RT_HEADER_LENGTH]);
-
-	/* ok, we can now read from 's->internal->packet' data into 'rr'
-	 * rr->input points at rr->length bytes, which
-	 * need to be copied into rr->data by either
-	 * the decryption or by the decompression
-	 * When the data is 'copied' into the rr->data buffer,
-	 * rr->input will be pointed at the new buffer */
-
-	/* We now have - encrypted [ MAC [ compressed [ plain ] ] ]
-	 * rr->length bytes of encrypted compressed stuff. */
-
-	/* check is not needed I believe */
-	if (rr->length > SSL3_RT_MAX_ENCRYPTED_LENGTH) {
-		al = SSL_AD_RECORD_OVERFLOW;
-		SSLerror(s, SSL_R_ENCRYPTED_LENGTH_TOO_LONG);
-		goto f_err;
-	}
-
-	/* decrypt in place in 'rr->input' */
-	rr->data = rr->input;
+	tls12_record_layer_set_version(s->internal->rl, s->version);
 
-	/* enc_err is:
-	 *    0: (in non-constant time) if the record is publically invalid.
-	 *    1: if the padding is valid
-	 *    -1: if the padding is invalid */
-	if ((enc_err = tls1_enc(s, 0)) == 0) {
-		al = SSL_AD_BAD_RECORD_MAC;
-		SSLerror(s, SSL_R_BLOCK_CIPHER_PAD_IS_WRONG);
-		goto f_err;
-	}
-
-	/* r->length is now the compressed data plus mac */
-	if ((sess != NULL) && (s->enc_read_ctx != NULL) &&
-	    (EVP_MD_CTX_md(s->read_hash) != NULL)) {
-		/* s->read_hash != NULL => mac_size != -1 */
-		unsigned char *mac = NULL;
-		unsigned char mac_tmp[EVP_MAX_MD_SIZE];
-
-		mac_size = EVP_MD_CTX_size(s->read_hash);
-		OPENSSL_assert(mac_size <= EVP_MAX_MD_SIZE);
-
-		orig_len = rr->length + rr->padding_length;
+	if (!tls12_record_layer_open_record(s->internal->rl, s->internal->packet,
+	    s->internal->packet_length, &out, &out_len)) {
+		tls12_record_layer_alert(s->internal->rl, &alert_desc);
 
-		/* orig_len is the length of the record before any padding was
-		 * removed. This is public information, as is the MAC in use,
-		 * therefore we can safely process the record in a different
-		 * amount of time if it's too short to possibly contain a MAC.
-		 */
-		if (orig_len < mac_size ||
-			/* CBC records must have a padding length byte too. */
-		    (EVP_CIPHER_CTX_mode(s->enc_read_ctx) == EVP_CIPH_CBC_MODE &&
-		    orig_len < mac_size + 1)) {
-			al = SSL_AD_DECODE_ERROR;
-			SSLerror(s, SSL_R_LENGTH_TOO_SHORT);
-			goto f_err;
-		}
-
-		if (EVP_CIPHER_CTX_mode(s->enc_read_ctx) == EVP_CIPH_CBC_MODE) {
-			/* We update the length so that the TLS header bytes
-			 * can be constructed correctly but we need to extract
-			 * the MAC in constant time from within the record,
-			 * without leaking the contents of the padding bytes.
-			 * */
-			mac = mac_tmp;
-			ssl3_cbc_copy_mac(mac_tmp, rr, mac_size, orig_len);
-			rr->length -= mac_size;
-		} else {
-			/* In this case there's no padding, so |orig_len|
-			 * equals |rec->length| and we checked that there's
-			 * enough bytes for |mac_size| above. */
-			rr->length -= mac_size;
-			mac = &rr->data[rr->length];
-		}
+		if (alert_desc == 0)
+			goto err;
 
-		i = tls1_mac(s,md,0 /* not send */);
-		if (i < 0 || mac == NULL ||
-		    timingsafe_memcmp(md, mac, (size_t)mac_size) != 0)
-			enc_err = -1;
-		if (rr->length >
-		    SSL3_RT_MAX_COMPRESSED_LENGTH + mac_size)
-			enc_err = -1;
-	}
+		if (alert_desc == SSL_AD_RECORD_OVERFLOW)
+			SSLerror(s, SSL_R_ENCRYPTED_LENGTH_TOO_LONG);
+		else if (alert_desc == SSL_AD_BAD_RECORD_MAC)
+			SSLerror(s, SSL_R_DECRYPTION_FAILED_OR_BAD_RECORD_MAC);
 
-	if (enc_err < 0) {
-		/*
-		 * A separate 'decryption_failed' alert was introduced with
-		 * TLS 1.0, SSL 3.0 only has 'bad_record_mac'. But unless a
-		 * decryption failure is directly visible from the ciphertext
-		 * anyway, we should not reveal which kind of error
-		 * occurred -- this might become visible to an attacker
-		 * (e.g. via a logfile)
-		 */
-		al = SSL_AD_BAD_RECORD_MAC;
-		SSLerror(s, SSL_R_DECRYPTION_FAILED_OR_BAD_RECORD_MAC);
-		goto f_err;
-	}
-
-	if (rr->length > SSL3_RT_MAX_PLAIN_LENGTH) {
-		al = SSL_AD_RECORD_OVERFLOW;
-		SSLerror(s, SSL_R_DATA_LENGTH_TOO_LONG);
+		al = alert_desc;
 		goto f_err;
 	}
 
+	rr->data = out;
+	rr->length = out_len;
 	rr->off = 0;
-	/*
-	 * So at this point the following is true
-	 *
-	 * ssl->s3->internal->rrec.type 	is the type of record
-	 * ssl->s3->internal->rrec.length	== number of bytes in record
-	 * ssl->s3->internal->rrec.off	== offset to first valid byte
-	 * ssl->s3->internal->rrec.data	== where to take bytes from, increment
-	 *			   after use :-).
-	 */
 
 	/* we have pulled in a full packet so zero things */
 	s->internal->packet_length = 0;
 
 	if (rr->length == 0) {
 		/*
-		 * CBC countermeasures for known IV weaknesses
-		 * can legitimately insert a single empty record,
-		 * so we allow ourselves to read once past a single
-		 * empty record without forcing want_read.
+		 * CBC countermeasures for known IV weaknesses can legitimately
+		 * insert a single empty record, so we allow ourselves to read
+		 * once past a single empty record without forcing want_read.
 		 */
 		if (s->internal->empty_record_count++ > SSL_MAX_EMPTY_RECORDS) {
 			SSLerror(s, SSL_R_PEER_BEHAVING_BADLY);
@@ -543,15 +443,15 @@ ssl3_get_record(SSL *s)
 			return -1;
 		}
 		goto again;
-	} else {
-		s->internal->empty_record_count = 0;
 	}
 
+	s->internal->empty_record_count = 0;
+
 	return (1);
 
-f_err:
+ f_err:
 	ssl3_send_alert(s, SSL3_AL_FATAL, al);
-err:
+ err:
 	return (ret);
 }