From 5b8f104ee8304f535f1ae9ebe4c749143eb4e8c5 Mon Sep 17 00:00:00 2001
From: David Benjamin <davidben@chromium.org>
Date: Mon, 25 Aug 2014 23:28:44 -0400
Subject: [PATCH] Revise hash management for reading the Finished message.

Upstream originally sampled the Finished message's hash at ChangeCipherSpec,
but our patches to add messages between the two complicated this. Move DTLS to
this path, but use the new SSL_GET_MESSAGE_DONT_HASH_MESSAGE flag to avoid
special-casing message types in ssl3_get_message.

Change-Id: I9c8ddd9cc500c94dff2ec2f696f89d50ab01b3ad
Reviewed-on: https://boringssl-review.googlesource.com/1632
Reviewed-by: Adam Langley <agl@google.com>
---
 ssl/s3_both.c | 21 +++++----------------
 ssl/s3_pkt.c  | 25 -------------------------
 2 files changed, 5 insertions(+), 41 deletions(-)

diff --git a/ssl/s3_both.c b/ssl/s3_both.c
index 2b4152b27..46b09601a 100644
--- a/ssl/s3_both.c
+++ b/ssl/s3_both.c
@@ -191,7 +191,6 @@ int ssl3_send_finished(SSL *s, int a, int b, const char *sender, int slen)
 	return ssl_do_write(s);
 	}
 
-#ifndef OPENSSL_NO_NEXTPROTONEG
 /* ssl3_take_mac calculates the Finished MAC for the handshakes messages seen to far. */
 static void ssl3_take_mac(SSL *s)
 	{
@@ -216,7 +215,6 @@ static void ssl3_take_mac(SSL *s)
 	s->s3->tmp.peer_finish_md_len = s->method->ssl3_enc->final_finish_mac(s,
 		sender,slen,s->s3->tmp.peer_finish_md);
 	}
-#endif
 
 int ssl3_get_finished(SSL *s, int a, int b)
 	{
@@ -224,22 +222,20 @@ int ssl3_get_finished(SSL *s, int a, int b)
 	long n;
 	unsigned char *p;
 
-#ifdef OPENSSL_NO_NEXTPROTONEG
-	/* the mac has already been generated when we received the
-	 * change cipher spec message and is in s->s3->tmp.peer_finish_md
-	 */ 
-#endif
-
 	n=s->method->ssl_get_message(s,
 		a,
 		b,
 		SSL3_MT_FINISHED,
 		64, /* should actually be 36+4 :-) */
-		SSL_GET_MESSAGE_HASH_MESSAGE,
+		SSL_GET_MESSAGE_DONT_HASH_MESSAGE,
 		&ok);
 
 	if (!ok) return((int)n);
 
+	/* Snapshot the finished hash before incorporating the new message. */
+	ssl3_take_mac(s);
+	ssl3_hash_current_message(s);
+
 	/* If this occurs, we have missed a message.
 	 * TODO(davidben): Is this check now redundant with
 	 * SSL3_FLAGS_EXPECT_CCS? */
@@ -466,13 +462,6 @@ long ssl3_get_message(SSL *s, int st1, int stn, int mt, long max, int hash_messa
 		n -= i;
 		}
 
-#ifndef OPENSSL_NO_NEXTPROTONEG
-	/* If receiving Finished, record MAC of prior handshake messages for
-	 * Finished verification. */
-	if (*s->init_buf->data == SSL3_MT_FINISHED)
-		ssl3_take_mac(s);
-#endif
-
 	/* Feed this message into MAC computation. */
 	if (hash_message != SSL_GET_MESSAGE_DONT_HASH_MESSAGE)
 		ssl3_hash_current_message(s);
diff --git a/ssl/s3_pkt.c b/ssl/s3_pkt.c
index 393d88318..12c546735 100644
--- a/ssl/s3_pkt.c
+++ b/ssl/s3_pkt.c
@@ -1426,8 +1426,6 @@ err:
 int ssl3_do_change_cipher_spec(SSL *s)
 	{
 	int i;
-	const char *sender;
-	int slen;
 
 	if (s->state & SSL_ST_ACCEPT)
 		i=SSL3_CHANGE_CIPHER_SERVER_READ;
@@ -1450,29 +1448,6 @@ int ssl3_do_change_cipher_spec(SSL *s)
 	if (!s->method->ssl3_enc->change_cipher_state(s,i))
 		return(0);
 
-	/* we have to record the message digest at
-	 * this point so we can get it before we read
-	 * the finished message */
-	if (s->state & SSL_ST_CONNECT)
-		{
-		sender=s->method->ssl3_enc->server_finished_label;
-		slen=s->method->ssl3_enc->server_finished_label_len;
-		}
-	else
-		{
-		sender=s->method->ssl3_enc->client_finished_label;
-		slen=s->method->ssl3_enc->client_finished_label_len;
-		}
-
-	i = s->method->ssl3_enc->final_finish_mac(s,
-		sender,slen,s->s3->tmp.peer_finish_md);
-	if (i == 0)
-		{
-		OPENSSL_PUT_ERROR(SSL, ssl3_do_change_cipher_spec, ERR_R_INTERNAL_ERROR);
-		return 0;
-		}
-	s->s3->tmp.peer_finish_md_len = i;
-
 	return(1);
 	}