Open maxsharabayko opened 1 year ago
It looks like the AEAD check failure makes the sequence number untrusted either. This means also that the packet must be decrypted before inserting into the buffer. This way, both decryption and signature check failures should prevent the packet from even searching a place in the receiver buffer to insert. This unfortunately requires inversion of the order of insertion and decryption.
Decrypting before inserting also raises CPU burden with packet retransmissions: you may decrypt a packet that's already in the RCV buffer.
Yes, I understand that, but I don't think you can make a compromise here without compromising AEAD whatsoever.
What you can save here is that you can first take the sequence number as a good deal and check if this packet has a sequence number that qualifies it for insertion into the buffer, and whether it's insertion-capable.
That is, if the sequence number of the packet - not yet checked - is already covered in the receiver buffer, you can skip the packet anyway because:
Another question is what would happen in case when this is an "out of the blue" sequence, which makes it incapable of inserting into the buffer. Likely you'd have to decrypt and authenticate it to make sure what you are currently facing - the sequence discrepancy on the line, or MITM attack. Distinction of these situations might be important if we decide to do a "forced leak" in the receiver buffer to prevent link breaks in "No room" situations.
Maybe if the signature check can be done without decryption, as something less performance consuming, that could be applied on a packet that is about to be rejected due to (alleged) duplication.
When packet decryption fails, the fields of the packet cannot be trusted, and therefore used for dropping the whole message. Given that SRT only allows AEAD in live streaming configuration at the moment, it is ok to drop a packet from the RCV buffer based on the packet sequence number used to insert it into the buffer. In the live streaming configuration, one packet represents a single message, thus by dropping one packet the whole message is dropped.
This approach should be good enough for v1.5.2 and for live streaming configuration in general. A more universal approach would require removing a packet from the RCV buffer (without dropping( and adding the packet sequence number to the RCV loss list (see PR #2649). Also, the RCV buffer state would have to be reset back to the one before the packet has been inserted. An alternative approach could be to check if the RCV buffer will accept the packet, decrypt and insert it into the buffer only if decryption succeeds. In any case, such changes would require a more noticeable restructuring of the source code. Something for v1.6.0.