Don't write to the session when computing TLS 1.3 keys

TLS 1.3 maintains a separate keys chedule in the SSL object, but
was writing to the 'master_key_length' field in the SSL_SESSION
when generating the per-SSL master_secret.  (The generate_master_secret
SSL3_ENC_METHOD function needs an output variable for the master secret
length, but the TLS 1.3 implementation just uses the output size of
the handshake hash function to get the lengths, so the only natural-looking
thing to use as the output length was the field in the session.
This would potentially involve writing to a SSL_SESSION object that was
in the cache (i.e., resumed) and shared with other threads, though.

The thread-safety impact should be minimal, since TLS 1.3 requires the
hash from the original handshake to be associated with the resumption
PSK and used for the subsequent connection.  This means that (in the
resumption case) the value being written would be the same value that was
previously there, so the only risk would be on architectures that can
produce torn writes/reads for aligned size_t values.

Since the value is essentially ignored anyway, just provide the
address of a local dummy variable to generate_master_secret() instead.

Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
(Merged from https://github.com/openssl/openssl/pull/10943)

diff --git a/ssl/statem/statem_lib.c b/ssl/statem/statem_lib.c
index c5956ea37c98a80954f3be2249d04189a02be285..812dabe860ced46c4e43ddbda975c71e00973ce8 100644 (file)
--- a/ssl/statem/statem_lib.c
+++ b/ssl/statem/statem_lib.c
@@ -860,9 +860,11 @@ MSG_PROCESS_RETURN tls_process_finished(SSL *s, PACKET *pkt)
                 return MSG_PROCESS_ERROR;
             }
         } else {
                 return MSG_PROCESS_ERROR;
             }
         } else {

+            /* TLS 1.3 gets the secret size from the handshake md */
+            size_t dummy;

             if (!s->method->ssl3_enc->generate_master_secret(s,
                     s->master_secret, s->handshake_secret, 0,
             if (!s->method->ssl3_enc->generate_master_secret(s,
                     s->master_secret, s->handshake_secret, 0,

-                    &s->session->master_key_length)) {
+                    &dummy)) {

                 /* SSLfatal() already called */
                 return MSG_PROCESS_ERROR;
             }
                 /* SSLfatal() already called */
                 return MSG_PROCESS_ERROR;
             }

diff --git a/ssl/statem/statem_srvr.c b/ssl/statem/statem_srvr.c
index ab032ae956fecc70622ec67af443079d8e226770..00905eb76076cdadfc63444c374aee5c477968cf 100644 (file)
--- a/ssl/statem/statem_srvr.c
+++ b/ssl/statem/statem_srvr.c
@@ -948,9 +948,11 @@ WORK_STATE ossl_statem_server_post_work(SSL *s, WORK_STATE wst)
         }
 #endif
         if (SSL_IS_TLS13(s)) {
         }
 #endif
         if (SSL_IS_TLS13(s)) {

+            /* TLS 1.3 gets the secret size from the handshake md */
+            size_t dummy;

             if (!s->method->ssl3_enc->generate_master_secret(s,
                         s->master_secret, s->handshake_secret, 0,
             if (!s->method->ssl3_enc->generate_master_secret(s,
                         s->master_secret, s->handshake_secret, 0,

-                        &s->session->master_key_length)
+                        &dummy)

                 || !s->method->ssl3_enc->change_cipher_state(s,
                         SSL3_CC_APPLICATION | SSL3_CHANGE_CIPHER_SERVER_WRITE))
             /* SSLfatal() already called */
                 || !s->method->ssl3_enc->change_cipher_state(s,
                         SSL3_CC_APPLICATION | SSL3_CHANGE_CIPHER_SERVER_WRITE))
             /* SSLfatal() already called */