Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions srtcore/core.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If all earlier checks are already there, then for every command packet it's checked if it contains at least a payload of 4 bytes (commands that don't use arguments have at least the 4-byte padding). So this part should be checked anyway before even calling this function. If so, this one is redundant.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The mentioned changes is in #3323 - so when this one is merged, the check for "LITE" ACK correct size is not necessary.

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];

Expand Down
45 changes: 45 additions & 0 deletions test/test_fec_rebuilding.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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; }
};
}

Expand Down Expand Up @@ -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);
}
Loading