From ec6713fd80f8e35e61ae9197849888434500886f Mon Sep 17 00:00:00 2001 From: Cameron Gutman Date: Tue, 12 Sep 2023 22:44:45 -0500 Subject: [PATCH] Improve checks for runt video packets Since we always allocate fixed size these aren't likely to be exploitable, but we ought to fix them anyway. Worst case, we will just read some garbage and generate corrupt video output. --- src/RtpVideoQueue.c | 5 +++++ src/VideoDepacketizer.c | 29 ++++++++++++++++++++--------- src/VideoStream.c | 5 +++++ 3 files changed, 30 insertions(+), 9 deletions(-) diff --git a/src/RtpVideoQueue.c b/src/RtpVideoQueue.c index 56d92e0..419070d 100644 --- a/src/RtpVideoQueue.c +++ b/src/RtpVideoQueue.c @@ -539,6 +539,11 @@ int RtpvAddPacket(PRTP_VIDEO_QUEUE queue, PRTP_PACKET packet, int length, PRTPV_ dataOffset += 4; // 2 additional fields } + if (length < dataOffset + (int)sizeof(NV_VIDEO_PACKET)) { + // Reject packets that are too small to fit a NV_VIDEO_PACKET header + return RTPF_RET_REJECTED; + } + PNV_VIDEO_PACKET nvPacket = (PNV_VIDEO_PACKET)(((char*)packet) + dataOffset); nvPacket->streamPacketIndex = LE32(nvPacket->streamPacketIndex); diff --git a/src/VideoDepacketizer.c b/src/VideoDepacketizer.c index 40295e0..0ef8810 100644 --- a/src/VideoDepacketizer.c +++ b/src/VideoDepacketizer.c @@ -152,18 +152,19 @@ void destroyVideoDepacketizer(void) { cleanupFrameState(); } +// NB: This function also ensures an additional byte for the NALU type exists after the start sequence static bool getAnnexBStartSequence(PBUFFER_DESC current, PBUFFER_DESC startSeq) { // We must not get called for other codecs LC_ASSERT(NegotiatedVideoFormat & (VIDEO_FORMAT_MASK_H264 | VIDEO_FORMAT_MASK_H265)); - if (current->length < 3) { + if (current->length <= 3) { return false; } if (current->data[current->offset] == 0 && current->data[current->offset + 1] == 0) { if (current->data[current->offset + 2] == 0) { - if (current->length >= 4 && current->data[current->offset + 3] == 1) { + if (current->length > 4 && current->data[current->offset + 3] == 1) { // Frame start if (startSeq != NULL) { startSeq->data = current->data; @@ -842,9 +843,11 @@ static void processRtpPayload(PNV_VIDEO_PACKET videoPacket, int length, // If this is the first packet, skip the frame header (if one exists) uint32_t frameHeaderSize; - if (firstPacket) { + LC_ASSERT(currentPos.length > 0); + if (firstPacket && currentPos.length > 0) { // Parse the frame type from the header - if (APP_VERSION_AT_LEAST(7, 1, 350)) { + LC_ASSERT(currentPos.length >= 4); + if (APP_VERSION_AT_LEAST(7, 1, 350) && currentPos.length >= 4) { switch (currentPos.data[currentPos.offset + 3]) { case 1: // Normal P-frame break; @@ -885,7 +888,8 @@ static void processRtpPayload(PNV_VIDEO_PACKET videoPacket, int length, } // Sunshine can provide host processing latency of the frame - if (IS_SUNSHINE()) { + LC_ASSERT(currentPos.length >= 3); + if (IS_SUNSHINE() && currentPos.length >= 3) { BYTE_BUFFER bb; BbInitializeWrappedBuffer(&bb, currentPos.data, currentPos.offset + 1, 2, BYTE_ORDER_LITTLE); BbGet16(&bb, &frameHostProcessingLatency); @@ -893,7 +897,8 @@ static void processRtpPayload(PNV_VIDEO_PACKET videoPacket, int length, // Codecs like H.264 and HEVC handle the FEC trailing zero padding just fine, but other // codecs need the exact length encoded separately. - if (!(NegotiatedVideoFormat & (VIDEO_FORMAT_MASK_H264 | VIDEO_FORMAT_MASK_H265))) { + LC_ASSERT(currentPos.length >= 6); + if (!(NegotiatedVideoFormat & (VIDEO_FORMAT_MASK_H264 | VIDEO_FORMAT_MASK_H265)) && currentPos.length >= 6) { BYTE_BUFFER bb; BbInitializeWrappedBuffer(&bb, currentPos.data, currentPos.offset + 4, 2, BYTE_ORDER_LITTLE); BbGet16(&bb, &lastPacketPayloadLength); @@ -952,9 +957,12 @@ static void processRtpPayload(PNV_VIDEO_PACKET videoPacket, int length, frameHeaderSize = 0; } - // Skip past the frame header - currentPos.offset += frameHeaderSize; - currentPos.length -= frameHeaderSize; + LC_ASSERT(currentPos.length >= frameHeaderSize); + if (currentPos.length >= frameHeaderSize) { + // Skip past the frame header + currentPos.offset += frameHeaderSize; + currentPos.length -= frameHeaderSize; + } // We only parse H.264 and HEVC at the NALU level if (NegotiatedVideoFormat & (VIDEO_FORMAT_MASK_H264 | VIDEO_FORMAT_MASK_H265)) { @@ -1153,6 +1161,9 @@ void queueRtpPacket(PRTPV_QUEUE_ENTRY queueEntryPtr) { dataOffset += 4; // 2 additional fields } + // The packet length was validated by the RtpVideoQueue + LC_ASSERT(queueEntry.length >= dataOffset + (int)sizeof(NV_VIDEO_PACKET)); + // Reuse the memory reserved for the RTPFEC_QUEUE_ENTRY to store the LENTRY_INTERNAL // now that we're in the depacketizer. We saved a copy of the real FEC queue entry // on the stack here so we can safely modify this memory in place. diff --git a/src/VideoStream.c b/src/VideoStream.c index 1128dfb..0429237 100644 --- a/src/VideoStream.c +++ b/src/VideoStream.c @@ -151,6 +151,11 @@ static void VideoReceiveThreadProc(void* context) { } } + if (err < (int)sizeof(RTP_PACKET)) { + // Runt packet + continue; + } + // Convert fields to host byte-order packet = (PRTP_PACKET)&buffer[0]; packet->sequenceNumber = BE16(packet->sequenceNumber);