Don't calculate the Finished MAC twice

In <= TLSv1.2 a Finished message always comes immediately after a CCS
except in the case of NPN where there is an additional message between
the CCS and Finished. Historically we always calculated the Finished MAC
when we processed the CCS. However to deal with NPN we also calculated it
when we receive the Finished message. Really this should only have been
done if we hand negotiated NPN.

This simplifies the code to only calculate the MAC when we receive the
Finished. In 1.1.1 we need to do it this way anyway because there is no
CCS (except in middlebox compat mode) in TLSv1.3.

Reviewed-by: Andy Polyakov <appro@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/5286)

diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h
index 9ff9b600aee84f8805eb91fab79710ad4b087cf9..abe4406126463bdc874abf8ee5fa719b06cb6748 100644 (file)
--- a/include/openssl/ssl.h
+++ b/include/openssl/ssl.h
@@ -2131,6 +2131,7 @@ int ERR_load_SSL_strings(void);
 # define SSL_F_SSL3_SETUP_KEY_BLOCK                       157
 # define SSL_F_SSL3_SETUP_READ_BUFFER                     156
 # define SSL_F_SSL3_SETUP_WRITE_BUFFER                    291
+# define SSL_F_SSL3_TAKE_MAC                              425
 # define SSL_F_SSL3_WRITE_BYTES                           158
 # define SSL_F_SSL3_WRITE_PENDING                         159
 # define SSL_F_SSL_ADD_CERT_CHAIN                         316

diff --git a/ssl/s3_msg.c b/ssl/s3_msg.c
index 82513d259086de5315cac974058b363fcef81319..8b08ba0bd11013f0bd84ea7e778a5e2fea21d084 100644 (file)
--- a/ssl/s3_msg.c
+++ b/ssl/s3_msg.c
@@ -13,8 +13,6 @@
 int ssl3_do_change_cipher_spec(SSL *s)
 {
     int i;
-    const char *sender;
-    int slen;
 
     if (s->server)
         i = SSL3_CHANGE_CIPHER_SERVER_READ;
@@ -36,28 +34,7 @@ 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->server) {
-        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) {
-        SSLerr(SSL_F_SSL3_DO_CHANGE_CIPHER_SPEC, ERR_R_INTERNAL_ERROR);
-        return 0;
-    }
-    s->s3->tmp.peer_finish_md_len = i;
-
-    return (1);
+    return 1;
 }
 
 int ssl3_send_alert(SSL *s, int level, int desc)

diff --git a/ssl/ssl_err.c b/ssl/ssl_err.c
index 96edd962427eefd1339a0364b92dc666bc6e39b0..3c2ebe1d180fab85c9f1e0e4e882f1ee3b3c1f56 100644 (file)
--- a/ssl/ssl_err.c
+++ b/ssl/ssl_err.c
@@ -1,6 +1,6 @@
 /*
  * Generated by util/mkerr.pl DO NOT EDIT
- * Copyright 1995-2016 The OpenSSL Project Authors. All Rights Reserved.
+ * Copyright 1995-2018 The OpenSSL Project Authors. All Rights Reserved.
  *
  * Licensed under the OpenSSL license (the "License").  You may not use
  * this file except in compliance with the License.  You can obtain a copy
@@ -75,6 +75,7 @@ static ERR_STRING_DATA SSL_str_functs[] = {
     {ERR_FUNC(SSL_F_SSL3_SETUP_KEY_BLOCK), "ssl3_setup_key_block"},
     {ERR_FUNC(SSL_F_SSL3_SETUP_READ_BUFFER), "ssl3_setup_read_buffer"},
     {ERR_FUNC(SSL_F_SSL3_SETUP_WRITE_BUFFER), "ssl3_setup_write_buffer"},
+    {ERR_FUNC(SSL_F_SSL3_TAKE_MAC), "ssl3_take_mac"},
     {ERR_FUNC(SSL_F_SSL3_WRITE_BYTES), "ssl3_write_bytes"},
     {ERR_FUNC(SSL_F_SSL3_WRITE_PENDING), "ssl3_write_pending"},
     {ERR_FUNC(SSL_F_SSL_ADD_CERT_CHAIN), "ssl_add_cert_chain"},

diff --git a/ssl/statem/statem_dtls.c b/ssl/statem/statem_dtls.c
index 37e7fea8abc048a2b8b6427eccc26798fa8884ee..22be8711f33a238ddd2c7d8c7dfe6350d6a0691c 100644 (file)
--- a/ssl/statem/statem_dtls.c
+++ b/ssl/statem/statem_dtls.c
@@ -371,6 +371,15 @@ int dtls_get_message(SSL *s, int *mt, unsigned long *len)
         msg_len += DTLS1_HM_HEADER_LENGTH;
     }
 
+    /*
+     * If receiving Finished, record MAC of prior handshake messages for
+     * Finished verification.
+     */
+    if (*mt == SSL3_MT_FINISHED && !ssl3_take_mac(s)) {
+        /* SSLfatal() already called */
+        return 0;
+    }
+
     if (!ssl3_finish_mac(s, p, msg_len))
         return 0;
     if (s->msg_callback)

diff --git a/ssl/statem/statem_lib.c b/ssl/statem/statem_lib.c
index 637c610ac38619c714e65103d5ddefe6cd22ba78..570214553a97e777c2ec6f71c6ad12b1bc040af0 100644 (file)
--- a/ssl/statem/statem_lib.c
+++ b/ssl/statem/statem_lib.c
@@ -95,21 +95,15 @@ int tls_construct_finished(SSL *s, const char *sender, int slen)
     return 1;
 }
 
-#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)
+int ssl3_take_mac(SSL *s)
 {
     const char *sender;
     int slen;
-    /*
-     * If no new cipher setup return immediately: other functions will set
-     * the appropriate error.
-     */
-    if (s->s3->tmp.new_cipher == NULL)
-        return;
+
     if (!s->server) {
         sender = s->method->ssl3_enc->server_finished_label;
         slen = s->method->ssl3_enc->server_finished_label_len;
@@ -118,12 +112,17 @@ static void ssl3_take_mac(SSL *s)
         slen = s->method->ssl3_enc->client_finished_label_len;
     }
 
-    s->s3->tmp.peer_finish_md_len = s->method->ssl3_enc->final_finish_mac(s,
-                                                                          sender,
-                                                                          slen,
-                                                                          s->s3->tmp.peer_finish_md);
+    s->s3->tmp.peer_finish_md_len =
+        s->method->ssl3_enc->final_finish_mac(s, sender, slen,
+                                              s->s3->tmp.peer_finish_md);
+
+    if (s->s3->tmp.peer_finish_md_len == 0) {
+        SSLerr(SSL_F_SSL3_TAKE_MAC, ERR_R_INTERNAL_ERROR);
+        return 0;
+    }
+
+    return 1;
 }
-#endif
 
 MSG_PROCESS_RETURN tls_process_change_cipher_spec(SSL *s, PACKET *pkt)
 {
@@ -465,14 +464,15 @@ int tls_get_message_body(SSL *s, unsigned long *len)
         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
+    if (*(s->init_buf->data) == SSL3_MT_FINISHED && !ssl3_take_mac(s)) {
+        /* SSLfatal() already called */
+        *len = 0;
+        return 0;
+    }
 
     /* Feed this message into MAC computation. */
     if (RECORD_LAYER_is_sslv2_record(&s->rlayer)) {

diff --git a/ssl/statem/statem_locl.h b/ssl/statem/statem_locl.h
index a46e6c79711a9058eb501296f88e85ab90f3be05..98e41fea517f12ecd827ea2f0aac9bad07dca7b1 100644 (file)
--- a/ssl/statem/statem_locl.h
+++ b/ssl/statem/statem_locl.h
@@ -42,6 +42,7 @@ typedef enum {
 
 /* Flush the write BIO */
 int statem_flush(SSL *s);
+int ssl3_take_mac(SSL *s);
 
 /*
  * TLS/DTLS client state machine functions