From b449e9c481ccd1344b359fc959ae2a5aceb52644 Mon Sep 17 00:00:00 2001 From: Nick Banks Date: Mon, 15 Apr 2024 11:47:52 -0400 Subject: [PATCH] Update SpinQuic to Validate Bytes in Stream Receive aren't Corrupted (#4237) --- src/core/api.c | 10 +++--- src/core/datagram.c | 12 +++++-- src/tools/spin/spinquic.cpp | 69 +++++++++++++++++++++++++++---------- 3 files changed, 66 insertions(+), 25 deletions(-) diff --git a/src/core/api.c b/src/core/api.c index d58a29957b..0dfed4d4d5 100644 --- a/src/core/api.c +++ b/src/core/api.c @@ -1118,6 +1118,12 @@ MsQuicStreamSend( goto Exit; } + // + // From here on, we cannot fail the call because the stream has been queued + // and possibly already started to be processed. + // + Status = QUIC_STATUS_PENDING; + if (SendInline) { CXPLAT_PASSIVE_CODE(); @@ -1140,8 +1146,6 @@ MsQuicStreamSend( "STRM_SEND operation", 0); - Status = QUIC_STATUS_OUT_OF_MEMORY; - // // We failed to alloc the operation we needed to queue, so make sure // to release the ref we took above. @@ -1179,8 +1183,6 @@ MsQuicStreamSend( QuicConnQueueOper(Connection, Oper); } - Status = QUIC_STATUS_PENDING; - Exit: QuicTraceEvent( diff --git a/src/core/datagram.c b/src/core/datagram.c index 76fe956e42..a972a05d6d 100644 --- a/src/core/datagram.c +++ b/src/core/datagram.c @@ -363,11 +363,16 @@ QuicDatagramQueueSend( goto Exit; } + // + // From here on, we cannot fail the call because the stream has been queued + // and possibly already started to be processed. + // + Status = QUIC_STATUS_PENDING; + if (QueueOper) { QUIC_OPERATION* Oper = QuicOperationAlloc(Connection->Worker, QUIC_OPER_TYPE_API_CALL); if (Oper == NULL) { - Status = QUIC_STATUS_OUT_OF_MEMORY; QuicTraceEvent( AllocFailure, "Allocation of '%s' failed. (%llu bytes)", @@ -375,6 +380,9 @@ QuicDatagramQueueSend( 0); goto Exit; } + + // TODO - Kill the connection? + Oper->API_CALL.Context->Type = QUIC_API_TYPE_DATAGRAM_SEND; // @@ -383,8 +391,6 @@ QuicDatagramQueueSend( QuicConnQueueOper(Connection, Oper); } - Status = QUIC_STATUS_PENDING; - Exit: return Status; diff --git a/src/tools/spin/spinquic.cpp b/src/tools/spin/spinquic.cpp index b8d275f5ee..ec5fb5c290 100644 --- a/src/tools/spin/spinquic.cpp +++ b/src/tools/spin/spinquic.cpp @@ -216,8 +216,14 @@ struct SpinQuicGlobals { std::vector ClientConfigurations; QUIC_BUFFER* Alpns {nullptr}; uint32_t AlpnCount {0}; - QUIC_BUFFER Buffers[BufferCount]; - SpinQuicGlobals() { CxPlatZeroMemory(Buffers, sizeof(Buffers)); } + const size_t SendBufferSize { MaxBufferSizes[BufferCount - 1] + UINT8_MAX }; + uint8_t* SendBuffer; + SpinQuicGlobals() { + SendBuffer = new uint8_t[SendBufferSize]; + for (size_t i = 0; i < SendBufferSize; i++) { + SendBuffer[i] = (uint8_t)i; + } + } ~SpinQuicGlobals() { while (ClientConfigurations.size() > 0) { auto Configuration = ClientConfigurations.back(); @@ -239,9 +245,7 @@ struct SpinQuicGlobals { #endif MsQuicClose(MsQuic); } - for (size_t j = 0; j < BufferCount; ++j) { - free(Buffers[j].Buffer); - } + delete [] SendBuffer; } }; @@ -272,6 +276,7 @@ typedef enum { struct SpinQuicStream { struct SpinQuicConnection& Connection; HQUIC Handle; + uint8_t SendOffset {0}; bool Deleting {false}; uint64_t PendingRecvLength {UINT64_MAX}; // UINT64_MAX means no pending receive SpinQuicStream(SpinQuicConnection& Connection, HQUIC Handle = nullptr) : @@ -388,6 +393,9 @@ QUIC_STATUS QUIC_API SpinQuicHandleStreamEvent(HQUIC Stream, void* , QUIC_STREAM } switch (Event->Type) { + case QUIC_STREAM_EVENT_SEND_COMPLETE: + delete (QUIC_BUFFER*)Event->SEND_COMPLETE.ClientContext; + break; case QUIC_STREAM_EVENT_PEER_SEND_SHUTDOWN: MsQuic.StreamShutdown(Stream, (QUIC_STREAM_SHUTDOWN_FLAGS)GetRandom(16), 0); break; @@ -401,6 +409,15 @@ QUIC_STATUS QUIC_API SpinQuicHandleStreamEvent(HQUIC Stream, void* , QUIC_STREAM ctx->PendingRecvLength = UINT64_MAX; // TODO - Add more complex handling break; } + auto Offset = Event->RECEIVE.AbsoluteOffset; + for (uint32_t i = 0; i < Event->RECEIVE.BufferCount; ++i) { + for (uint32_t j = 0; j < Event->RECEIVE.Buffers[i].Length; ++j) { + if (Event->RECEIVE.Buffers[i].Buffer[j] != (uint8_t)(Offset + j)) { + CXPLAT_FRE_ASSERT(FALSE); // Value is corrupt! + } + } + Offset += Event->RECEIVE.Buffers[i].Length; + } int Random = GetRandom(5); std::lock_guard Lock(ctx->Connection.Lock); CXPLAT_DBG_ASSERT(ctx->PendingRecvLength == UINT64_MAX); @@ -477,6 +494,11 @@ QUIC_STATUS QUIC_API SpinQuicHandleConnectionEvent(HQUIC Connection, void* , QUI ctx->AddStream(Event->PEER_STREAM_STARTED.Stream); break; } + case QUIC_CONNECTION_EVENT_DATAGRAM_SEND_STATE_CHANGED: + if (QUIC_DATAGRAM_SEND_STATE_IS_FINAL(Event->DATAGRAM_SEND_STATE_CHANGED.State)) { + delete (QUIC_BUFFER*)Event->DATAGRAM_SEND_STATE_CHANGED.ClientContext; + } + break; default: break; } @@ -769,9 +791,9 @@ void SpinQuicSetRandomConnectionParam(HQUIC Connection, uint16_t ThreadID) break; case QUIC_PARAM_CONN_DATAGRAM_SEND_ENABLED: // uint8_t (BOOLEAN) break; // Get Only - case QUIC_PARAM_CONN_DISABLE_1RTT_ENCRYPTION: // uint8_t (BOOLEAN) - Helper.SetUint8(QUIC_PARAM_CONN_DISABLE_1RTT_ENCRYPTION, (uint8_t)GetRandom(2)); - break; + //case QUIC_PARAM_CONN_DISABLE_1RTT_ENCRYPTION: // uint8_t (BOOLEAN) + // Helper.SetUint8(QUIC_PARAM_CONN_DISABLE_1RTT_ENCRYPTION, (uint8_t)GetRandom(2)); + // break; case QUIC_PARAM_CONN_RESUMPTION_TICKET: // uint8_t[] // TODO break; @@ -978,8 +1000,19 @@ void Spin(Gbs& Gb, LockableVector& Connections, std::vector* Liste std::lock_guard Lock(ctx->Lock); auto Stream = ctx->TryGetStream(); if (Stream == nullptr) continue; - auto Buffer = &Gb.Buffers[GetRandom(BufferCount)]; - MsQuic.StreamSend(Stream, Buffer, 1, (QUIC_SEND_FLAGS)GetRandom(16), nullptr); + auto StreamCtx = SpinQuicStream::Get(Stream); + auto Buffer = new(std::nothrow) QUIC_BUFFER; + if (Buffer) { + const uint32_t Length = MaxBufferSizes[GetRandom(BufferCount)]; + Buffer->Buffer = Gb.SendBuffer + StreamCtx->SendOffset; + Buffer->Length = Length; + if (QUIC_SUCCEEDED( + MsQuic.StreamSend(Stream, Buffer, 1, (QUIC_SEND_FLAGS)GetRandom(16), Buffer))) { + StreamCtx->SendOffset = (uint8_t)(StreamCtx->SendOffset + Length); + } else { + delete Buffer; + } + } } break; } @@ -1105,8 +1138,14 @@ void Spin(Gbs& Gb, LockableVector& Connections, std::vector* Liste case SpinQuicAPICallDatagramSend: { auto Connection = Connections.TryGetRandom(); BAIL_ON_NULL_CONNECTION(Connection); - auto Buffer = &Gb.Buffers[GetRandom(BufferCount)]; - MsQuic.DatagramSend(Connection, Buffer, 1, (QUIC_SEND_FLAGS)GetRandom(8), nullptr); + auto Buffer = new(std::nothrow) QUIC_BUFFER; + if (Buffer) { + Buffer->Buffer = Gb.SendBuffer; + Buffer->Length = MaxBufferSizes[GetRandom(BufferCount)]; + if (QUIC_FAILED(MsQuic.DatagramSend(Connection, Buffer, 1, (QUIC_SEND_FLAGS)GetRandom(8), Buffer))) { + delete Buffer; + } + } break; } case SpinQuicAPICallCompleteTicketValidation: { @@ -1322,12 +1361,6 @@ CXPLAT_THREAD_CALLBACK(RunThread, Context) do { Gbs Gb; - for (size_t j = 0; j < BufferCount; ++j) { - Gb.Buffers[j].Length = MaxBufferSizes[j]; // TODO - Randomize? - Gb.Buffers[j].Buffer = (uint8_t*)malloc(Gb.Buffers[j].Length); - ASSERT_ON_NOT(Gb.Buffers[j].Buffer); - } - #ifdef QUIC_BUILD_STATIC CxPlatLockAcquire(&RunThreadLock); QUIC_STATUS Status = MsQuicOpen2(&Gb.MsQuic);