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

fix: don't enable custom random for openssl fips #5093

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

jmayclin
Copy link
Contributor

@jmayclin jmayclin commented Feb 5, 2025

Resolved issues:

This addresses #5044

Description of changes:

Revert s2n-tls behavior to the behavior before #4878.

Before 4878, compiling s2n-tls with an openssl-fips libcrypto disabled custom rand, independent of whether s2n-tls was set to fips mode.

After 4878, compiling s2n-tls with an openssl-fips libcrypto only disables custom rand if s2n-tls is in fips mode.

Thie PR reverts to the previous behavior.

Call-outs:

I would love some additional thoughts on how to test this :(

I'd like something that isn't too bogged down in the details of libcrypto versions. E.g. "check that there is no memory leak when overriding the random engine". But I don't think that test would pass for standard openssl versions right now.

Testing:

I ran the reproducer provided in the customer issue.

Mainline fails as expected

ubuntu@ip-172-31-49-198:~/workspace/s2n-tls$ sudo docker run --name mainline-image mainline-image /test/build/test_s2n_init

=================================================================
==1==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 216 byte(s) in 1 object(s) allocated from:
    #0 0xee42adf1edb3 in __interceptor_malloc (/lib64/libasan.so.4+0xdbdb3)
    #1 0xee42adbbe35f in CRYPTO_malloc (/lib64/libcrypto.so.10+0x6f35f)
    #2 0xee42adc25d8b in ENGINE_new (/lib64/libcrypto.so.10+0xd6d8b)
    #3 0x40c697 in s2n_rand_init /s2n-tls/utils/s2n_random.c:576
    #4 0x406303 in s2n_init /s2n-tls/utils/s2n_init.c:78
    #5 0x405faf in main /test/main.cpp:5
    #6 0xee42ad6cada3 in __libc_start_main (/lib64/libc.so.6+0x1fda3)
    #7 0x405ec7  (/test/build/test_s2n_init+0x405ec7)

SUMMARY: AddressSanitizer: 216 byte(s) leaked in 1 allocation(s).

Whereas the fix exits with no error

ubuntu@ip-172-31-49-198:~/workspace/s2n-tls$ sudo docker run --name fix-image fix-image /test/build/test_s2n_init

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@github-actions github-actions bot added the s2n-core team label Feb 5, 2025
crypto/s2n_libcrypto.c Outdated Show resolved Hide resolved
crypto/s2n_libcrypto.c Outdated Show resolved Hide resolved
@jmayclin jmayclin marked this pull request as ready for review February 5, 2025 23:00
/* OpenSSL-FIPS never supports custom rand, regardless of mode */
/* OpenSSL non-fips always supports custom rand */
/* other libcryptos never support custom rand */
bool awslc_fips_with_fips_enabled = s2n_libcrypto_is_awslc() && s2n_is_in_fips_mode();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the AWS-LC check necessary? The custom RAND is never used with aws-lc anyway, right?

*
* This method does not check whether the linked libcrypto has fips mode enabled.
*/
bool s2n_libcrypto_is_openssl_fips(void)
Copy link
Contributor

@goatgoose goatgoose Feb 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was also added in #5081, but with a different meaning.

This also won't return true for openssl3-fips (and we probably wouldn't want it to for this use case). To avoid having two similar APIs here, would it maybe make sense to just add a #elif to s2n_supports_custom_rand and return false specifically for OpenSSL 1.0.2-fips (OPENSSL_FIPS is defined)? I'm not sure it's going to be that useful anywhere else anyway.

@jmayclin jmayclin requested a review from goatgoose February 8, 2025 00:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants