search

cisco / libsrtp

Library for SRTP (Secure Realtime Transport Protocol)
Other
1.19k stars 470 forks source link

srtp_policy: add force_zero_roc policy flag #653

Closed ThomasDevoogdt closed 10 months ago

ThomasDevoogdt commented 10 months ago

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.

pabuhler commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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.