Skip to content

[BUG] Reject DROPREQ with reversed seqno range #3321

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
mszatmary-netflix wants to merge 2 commits into
base: master
Choose a base branch
from
+48 −0
Open

[BUG] Reject DROPREQ with reversed seqno range #3321

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7336ad1
[BUG] Reject DROPREQ with reversed seqno range
mszatmary-netflix May 19, 2026
0928c7e
Merge branch 'master' into fix/dropreq-reversed-range
May 21, 2026
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
12 changes: 12 additions & 0 deletions srtcore/core.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9343,6 +9343,18 @@ void srt::CUDT::processCtrlDropReq(const CPacket& ctrlpkt)

const int32_t* dropdata = (const int32_t*) ctrlpkt.m_pcData;

// The wire format carries a (lo, hi) seqno range. Reject reversed
// ranges: dropMessage walks a circular buffer from offset(lo) to
// offset(hi)+1 via incPos(); when seqcmp(lo, hi) > 0 the loop wraps
// and clears nearly the entire receive buffer (DoS primitive). The
// analogous LOSSREPORT path already rejects reversed ranges.
if (CSeqNo::seqcmp(dropdata[0], dropdata[1]) > 0)
Copy link
Copy Markdown
Collaborator

@ethouris ethouris May 19, 2026

Choose a reason for hiding this comment

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

This doesn't guarantee to reject any "stupid set of data" received in the UMSG_DROPREQ message.

Check on srt::CRcvBuffer::dropMessage for any real vulnerabilities that any kind of stupid data can cause. This function has important base data to check the incoming message if it provided any sensible data - it may need a fix, but any check like that here is completely pointless.

Copy link
Copy Markdown
Contributor Author

@mszatmary-netflix mszatmary-netflix May 19, 2026

Choose a reason for hiding this comment

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

processCtrlDropReq does more than call dropMessage; after that call it also runs dropFromLossLists(lo, hi) and conditionally writes m_iRcvCurrSeqNo = dropdata[1]. Those don't go through the buffer, so a hostile (lo, hi) slips past any defense that lives only in dropMessage.

The pattern matches processCtrlLossReport. That handler does the identical seqcmp > 0 check at the caller layer

I mirrored an existing codebase convention. If their argument is right, that check is "pointless" too and the LOSSREPORT path needs the same restructuring

Copy link
Copy Markdown
Collaborator

@ethouris ethouris May 19, 2026

Choose a reason for hiding this comment

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

Yes, and these functions do their checks as well.

If the data are sent by a rogue party, all we need to do is to perform only the minimum checks to prevent the code from crashing or the procedures from going haywire. Not from destroying the data or killing the transmission.

Copy link
Copy Markdown
Contributor Author

@mszatmary-netflix mszatmary-netflix May 19, 2026

Choose a reason for hiding this comment

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

all we need to do is to perform only the minimum checks to prevent the code from crashing or the procedures from going haywire.

This is where we disagree. You also need to prevent a malicious party from setting internal state to arbitrary values that they control and are outside any valid range. m_iRcvCurrSeqNo = dropdata[1] for example. Perhaps as part of a longer attack chain.

Copy link
Copy Markdown
Collaborator

@ethouris ethouris May 19, 2026

Choose a reason for hiding this comment

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

Yes, that's what I call the "haywire". Can you state that this field can potentially be set to the value that is outside of the range of sent packets?

Copy link
Copy Markdown
Contributor Author

@mszatmary-netflix mszatmary-netflix May 19, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Can you state that this field can potentially be set to the value that is outside of the range of sent packets?

No, Not potentially, absolutely. But even if I'm wrong. Why take the risk?

Copy link
Copy Markdown
Collaborator

@ethouris ethouris May 20, 2026

Choose a reason for hiding this comment

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

It's not that I'm ignoring the danger, I'm only trying to prevent things from getting worse.

The whole transmission bases on the premise that both sides cooperate with one another. So if one goes rogue, all the other party has to do is "you'll get what you deserve", in the form of stuck connection, inability to transmit, or even breaking the connection. A crazy sequence number in DROPREQ is the same problem as a crazy sequence number in the DATA packet or virtually anything else that uses a sequence number. The only things that we need to prevent are things like:

  • blindly accessing the memory at the location indexed out of bounds
  • leading to allocate memory outside of the configured side limits
  • having the internal structures of the library unable to recover - running into things like inability to process further incoming packets, hangup in the API call, or inability to restore order by simply closing the socket

The field m_iRcvCurrSeqNo keeps the newest sequence number of the incoming packet. Here it's set only if the sequence number is further in the future than the current value (see condition). If the value is further than any packet this party is about to send, because it's rogue, SO BE IT. The sending party KNOWS BETTER, even if it's stupid. The consequences of that will be at best that every packet with an older sequence will be treated as being in the past. The same thing would happen if you once receive a data packet with such a sequence. Of course this will be tried to be inserted into the buffer, which will possibly fail because it's outside the capacity.

If you think this additional verification is worth doing just to limit the risk, I agree completely, but we need to have some basis for the verification, as sequence number are cirtular numbers: they represent a fragment of an infinite axis by being approximated to a fragmet of a wheel. To stay safe, we define the operational range for the numbers on the half of the wheel (see CSeqNo::m_iSeqNoTH). But we need to have the first hook, and the best I think would be the base sequence of the receiver buffer, and the incoming sequence numbers shall not be distant to this value by more than quater of a wheel (CSeqNo::m_iSeqNoTH/2). Then, if this passes, we can compare these two numbers themselves, or even better, the relationship between offset values between them and the first sequence.

As the dropMessage function is called first, I think it is most suited to do this verification, and this can be upgraded by doing the rogue value verification, hence it can also return -1 in case when the values are rogue. The value would have to be remember in a variable of wider scope so that after exiting the lock section it can be verified and further operations would be rejected.

{
LOGC(inlog.Warn, log << CONID() << "rcv DROPREQ rng %"
<< dropdata[0] << " - %" << dropdata[1] << " - REVERSED RANGE, DISCARDING");
return;
}

{
CUniqueSync rcvtscc (m_RecvLock, m_RcvTsbPdCond);
// With both TLPktDrop and TsbPd enabled, a message always consists only of one packet.
Expand Down
36 changes: 36 additions & 0 deletions test/test_control_packets.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,42 @@ TEST(ControlPackets, DropReqRejectsShortPayload)
srt_close(sid1);
}

// processCtrlDropReq must reject DROPREQs whose (lo, hi) seqno range is
// reversed. Otherwise CRcvBuffer::dropMessage walks the circular buffer from
// offset(lo) past offset(hi)+1 via incPos() and wipes nearly every entry --
// a DoS primitive triggerable by a single malicious DROPREQ.
TEST(ControlPackets, DropReqRejectsReversedRange)
{
srt::TestInit srtinit;

CUDTSocket* s = NULL;
SRTSOCKET sid = CUDT::uglobal().newSocket(&s);
ASSERT_NE(sid, SRT_INVALID_SOCK);

TestMockControlPackets m;
m.core = &s->core();

const int32_t sentinel = 1000;
m.setRcvCurrSeqNo(sentinel);

CPacket pkt;
pkt.allocate(8);
int32_t* data = (int32_t*) pkt.m_pcData;
data[0] = 2000; // lo
data[1] = 1500; // hi (seqcmp(lo, hi) > 0)
pkt.setLength(8);
pkt.setControl(UMSG_DROPREQ);

// With the guard, this returns before touching m_pRcvBuffer (NULL on
// an unconnected socket -- would crash if the guard were missing).
m.processCtrlDropReq(pkt);

EXPECT_EQ(m.rcvCurrSeqNo(), sentinel);

pkt.deallocate();
srt_close(sid);
}

// processCtrlLossReport must reject a LOSSREPORT whose final cell carries
// the LOSSDATA_SEQNO_RANGE_FIRST marker but has no HI cell behind it;
// otherwise losslist[i+1] reads past the wire payload (4-byte OOB read of
Expand Down
Loading