Closed bifurcation closed 2 years ago
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.
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
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.
Right now,
srtp.c
has the following code to compute the key provided to the SRTP KDF (summarized):When a null cipher is used,
rtp_keylen
,rtp_base_key_len
, andrtp_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:
memcpy
line above so that it copieskdf_keylen
bytes rather than zeroThe 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:
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.