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

Use OpenSSL 3.0 library context for MD4/MD5 when in FIPS mode #4617

Open
wants to merge 1 commit into
base: v3.2.x
Choose a base branch
from

Conversation

antoniotorresm
Copy link

When the system is in FIPS mode, certain algorithms are removed from the
system default provider, including MD5 and MD4. These won't be available
even if the default and legacy providers are enabled on the default
context.
To bypass this limitation, we need to create a lib context that will hold both default
and legacy providers. Since the OSSL_LIB_CTX from OpenSSL 3.0 is not
thread-safe, we define it as thread-local.

While this was previously solved by just resorting in internal
implementations, using the libctx instead brings the benefit of
FreeRADIUS being able to work on FIPS systems without the need of
recompiling the package, as well as performance improvements from the
OpenSSL implementations.

src/lib/hmacmd5.c Outdated Show resolved Hide resolved
When the system is in FIPS mode, certain algorithms are removed from the
system default provider, including MD5 and MD4. These won't be available
even if the default and legacy providers are enabled on the default
context.
To bypass this limitation, we need to create a lib context that will hold both default
and legacy providers. Since the OSSL_LIB_CTX from OpenSSL 3.0 is not
thread-safe, we define it as thread-local.

While this was previously solved by just resorting in internal
implementations, using the libctx instead brings the benefit of
FreeRADIUS being able to work on FIPS systems without the need of
recompiling the package, as well as performance improvements from the
OpenSSL implementations.

Signed-off-by: Antonio Torres <[email protected]>
@mcnewton mcnewton added the v3.2.x meta: relates to the v3.2.x branch label Sep 21, 2022
@arr2036
Copy link
Member

arr2036 commented Dec 10, 2022

@alandekok @mcnewton any feedback?

@alandekok
Copy link
Member

I'd prefer a different way. I'll see if I can find time to take a look at it.

@protectivedad
Copy link

Using the internal MD4 removes the need for OpenSSL 3.0 legacy compile. Since MD4 is on it's way out of OpenSSL maybe leave it in Freeradius.

@alandekok
Copy link
Member

@protectivedad I agree. Given the deprecation of MD4 and MD5, it's likely best to just drop the OpenSSL wrappers entirely, and move to using our own implementation.

FreeRADIUS has been around long enough that we went from using our own MD4 / MD5 (because OpenSSL wasn't available) to using OpenSSL, to going back to our own MD4 / MD5.

@arr2036
Copy link
Member

arr2036 commented Nov 23, 2023

Other than that represents a significant performance penalty as our version is not hardware accelerated.

@alandekok
Copy link
Member

@arr2036 If OpenSSL removes MD4 entirely....

@protectivedad
Copy link

protectivedad commented Nov 23, 2023

Other than that represents a significant performance penalty as our version is not hardware accelerated.

I'm updating my router, I'd like it to be as efficient as possible, but how much effect will a non-accelerated MD4 have on the overall communications? I know that it is a very open question maybe like how long is a string? But as a thumb in the air ballpark best guess. Where it is used is it a significant portion, like half the time is done hashing or is it tiny like 1 percent? For my own uses I can compile with legacy or patch it not to use the MD4 (which is what I'm doing). The next question is the same only with MD5. Though I still use the OpenSSL MD5.

@arr2036
Copy link
Member

arr2036 commented Nov 24, 2023

Distinguishing between MD4 and MD5 is irrelevant. Both are provided by the legacy OpenSSL provider, so if OpenSSL gets rid of MD4, it'll probably remove MD5 too, there's no reason to handle MD4 separately from MD5.... The performance of MD5 is critical to processing RADIUS packets. Processing a typical PAP packet requires 1-3 hashes depending on password length and whether Message-Authenticator is included. Signing an Access-Accept is usually between 1-5 hashes depending on whether encrypted attributes and Message-Authenticator is being returned.

@alandekok did you do a flame graph once of RADIUS packet decoding? I think software based MD5 was somewhere between 10-25% of the total CPU time.

@alandekok
Copy link
Member

@arr2036 When doing little more than replying as quickly as possible, MD5 was a noticeable percentage of the CPU time. Not 25%, but maybe 5%. If we add long unlang rules, database lookups, etc., it becomes lost in the noise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v3.2.x meta: relates to the v3.2.x branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants