Certificate rejection via ConnectionCertificateValidationComplete leads to connection timeout.

[rzikm]

Describe the bug

I am offloading computation-heavy certificate validation to a non-MsQuic thread in System.Net.Quic, and a unit test where the cert is rejected keeps failing.

Wireshark capture of when the ConnectionCertificateValidationComplete function is invoked from inside the PEER_CERTIFICATE_RECEIVED handler:

Notably, the Connection Close frame is sent at the Handshake protection level. This works fine.

When the invocation is moved to be done later from another thread:

Note that this time, the Connection Close is sent at 1RTT level, which seems to get ignored by the server. The connection attempt times out and get's closed by the server (the last line). The capture is attached below, together with sslkeylogfile to decrypt it.

quic-cert-validation.zip

Affected OS

• Windows
• Linux
• macOS
• Other (specify below)

Additional OS information

Reproduced on Windows, didn't try other OSes

MsQuic version

main

Steps taken to reproduce bug

Use ConnectionCertificateValidationComplete to reject certificate from another thread.

Expected behavior

The server-side connection receives TLS alert which was sent by the client.

Actual outcome

The connection attempt times out

Additional details

No response

[rzikm]

I think I know what is going on.

As per RFC 9001

[...] the server's use of 1-RTT keys before the handshake is complete is limited to sending data. A server MUST NOT process incoming 1-RTT protected packets before the TLS handshake is complete.

This means the mistake is in the client sending the CONNECTION_CLOSE on 1-RTT level.

There is also this comment

msquic/src/core/packet_builder.c

Lines 529 to 537 in ae3db5c

	if (SendFlags & (QUIC_CONN_SEND_FLAG_CONNECTION_CLOSE | QUIC_CONN_SEND_FLAG_PING)) {
	//
	// CLOSE or PING is ready to be sent. This is always sent with the
	// current write key.
	//
	// TODO - This logic isn't correct. The peer might not be able to read
	// this key, so the CLOSE frame should be sent at the current and
	// previous encryption level if the handshake hasn't been confirmed.
	//

I don't know how to go about sending the CONNECTION_CLOSE/PING frames on two protection levels, but in this specific scenario the code does not get into that block anyway because of

msquic/src/core/packet_builder.c

Lines 496 to 503 in ae3db5c

	QUIC_ENCRYPT_LEVEL EncryptLevel = QuicKeyTypeToEncryptLevel(KeyType);
	if (EncryptLevel == QUIC_ENCRYPT_LEVEL_1_RTT) {
	//
	// Always allowed to send with 1-RTT.
	//
	*PacketKeyType = QUIC_PACKET_KEY_1_RTT;
	return TRUE;
	}

In the working (synchronous) case, there was data (ACKs) to be sent on the Handshake level, so the CONNECTION_CLOSE piggybacked on top of that.

I had some success with fixing it in PacketBuilder

diff --git a/src/core/packet_builder.c b/src/core/packet_builder.c
index 84d1a2705..52704b836 100644
--- a/src/core/packet_builder.c
+++ b/src/core/packet_builder.c
@@ -479,8 +479,22 @@ QuicPacketBuilderGetPacketTypeAndKeyForControlFrames(
     CXPLAT_DBG_ASSERT(SendFlags != 0);
     QuicSendValidate(&Builder->Connection->Send);

+    QUIC_PACKET_KEY_TYPE MaxKeyType = Connection->Crypto.TlsState.WriteKey;
+
+    if (QuicConnIsClient(Connection) &&
+        !Connection->State.HandshakeConfirmed &&
+        MaxKeyType == QUIC_PACKET_KEY_1_RTT &&
+        (SendFlags & QUIC_CONN_SEND_FLAG_CONNECTION_CLOSE)) {
+        //
+        // Server is not allowed to process 1-RTT packets until the handshake is confirmed and since we are
+        // closing the connection, the handshake is unlikely to complete. Ensure the CONNECTION_CLOSE is sent
+        // in a packet which server can process.
+        //
+        MaxKeyType = QUIC_PACKET_KEY_HANDSHAKE;
+    }
+
     for (QUIC_PACKET_KEY_TYPE KeyType = 0;
-         KeyType <= Connection->Crypto.TlsState.WriteKey;
+         KeyType <= MaxKeyType;
          ++KeyType) {

         if (KeyType == QUIC_PACKET_KEY_0_RTT) {
@@ -538,7 +552,7 @@ QuicPacketBuilderGetPacketTypeAndKeyForControlFrames(
         if (Connection->Crypto.TlsState.WriteKey == QUIC_PACKET_KEY_0_RTT) {
             *PacketKeyType = QUIC_PACKET_KEY_INITIAL;
         } else {
-            *PacketKeyType = Connection->Crypto.TlsState.WriteKey;
+            *PacketKeyType = MaxKeyType;
         }
         return TRUE;
     }

[rzikm]

@nibanks does the fix above sound reasonable? Should I open a PR?

[nibanks]

Go ahead and create the PR. We can go from there. Let's see if all the tests still pass.