It is possible (although unlikely in practice) for peer_finish_md_len to

end up with a value of zero, primarily since ssl3_take_mac() fails to check
the return value from the final_finish_mac() call. This would then mean that
an SSL finished message with a zero-byte payload would successfully match
against the calculated finish MAC.

Avoid this by checking the length of peer_finish_md_len and the SSL
finished message payload, against the known length already stored in
the SSL3_ENC_METHOD finish_mac_length field (making use of a previously
unused field).

ok miod@ (a little while back)

2 files changed, 22 insertions, 26 deletions

diff --git a/src/lib/libssl/s3_both.c b/src/lib/libssl/s3_both.c
index 6ba3d4bfce..17368f1107 100644
--- a/src/lib/libssl/s3_both.c
+++ b/src/lib/libssl/s3_both.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: s3_both.c,v 1.28 2014/08/07 20:02:23 miod Exp $ */
+/* $OpenBSD: s3_both.c,v 1.29 2014/09/22 12:36:06 jsing Exp $ */
 /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
  * All rights reserved.
  *
@@ -223,7 +223,7 @@ ssl3_take_mac(SSL *s)
 int
 ssl3_get_finished(SSL *s, int a, int b)
 {
-        int al, i, ok;
+        int al, ok, md_len;
         long n;
         unsigned char *p;
 
@@ -247,33 +247,31 @@ ssl3_get_finished(SSL *s, int a, int b)
         }
         s->s3->change_cipher_spec = 0;
 
+        md_len = s->method->ssl3_enc->finish_mac_length;
         p = (unsigned char *)s->init_msg;
-        i = s->s3->tmp.peer_finish_md_len;
 
-        if (i != n) {
+        if (s->s3->tmp.peer_finish_md_len != md_len || n != md_len) {
                 al = SSL_AD_DECODE_ERROR;
                 SSLerr(SSL_F_SSL3_GET_FINISHED, SSL_R_BAD_DIGEST_LENGTH);
                 goto f_err;
         }
 
-        if (timingsafe_memcmp(p, s->s3->tmp.peer_finish_md, i) != 0) {
+        if (timingsafe_memcmp(p, s->s3->tmp.peer_finish_md, md_len) != 0) {
                 al = SSL_AD_DECRYPT_ERROR;
                 SSLerr(SSL_F_SSL3_GET_FINISHED, SSL_R_DIGEST_CHECK_FAILED);
                 goto f_err;
         }
 
-        /* Copy the finished so we can use it for
-           renegotiation checks */
+        /* Copy finished so we can use it for renegotiation checks. */
+        OPENSSL_assert(md_len <= EVP_MAX_MD_SIZE);
         if (s->type == SSL_ST_ACCEPT) {
-                OPENSSL_assert(i <= EVP_MAX_MD_SIZE);
                 memcpy(s->s3->previous_client_finished,
-                s->s3->tmp.peer_finish_md, i);
-                s->s3->previous_client_finished_len = i;
+                    s->s3->tmp.peer_finish_md, md_len);
+                s->s3->previous_client_finished_len = md_len;
         } else {
-                OPENSSL_assert(i <= EVP_MAX_MD_SIZE);
                 memcpy(s->s3->previous_server_finished,
-                s->s3->tmp.peer_finish_md, i);
-                s->s3->previous_server_finished_len = i;
+                    s->s3->tmp.peer_finish_md, md_len);
+                s->s3->previous_server_finished_len = md_len;
         }
 
         return (1);
diff --git a/src/lib/libssl/src/ssl/s3_both.c b/src/lib/libssl/src/ssl/s3_both.c
index 6ba3d4bfce..17368f1107 100644
--- a/src/lib/libssl/src/ssl/s3_both.c
+++ b/src/lib/libssl/src/ssl/s3_both.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: s3_both.c,v 1.28 2014/08/07 20:02:23 miod Exp $ */
+/* $OpenBSD: s3_both.c,v 1.29 2014/09/22 12:36:06 jsing Exp $ */
 /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
  * All rights reserved.
  *
@@ -223,7 +223,7 @@ ssl3_take_mac(SSL *s)
 int
 ssl3_get_finished(SSL *s, int a, int b)
 {
-        int al, i, ok;
+        int al, ok, md_len;
         long n;
         unsigned char *p;
 
@@ -247,33 +247,31 @@ ssl3_get_finished(SSL *s, int a, int b)
         }
         s->s3->change_cipher_spec = 0;
 
+        md_len = s->method->ssl3_enc->finish_mac_length;
         p = (unsigned char *)s->init_msg;
-        i = s->s3->tmp.peer_finish_md_len;
 
-        if (i != n) {
+        if (s->s3->tmp.peer_finish_md_len != md_len || n != md_len) {
                 al = SSL_AD_DECODE_ERROR;
                 SSLerr(SSL_F_SSL3_GET_FINISHED, SSL_R_BAD_DIGEST_LENGTH);
                 goto f_err;
         }
 
-        if (timingsafe_memcmp(p, s->s3->tmp.peer_finish_md, i) != 0) {
+        if (timingsafe_memcmp(p, s->s3->tmp.peer_finish_md, md_len) != 0) {
                 al = SSL_AD_DECRYPT_ERROR;
                 SSLerr(SSL_F_SSL3_GET_FINISHED, SSL_R_DIGEST_CHECK_FAILED);
                 goto f_err;
         }
 
-        /* Copy the finished so we can use it for
-           renegotiation checks */
+        /* Copy finished so we can use it for renegotiation checks. */
+        OPENSSL_assert(md_len <= EVP_MAX_MD_SIZE);
         if (s->type == SSL_ST_ACCEPT) {
-                OPENSSL_assert(i <= EVP_MAX_MD_SIZE);
                 memcpy(s->s3->previous_client_finished,
-                s->s3->tmp.peer_finish_md, i);
-                s->s3->previous_client_finished_len = i;
+                    s->s3->tmp.peer_finish_md, md_len);
+                s->s3->previous_client_finished_len = md_len;
         } else {
-                OPENSSL_assert(i <= EVP_MAX_MD_SIZE);
                 memcpy(s->s3->previous_server_finished,
-                s->s3->tmp.peer_finish_md, i);
-                s->s3->previous_server_finished_len = i;
+                    s->s3->tmp.peer_finish_md, md_len);
+                s->s3->previous_server_finished_len = md_len;
         }
 
         return (1);