Skip to content

Commit

Permalink
Fix Memory Leak from Multiple Decodes of TP
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@nibanks
nibanks committed Oct 10, 2023
1 parent 3226cff commit d364fee
Show file tree
Hide file tree
Showing 6 changed files with 99 additions and 11 deletions.
2 changes: 1 addition & 1 deletion src/core/connection.c
View file
Expand Up @@ -2159,7 +2159,7 @@ QuicConnRecvResumptionTicket(
)
{
BOOLEAN ResumptionAccepted = FALSE;
QUIC_TRANSPORT_PARAMETERS ResumedTP;
QUIC_TRANSPORT_PARAMETERS ResumedTP = {0};
CxPlatZeroMemory(&ResumedTP, sizeof(ResumedTP));
if (QuicConnIsServer(Connection)) {
if (Connection->Crypto.TicketValidationRejecting) {
Expand Down
4 changes: 2 additions & 2 deletions src/core/crypto.c
View file
Expand Up @@ -2244,7 +2244,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
Expand Down Expand Up @@ -2497,7 +2497,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,
Expand Down
4 changes: 2 additions & 2 deletions src/core/crypto.h
View file
Expand Up @@ -386,7 +386,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
Expand Down Expand Up @@ -421,7 +421,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,
Expand Down
41 changes: 40 additions & 1 deletion src/core/crypto_tls.c
View file
Expand Up @@ -348,6 +348,8 @@ QuicCryptoTlsReadExtensions(
} Extension;
*/

BOOLEAN FoundSNI = FALSE;
BOOLEAN FoundALPN = FALSE;
BOOLEAN FoundTransportParameters = FALSE;
while (BufferLength) {
//
Expand Down Expand Up @@ -377,23 +379,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,
Expand All @@ -407,6 +435,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,
Expand Down Expand Up @@ -1254,7 +1290,7 @@ QuicCryptoTlsDecodeTransportParameters( // NOLINT(readability-function-size, goo
_In_reads_(TPLen)
const uint8_t* TPBuf,
_In_ uint16_t TPLen,
_Out_ QUIC_TRANSPORT_PARAMETERS* TransportParams
_Inout_ QUIC_TRANSPORT_PARAMETERS* TransportParams
)
{
BOOLEAN Result = FALSE;
Expand All @@ -1263,6 +1299,9 @@ QuicCryptoTlsDecodeTransportParameters( // NOLINT(readability-function-size, goo

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;
Expand Down
2 changes: 1 addition & 1 deletion src/core/transport_params.h
View file
Expand Up @@ -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
);

//
Expand Down
57 changes: 53 additions & 4 deletions src/core/unittest/TransportParamTest.cpp
View file
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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;
Expand Down

0 comments on commit d364fee

Please sign in to comment.