Fix Memory Leak from Multiple Decodes of TP

Fixes a memory leak in the QUIC transport parameters TLS extension decode code when multiple instances are present or multiple calls to the decode happen.
microsoft · Oct 10, 2023 · c80f35d · c80f35d
1 parent d1cb96a
commit c80f35d
Show file tree

Hide file tree

Showing 6 changed files with 99 additions and 11 deletions.
diff --git a/src/core/connection.c b/src/core/connection.c
@@ -2133,7 +2133,7 @@ QuicConnRecvResumptionTicket(
     )
 {
     BOOLEAN ResumptionAccepted = FALSE;
-    QUIC_TRANSPORT_PARAMETERS ResumedTP;
+    QUIC_TRANSPORT_PARAMETERS ResumedTP = {0};
     CxPlatZeroMemory(&ResumedTP, sizeof(ResumedTP));
     if (QuicConnIsServer(Connection)) {
         const uint8_t* AppData = NULL;

diff --git a/src/core/crypto.c b/src/core/crypto.c
@@ -2172,7 +2172,7 @@ QuicCryptoDecodeServerTicket(
         const uint8_t* Ticket,
     _In_ const uint8_t* AlpnList,
     _In_ uint16_t AlpnListLength,
-    _Out_ QUIC_TRANSPORT_PARAMETERS* DecodedTP,
+    _Inout_ QUIC_TRANSPORT_PARAMETERS* DecodedTP,
     _Outptr_result_buffer_maybenull_(*AppDataLength)
         const uint8_t** AppData,
     _Out_ uint32_t* AppDataLength
@@ -2425,7 +2425,7 @@ QuicCryptoDecodeClientTicket(
     _In_ uint16_t ClientTicketLength,
     _In_reads_bytes_(ClientTicketLength)
         const uint8_t* ClientTicket,
-    _Out_ QUIC_TRANSPORT_PARAMETERS* DecodedTP,
+    _Inout_ QUIC_TRANSPORT_PARAMETERS* DecodedTP,
     _Outptr_result_buffer_maybenull_(*ServerTicketLength)
         uint8_t** ServerTicket,
     _Out_ uint32_t* ServerTicketLength,

diff --git a/src/core/crypto.h b/src/core/crypto.h
@@ -356,7 +356,7 @@ QuicCryptoDecodeServerTicket(
         const uint8_t* Ticket,
     _In_ const uint8_t* AlpnList,
     _In_ uint16_t AlpnListLength,
-    _Out_ QUIC_TRANSPORT_PARAMETERS* DecodedTP,
+    _Inout_ QUIC_TRANSPORT_PARAMETERS* DecodedTP,
     _Outptr_result_buffer_maybenull_(*AppDataLength)
         const uint8_t** AppData,
     _Out_ uint32_t* AppDataLength
@@ -391,7 +391,7 @@ QuicCryptoDecodeClientTicket(
     _In_ uint16_t ClientTicketLength,
     _In_reads_bytes_(ClientTicketLength)
         const uint8_t* ClientTicket,
-    _Out_ QUIC_TRANSPORT_PARAMETERS* DecodedTP,
+    _Inout_ QUIC_TRANSPORT_PARAMETERS* DecodedTP,
     _Outptr_result_buffer_maybenull_(*ServerTicketLength)
         uint8_t** ServerTicket,
     _Out_ uint32_t* ServerTicketLength,

diff --git a/src/core/crypto_tls.c b/src/core/crypto_tls.c
@@ -345,6 +345,8 @@ QuicCryptoTlsReadExtensions(
       } Extension;
     */
 
+    BOOLEAN FoundSNI = FALSE;
+    BOOLEAN FoundALPN = FALSE;
     BOOLEAN FoundTransportParameters = FALSE;
     while (BufferLength) {
         //
@@ -374,23 +376,49 @@ QuicCryptoTlsReadExtensions(
         }
 
         if (ExtType == TlsExt_ServerName) {
+            if (FoundSNI) {
+                QuicTraceEvent(
+                    ConnError,
+                    "[conn][%p] ERROR, %s.",
+                    Connection,
+                    "Duplicate SNI extension present");
+                return QUIC_STATUS_INVALID_PARAMETER;
+            }
             QUIC_STATUS Status =
                 QuicCryptoTlsReadSniExtension(
                     Connection, Buffer, ExtLen, Info);
             if (QUIC_FAILED(Status)) {
                 return Status;
             }
+            FoundSNI = TRUE;
 
         } else if (ExtType == TlsExt_AppProtocolNegotiation) {
+            if (FoundALPN) {
+                QuicTraceEvent(
+                    ConnError,
+                    "[conn][%p] ERROR, %s.",
+                    Connection,
+                    "Duplicate ALPN extension present");
+                return QUIC_STATUS_INVALID_PARAMETER;
+            }
             QUIC_STATUS Status =
                 QuicCryptoTlsReadAlpnExtension(
                     Connection, Buffer, ExtLen, Info);
             if (QUIC_FAILED(Status)) {
                 return Status;
             }
+            FoundALPN = TRUE;
 
         } else if (Connection->Stats.QuicVersion != QUIC_VERSION_DRAFT_29) {
             if (ExtType == TLS_EXTENSION_TYPE_QUIC_TRANSPORT_PARAMETERS) {
+                if (FoundTransportParameters) {
+                    QuicTraceEvent(
+                        ConnError,
+                        "[conn][%p] ERROR, %s.",
+                        Connection,
+                        "Duplicate QUIC TP extension present");
+                    return QUIC_STATUS_INVALID_PARAMETER;
+                }
                 if (!QuicCryptoTlsDecodeTransportParameters(
                         Connection,
                         FALSE,
@@ -404,6 +432,14 @@ QuicCryptoTlsReadExtensions(
 
         } else {
             if (ExtType == TLS_EXTENSION_TYPE_QUIC_TRANSPORT_PARAMETERS_DRAFT) {
+                if (FoundTransportParameters) {
+                    QuicTraceEvent(
+                        ConnError,
+                        "[conn][%p] ERROR, %s.",
+                        Connection,
+                        "Duplicate QUIC (draft) TP extension present");
+                    return QUIC_STATUS_INVALID_PARAMETER;
+                }
                 if (!QuicCryptoTlsDecodeTransportParameters(
                         Connection,
                         FALSE,
@@ -1176,7 +1212,7 @@ QuicCryptoTlsDecodeTransportParameters(
     _In_reads_(TPLen)
         const uint8_t* TPBuf,
     _In_ uint16_t TPLen,
-    _Out_ QUIC_TRANSPORT_PARAMETERS* TransportParams
+    _Inout_ QUIC_TRANSPORT_PARAMETERS* TransportParams
     )
 {
     BOOLEAN Result = FALSE;
@@ -1185,6 +1221,9 @@ QuicCryptoTlsDecodeTransportParameters(
 
     UNREFERENCED_PARAMETER(Connection);
 
+    if (TransportParams->VersionInfo) {
+        CXPLAT_FREE(TransportParams->VersionInfo, QUIC_POOL_VERSION_INFO);
+    }
     CxPlatZeroMemory(TransportParams, sizeof(QUIC_TRANSPORT_PARAMETERS));
     TransportParams->MaxUdpPayloadSize = QUIC_TP_MAX_PACKET_SIZE_DEFAULT;
     TransportParams->AckDelayExponent = QUIC_TP_ACK_DELAY_EXPONENT_DEFAULT;

diff --git a/src/core/transport_params.h b/src/core/transport_params.h
@@ -180,7 +180,7 @@ QuicCryptoTlsDecodeTransportParameters(
     _In_reads_(TPLen)
         const uint8_t* TPBuf,
     _In_ uint16_t TPLen,
-    _Out_ QUIC_TRANSPORT_PARAMETERS* TransportParams
+    _Inout_ QUIC_TRANSPORT_PARAMETERS* TransportParams
     );
 
 //

diff --git a/src/core/unittest/TransportParamTest.cpp b/src/core/unittest/TransportParamTest.cpp
@@ -82,21 +82,49 @@ void EncodeDecodeAndCompare(
     auto TPBuffer = Buffer + CxPlatTlsTPHeaderSize;
     uint16_t TPBufferLength = (uint16_t)(BufferLength - CxPlatTlsTPHeaderSize);
 
-    QUIC_TRANSPORT_PARAMETERS Decoded;
+    QUIC_TRANSPORT_PARAMETERS Decoded = {0};
+    TransportParametersScope TPScope(&Decoded);
     BOOLEAN DecodedSuccessfully =
         QuicCryptoTlsDecodeTransportParameters(
             &JunkConnection, IsServer, TPBuffer, TPBufferLength, &Decoded);
 
     CXPLAT_FREE(Buffer, QUIC_POOL_TLS_TRANSPARAMS);
-    TransportParametersScope TPScope(&Decoded);
-
     ASSERT_EQ(ShouldDecodeSuccessfully, DecodedSuccessfully);
-
     if (ShouldDecodeSuccessfully) {
         CompareTransportParams(Original, &Decoded, IsServer);
     }
 }
 
+void DecodeTwice(
+    _In_ const QUIC_TRANSPORT_PARAMETERS* Original,
+    _In_ bool IsServer = false
+    )
+{
+    uint32_t BufferLength;
+    auto Buffer =
+        QuicCryptoTlsEncodeTransportParameters(
+            &JunkConnection, IsServer, Original, NULL, &BufferLength);
+    ASSERT_NE(nullptr, Buffer);
+
+    ASSERT_TRUE(UINT16_MAX >= (BufferLength - CxPlatTlsTPHeaderSize));
+
+    auto TPBuffer = Buffer + CxPlatTlsTPHeaderSize;
+    uint16_t TPBufferLength = (uint16_t)(BufferLength - CxPlatTlsTPHeaderSize);
+
+    QUIC_TRANSPORT_PARAMETERS Decoded = {0};
+    TransportParametersScope TPScope(&Decoded);
+    BOOLEAN DecodedSuccessfullyOnce =
+        QuicCryptoTlsDecodeTransportParameters(
+            &JunkConnection, IsServer, TPBuffer, TPBufferLength, &Decoded);
+    BOOLEAN DecodedSuccessfullyTwice =
+        QuicCryptoTlsDecodeTransportParameters(
+            &JunkConnection, IsServer, TPBuffer, TPBufferLength, &Decoded);
+
+    CXPLAT_FREE(Buffer, QUIC_POOL_TLS_TRANSPARAMS);
+    ASSERT_TRUE(DecodedSuccessfullyOnce);
+    ASSERT_TRUE(DecodedSuccessfullyTwice);
+}
+
 TEST(TransportParamTest, EmptyClient)
 {
     QUIC_TRANSPORT_PARAMETERS Original;
@@ -120,6 +148,15 @@ TEST(TransportParamTest, Preset1)
     EncodeDecodeAndCompare(&Original);
 }
 
+TEST(TransportParamTest, Preset1DecodeTwice)
+{
+    QUIC_TRANSPORT_PARAMETERS Original;
+    CxPlatZeroMemory(&Original, sizeof(Original));
+    Original.Flags |= QUIC_TP_FLAG_IDLE_TIMEOUT;
+    Original.IdleTimeout = 100000;
+    DecodeTwice(&Original);
+}
+
 TEST(TransportParamTest, ZeroTP)
 {
     QUIC_TRANSPORT_PARAMETERS OriginalTP;
@@ -149,6 +186,18 @@ TEST(TransportParamTest, VersionNegotiationExtension)
     EncodeDecodeAndCompare(&OriginalTP);
 }
 
+TEST(TransportParamTest, VersionNegotiationExtensionDecodeTwice)
+{
+    QUIC_TRANSPORT_PARAMETERS OriginalTP;
+    CxPlatZeroMemory(&OriginalTP, sizeof(OriginalTP));
+    uint8_t VerInfo[21];
+    OriginalTP.VersionInfo = VerInfo;
+    OriginalTP.VersionInfoLength = sizeof(VerInfo);
+    OriginalTP.Flags = QUIC_TP_FLAG_VERSION_NEGOTIATION;
+
+    DecodeTwice(&OriginalTP);
+}
+
 TEST(TransportParamTest, CibirEncodingOne)
 {
     QUIC_TRANSPORT_PARAMETERS OriginalTP;