diff --git a/srtcore/core.cpp b/srtcore/core.cpp index 3d31ba331..8da10938d 100644 --- a/srtcore/core.cpp +++ b/srtcore/core.cpp @@ -8690,6 +8690,23 @@ void srt::CUDT::updateSndLossListOnACK(int32_t ackdata_seqno) void srt::CUDT::processCtrlAck(const CPacket &ctrlpkt, const steady_clock::time_point& currtime) { + // Valid ACK payloads are either LITE (just an ack seqno) or at least SMALL + // (RCVLASTACK + RTT + RTTVAR + BUFFERLEFT = 16 B). Anything else would OOB-read. + const size_t pktlen = ctrlpkt.getLength(); + if (pktlen < (size_t)SEND_LITE_ACK) + { + LOGC(inlog.Warn, log << CONID() << "ACK: payload " << pktlen + << " bytes is shorter than LITE (" << SEND_LITE_ACK << ") - rejecting"); + return; + } + if (pktlen != (size_t)SEND_LITE_ACK && pktlen < ACKD_TOTAL_SIZE_SMALL * ACKD_FIELD_SIZE) + { + LOGC(inlog.Warn, log << CONID() << "ACK: non-lite payload " << pktlen + << " bytes is shorter than SMALL (" << (ACKD_TOTAL_SIZE_SMALL * ACKD_FIELD_SIZE) + << ") - rejecting"); + return; + } + const int32_t* ackdata = (const int32_t*)ctrlpkt.m_pcData; const int32_t ackdata_seqno = ackdata[ACKD_RCVLASTACK]; diff --git a/test/test_fec_rebuilding.cpp b/test/test_fec_rebuilding.cpp index 3910c959b..96281d9c5 100644 --- a/test/test_fec_rebuilding.cpp +++ b/test/test_fec_rebuilding.cpp @@ -119,6 +119,11 @@ namespace srt { { return core->checkApplyFilterConfig(s); } + + // Exposed for unit tests that need to drive private CUDT entry points. + void processCtrlAck(const CPacket& pkt, const sync::steady_clock::time_point& t) { core->processCtrlAck(pkt, t); } + int flowWindowSize() const { return core->m_iFlowWindowSize; } + void setFlowWindowSize(int v) { core->m_iFlowWindowSize = v; } }; } @@ -946,3 +951,43 @@ TEST_F(TestFECRebuilding, Rebuild) EXPECT_EQ(memcmp(skipped.data(), rebuilt.data(), rebuilt.size()), 0); } + +// processCtrlAck has two OOB-read sites for intermediate payload sizes: +// - ackdata[ACKD_RCVLASTACK] (index 0) is read up front, OOB for 0-3 byte payloads; +// - ackdata[ACKD_BUFFERLEFT] (index 3) is read in the slow path, OOB for 5-15 byte +// payloads (the lite-ACK fast path matches exactly 4 bytes). +// Valid payloads are LITE (4 B) or SMALL+ (>=16 B). The guard at the top of the +// handler rejects everything else. +TEST(TestCUDT, AckRejectsIntermediatePayload) +{ + srt::TestInit srtinit; + + CUDTSocket* s1 = NULL; + SRTSOCKET sid1 = CUDT::uglobal().newSocket(&s1); + + TestMockCUDT m1; + m1.core = &s1->core(); + + const int sentinel = 0x5A5A5A5A; + m1.setFlowWindowSize(sentinel); + + CPacket pkt; + pkt.allocate(1500); + + // Fill the payload with bytes that would be plausible ack-seqnos if interpreted + // as int32 (non-negative), so the ackdata_seqno < 0 early return doesn't mask + // the bug for the 0-3 byte cases. + std::memset(pkt.m_pcData, 0x01, 1500); + + const size_t bad_lens[] = { 0, 1, 3, 5, 8, 12, 15 }; + const sync::steady_clock::time_point now = sync::steady_clock::now(); + for (size_t i = 0; i < sizeof(bad_lens) / sizeof(bad_lens[0]); ++i) + { + pkt.setLength(bad_lens[i]); + m1.processCtrlAck(pkt, now); + EXPECT_EQ(m1.flowWindowSize(), sentinel) + << "ACK with payload " << bad_lens[i] << " bytes must not corrupt m_iFlowWindowSize"; + } + + srt_close(sid1); +}