Use a full-length key even with null ciphers

[bifurcation]

Right now, srtp.c has the following code to compute the key provided to the SRTP KDF (summarized):

int kdf_keylen = 30;

rtp_keylen = srtp_cipher_get_key_length(session_keys->rtp_cipher);
rtp_base_key_len =
    base_key_length(session_keys->rtp_cipher->type, rtp_keylen);
rtp_salt_len = rtp_keylen - rtp_base_key_len;

if (rtp_keylen > kdf_keylen) {
    kdf_keylen = 46; /* AES-CTR mode is always used for KDF */
}

memset(tmp_key, 0x0, MAX_SRTP_KEY_LEN);
memcpy(tmp_key, key, (rtp_base_key_len + rtp_salt_len));

When a null cipher is used, rtp_keylen, rtp_base_key_len, and rtp_salt_len are all zero. As a result, use of a null cipher results in the use of a constant, all-zero KDF key.

This behavior creates a security vulnerability when the null cipher is used with non-null authentication, e.g., using srtp_crypto_policy_set_null_cipher_hmac_sha1_80). In such cases, an attacker can undermine the authentication by recomputing an auth tag on a tampered packet using the constant auth key that results fro mthe all-zero KDF key.

This behavior is also arguably not spec compliant. RFC 3711 specifies that the master key for null ciphers is to be 128 bits long, and the current behavior ignores the key provided by the application.

This PR makes two changes:

1. Change the memcpy line above so that it copies kdf_keylen bytes rather than zero
2. Add a validation test that verifies that the null cipher produces correct output (in particular, not the output that results from the all-zero key)

The ciphertext in the validation test was produced by the patched library and verified by the draft Rust implementation in the rs branch.

There are two main risks to merging this PR:

1. Binaries produced after this change will be incompatible with older implementations when configured to use a null cipher
2. Since srtp_policy_t does not specify the input key length, if the caller expects zero bytes of key to be read, then the new code might read uninitialized memory or cause a segmentation fault if the caller passed a NULL pointer (which would currently work)

I would argue that these risks should not block merging. The incompatibility risk only applies to cases where a null cipher is used, and those cases are already not spec compliant. The risk of a bad memory read is minimal; the uninitialized memory is just used as input to a KDF that generates keys, so the only effect would be interop failure, not information leakage. And the spec already indicates that callers should be providing 16 byets of key. Firefox, for example, checks the master key size against a minimum.

[paulej]

Only one minor edit.

However, I'm wondering if setting the cipher key length appropriately in the policy object would avoid the need for these changes. As I understand, the cipher's key length is taken from that policy object.

[paulej]

extra ; before :

[pabuhler]

Only one minor edit.

However, I'm wondering if setting the cipher key length appropriately in the policy object would avoid the need for these changes. As I understand, the cipher's key length is taken from that policy object.

This may work but would I think require a change to at least the internal base_key_length() function.

[paulej]

Only one minor edit.
However, I'm wondering if setting the cipher key length appropriately in the policy object would avoid the need for these changes. As I understand, the cipher's key length is taken from that policy object.

This may work but would I think require a change to at least the internal base_key_length() function.

Likely, but that's effectively what the new function is doing

[bifurcation]

Thanks for the suggestion @paulej. I have implemented. To @pabuhler's point, I don't think it requires adjustment to base_key_length, since the key_length parameter to that function is only used for the ICM modes.

[lminiero]

Shouldn't this be SRTP_AES_GCM_256_KEY_LEN_WSALT? SRTP_AES_ICM_256_KEY_LEN_WSALT is 32+14 while SRTP_AES_GCM_256_KEY_LEN_WSALT is 32+12. This is causing SRTP/SRTCP errors in our WebRTC servers when GCM-256 is used.

[paulej]

Yes.

[lminiero]

Ack, thanks, I created a mini-PR in #562 that addresses the typo.