Change the TLS handshake keys early if we're not doing early data

We change the client TLS handshake keys as late as possible so that we
don't disturb the keys if we are writing early data. However for QUIC we
want to do this as early as possible (after ServerHello). Since we will
never do TLS early data with QUIC we just do it as early as possible if
early data is not being used.

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/21810)

diff --git a/ssl/statem/statem_clnt.c b/ssl/statem/statem_clnt.c
index 9336363ca1af57bcee622d4eabed788c0ad9c85a..f2dec2fc0fd445e68a8290648bb12d8568b10fce 100644 (file)
--- a/ssl/statem/statem_clnt.c
+++ b/ssl/statem/statem_clnt.c
@@ -1782,12 +1782,29 @@ MSG_PROCESS_RETURN tls_process_server_hello(SSL_CONNECTION *s, PACKET *pkt)
      * In TLSv1.3 we have some post-processing to change cipher state, otherwise
      * we're done with this message
      */
-    if (SSL_CONNECTION_IS_TLS13(s)
-            && (!ssl->method->ssl3_enc->setup_key_block(s)
+    if (SSL_CONNECTION_IS_TLS13(s)) {
+        if (!ssl->method->ssl3_enc->setup_key_block(s)
                 || !ssl->method->ssl3_enc->change_cipher_state(s,
-                    SSL3_CC_HANDSHAKE | SSL3_CHANGE_CIPHER_CLIENT_READ))) {
-        /* SSLfatal() already called */
-        goto err;
+                    SSL3_CC_HANDSHAKE | SSL3_CHANGE_CIPHER_CLIENT_READ)) {
+            /* SSLfatal() already called */
+            goto err;
+        }
+        /*
+         * If we're not doing early-data and we're not going to send a dummy CCS
+         * (i.e. no middlebox compat mode) then we can change the write keys
+         * immediately. Otherwise we have to defer this until after all possible
+         * early data is written. We could just alway defer until the last
+         * moment except QUIC needs it done at the same time as the read keys
+         * are changed. Since QUIC doesn't do TLS early data or need middlebox
+         * compat this doesn't cause a problem.
+         */
+        if (s->early_data_state == SSL_EARLY_DATA_NONE
+                && (s->options & SSL_OP_ENABLE_MIDDLEBOX_COMPAT) == 0
+                && !ssl->method->ssl3_enc->change_cipher_state(s,
+                    SSL3_CC_HANDSHAKE | SSL3_CHANGE_CIPHER_CLIENT_WRITE)) {
+            /* SSLfatal() already called */
+            goto err;
+        }
     }
 
     OPENSSL_free(extensions);
@@ -3772,8 +3789,15 @@ CON_FUNC_RETURN tls_construct_client_certificate(SSL_CONNECTION *s,
         return CON_FUNC_ERROR;
     }
 
+    /*
+     * If we attempted to write early data or we're in middlebox compat mode
+     * then we deferred changing the handshake write keys to the last possible
+     * moment. We need to do it now.
+     */
     if (SSL_CONNECTION_IS_TLS13(s)
             && SSL_IS_FIRST_HANDSHAKE(s)
+            && (s->early_data_state != SSL_EARLY_DATA_NONE
+                || (s->options & SSL_OP_ENABLE_MIDDLEBOX_COMPAT) != 0)
             && (!ssl->method->ssl3_enc->change_cipher_state(s,
                     SSL3_CC_HANDSHAKE | SSL3_CHANGE_CIPHER_CLIENT_WRITE))) {
         /*
@@ -3855,7 +3879,14 @@ CON_FUNC_RETURN tls_construct_client_compressed_certificate(SSL_CONNECTION *sc,
             || !WPACKET_close(pkt))
         goto err;
 
+    /*
+     * If we attempted to write early data or we're in middlebox compat mode
+     * then we deferred changing the handshake write keys to the last possible
+     * moment. We need to do it now.
+     */
     if (SSL_IS_FIRST_HANDSHAKE(sc)
+            && (sc->early_data_state != SSL_EARLY_DATA_NONE
+                || (sc->options & SSL_OP_ENABLE_MIDDLEBOX_COMPAT) != 0)
             && (!ssl->method->ssl3_enc->change_cipher_state(sc,
                     SSL3_CC_HANDSHAKE | SSL3_CHANGE_CIPHER_CLIENT_WRITE))) {
         /*

diff --git a/ssl/statem/statem_lib.c b/ssl/statem/statem_lib.c
index e94b2f76cd5bafacedf6aa6ca25a786bae2ae41b..f339b2694c673d681e9aecc7461a29786bd5d0eb 100644 (file)
--- a/ssl/statem/statem_lib.c
+++ b/ssl/statem/statem_lib.c
@@ -615,11 +615,15 @@ CON_FUNC_RETURN tls_construct_finished(SSL_CONNECTION *s, WPACKET *pkt)
         s->statem.cleanuphand = 1;
 
     /*
-     * We only change the keys if we didn't already do this when we sent the
-     * client certificate
+     * If we attempted to write early data or we're in middlebox compat mode
+     * then we deferred changing the handshake write keys to the last possible
+     * moment. If we didn't already do this when we sent the client certificate
+     * then we need to do it now.
      */
     if (SSL_CONNECTION_IS_TLS13(s)
             && !s->server
+            && (s->early_data_state != SSL_EARLY_DATA_NONE
+                || (s->options & SSL_OP_ENABLE_MIDDLEBOX_COMPAT) != 0)
             && s->s3.tmp.cert_req == 0
             && (!ssl->method->ssl3_enc->change_cipher_state(s,
                     SSL3_CC_HANDSHAKE | SSL3_CHANGE_CIPHER_CLIENT_WRITE))) {;

diff --git a/test/recipes/75-test_quicapi_data/ssltraceref.txt b/test/recipes/75-test_quicapi_data/ssltraceref.txt
index 2ef20d34b92b6576d44ed8e551a6908f9527d9ef..cbca1cad70835a97cf03f45fc6494ef807f8b484 100644 (file)
--- a/test/recipes/75-test_quicapi_data/ssltraceref.txt
+++ b/test/recipes/75-test_quicapi_data/ssltraceref.txt
@@ -233,6 +233,11 @@ YeeuLO02zToHhnQ6KbPXOrQAqcL1kngO4g+j/ru+4AZThFkdkGnltvk=
 ------------------
         No extensions
 
+Sent Frame: Ack  (without ECN)
+    Largest acked: 0
+    Ack delay (raw) 0
+    Ack range count: 0
+    First ack range: 0
 Sent Frame: Ack  (without ECN)
     Largest acked: 0
     Ack delay (raw) 0
@@ -244,9 +249,10 @@ Sent Packet
   Version: 0x00000001
   Destination Conn Id: 0x????????????????
   Source Conn Id: <zero length id>
-  Payload length: 1178
+  Payload length: 1137
   Token: <zero length token>
   Packet Number: 0x00000001
+
 Sent Datagram
   Length: 1200
 Received Datagram
@@ -297,6 +303,6 @@ Sent Packet
   Destination Conn Id: 0x????????????????
   Source Conn Id: <zero length id>
   Payload length: 60
-  Packet Number: 0x00000000
+  Packet Number: 0x00000001
 Sent Datagram
   Length: 81