Convert application close frame to transport close frame during handshake. (#4169)

Author: rzikm

src/core/connection.c

@@ -1390,6 +1390,7 @@ QuicErrorCodeToStatus(
     case QUIC_ERROR_NO_ERROR:                       return QUIC_STATUS_SUCCESS;
     case QUIC_ERROR_CONNECTION_REFUSED:             return QUIC_STATUS_CONNECTION_REFUSED;
     case QUIC_ERROR_PROTOCOL_VIOLATION:             return QUIC_STATUS_PROTOCOL_ERROR;
+    case QUIC_ERROR_APPLICATION_ERROR:
     case QUIC_ERROR_CRYPTO_USER_CANCELED:           return QUIC_STATUS_USER_CANCELED;
     case QUIC_ERROR_CRYPTO_HANDSHAKE_FAILURE:       return QUIC_STATUS_HANDSHAKE_FAILURE;
     case QUIC_ERROR_CRYPTO_NO_APPLICATION_PROTOCOL: return QUIC_STATUS_ALPN_NEG_FAILURE;
@@ -1450,27 +1451,6 @@ QuicConnTryClose(
         }
     }
 
-    if (!ClosedRemotely) {
-
-        if ((Flags & QUIC_CLOSE_APPLICATION) &&
-            Connection->Crypto.TlsState.WriteKey < QUIC_PACKET_KEY_1_RTT) {
-            //
-            // Application close can only happen if we are using 1-RTT keys.
-            // Otherwise we have to send "user_canceled" TLS error code as a
-            // connection close. Overwrite all application provided parameters.
-            //
-            Flags &= ~QUIC_CLOSE_APPLICATION;
-            ErrorCode = QUIC_ERROR_CRYPTO_USER_CANCELED;
-            RemoteReasonPhrase = NULL;
-            RemoteReasonPhraseLength = 0;
-
-            QuicTraceLogConnInfo(
-                CloseUserCanceled,
-                Connection,
-                "Connection close using user canceled error");
-        }
-    }
-
     BOOLEAN ResultQuicStatus = !!(Flags & QUIC_CLOSE_QUIC_STATUS);
 
     BOOLEAN IsFirstCloseForConnection = TRUE;
@@ -3694,6 +3674,15 @@ QuicConnGetKeyOrDeferDatagram(
         return FALSE;
     }
 
+    if (QuicConnIsServer(Connection) && !Connection->State.HandshakeConfirmed &&
+        Packet->KeyType == QUIC_PACKET_KEY_1_RTT) {
+        //
+        // A server MUST NOT process incoming 1-RTT protected packets before the TLS
+        // handshake is complete.
+        //
+        return FALSE;
+    }
+
     _Analysis_assume_(Packet->KeyType >= 0 && Packet->KeyType < QUIC_PACKET_KEY_COUNT);
     if (Connection->Crypto.TlsState.ReadKeys[Packet->KeyType] == NULL) {
         //
@@ -5090,12 +5079,33 @@ QuicConnRecvFrames(
             if (Frame.ApplicationClosed) {
                 Flags |= QUIC_CLOSE_APPLICATION;
             }
-            QuicConnTryClose(
-                Connection,
-                Flags,
-                Frame.ErrorCode,
-                Frame.ReasonPhrase,
-                (uint16_t)Frame.ReasonPhraseLength);
+
+            if (!Frame.ApplicationClosed && Frame.ErrorCode == QUIC_ERROR_APPLICATION_ERROR) {
+                //
+                // The APPLICATION_ERROR transport error should be sent only
+                // when closing the connection before the handshake is
+                // confirmed. In such case, we can also expect peer to send the
+                // application CONNECTION_CLOSE frame in a 1-RTT packet
+                // (presumably also in the same UDP datagram).
+                //
+                // We want to prioritize reporting the application-layer error
+                // code to the application, so we postpone the call to
+                // QuicConnTryClose and check again after processing incoming
+                // datagrams in case it does not arrive.
+                //
+                QuicTraceEvent(
+                    ConnDelayCloseApplicationError,
+                    "[conn][%p] Received APPLICATION_ERROR error, delaying close in expectation of a 1-RTT CONNECTION_CLOSE frame.",
+                    Connection);
+                Connection->State.DelayedApplicationError = TRUE;
+            } else {
+                QuicConnTryClose(
+                    Connection,
+                    Flags,
+                    Frame.ErrorCode,
+                    Frame.ReasonPhrase,
+                    (uint16_t)Frame.ReasonPhraseLength);
+            }
 
             AckEliciting = TRUE;
             Packet->HasNonProbingFrame = TRUE;
@@ -5126,7 +5136,7 @@ QuicConnRecvFrames(
                     HandshakeConfirmedFrame,
                     Connection,
                     "Handshake confirmed (frame)");
-                QuicCryptoHandshakeConfirmed(&Connection->Crypto);
+                QuicCryptoHandshakeConfirmed(&Connection->Crypto, TRUE);
             }
 
             AckEliciting = TRUE;
@@ -5628,9 +5638,7 @@ QuicConnRecvDatagrams(
                     Packet->BufferLength - (uint16_t)(Packet->AvailBuffer - Packet->Buffer);
             }
 
-            if (Connection->Crypto.CertValidationPending ||
-                Connection->Crypto.TicketValidationPending ||
-                !QuicConnRecvHeader(
+            if (!QuicConnRecvHeader(
                     Connection,
                     Packet,
                     Cipher + BatchCount * CXPLAT_HP_SAMPLE_LENGTH)) {
@@ -5737,6 +5745,20 @@ QuicConnRecvDatagrams(
         BatchCount = 0; // cppcheck-suppress unreadVariable; NOLINT
     }
 
+    if (Connection->State.DelayedApplicationError && Connection->CloseStatus == 0) {
+        //
+        // We received transport APPLICATION_ERROR, but didn't receive the expected
+        // CONNECTION_ERROR frame, so close the connection with originally postponed
+        // APPLICATION_ERROR.
+        //
+        QuicConnTryClose(
+            Connection,
+            QUIC_CLOSE_REMOTE | QUIC_CLOSE_SEND_NOTIFICATION,
+            QUIC_ERROR_APPLICATION_ERROR,
+            NULL,
+            (uint16_t)0);
+    }
+
     if (RecvState.ResetIdleTimeout) {
         QuicConnResetIdleTimeout(Connection);
     }

src/core/connection.h

@@ -194,6 +194,12 @@ typedef union QUIC_CONNECTION_STATE {
         //
         BOOLEAN TimestampRecvNegotiated : 1;
 
+        //
+        // Indicates we received APPLICATION_ERROR transport error and are checking also
+        // later packets in case they contain CONNECTION_CLOSE frame with application-layer error.
+        //
+        BOOLEAN DelayedApplicationError : 1;
+
 #ifdef CxPlatVerifierEnabledByAddr
         //
         // The calling app is being verified (app or driver verifier).

src/core/crypto.c

@@ -485,15 +485,18 @@ QuicCryptoOnVersionChange(
 _IRQL_requires_max_(PASSIVE_LEVEL)
 void
 QuicCryptoHandshakeConfirmed(
-    _In_ QUIC_CRYPTO* Crypto
+    _In_ QUIC_CRYPTO* Crypto,
+    _In_ BOOLEAN SignalBinding
     )
 {
     QUIC_CONNECTION* Connection = QuicCryptoGetConnection(Crypto);
     Connection->State.HandshakeConfirmed = TRUE;
 
-    QUIC_PATH* Path = &Connection->Paths[0];
-    CXPLAT_DBG_ASSERT(Path->Binding != NULL);
-    QuicBindingOnConnectionHandshakeConfirmed(Path->Binding, Connection);
+    if (SignalBinding) {
+        QUIC_PATH* Path = &Connection->Paths[0];
+        CXPLAT_DBG_ASSERT(Path->Binding != NULL);
+        QuicBindingOnConnectionHandshakeConfirmed(Path->Binding, Connection);
+    }
 
     QuicCryptoDiscardKeys(Crypto, QUIC_PACKET_KEY_HANDSHAKE);
 }
@@ -1578,7 +1581,13 @@ QuicCryptoProcessTlsCompletion(
                 Connection,
                 "Handshake confirmed (server)");
             QuicSendSetSendFlag(&Connection->Send, QUIC_CONN_SEND_FLAG_HANDSHAKE_DONE);
-            QuicCryptoHandshakeConfirmed(&Connection->Crypto);
+            //
+            // Don't signal handshake confirmed to binding yet, we need to keep
+            // the hash entry around to be able to associate potential Handshake
+            // packets to this connection. The binding will be signaled when the
+            // HANDSHAKE_DONE frame is confirmed received by the client.
+            //
+            QuicCryptoHandshakeConfirmed(&Connection->Crypto, FALSE);
 
             //
             // Take this opportinuty to clean up the client chosen initial CID.
@@ -1720,6 +1729,13 @@ QuicCryptoCustomCertValidationComplete(
             QuicCryptoGetConnection(Crypto),
             "Custom cert validation succeeded");
         QuicCryptoProcessDataComplete(Crypto, Crypto->PendingValidationBufferLength);
+
+        if (QuicRecvBufferHasUnreadData(&Crypto->RecvBuffer)) {
+            //
+            // More data was received while waiting for user to perform the validation.
+            //
+            QuicCryptoProcessData(Crypto, FALSE);
+        }
     } else {
         QuicTraceEvent(
             ConnError,
@@ -1757,6 +1773,13 @@ QuicCryptoCustomTicketValidationComplete(
         //
         Crypto->TicketValidationPending = FALSE;
         QuicCryptoProcessDataComplete(Crypto, Crypto->PendingValidationBufferLength);
+
+        if (QuicRecvBufferHasUnreadData(&Crypto->RecvBuffer)) {
+            //
+            // More data was received while waiting for user to perform the validation.
+            //
+            QuicCryptoProcessData(Crypto, FALSE);
+        }
     } else {
         //
         // Need to rollback status before processing client's initial packet, because outgoing buffer and
@@ -1800,6 +1823,14 @@ QuicCryptoProcessData(
     uint32_t BufferCount = 1;
     QUIC_BUFFER Buffer;
 
+    if (Crypto->CertValidationPending ||
+        (Crypto->TicketValidationPending && !Crypto->TicketValidationRejecting)) {
+        //
+        // An async validation is pending, don't process any more data until it is complete.
+        //
+        return Status;
+    }
+
     if (IsClientInitial) {
         Buffer.Length = 0;
         Buffer.Buffer = NULL;

src/core/crypto.h

@@ -175,7 +175,8 @@ QuicCryptoReset(
 _IRQL_requires_max_(PASSIVE_LEVEL)
 void
 QuicCryptoHandshakeConfirmed(
-    _In_ QUIC_CRYPTO* Crypto
+    _In_ QUIC_CRYPTO* Crypto,
+    _In_ BOOLEAN SignalBinding
     );
 
 //

src/core/frame.h

@@ -69,6 +69,10 @@ extern "C" {
 //
 #define QUIC_ERROR_PROTOCOL_VIOLATION           0xA
 //
+// The application or application protocol caused the connection to be closed.
+//
+#define QUIC_ERROR_APPLICATION_ERROR            0xB
+//
 // An endpoint has received more data in CRYPTO frames than it can buffer.
 //
 #define QUIC_ERROR_CRYPTO_BUFFER_EXCEEDED       0xD

src/core/loss_detection.c

@@ -514,7 +514,7 @@ QuicLossDetectionOnPacketAcknowledged(
             HandshakeConfirmedAck,
             Connection,
             "Handshake confirmed (ack)");
-        QuicCryptoHandshakeConfirmed(&Connection->Crypto);
+        QuicCryptoHandshakeConfirmed(&Connection->Crypto, TRUE);
     }
 
     QUIC_PACKET_SPACE* PacketSpace = Connection->Packets[QUIC_ENCRYPT_LEVEL_1_RTT];
@@ -624,6 +624,10 @@ QuicLossDetectionOnPacketAcknowledged(
                     QUIC_DATAGRAM_SEND_ACKNOWLEDGED_SPURIOUS :
                     QUIC_DATAGRAM_SEND_ACKNOWLEDGED);
             break;
+
+        case QUIC_FRAME_HANDSHAKE_DONE:
+            QuicCryptoHandshakeConfirmed(&Connection->Crypto, TRUE);
+            break;
         }
     }
 

src/core/packet_builder.c

@@ -481,16 +481,34 @@ QuicPacketBuilderGetPacketTypeAndKeyForControlFrames(
 
     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)) {
+    if (SendFlags & (QUIC_CONN_SEND_FLAG_CONNECTION_CLOSE | QUIC_CONN_SEND_FLAG_APPLICATION_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.
+        // CLOSE is ready to be sent. The peer might not be able to read current
+        // highest key, so the CLOSE frame should be sent at the current and
+        // previous encryption levels if the handshake hasn't been confirmed.
         //
-        MaxKeyType = QUIC_PACKET_KEY_HANDSHAKE;
+        if (!Connection->State.HandshakeConfirmed && MaxKeyType >= QUIC_PACKET_KEY_HANDSHAKE) {
+            QUIC_PACKET_KEY_TYPE PreviousKeyType =
+                MaxKeyType == QUIC_PACKET_KEY_1_RTT
+                    ? QUIC_PACKET_KEY_HANDSHAKE
+                    : QUIC_PACKET_KEY_INITIAL;
+
+            if ((Builder->Datagram == NULL || Builder->DatagramLength == 0) &&
+                Connection->Crypto.TlsState.WriteKeys[PreviousKeyType] != NULL) {
+                MaxKeyType = PreviousKeyType; // Use the lower key for the first packet in a datagram.
+            }
+        }
+
+        //
+        // Don't use 0-RTT key for sending CLOSE frames.
+        //
+        if (MaxKeyType == QUIC_PACKET_KEY_0_RTT) {
+            *PacketKeyType = QUIC_PACKET_KEY_INITIAL;
+        } else {
+            *PacketKeyType = MaxKeyType;
+        }
+
+        return TRUE;
     }
 
     for (QUIC_PACKET_KEY_TYPE KeyType = 0;
@@ -540,16 +558,11 @@ QuicPacketBuilderGetPacketTypeAndKeyForControlFrames(
         }
     }
 
-    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.
+    if (SendFlags & QUIC_CONN_SEND_FLAG_PING) {
         //
-        // 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.
+        // PING is ready to be sent. This is always sent with the current write key.
         //
-        if (Connection->Crypto.TlsState.WriteKey == QUIC_PACKET_KEY_0_RTT) {
+        if (MaxKeyType == QUIC_PACKET_KEY_0_RTT) {
             *PacketKeyType = QUIC_PACKET_KEY_INITIAL;
         } else {
             *PacketKeyType = MaxKeyType;

src/core/send.c

@@ -516,8 +516,7 @@ QuicSendWriteFrames(
         }
     }
 
-    if ((Send->SendFlags & QUIC_CONN_SEND_FLAG_CONNECTION_CLOSE) ||
-        ((Send->SendFlags & QUIC_CONN_SEND_FLAG_APPLICATION_CLOSE) && Is1RttEncryptionLevel)) {
+    if (Send->SendFlags & (QUIC_CONN_SEND_FLAG_CONNECTION_CLOSE | QUIC_CONN_SEND_FLAG_APPLICATION_CLOSE)) {
         BOOLEAN IsApplicationClose =
             !!(Send->SendFlags & QUIC_CONN_SEND_FLAG_APPLICATION_CLOSE);
         if (Connection->State.ClosedRemotely) {
@@ -529,12 +528,28 @@ QuicSendWriteFrames(
             IsApplicationClose = FALSE;
         }
 
+        QUIC_VAR_INT CloseErrorCode = Connection->CloseErrorCode;
+        char* CloseReasonPhrase = Connection->CloseReasonPhrase;
+
+        if (IsApplicationClose && ! Is1RttEncryptionLevel) {
+            // 
+            // A CONNECTION_CLOSE of type 0x1d MUST be replaced by a CONNECTION_CLOSE of
+            // type 0x1c when sending the frame in Initial or Handshake packets. Otherwise,
+            // information about the application state might be revealed. Endpoints MUST
+            // clear the value of the Reason Phrase field and SHOULD use the APPLICATION_ERROR
+            // code when converting to a CONNECTION_CLOSE of type 0x1c.
+            //
+            CloseErrorCode = QUIC_ERROR_APPLICATION_ERROR;
+            CloseReasonPhrase = NULL;
+            IsApplicationClose = FALSE;
+        }
+
         QUIC_CONNECTION_CLOSE_EX Frame = {
             IsApplicationClose,
-            Connection->State.ClosedRemotely ? 0 : Connection->CloseErrorCode,
+            CloseErrorCode,
             0, // TODO - Set the FrameType field.
-            Connection->CloseReasonPhrase == NULL ? 0 : strlen(Connection->CloseReasonPhrase),
-            Connection->CloseReasonPhrase
+            CloseReasonPhrase == NULL ? 0 : strlen(CloseReasonPhrase),
+            CloseReasonPhrase
         };
 
         if (QuicConnCloseFrameEncode(
@@ -543,7 +558,15 @@ QuicSendWriteFrames(
                 AvailableBufferLength,
                 Builder->Datagram->Buffer)) {
 
-            Send->SendFlags &= ~(QUIC_CONN_SEND_FLAG_CONNECTION_CLOSE | QUIC_CONN_SEND_FLAG_APPLICATION_CLOSE);
+            //
+            // We potentially send the close frame on multiple protection levels.
+            // We send in increasing encryption level so clear the flag only once
+            // we send on the current protection level.
+            //
+            if (Builder->Key->Type == Connection->Crypto.TlsState.WriteKey) {
+                Send->SendFlags &= ~(QUIC_CONN_SEND_FLAG_CONNECTION_CLOSE | QUIC_CONN_SEND_FLAG_APPLICATION_CLOSE);
+            }
+
             (void)QuicPacketBuilderAddFrame(
                 Builder, IsApplicationClose ? QUIC_FRAME_CONNECTION_CLOSE_1 : QUIC_FRAME_CONNECTION_CLOSE, FALSE);
         } else {

src/core/send.h

@@ -187,6 +187,7 @@ QuicPacketTypeToEncryptLevelV2(
     QUIC_CONN_SEND_FLAG_ACK | \
     QUIC_CONN_SEND_FLAG_CRYPTO | \
     QUIC_CONN_SEND_FLAG_CONNECTION_CLOSE | \
+    QUIC_CONN_SEND_FLAG_APPLICATION_CLOSE | /* not allowed directly, but will be converted to CONNECTION_CLOSE */ \
     QUIC_CONN_SEND_FLAG_PING \
 )