From 622ddb57798bb5c895b0fe40e3fd89fdb4cdbc65 Mon Sep 17 00:00:00 2001 From: Matt Caswell Date: Thu, 8 Feb 2018 14:48:51 +0000 Subject: [PATCH] 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 (Merged from https://github.com/openssl/openssl/pull/5286) --- include/openssl/ssl.h | 1 + ssl/s3_msg.c | 25 +------------------------ ssl/ssl_err.c | 3 ++- ssl/statem/statem_dtls.c | 9 +++++++++ ssl/statem/statem_lib.c | 34 +++++++++++++++++----------------- ssl/statem/statem_locl.h | 1 + 6 files changed, 31 insertions(+), 42 deletions(-) diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index 9ff9b600ae..abe4406126 100644 --- 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 82513d2590..8b08ba0bd1 100644 --- 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 96edd96242..3c2ebe1d18 100644 --- 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 37e7fea8ab..22be8711f3 100644 --- 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 637c610ac3..570214553a 100644 --- 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 a46e6c7971..98e41fea51 100644 --- 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 -- 2.25.1