Skip to content
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.

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

Revise RCV message dropping logic #2626

Open
maxsharabayko opened this issue Jan 23, 2023 · 3 comments
Open

Revise RCV message dropping logic #2626

maxsharabayko opened this issue Jan 23, 2023 · 3 comments
Labels
[core] Area: Changes in SRT library core Type: Bug Indicates an unexpected problem or unintended behavior
Milestone

Comments

@maxsharabayko
Copy link
Collaborator

maxsharabayko commented Jan 23, 2023

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.

@maxsharabayko maxsharabayko added Type: Bug Indicates an unexpected problem or unintended behavior [core] Area: Changes in SRT library core labels Jan 23, 2023
@maxsharabayko maxsharabayko added this to the v1.5.2 milestone Jan 23, 2023
@ethouris
Copy link
Collaborator

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.

@maxsharabayko
Copy link
Collaborator Author

Decrypting before inserting also raises CPU burden with packet retransmissions: you may decrypt a packet that's already in the RCV buffer.

@ethouris
Copy link
Collaborator

ethouris commented Jan 24, 2023

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:

  • if it was correct - the rejection is legitimate
  • if it was scewed - the rejection is legitimate either as it being a rogue packet, although you don't notify yet that fact, and you can't do it without decryption

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.

@maxsharabayko maxsharabayko changed the title Dropping a message on decryption failure. Revise RCV message dropping logic Feb 10, 2023
@maxsharabayko maxsharabayko modified the milestones: v1.5.2, v1.6.0 Feb 10, 2023
@maxsharabayko maxsharabayko modified the milestones: v1.5.4, v1.6.0 Jun 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[core] Area: Changes in SRT library core Type: Bug Indicates an unexpected problem or unintended behavior
Projects
None yet
Development

No branches or pull requests

2 participants