From f1abf68627e51bc3621a614ccccf1e70315337bb Mon Sep 17 00:00:00 2001 From: jsing Date: Wed, 5 Sep 2018 16:48:11 +0000 Subject: [PATCH] Correctly clear the current cipher state, when changing cipher state. When a renegotiation results in a change of cipher suite, the renegotation would fail if it switched from AEAD to non-AEAD or vice versa. This is due to the fact that the previous EVP_AEAD or EVP_CIPHER state remained, resulting in incorrect logic that caused MAC failures. Rename ssl_clear_cipher_ctx() to ssl_clear_cipher_state() and split it into separate read/write components, then call these functions from the appropriate places when a ChangeCipherSpec message is being processed. Also, remove the separate ssl_clear_hash_ctx() calls and fold these into the ssl_clear_cipher_{read,write}_state() functions. Issue reported by Bernard Spil, who also tested this diff. ok tb@ --- lib/libssl/ssl_lib.c | 52 +++++++++++++++++++++---------------------- lib/libssl/ssl_locl.h | 7 +++--- lib/libssl/t1_enc.c | 19 +++++++--------- 3 files changed, 37 insertions(+), 41 deletions(-) diff --git a/lib/libssl/ssl_lib.c b/lib/libssl/ssl_lib.c index 938139e18ed..44d11d4b164 100644 --- a/lib/libssl/ssl_lib.c +++ b/lib/libssl/ssl_lib.c @@ -1,4 +1,4 @@ -/* $OpenBSD: ssl_lib.c,v 1.187 2018/08/30 16:56:16 jsing Exp $ */ +/* $OpenBSD: ssl_lib.c,v 1.188 2018/09/05 16:48:11 jsing Exp $ */ /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com) * All rights reserved. * @@ -191,9 +191,7 @@ SSL_clear(SSL *s) BUF_MEM_free(s->internal->init_buf); s->internal->init_buf = NULL; - ssl_clear_cipher_ctx(s); - ssl_clear_hash_ctx(&s->read_hash); - ssl_clear_hash_ctx(&s->internal->write_hash); + ssl_clear_cipher_state(s); s->internal->first_packet = 0; @@ -534,9 +532,7 @@ SSL_free(SSL *s) SSL_SESSION_free(s->session); } - ssl_clear_cipher_ctx(s); - ssl_clear_hash_ctx(&s->read_hash); - ssl_clear_hash_ctx(&s->internal->write_hash); + ssl_clear_cipher_state(s); ssl_cert_free(s->cert); @@ -2431,10 +2427,7 @@ SSL_set_accept_state(SSL *s) s->internal->shutdown = 0; S3I(s)->hs.state = SSL_ST_ACCEPT|SSL_ST_BEFORE; s->internal->handshake_func = s->method->internal->ssl_accept; - /* clear the current cipher */ - ssl_clear_cipher_ctx(s); - ssl_clear_hash_ctx(&s->read_hash); - ssl_clear_hash_ctx(&s->internal->write_hash); + ssl_clear_cipher_state(s); } void @@ -2444,10 +2437,7 @@ SSL_set_connect_state(SSL *s) s->internal->shutdown = 0; S3I(s)->hs.state = SSL_ST_CONNECT|SSL_ST_BEFORE; s->internal->handshake_func = s->method->internal->ssl_connect; - /* clear the current cipher */ - ssl_clear_cipher_ctx(s); - ssl_clear_hash_ctx(&s->read_hash); - ssl_clear_hash_ctx(&s->internal->write_hash); + ssl_clear_cipher_state(s); } int @@ -2623,24 +2613,40 @@ SSL_dup(SSL *s) } void -ssl_clear_cipher_ctx(SSL *s) +ssl_clear_cipher_state(SSL *s) +{ + ssl_clear_cipher_read_state(s); + ssl_clear_cipher_write_state(s); +} + +void +ssl_clear_cipher_read_state(SSL *s) { EVP_CIPHER_CTX_free(s->enc_read_ctx); s->enc_read_ctx = NULL; - EVP_CIPHER_CTX_free(s->internal->enc_write_ctx); - s->internal->enc_write_ctx = NULL; + EVP_MD_CTX_destroy(s->read_hash); + s->read_hash = NULL; if (s->internal->aead_read_ctx != NULL) { EVP_AEAD_CTX_cleanup(&s->internal->aead_read_ctx->ctx); free(s->internal->aead_read_ctx); s->internal->aead_read_ctx = NULL; } +} + +void +ssl_clear_cipher_write_state(SSL *s) +{ + EVP_CIPHER_CTX_free(s->internal->enc_write_ctx); + s->internal->enc_write_ctx = NULL; + EVP_MD_CTX_destroy(s->internal->write_hash); + s->internal->write_hash = NULL; + if (s->internal->aead_write_ctx != NULL) { EVP_AEAD_CTX_cleanup(&s->internal->aead_write_ctx->ctx); free(s->internal->aead_write_ctx); s->internal->aead_write_ctx = NULL; } - } /* Fix this function so that it takes an optional type parameter */ @@ -3020,14 +3026,6 @@ SSL_set_msg_callback(SSL *ssl, void (*cb)(int write_p, int version, SSL_callback_ctrl(ssl, SSL_CTRL_SET_MSG_CALLBACK, (void (*)(void))cb); } -void -ssl_clear_hash_ctx(EVP_MD_CTX **hash) -{ - if (*hash) - EVP_MD_CTX_destroy(*hash); - *hash = NULL; -} - void SSL_set_debug(SSL *s, int debug) { diff --git a/lib/libssl/ssl_locl.h b/lib/libssl/ssl_locl.h index b6d71492fd0..a4e831577d0 100644 --- a/lib/libssl/ssl_locl.h +++ b/lib/libssl/ssl_locl.h @@ -1,4 +1,4 @@ -/* $OpenBSD: ssl_locl.h,v 1.212 2018/08/30 16:56:16 jsing Exp $ */ +/* $OpenBSD: ssl_locl.h,v 1.213 2018/09/05 16:48:11 jsing Exp $ */ /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com) * All rights reserved. * @@ -1039,7 +1039,9 @@ extern SSL3_ENC_METHOD TLSv1_enc_data; extern SSL3_ENC_METHOD TLSv1_1_enc_data; extern SSL3_ENC_METHOD TLSv1_2_enc_data; -void ssl_clear_cipher_ctx(SSL *s); +void ssl_clear_cipher_state(SSL *s); +void ssl_clear_cipher_read_state(SSL *s); +void ssl_clear_cipher_write_state(SSL *s); int ssl_clear_bad_session(SSL *s); CERT *ssl_cert_new(void); CERT *ssl_cert_dup(CERT *cert); @@ -1279,7 +1281,6 @@ int tls12_get_sigid(const EVP_PKEY *pk); int tls12_get_hashandsig(CBB *cbb, const EVP_PKEY *pk, const EVP_MD *md); const EVP_MD *tls12_get_hash(unsigned char hash_alg); -void ssl_clear_hash_ctx(EVP_MD_CTX **hash); long ssl_get_algorithm2(SSL *s); int tls1_process_sigalgs(SSL *s, CBS *cbs); void tls12_get_req_sig_algs(SSL *s, unsigned char **sigalgs, diff --git a/lib/libssl/t1_enc.c b/lib/libssl/t1_enc.c index 24fc8ede683..39f542215b4 100644 --- a/lib/libssl/t1_enc.c +++ b/lib/libssl/t1_enc.c @@ -1,4 +1,4 @@ -/* $OpenBSD: t1_enc.c,v 1.110 2018/08/31 18:31:34 jsing Exp $ */ +/* $OpenBSD: t1_enc.c,v 1.111 2018/09/05 16:48:11 jsing Exp $ */ /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com) * All rights reserved. * @@ -397,10 +397,13 @@ tls1_change_cipher_state_aead(SSL *s, char is_read, const unsigned char *key, SSL_AEAD_CTX *aead_ctx; if (is_read) { + ssl_clear_cipher_read_state(s); if (!tls1_aead_ctx_init(&s->internal->aead_read_ctx)) return 0; aead_ctx = s->internal->aead_read_ctx; } else { + /* XXX - Need to correctly handle DTLS. */ + ssl_clear_cipher_write_state(s); if (!tls1_aead_ctx_init(&s->internal->aead_write_ctx)) return 0; aead_ctx = s->internal->aead_write_ctx; @@ -468,10 +471,7 @@ tls1_change_cipher_state_cipher(SSL *s, char is_read, else s->internal->mac_flags &= ~SSL_MAC_FLAG_READ_MAC_STREAM; - EVP_CIPHER_CTX_free(s->enc_read_ctx); - s->enc_read_ctx = NULL; - EVP_MD_CTX_destroy(s->read_hash); - s->read_hash = NULL; + ssl_clear_cipher_read_state(s); if ((cipher_ctx = EVP_CIPHER_CTX_new()) == NULL) goto err; @@ -492,12 +492,9 @@ tls1_change_cipher_state_cipher(SSL *s, char is_read, * contexts that are used for DTLS - these are instead freed * by DTLS when its frees a ChangeCipherSpec fragment. */ - if (!SSL_IS_DTLS(s)) { - EVP_CIPHER_CTX_free(s->internal->enc_write_ctx); - s->internal->enc_write_ctx = NULL; - EVP_MD_CTX_destroy(s->internal->write_hash); - s->internal->write_hash = NULL; - } + if (!SSL_IS_DTLS(s)) + ssl_clear_cipher_write_state(s); + if ((cipher_ctx = EVP_CIPHER_CTX_new()) == NULL) goto err; s->internal->enc_write_ctx = cipher_ctx; -- 2.20.1