From 463a204c858ff0b4b4b13aed4ed5f6d5670a5c8b Mon Sep 17 00:00:00 2001
From: jsing <>
Date: Thu, 3 Nov 2016 08:15:22 +0000
Subject: Clean up the TLS handshake digest handling - this refactors some of
 the code for improved readability, however it also address two issues.

The first of these is a hard-to-hit double free that will occur if
EVP_DigestInit_ex() fails. To avoid this and to be more robust, ensure
that tls1_digest_cached_records() either completes successfully and sets
up all of the necessary digests, or it cleans up and frees everything
that was allocated.

The second issue is that EVP_DigestUpdate() can fail - detect and handle
this in tls1_finish_mac() and change the return type to an int so that a
failure can be propagated to the caller (the callers still need to be
fixed to handle this, in a later diff).

The double-free was reported by Matthew Dillon.

ok beck@ doug@ miod@
---
 src/lib/libssl/ssl_locl.h |  4 +--
 src/lib/libssl/t1_enc.c   | 69 ++++++++++++++++++++++++++++-------------------
 2 files changed, 43 insertions(+), 30 deletions(-)

(limited to 'src')

diff --git a/src/lib/libssl/ssl_locl.h b/src/lib/libssl/ssl_locl.h
index 1b768e3939..b79e9269ce 100644
--- a/src/lib/libssl/ssl_locl.h
+++ b/src/lib/libssl/ssl_locl.h
@@ -1,4 +1,4 @@
-/* $OpenBSD: ssl_locl.h,v 1.130 2016/10/19 16:38:40 jsing Exp $ */
+/* $OpenBSD: ssl_locl.h,v 1.131 2016/11/03 08:15:22 jsing Exp $ */
 /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
  * All rights reserved.
  *
@@ -754,7 +754,7 @@ int ssl_init_wbio_buffer(SSL *s, int push);
 void ssl_free_wbio_buffer(SSL *s);
 
 int tls1_init_finished_mac(SSL *s);
-void tls1_finish_mac(SSL *s, const unsigned char *buf, int len);
+int tls1_finish_mac(SSL *s, const unsigned char *buf, int len);
 void tls1_free_digest_list(SSL *s);
 void tls1_cleanup_key_block(SSL *s);
 int tls1_digest_cached_records(SSL *s);
diff --git a/src/lib/libssl/t1_enc.c b/src/lib/libssl/t1_enc.c
index 53570b2d4f..6305a6ffb9 100644
--- a/src/lib/libssl/t1_enc.c
+++ b/src/lib/libssl/t1_enc.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: t1_enc.c,v 1.85 2016/04/28 16:39:45 jsing Exp $ */
+/* $OpenBSD: t1_enc.c,v 1.86 2016/11/03 08:15:22 jsing Exp $ */
 /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
  * All rights reserved.
  *
@@ -177,9 +177,9 @@ tls1_free_digest_list(SSL *s)
 
 	if (s == NULL)
 		return;
-
 	if (s->s3->handshake_dgst == NULL)
 		return;
+
 	for (i = 0; i < SSL_MAX_DIGEST; i++) {
 		if (s->s3->handshake_dgst[i])
 			EVP_MD_CTX_destroy(s->s3->handshake_dgst[i]);
@@ -188,61 +188,70 @@ tls1_free_digest_list(SSL *s)
 	s->s3->handshake_dgst = NULL;
 }
 
-void
+int
 tls1_finish_mac(SSL *s, const unsigned char *buf, int len)
 {
+	int i;
+
 	if (s->s3->handshake_buffer &&
 	    !(s->s3->flags & TLS1_FLAGS_KEEP_HANDSHAKE)) {
 		BIO_write(s->s3->handshake_buffer, (void *)buf, len);
-	} else {
-		int i;
-		for (i = 0; i < SSL_MAX_DIGEST; i++) {
-			if (s->s3->handshake_dgst[i]!= NULL)
-				EVP_DigestUpdate(s->s3->handshake_dgst[i], buf, len);
+		return 1;
+	}
+
+	for (i = 0; i < SSL_MAX_DIGEST; i++) {
+		if (s->s3->handshake_dgst[i] == NULL)
+			continue;
+		if (!EVP_DigestUpdate(s->s3->handshake_dgst[i], buf, len)) {
+			SSLerr(SSL_F_SSL3_DIGEST_CACHED_RECORDS, ERR_R_EVP_LIB);
+			return 0;
 		}
 	}
+
+	return 1;
 }
 
 int
 tls1_digest_cached_records(SSL *s)
 {
-	int i;
-	long mask;
 	const EVP_MD *md;
-	long hdatalen;
+	long hdatalen, mask;
 	void *hdata;
+	int i;
 
 	tls1_free_digest_list(s);
 
 	s->s3->handshake_dgst = calloc(SSL_MAX_DIGEST, sizeof(EVP_MD_CTX *));
 	if (s->s3->handshake_dgst == NULL) {
 		SSLerr(SSL_F_SSL3_DIGEST_CACHED_RECORDS, ERR_R_MALLOC_FAILURE);
-		return 0;
+		goto err;
 	}
 	hdatalen = BIO_get_mem_data(s->s3->handshake_buffer, &hdata);
 	if (hdatalen <= 0) {
 		SSLerr(SSL_F_SSL3_DIGEST_CACHED_RECORDS,
 		    SSL_R_BAD_HANDSHAKE_LENGTH);
-		return 0;
+		goto err;
 	}
 
 	/* Loop through bits of the algorithm2 field and create MD contexts. */
 	for (i = 0; ssl_get_handshake_digest(i, &mask, &md); i++) {
-		if ((mask & ssl_get_algorithm2(s)) && md) {
-			s->s3->handshake_dgst[i] = EVP_MD_CTX_create();
-			if (s->s3->handshake_dgst[i] == NULL) {
-				SSLerr(SSL_F_SSL3_DIGEST_CACHED_RECORDS,
-				    ERR_R_MALLOC_FAILURE);
-				return 0;
-			}
-			if (!EVP_DigestInit_ex(s->s3->handshake_dgst[i],
-			    md, NULL)) {
-				EVP_MD_CTX_destroy(s->s3->handshake_dgst[i]);
-				return 0;
-			}
-			if (!EVP_DigestUpdate(s->s3->handshake_dgst[i], hdata,
-			    hdatalen))
-				return 0;
+		if ((mask & ssl_get_algorithm2(s)) == 0 || md == NULL)
+			continue;
+
+		s->s3->handshake_dgst[i] = EVP_MD_CTX_create();
+		if (s->s3->handshake_dgst[i] == NULL) {
+			SSLerr(SSL_F_SSL3_DIGEST_CACHED_RECORDS,
+			    ERR_R_MALLOC_FAILURE);
+			goto err;
+		}
+		if (!EVP_DigestInit_ex(s->s3->handshake_dgst[i], md, NULL)) {
+			SSLerr(SSL_F_SSL3_DIGEST_CACHED_RECORDS, ERR_R_EVP_LIB);
+			goto err;
+		}
+		if (!EVP_DigestUpdate(s->s3->handshake_dgst[i], hdata,
+		    hdatalen)) {
+			SSLerr(SSL_F_SSL3_DIGEST_CACHED_RECORDS, ERR_R_EVP_LIB);
+			goto err;
 		}
 	}
 
@@ -252,6 +261,10 @@ tls1_digest_cached_records(SSL *s)
 	}
 
 	return 1;
+
+ err:
+	tls1_free_digest_list(s);
+	return 0;
 }
 
 void
-- 
cgit v1.2.3-55-g6feb