Move the Handshake read secret change earlier in the process for QUIC 0-RTT

On the server side we were changing the handshake rx secret a little late.
This meant the application was forced to call SSL_do_handshake() again
even if there was nothing to read in order to get the secret. We move it
a little earlier int the process to avoid this.

Fixes the issue described in:
https://github.com/ngtcp2/ngtcp2/pull/1582#issuecomment-2735950083

Reviewed-by: Tim Hudson <tjh@openssl.org>
Reviewed-by: Neil Horman <nhorman@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/27101)

diff --git a/include/internal/statem.h b/include/internal/statem.h
index 62dc4eec0ba7c4251c648141ece459fa86c20a31..261d7967cc9a6a0de15e934e955c55750b4731d4 100644 (file)
--- a/include/internal/statem.h
+++ b/include/internal/statem.h
@@ -26,6 +26,8 @@ typedef enum {
     WORK_FINISHED_STOP,
     /* We're done working move onto the next thing */
     WORK_FINISHED_CONTINUE,
+    /* We're done writing, start reading (or vice versa) */
+    WORK_FINISHED_SWAP,
     /* We're working on phase A */
     WORK_MORE_A,
     /* We're working on phase B */

diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c
index 912c6b121e7331ed6f59fb162a423ce9de4030ff..4c7b62e14232d0cbe6752406ce879e0dbd1354cd 100644 (file)
--- a/ssl/ssl_lib.c
+++ b/ssl/ssl_lib.c
@@ -4968,12 +4968,6 @@ int SSL_do_handshake(SSL *s)
         }
     }
 
-    if (ret == 1 && SSL_IS_QUIC_HANDSHAKE(sc) && !SSL_is_init_finished(s)) {
-        sc->rwstate = SSL_READING;
-        BIO_clear_retry_flags(SSL_get_rbio(s));
-        BIO_set_retry_read(SSL_get_rbio(s));
-        ret = 0;
-    }
     return ret;
 }
 

diff --git a/ssl/statem/statem.c b/ssl/statem/statem.c
index e76fde810b6b4a29ae518b36333dc072ca53a5d4..05b491c3956af1233464f799b8a1f63b24937b04 100644 (file)
--- a/ssl/statem/statem.c
+++ b/ssl/statem/statem.c
@@ -244,15 +244,6 @@ int ossl_statem_skip_early_data(SSL_CONNECTION *s)
  */
 int ossl_statem_check_finish_init(SSL_CONNECTION *s, int sending)
 {
-    int i = SSL3_CC_HANDSHAKE | SSL3_CHANGE_CIPHER_SERVER_READ;
-
-    if (s->server && SSL_NO_EOED(s) && s->ext.early_data == SSL_EARLY_DATA_ACCEPTED
-        && s->early_data_state != SSL_EARLY_DATA_FINISHED_READING
-            && s->statem.hand_state == TLS_ST_EARLY_DATA) {
-        s->early_data_state = SSL_EARLY_DATA_FINISHED_READING;
-        if (!SSL_CONNECTION_GET_SSL(s)->method->ssl3_enc->change_cipher_state(s, i))
-            return 0;
-    }
     if (sending == -1) {
         if (s->statem.hand_state == TLS_ST_PENDING_EARLY_DATA_END
                 || s->statem.hand_state == TLS_ST_EARLY_DATA) {
@@ -737,6 +728,7 @@ static SUB_STATE_RETURN read_state_machine(SSL_CONNECTION *s)
                 st->read_state = READ_STATE_HEADER;
                 break;
 
+            case WORK_FINISHED_SWAP:
             case WORK_FINISHED_STOP:
                 if (SSL_CONNECTION_IS_DTLS(s)) {
                     dtls1_stop_timer(s);
@@ -882,6 +874,9 @@ static SUB_STATE_RETURN write_state_machine(SSL_CONNECTION *s)
                 st->write_state = WRITE_STATE_SEND;
                 break;
 
+            case WORK_FINISHED_SWAP:
+                return SUB_STATE_FINISHED;
+
             case WORK_FINISHED_STOP:
                 return SUB_STATE_END_HANDSHAKE;
             }
@@ -955,6 +950,9 @@ static SUB_STATE_RETURN write_state_machine(SSL_CONNECTION *s)
                 st->write_state = WRITE_STATE_TRANSITION;
                 break;
 
+            case WORK_FINISHED_SWAP:
+                return SUB_STATE_FINISHED;
+
             case WORK_FINISHED_STOP:
                 return SUB_STATE_END_HANDSHAKE;
             }

diff --git a/ssl/statem/statem_clnt.c b/ssl/statem/statem_clnt.c
index 9989d6bb93421a60c21e81a56cc6a08fa910404a..3990a2b0c219478c0e7a80fdd632201951f7963e 100644 (file)
--- a/ssl/statem/statem_clnt.c
+++ b/ssl/statem/statem_clnt.c
@@ -573,7 +573,8 @@ WRITE_TRAN ossl_statem_client_write_transition(SSL_CONNECTION *s)
         return WRITE_TRAN_CONTINUE;
 
     case TLS_ST_CW_CLNT_HELLO:
-        if (s->early_data_state == SSL_EARLY_DATA_CONNECTING) {
+        if (s->early_data_state == SSL_EARLY_DATA_CONNECTING
+                && !SSL_IS_QUIC_HANDSHAKE(s)) {
             /*
              * We are assuming this is a TLSv1.3 connection, although we haven't
              * actually selected a version yet.

diff --git a/ssl/statem/statem_srvr.c b/ssl/statem/statem_srvr.c
index cd062a00d5be8db3cbe820ee3f58b689cb1bf6fd..b93a97999de262d971c8e740a5d1137a429167e0 100644 (file)
--- a/ssl/statem/statem_srvr.c
+++ b/ssl/statem/statem_srvr.c
@@ -839,6 +839,21 @@ WORK_STATE ossl_statem_server_pre_work(SSL_CONNECTION *s, WORK_STATE wst)
         if (s->early_data_state != SSL_EARLY_DATA_ACCEPTING
                 && (s->s3.flags & TLS1_FLAGS_STATELESS) == 0)
             return WORK_FINISHED_CONTINUE;
+
+        /*
+         * In QUIC with 0-RTT we just carry on when otherwise we would stop
+         * to allow the server to read early data
+         */
+        if (SSL_NO_EOED(s) && s->ext.early_data == SSL_EARLY_DATA_ACCEPTED
+            && s->early_data_state != SSL_EARLY_DATA_FINISHED_READING) {
+            s->early_data_state = SSL_EARLY_DATA_FINISHED_READING;
+            if (!ssl->method->ssl3_enc->change_cipher_state(s, SSL3_CC_HANDSHAKE
+                                                               | SSL3_CHANGE_CIPHER_SERVER_READ)) {
+                SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR);
+                return WORK_ERROR;
+            }
+            return WORK_FINISHED_SWAP;
+        }
         /* Fall through */
 
     case TLS_ST_OK:

diff --git a/test/sslapitest.c b/test/sslapitest.c
index 413e3181ba4369c2fa65a9e030d8f7e6b9a6a0e9..a20d1f720342c2da25b25d292a8c7b68560bb60d 100644 (file)
--- a/test/sslapitest.c
+++ b/test/sslapitest.c
@@ -12969,15 +12969,15 @@ static int test_quic_tls_early_data(void)
     SSL_set_msg_callback(serverssl, assert_no_end_of_early_data);
     SSL_set_msg_callback(clientssl, assert_no_end_of_early_data);
 
-    if (!TEST_int_eq(SSL_connect(clientssl), 0)
-            || !TEST_int_eq(SSL_accept(serverssl), 0)
+    if (!TEST_int_eq(SSL_connect(clientssl), -1)
+            || !TEST_int_eq(SSL_accept(serverssl), -1)
             || !TEST_int_eq(SSL_get_early_data_status(serverssl), SSL_EARLY_DATA_ACCEPTED)
             || !TEST_int_eq(SSL_get_error(clientssl, 0), SSL_ERROR_WANT_READ)
             || !TEST_int_eq(SSL_get_error(serverssl, 0), SSL_ERROR_WANT_READ))
         goto end;
 
     /* Check the encryption levels are what we expect them to be */
-    if (!TEST_true(sdata.renc_level == OSSL_RECORD_PROTECTION_LEVEL_EARLY)
+    if (!TEST_true(sdata.renc_level == OSSL_RECORD_PROTECTION_LEVEL_HANDSHAKE)
             || !TEST_true(sdata.wenc_level == OSSL_RECORD_PROTECTION_LEVEL_APPLICATION)
             || !TEST_true(cdata.renc_level == OSSL_RECORD_PROTECTION_LEVEL_NONE)
             || !TEST_true(cdata.wenc_level == OSSL_RECORD_PROTECTION_LEVEL_EARLY))