From 098cfd216b9b54106cbf9338a511c7dda972b8c1 Mon Sep 17 00:00:00 2001 From: Matt Caswell Date: Thu, 29 May 2025 12:52:35 +0100 Subject: [PATCH] Ensure client read app data secret change occurs after write for QUIC We don't want read secrets to be issue before write for QUIC, because we want to avoid the situation where we want to ack something we've read but we don't have the write secret yet. Reviewed-by: Neil Horman Reviewed-by: Tomas Mraz (Merged from https://github.com/openssl/openssl/pull/27732) --- ssl/statem/statem_clnt.c | 11 +++++++++++ ssl/statem/statem_lib.c | 14 ++++++++++++-- ssl/tls13_enc.c | 1 + 3 files changed, 24 insertions(+), 2 deletions(-) diff --git a/ssl/statem/statem_clnt.c b/ssl/statem/statem_clnt.c index 8134afa3310..ba4500dd659 100644 --- a/ssl/statem/statem_clnt.c +++ b/ssl/statem/statem_clnt.c @@ -911,6 +911,17 @@ WORK_STATE ossl_statem_client_post_work(SSL_CONNECTION *s, WORK_STATE wst) /* SSLfatal() already called */ return WORK_ERROR; } + /* + * For QUIC we deferred setting up these keys until now so + * that we can ensure write keys are always set up before read + * keys. + */ + if (SSL_IS_QUIC_HANDSHAKE(s) + && !ssl->method->ssl3_enc->change_cipher_state(s, + SSL3_CC_APPLICATION | SSL3_CHANGE_CIPHER_CLIENT_READ)) { + /* SSLfatal() already called */ + return WORK_ERROR; + } } } break; diff --git a/ssl/statem/statem_lib.c b/ssl/statem/statem_lib.c index cb1e4f8c4b8..1e11d077f9e 100644 --- a/ssl/statem/statem_lib.c +++ b/ssl/statem/statem_lib.c @@ -938,8 +938,18 @@ MSG_PROCESS_RETURN tls_process_finished(SSL_CONNECTION *s, PACKET *pkt) /* SSLfatal() already called */ return MSG_PROCESS_ERROR; } - if (!ssl->method->ssl3_enc->change_cipher_state(s, - SSL3_CC_APPLICATION | SSL3_CHANGE_CIPHER_CLIENT_READ)) { + + /* + * For non-QUIC we set up the client's app data read keys now, so + * that we can go straight into reading 0.5RTT data from the server. + * For QUIC we don't do that, and instead defer setting up the keys + * until after we have set up the write keys in order to ensure that + * write keys are always set up before read keys (so that if we read + * a message we have the correct keys in place to ack it) + */ + if (!SSL_IS_QUIC_HANDSHAKE(s) + && !ssl->method->ssl3_enc->change_cipher_state(s, + SSL3_CC_APPLICATION | SSL3_CHANGE_CIPHER_CLIENT_READ)) { /* SSLfatal() already called */ return MSG_PROCESS_ERROR; } diff --git a/ssl/tls13_enc.c b/ssl/tls13_enc.c index f4af3ad3c89..0aa97648c4e 100644 --- a/ssl/tls13_enc.c +++ b/ssl/tls13_enc.c @@ -658,6 +658,7 @@ int tls13_change_cipher_state(SSL_CONNECTION *s, int which) label = server_application_traffic; labellen = sizeof(server_application_traffic) - 1; log_label = SERVER_APPLICATION_LABEL; + hash = s->server_finished_hash; } } -- 2.47.2