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, 25 insertions, 27 deletions

diff --git a/src/lib/libssl/ssl_lib.c b/src/lib/libssl/ssl_lib.c
index 938139e18e..44d11d4b16 100644
--- a/src/lib/libssl/ssl_lib.c
+++ b/src/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 */
@@ -3021,14 +3027,6 @@ SSL_set_msg_callback(SSL *ssl, void (*cb)(int write_p, int version,
 }
 
 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)
 {
         s->internal->debug = debug;