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@

1 files changed, 8 insertions, 11 deletions

diff --git a/src/lib/libssl/t1_enc.c b/src/lib/libssl/t1_enc.c
index 24fc8ede68..39f542215b 100644
--- a/src/lib/libssl/t1_enc.c
+++ b/src/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;