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:
ngtcp2/ngtcp2#1582 (comment)

Reviewed-by: Tim Hudson <[email protected]>
Reviewed-by: Neil Horman <[email protected]>
Reviewed-by: Tomas Mraz <[email protected]>
(Merged from openssl#27101)

Author: mattcaswell

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 */

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;
 }
 

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;
             }

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.

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:

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))