Properly support null crypto and null auth scenario#757
Conversation
|
This shall be back-ported to 2.5 (which is in debian stable) and newer releases, as they are currently broken in null/null scenario, IIUC. Do I have to make pull requests for those versions? |
Once this PR is landed then either you or I can back port to 2_x_dev in order to create what ever 2.x releases are needed. |
|
@freemangordon I understand that this was a change in behavior, but from my reading of the RFC it does not allow for SRTCP with a null authentication https://datatracker.ietf.org/doc/html/rfc3711#section-9.5 . |
Yes, RFC mandates authentication for SRTCP, however it seems the functionality has been there since the beginning of time. One may consider SRTCP with no crypto/auth to be a fallback to RTCP, no? Also, see https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/-/blob/master/ext/srtp/gstsrtpenc.c?ref_type=heads#L80. Looks like back then somebody didn't read/grok the specs :) .
Agree, but from the application POV it is easier to use a single SRTP/RTP library instead of maintaining separate interfaces for 2 libraries. At least gstreamer and farstream seem to count on libsrtp doing fallback to RTP when encryption and authentication are not required: https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/-/blob/master/ext/srtp/gstsrtpenc.c?ref_type=heads#L385
That's clearly an error to set MKI without providing a key and the library can handle it in various ways (like simply ignore it or error out), but that's another story. So, for us as downstream users, it is much easier if a library does what is told to do, even if specs need to be bent a bit :). After all it is not library's fault if application misuses it. Just think about the amount of changes such a removal would require (I gave 2 examples I know of, I guess pidgin is affected too, and who knows what else). |
ae91e48 to
713f377
Compare
Currently, it is silently assumed that we always have master key provided, even when the policy says we don't use encryption and authentication. This results in streams with such policy to fail. Fix that by properly calculating the expected master key length by taking in account authentication type too. Correctly set expected key length for null crypto policy to 0. Do not assume we have HMAC if the policy is set to not have. Add a test case for null/null policy while at it. Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
713f377 to
23d70f3
Compare
|
@freemangordon I will attempt to review the PR in depth over the weekend. I do agree that it should be fixed mostly as it is a regression. But going forward we may consider making it "harder" or requiring more explicit configure to allow libsrtp to be configured in a totally insecure manor, I think as a security library we have some responsibility to prevent users from accidentally configuring it in an insecure manor. |
|
@freemangordon I made one small change but otherwise I think this is ok. If there are no more comments then will merge shortly. |
Back ported cisco#757 from main.
Properly support null crypto and null auth scenario
Currently, it is silently assumed that we always have master key provided, even when the policy says we don't use encryption and authentication. This results in streams with such policy to fail.
Fix that by properly calculating the expected master key length by taking in account authentication type too. Correctly set expected key length for null crypto policy to 0. Do not assume we have HMAC if the policy is set to not have.
Add a test case for null/null policy while at it.