[BUG] Reject DROPREQ with reversed seqno range

[mszatmary-netflix]

processCtrlDropReq read dropdata[0] (lo) and dropdata[1] (hi) from the wire without checking seqcmp(lo, hi). When lo > hi, the call to CRcvBuffer::dropMessage(lo, hi, ...) computes
offset_a = seqoff(start, lo) > offset_b = seqoff(start, hi), and the loop walks the circular buffer via incPos() until it wraps around to end_pos -- wiping nearly every entry in the receive buffer. A single malicious DROPREQ packet is a clean DoS primitive.

The analogous LOSSREPORT handler (processCtrlLossReport) already rejects reversed ranges. Mirror that defense here: validate seqcmp(dropdata[0], dropdata[1]) <= 0 before doing any work.

Add ControlPackets.DropReqRejectsReversedRange covering the new check via the existing TestMockControlPackets friend wrapper.

[ethouris]

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.

[mszatmary-netflix]

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

[ethouris]

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.

[mszatmary-netflix]

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.

[ethouris]

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?

[mszatmary-netflix]

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?

[ethouris]

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.