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 custom library context for rc4 instead of global default context #3980

Merged
merged 2 commits into from
May 5, 2023

Conversation

lrstewart
Copy link
Contributor

@lrstewart lrstewart commented May 4, 2023

Description of changes:

We forgot to add Openssl-3.0 to the unit tests, so missed that the newest unit test broke with Openssl-3.0.

The problem is that the default openssl context is global. So when we manually loaded providers so that we could access the legacy RC4 algorithm, that change affected any other uses of openssl within the same process. When we then unloaded those providers, we unloaded them for everyone, and openssl basically stopped working because it had no algorithms.

So instead, I now load the legacy provider required for RC4 into a custom openssl context so that we can retrieve the RC4 algorithm.

Callouts

I put the call to s2n_rc4_init in s2n_cipher_suites_init instead of the base s2n_init method because we have to setup RC4 before we can setup any cipher suites that use RC4. I thought it might be clearer. But maybe I should just put it in s2n_init with a comment?

Testing:

The failing s2n_random_test now passes. I also added some sanity checks to s2n_rc4_test to make sure we're still using RC4 where expected.
I also double checked that the Openssl-3.0 test was actually running (s2nUnitOpenSSL3GCC9 in GeneralBatch).

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 May 4, 2023
@lrstewart lrstewart force-pushed the rc4_fix branch 2 times, most recently from d4a0db0 to 6841246 Compare May 4, 2023 19:37
@lrstewart lrstewart marked this pull request as ready for review May 4, 2023 20:50
@lrstewart lrstewart requested review from toidiu and jmayclin May 4, 2023 20:50
Copy link
Contributor Author

@lrstewart lrstewart left a comment

Choose a reason for hiding this comment

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

I'm wondering if we should just remove the complexity, accept that RC4 is incredibly deprecated, and just drop support if built with Openssl-3.0. I can't imagine that's going to break someone.

That would be this as an alternative: #3982

@toidiu toidiu merged commit fd330d0 into aws:main May 5, 2023
22 checks passed
dougch pushed a commit to dougch/s2n-tls that referenced this pull request May 31, 2023
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.

None yet

3 participants