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

srtp_policy: add force_zero_roc policy flag #653

Conversation

ThomasDevoogdt
Copy link

I assume that the chance that this gets accepted is near zero (as this is not something that anyone should use), but please let me know what your thoughts are, if not, then simply close this PR.


Note that if the ROC is forced to zero, only 2^16 packets can be sent with a given (master or session) key or a severe security weakness is introduced!

See Section 3.3.1 of RFC 3711:
Each time the RTP sequence number, SEQ, wraps
modulo 2^16, the sender side MUST increment ROC by one, modulo 2^32.

This patch is added to cover an old and wrong srtp implementation where no authentication (and related to that), no ROC increment was done.

So, please don't use it, unless you are for legacy reasons enforced to do so.

Note that if the ROC is forced to zero, only 2^16 packets
can be sent with a given (master or session) key or a severe
security weakness is introduced!

See Section 3.3.1 of RFC 3711:
Each time the RTP sequence number, SEQ, wraps
   modulo 2^16, the sender side MUST increment ROC by one, modulo 2^32.

This patch is added to cover an old and wrong srtp implementation where
no authentication (and related to that), no ROC increment was done.

So, please don't use it, unless you are for legacy reasons enforced
to do so.

Signed-off-by: Thomas Devoogdt <[email protected]>
@pabuhler
Copy link
Member

pabuhler commented Sep 4, 2023

Yeah not really sure about this. Is it an old version of libSRTP or a separate implementation.
If there was a large number of instances where this was needed then maybe, but just to support some older software for a single user of the library?
Can't you implement this outside of libSRTP by monitoring SSRC & seq of packets and just rest libSRTP when there is a wrap?
If you can demonstrate where this would be useful to others then maybe we can discuss it, but as it stands I would suggest we dont merge this and you can just keep a local patch.

@ThomasDevoogdt
Copy link
Author

It's a separate and proprietary implementation with a relative small install base. So I think that I will have to keep it on a local patch.

@ThomasDevoogdt
Copy link
Author

Something asside, srtp_policy_t does not provide an easy way to add flags, this should allow to add/modify behaviour without breaking the ABI. E.g. the allow_repeat_tx could be renamed to policy_flags, where 0x01 is the flag for allow_repeat_tx.

@pabuhler
Copy link
Member

pabuhler commented Sep 6, 2023

yes this is something we are a wear of, will hopefully change in v3, unless there is a pressing need before. In which case your suggestion is an ok idea.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants