From f4871816fd4b8146c7018c47f76d3ddfc727efca Mon Sep 17 00:00:00 2001 From: Guillaume Hetier Date: Fri, 27 Mar 2026 13:28:11 -0700 Subject: [PATCH 1/2] Drop initial key state as soon as a handshake packet is sent --- src/core/crypto.c | 10 --------- src/core/packet_builder.c | 12 +++++++++++ src/platform/tls_openssl.c | 42 +++++++++++++++++++++----------------- src/platform/tls_quictls.c | 42 +++++++++++++++++++++----------------- 4 files changed, 58 insertions(+), 48 deletions(-) diff --git a/src/core/crypto.c b/src/core/crypto.c index d5d9efc5c9..12d209a18c 100644 --- a/src/core/crypto.c +++ b/src/core/crypto.c @@ -941,16 +941,6 @@ QuicCryptoWriteFrames( return TRUE; } - if (QuicConnIsClient(Connection) && - Builder->Key == Crypto->TlsState.WriteKeys[QUIC_PACKET_KEY_HANDSHAKE]) { - CXPLAT_DBG_ASSERT(Builder->Key); - // - // Per spec, client MUST discard Initial keys when it starts - // encrypting packets with handshake keys. - // - QuicCryptoDiscardKeys(Crypto, QUIC_PACKET_KEY_INITIAL); - } - uint8_t PrevFrameCount = Builder->Metadata->FrameCount; uint16_t AvailableBufferLength = diff --git a/src/core/packet_builder.c b/src/core/packet_builder.c index 854e420db7..2bf33e918c 100644 --- a/src/core/packet_builder.c +++ b/src/core/packet_builder.c @@ -997,6 +997,18 @@ QuicPacketBuilderFinalize( Builder->Path, Builder->Metadata); + // + // Per RFC 9001 s4.9.1, a client MUST discard Initial keys when it first + // sends a Handshake packet. This fires on ANY Handshake packet (including + // ACK-only) so that Initial bytes in flight are released from congestion + // control. The next iteration of the send loop will then have CC allowance + // to write the CRYPTO frame with the TLS Finished. + // + if (QuicConnIsClient(Connection) && + Builder->Key->Type == QUIC_PACKET_KEY_HANDSHAKE) { + QuicCryptoDiscardKeys(&Connection->Crypto, QUIC_PACKET_KEY_INITIAL); + } + Builder->Metadata->FrameCount = 0; if (Builder->Metadata->Flags.IsAckEliciting) { diff --git a/src/platform/tls_openssl.c b/src/platform/tls_openssl.c index c6c79db5e9..d8e1bfa0fb 100644 --- a/src/platform/tls_openssl.c +++ b/src/platform/tls_openssl.c @@ -3321,25 +3321,29 @@ CxPlatTlsProcessData( Exit: - if (!(TlsContext->ResultFlags & CXPLAT_TLS_RESULT_ERROR)) { - if (State->WriteKeys[QUIC_PACKET_KEY_HANDSHAKE] != NULL && - State->BufferOffsetHandshake == 0) { - State->BufferOffsetHandshake = State->BufferTotalLength; - QuicTraceLogConnInfo( - OpenSslHandshakeDataStart, - TlsContext->Connection, - "Writing Handshake data starts at %u", - State->BufferOffsetHandshake); - } - if (State->WriteKeys[QUIC_PACKET_KEY_1_RTT] != NULL && - State->BufferOffset1Rtt == 0) { - State->BufferOffset1Rtt = State->BufferTotalLength; - QuicTraceLogConnInfo( - OpenSsl1RttDataStart, - TlsContext->Connection, - "Writing 1-RTT data starts at %u", - State->BufferOffset1Rtt); - } + // + // Set buffer offsets unconditionally (not gated on !ERROR) because write + // keys may be installed asynchronously via the secrets callback before + // this exit code runs. On error, the connection is torn down before + // these offsets are consumed. + // + if (State->WriteKeys[QUIC_PACKET_KEY_HANDSHAKE] != NULL && + State->BufferOffsetHandshake == 0) { + State->BufferOffsetHandshake = State->BufferTotalLength; + QuicTraceLogConnInfo( + OpenSslHandshakeDataStart, + TlsContext->Connection, + "Writing Handshake data starts at %u", + State->BufferOffsetHandshake); + } + if (State->WriteKeys[QUIC_PACKET_KEY_1_RTT] != NULL && + State->BufferOffset1Rtt == 0) { + State->BufferOffset1Rtt = State->BufferTotalLength; + QuicTraceLogConnInfo( + OpenSsl1RttDataStart, + TlsContext->Connection, + "Writing 1-RTT data starts at %u", + State->BufferOffset1Rtt); } return TlsContext->ResultFlags; diff --git a/src/platform/tls_quictls.c b/src/platform/tls_quictls.c index 65131bf2ad..43795c8650 100644 --- a/src/platform/tls_quictls.c +++ b/src/platform/tls_quictls.c @@ -2145,25 +2145,29 @@ CxPlatTlsProcessData( Exit: - if (!(TlsContext->ResultFlags & CXPLAT_TLS_RESULT_ERROR)) { - if (State->WriteKeys[QUIC_PACKET_KEY_HANDSHAKE] != NULL && - State->BufferOffsetHandshake == 0) { - State->BufferOffsetHandshake = State->BufferTotalLength; - QuicTraceLogConnInfo( - OpenSslHandshakeDataStart, - TlsContext->Connection, - "Writing Handshake data starts at %u", - State->BufferOffsetHandshake); - } - if (State->WriteKeys[QUIC_PACKET_KEY_1_RTT] != NULL && - State->BufferOffset1Rtt == 0) { - State->BufferOffset1Rtt = State->BufferTotalLength; - QuicTraceLogConnInfo( - OpenSsl1RttDataStart, - TlsContext->Connection, - "Writing 1-RTT data starts at %u", - State->BufferOffset1Rtt); - } + // + // Set buffer offsets unconditionally (not gated on !ERROR) because write + // keys may be installed asynchronously via the secrets callback before + // this exit code runs. On error, the connection is torn down before + // these offsets are consumed. + // + if (State->WriteKeys[QUIC_PACKET_KEY_HANDSHAKE] != NULL && + State->BufferOffsetHandshake == 0) { + State->BufferOffsetHandshake = State->BufferTotalLength; + QuicTraceLogConnInfo( + OpenSslHandshakeDataStart, + TlsContext->Connection, + "Writing Handshake data starts at %u", + State->BufferOffsetHandshake); + } + if (State->WriteKeys[QUIC_PACKET_KEY_1_RTT] != NULL && + State->BufferOffset1Rtt == 0) { + State->BufferOffset1Rtt = State->BufferTotalLength; + QuicTraceLogConnInfo( + OpenSsl1RttDataStart, + TlsContext->Connection, + "Writing 1-RTT data starts at %u", + State->BufferOffset1Rtt); } return TlsContext->ResultFlags; From 11632fc20e6f76da0cfcbb117b3b1dad3949bd05 Mon Sep 17 00:00:00 2001 From: Guillaume Hetier Date: Mon, 20 Apr 2026 10:14:21 -0700 Subject: [PATCH 2/2] Improve comments --- src/core/packet_builder.c | 7 +++---- src/platform/tls_openssl.c | 6 ++---- src/platform/tls_quictls.c | 6 ++---- 3 files changed, 7 insertions(+), 12 deletions(-) diff --git a/src/core/packet_builder.c b/src/core/packet_builder.c index 2bf33e918c..2c7d8792e7 100644 --- a/src/core/packet_builder.c +++ b/src/core/packet_builder.c @@ -999,10 +999,9 @@ QuicPacketBuilderFinalize( // // Per RFC 9001 s4.9.1, a client MUST discard Initial keys when it first - // sends a Handshake packet. This fires on ANY Handshake packet (including - // ACK-only) so that Initial bytes in flight are released from congestion - // control. The next iteration of the send loop will then have CC allowance - // to write the CRYPTO frame with the TLS Finished. + // sends a Handshake packet. It must be done on ANY Handshake packet, to + // ensure the congestion control state is cleared and Initial bytes in flight + // are not limiting Handshake packets. // if (QuicConnIsClient(Connection) && Builder->Key->Type == QUIC_PACKET_KEY_HANDSHAKE) { diff --git a/src/platform/tls_openssl.c b/src/platform/tls_openssl.c index d8e1bfa0fb..f708d74229 100644 --- a/src/platform/tls_openssl.c +++ b/src/platform/tls_openssl.c @@ -3322,10 +3322,8 @@ CxPlatTlsProcessData( Exit: // - // Set buffer offsets unconditionally (not gated on !ERROR) because write - // keys may be installed asynchronously via the secrets callback before - // this exit code runs. On error, the connection is torn down before - // these offsets are consumed. + // Always set buffer offsets if keys have been installed to preserve code invariants. + // On error, the connection will be torn down anyway. // if (State->WriteKeys[QUIC_PACKET_KEY_HANDSHAKE] != NULL && State->BufferOffsetHandshake == 0) { diff --git a/src/platform/tls_quictls.c b/src/platform/tls_quictls.c index 43795c8650..0cbbcbb61c 100644 --- a/src/platform/tls_quictls.c +++ b/src/platform/tls_quictls.c @@ -2146,10 +2146,8 @@ CxPlatTlsProcessData( Exit: // - // Set buffer offsets unconditionally (not gated on !ERROR) because write - // keys may be installed asynchronously via the secrets callback before - // this exit code runs. On error, the connection is torn down before - // these offsets are consumed. + // Always set buffer offsets if keys have been installed to preserve code invariants. + // On error, the connection will be torn down anyway. // if (State->WriteKeys[QUIC_PACKET_KEY_HANDSHAKE] != NULL && State->BufferOffsetHandshake == 0) {