search

cisco / libsrtp

Library for SRTP (Secure Realtime Transport Protocol)
Other
1.19k stars 472 forks source link

Openssl EVP MAC #602

Closed pabuhler closed 1 year ago

pabuhler commented 2 years ago

This is a fix for issue #599 .

With OpenSSL 3 the HAMC CTX api has been deprecated and EVP MAC should be used instead. This change adds a compile time check on OpeenSSL version and will use the new api if the version is greater than or equal to 3 .

As part of this change cmake CI build now has warnings as errors in order to catch the deprecated api warning. There will be a follow up PR that also adds warnings as errors to windows as well as enabling more warnings.

pabuhler commented 2 years ago

There seams to be a difference between OpenSSL 3.0.2 and OpenSSL 3.0.3-dev when it comes to EVP_MAC_CTX reuse. The changes here work with 3.0.3 but not 3.0.2, need to investigate why.

pabuhler commented 2 years ago

https://github.com/openssl/openssl/issues/17811 --- looks like need to either store the key or else only support 3.0.3 and greater :( . @bifurcation thoughts ?

JonathanLennox commented 2 years ago

openssl/openssl#17811 --- looks like need to either store the key or else only support 3.0.3 and greater :( . @bifurcation thoughts ?

Storing the key and re-applying it will likely be a noticeable performance regression vs. using the HMAC_CTX APIs, since re-applying the key re-computes the HMAC's i_ctx and o_ctx.

pabuhler commented 2 years ago

@JonathanLennox in that case I would think we need to wait for 3.0.3 release, and for now have the code behind a configure flag or something similar with a min version check. Far more work than was planned.

JonathanLennox commented 2 years ago

Couldn't the ifdef that sets SRTP_OSSL_USE_EVP_MAC be conditional on being OpenSSL 3.0.3 or later, and just use the HMAC-flavored functions for OpenSSL <= 3.0.2?

You'd have to remove the -Werror addition, but everything else should be fine. (Alternately it may be possible to set OPENSSL_NO_DEPRECATED_3_0 for OpenSSL versions between 3 and 3.0.2, but I'm quite quite clear on how that works.)

Unless you're worried about building with 3.0.3+, and then running with 3.0.2-, which I guess could be possible.

pabuhler commented 2 years ago

Unless you're worried about building with 3.0.3+, and then running with 3.0.2-, which I guess could be possible.

I think this is the biggest problem, do not wish to have runtime checks on a patch version. I think for now will add an ENABLE_OPENSSL3 compile flag and if that is set verify the version is 3.0.3+ for compiling. The thought is if this is set then it is the responsibility of the developer to run against the right version. Such behavior change in OpenSSL really should be with a minor version change so we could just require 3.1 and be happy.

JonathanLennox commented 2 years ago

The other possibility - which I think should work on 3.0.2 without a performance penalty, though I haven't tried it - would be to have srtp_hmac_alloc store a "pristine" MAC ctx which has had its key initialized, and then in srtp_hmac_start construct a "working" MAC ctx that's used for the actual calculations, using EVP_MAC_CTX_dup().

pabuhler commented 2 years ago

@JonathanLennox I also read that using dup was a possibility with little impact. I will try it out

pabuhler commented 2 years ago

@JonathanLennox I added the dup function but the performance hit is noticeable. It will only use this based on a runtime check of the version otherwise it will reinit the CTX. Not sure if all this was worth it to avoid compiler warnings.

JonathanLennox commented 2 years ago

@JonathanLennox I added the dup function but the performance hit is noticeable. It will only use this based on a runtime check of the version otherwise it will reinit the CTX.

I assume dup is still faster than rekey, though.

Not sure if all this was worth it to avoid compiler warnings.

You'll notice I said that in https://github.com/cisco/libsrtp/issues/599. 🙂

pabuhler commented 2 years ago

@JonathanLennox, I think the performance hit is great enough to not want to use dup and since 3.0.3 is not yet released there is no point in this commit yet. For now will just avoid the warning. Was still a good learning exercise.

pabuhler commented 1 year ago

With git hub going to Ubuntu 22.04 as latest OpensSSL3 will become the default, so maybe time to get this in. OpenSSL3 is now at 3.0.7 which includes the fix to reintialize a EVP MAC context so the chance that libsrtp will be running against 3.0.2 or less and getting the performance hit of doing a context dup is very small. @bifurcation & @JonathanLennox could you guys chime in on this and review the code. I have reset this branch to the tip of mina and reapplied the 3 commits needed.