From 95051052b319d346a8aa3d34d6105d683bb77294 Mon Sep 17 00:00:00 2001 From: Matt Caswell Date: Wed, 19 Mar 2025 15:18:06 +0000 Subject: [PATCH] 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 Reviewed-by: Neil Horman Reviewed-by: Tomas Mraz (Merged from https://github.com/openssl/openssl/pull/27101) --- include/internal/statem.h | 2 ++ ssl/ssl_lib.c | 6 ------ ssl/statem/statem.c | 16 +++++++--------- ssl/statem/statem_clnt.c | 3 ++- ssl/statem/statem_srvr.c | 15 +++++++++++++++ test/sslapitest.c | 6 +++--- 6 files changed, 29 insertions(+), 19 deletions(-) diff --git a/include/internal/statem.h b/include/internal/statem.h index 62dc4eec0b..261d7967cc 100644 --- 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 912c6b121e..4c7b62e142 100644 --- 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 e76fde810b..05b491c395 100644 --- 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 9989d6bb93..3990a2b0c2 100644 --- 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 cd062a00d5..b93a97999d 100644 --- 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 413e3181ba..a20d1f7203 100644 --- 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))