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@

1 files changed, 41 insertions, 28 deletions

diff --git a/lib/libssl/t1_enc.c b/lib/libssl/t1_enc.c
index 53570b2d4f7..6305a6ffb9c 100644
--- a/lib/libssl/t1_enc.c
+++ b/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