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: ossl3 legacy provider mem leak #4033

Merged
merged 10 commits into from
Jun 6, 2023

Conversation

jmayclin
Copy link
Contributor

@jmayclin jmayclin commented Jun 3, 2023

Resolved issues:

#4000

Description of changes:

s2n currently leaks memory when used with ossl3. This was originally discovered when I was futzing about with an attempt at cmake support for asan. Turns out it wasn't a false positive. Valgrind also reported the memory leak, which can be seen here

Running s2n_self_talk_tls12_test.c                         ... 
==4682== 5,627 (224 direct, 5,403 indirect) bytes in 1 blocks are definitely lost in loss record 82 of 85
==4682==    at 0x4C31B0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==4682==    by 0x5074F18: CRYPTO_zalloc (mem.c:197)
==4682==    by 0x5080CCE: provider_new (provider_core.c:459)
==4682==    by 0x5080CCE: ossl_provider_new (provider_core.c:570)
==4682==    by 0x507DBFE: OSSL_PROVIDER_try_load (provider.c:26)
==4682==    by 0x151EE6: s2n_rc4_init (s2n_stream_cipher_rc4.c:39)
==4682==    by 0x125DB4: s2n_cipher_suites_init (s2n_cipher_suites.c:975)
==4682==    by 0x123F3D: s2n_init (s2n_init.c:76)
==4682==    by 0x11EE88: main (s2n_self_talk_tls12_test.c:110)
==4682== 

We have a different codepath involving loading a legacy provider to get rc4 support when using ossl3. It looks like that is leaking memory. @lrstewart Had a previous solution for rc4 support that involved just disabling rc4 support for ossl3, which is fine since it's use is explicitly banned by RFC 7465

This document requires that TLS clients and servers never negotiate the use of RC4 cipher suites.

This PR pulls in lrstewarts commits, and adds some integration test configuration that was needed to make them pass CI. It also adds CI jobs for openssl 3.

Testing:

I was able to test locally to confirm that the memory leak was no longer occuring in locally run ASAN jobs. I'll also manually add the new CI jobs to the CI for this PR.
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 Jun 3, 2023
* Pep is complaining about my aligned formatting. I'm of half a mind to
  have a "you don't get to add a formater to CI unless I can easily run
it locally" rule.
@jmayclin jmayclin marked this pull request as ready for review June 5, 2023 16:27
@jmayclin jmayclin requested a review from dougch as a code owner June 5, 2023 16:27
@jmayclin jmayclin requested a review from lrstewart June 5, 2023 16:27
tests/integrationv2/providers.py Outdated Show resolved Hide resolved
@jmayclin jmayclin enabled auto-merge (squash) June 5, 2023 18:43
@jmayclin
Copy link
Contributor Author

jmayclin commented Jun 5, 2023

The successful s2nGeneralBatch with openssl3 can be found in this CI run

@jmayclin jmayclin requested a review from toidiu June 5, 2023 19:12
@jmayclin jmayclin disabled auto-merge June 5, 2023 19:37
@jmayclin jmayclin enabled auto-merge (squash) June 6, 2023 17:17
@jmayclin jmayclin removed the request for review from toidiu June 6, 2023 17:18
@jmayclin jmayclin merged commit 382847c into aws:main Jun 6, 2023
jmayclin added a commit to jmayclin/s2n-tls that referenced this pull request Jun 13, 2023
@jmayclin jmayclin deleted the slindsay-fix-with-disable branch December 22, 2023 02:03
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.

3 participants