Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Detect presence of GCM (and ICM-192) at runtime, not compile time #387

Open
JonathanLennox opened this issue Nov 17, 2017 · 4 comments
Open

Comments

@JonathanLennox
Copy link

Even if LibSRTP is compiled without OpenSSL support, it should be possible to use srtp_replace_cipher_type to install SRTP_AES_GCM_128, SRTP_AES_GCM_256, or SRTP_AES_ICM_192 ciphers and have things work.

This came up in my discussions with @nils-ohlmeier; he wants to get libsrtp to use NSS (Mozilla's native crypto library) in Firefox, since they don't use OpenSSL. I suggested that using srtp_replace_cipher_type would likely be more maintainable than contributing back NSS patches to the mainline project. But if he wants to install crypto modules for all the modes, libsrtp would need to support using those modes even if when doesn't have native crypto support for them.

My expectation would be that if you tried to use a mode for which you hadn't installed a cipher, srtp_add_stream would fail. The current code will indeed do this, failing with srtp_err_status_fail, but a better error status could be added. (Maybe srtp_err_status_bad_cipher?)

@pabuhler
Copy link
Member

Have you tried this? From just looking at the code it should just work ... ie srtp_crypto_kernel_do_load_cipher_type will add a new cipher if needed. I have not tested it and adding a test would be a good idea. Changing the error code to srtp_err_status_bad_cipher sounds reasonable.

@JonathanLennox
Copy link
Author

I agree that srtp_crypto_kernel_do_load_cipher_type will do the right thing; the problem is that a bunch of the code in srtp.c to set up the GCM modes is conditionally compiled under #ifdef OPENSSL.

This might just be as simple as removing the #ifdef's, but there's something going on with OPENSSL_KDF that I don't quite understand.

@JonathanLennox
Copy link
Author

One other thing it'd be nice to have -- right now srtp_replace_cipher_type runs the replaced cipher's self-test functions for ciphers it's replacing, but obviously in non-OpenSSL mode we don't have self tests for the ciphers that aren't defined. It'd be good to have them, to verify that an installed cipher was really what it claimed to be.

@pabuhler
Copy link
Member

Looking at the #ifdef OPENSSL in srtp.c, I don't see anything that would stop being able to add a GCM cipher at run time today. The only down side is that you would have to manually set up the policy. Could remove these checks but if I remember correctly people have used the presences of the srtp_crypto_policy_set_aes_gcm_XXX symbols at run time to check if GCM is supported, not the best approach but probably should not break it. In the future we could add a get_supported_ciphers function to enable a run time check on what is available.
When it comes to OPENSSL_KDF I dont think anyone is using that. If it is required that the entire kdf is replaceable at run time then will probably need to modify that code a bit.

Separating cipher tests from cipher implementation is an interesting idea, and some thing we could look at.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants