Non compliance with RFC 6188 for AES192CM

[jeannotlapin]

The key derivation function is not compliant with the RFC6188 when dealing with AES192CM.

The problem source is in the function srtp_stream_init_keys, precisely in the kdf_keylen computation.
When using AES192CM HMACSHA180 for example, for both rtp and rtcp policy to make it simple we'll have:

input_keylen , rtp_keylen and rtcp_keylen to 38, rtp_base_key_len being 24 and salt len 14

The current code makes kdf_keylen set to 46, as rtp_keylen is larger than 30 - the default kdf_keylen:
https://github.com/cisco/libsrtp/blob/main/srtp/srtp.c#L1298

But it should stick to 38 for AES192CM because srtp_kdf_init (for generic and wolfssl implementations, openssl-kdf is immune to the problem as it does not use the kdf_keylen parameter) starts by guessing the algorithm variant to use based on the key_len parameter. As we pass 46, it picks AES256 using the extra 8 bytes (set to 0x00) to get enough data for key and salt.

The result is not compliant with the RFC 6188. Passing as master key and salt the values in rfc section 7.4:

master key: 73edc66c4fa15776fb57f9505c171365
50ffda71f3e8e5f1
master salt: c8522f3acd4ce86d5add78edbb11

gives
cipher key : 7af9f9f2cb705d213cce5fa377b290a0183389701878e7d7
cipher salt: 2166ce644b08c8f855708bc2189e

and not the expected

cipher key: 31874736a8f1143870c26e4857d8a5b2
c4a354407faadabb
cipher salt: 2372b82d639b6d8503a47adc0a6c

Using openssl-kdf should comply to the RFC (I didn't test it) but does not seem to be the default case even when using openssl as crypto backend. This may produce non interoperable stream with other libsrtp endpoints that do not use the openssl-kdf version.

A simple fix would be to have a better kdf_keylen computation, assuming the salt is always provided, input_keylen can be:

• AES128CM -> 30 bytes
• AES192CM -> 38 bytes
• AES256CM -> 46 bytes
• AES128GCM-> 28 bytes but we need to 30 to perform CM kdf
• AES256GCM-> 44 bytes same, we need 46
  Only the GCM variants need kdf_keylen correction to get the extra 2 bytes they need.

so something like:

 kdf_keylen = input_keylen;

 if (kdf_keylen == SRTP_AES_GCM_128_KEY_LEN_WSALT || kdf_keylen == SRTP_AES_GCM_256_KEY_LEN_WSALT ) {
	 kdf_keylen += 2;
 }

should fix the problem.

[pabuhler]

Hi, @jeannotlapin thank you for reporting this, I will try to look over it tomorrow and give a reply.

[pabuhler]

@jeannotlapin from looking at the code I agree with you, it seams the original logic only took 128 & 256 in to consideration.
But before making any changes I would like to refactor the code to be able to test the kdf based test vectors in RFC's, that should prove there is a bug and can then verify the fix.

[pabuhler]

@jeannotlapin I have add a fix to the PR #765 based on your suggestion with as little changes as possible.
Are you able to review?
I have also created a follow up task to clean up that code and make it easier to test.

[jeannotlapin]

@pabuhler it is all fine to me for the fix itself. For the test, I think the #ifdef GCM is not needed.

I completely support the idea of being able to test directly the kdf function itself, exposing the srtp_stream_init_keys (or whatever function will actually perform the kdf after you refactor) through the srtp_priv header seems sensible and won't bring any modification to the API.

[pabuhler]

@pabuhler it is all fine to me for the fix itself. For the test, I think the #ifdef GCM is not needed.

I completely support the idea of being able to test directly the kdf function itself, exposing the srtp_stream_init_keys (or whatever function will actually perform the kdf after you refactor) through the srtp_priv header seems sensible and won't bring any modification to the API.

The GCM is miss used in this context, it indicates that the crypto engine is external as there is no internal implementation of AES 192 :( ... this should also be changed at some point.