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

openssl3 integration: load legacy provider for rc4 cipher #3457

Merged
merged 5 commits into from
Aug 24, 2022

Conversation

toidiu
Copy link
Contributor

@toidiu toidiu commented Aug 18, 2022

#3082

Description of changes:

openssl3 has a concept of legacy algorithms which it deems outdated and therefore does not load them by default. The way to access these old algorithms is to load the "legacy" provider (discussed in section above). This provider is loaded along side other providers and make legacy algorithms available for use.

Fix1

It is possible to load the "legacy" provider at library initialization via a config file. This is done along side the "default" provider so that other ciphers are also available. This solution is more ideal since it would avoid any extra load time during application execution. It also avoid #ifdef usage for the OSSL_PROVIDER_load(NULL, "legacy"); call, which is ideal for keeping the code clean.

The custom openssl config can be loaded by specifing the OPENSSL_CONF=$(realpath codebuild/openssl.config); env variable as stated in https://github.com/openssl/openssl/blob/master/README-PROVIDERS.md#loading-providers and https://www.openssl.org/docs/manmaster/man5/config.html

Fix2 (recommended)

Why recommended: while we generally try to avoid ifdefs, in this case the library will not work if the provider is not loaded for openssl3. This paired with the fact that its possible for flags to get lost in our build scripts, the preferred solution here is to load the provider from C code if we detect that openssl3 is being used.

RC4 is considered a legacy algorithm and needs to be loaded via a provider. OSSL_PROVIDER_load(NULL, "legacy"); can be used to load the provider globally (NULL). The loading can happen in s2n_stream_cipher_rc4_init, which is called prior to the cipher usage.

Testing:

  • tested that s2n-tls compiled with openssl1.1.1 is not affected
  • tested that specifying config file with openssl3.0 results in test passing
  • tested that both "legacy" and "default" providers are loaded simultaneously
Test passes when config is specified. OPENSSL_CONF=$(realpath codebuild/openssl.config);

s2n-tls/tests/unit/s2n_rc4_test.c ... PASSED     161781 tests
Test fails without specifying config.

FAILED test... 'Error encountered in s2n-tls/crypto/s2n_stream_cipher_rc4.c:74'

Local testing also shows that both providers were loaded correctly.

int da = OSSL_PROVIDER_available(NULL, "default");
int la = OSSL_PROVIDER_available(NULL, "legacy");
printf("------------ def %d leg %d \n", da, la); // ------------ def 1 leg 0

OSSL_PROVIDER_load(NULL, "legacy");

int ada = OSSL_PROVIDER_available(NULL, "default");
int ala = OSSL_PROVIDER_available(NULL, "legacy");
printf("--------adef %d aleg %d \n", ada, ala); // --------adef 1 aleg 1

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

@toidiu toidiu requested review from dougch and a team as code owners August 18, 2022 06:54
@github-actions github-actions bot added the s2n-core team label Aug 18, 2022
@toidiu
Copy link
Contributor Author

toidiu commented Aug 18, 2022

tagging @loqs

@loqs
Copy link

loqs commented Aug 18, 2022

Using just the C code 4a439b9 on top of 8ca9445 with OpenSSL 1.1.1q

/build/s2n-tls/src/s2n-tls/crypto/s2n_stream_cipher_rc4.c:17:10: fatal error: openssl/provider.h: No such file or directory
   17 | #include <openssl/provider.h>
      |          ^~~~~~~~~~~~~~~~~~~~

Which is expected as the header was added with OpenSSL 3.

@toidiu toidiu requested a review from lrstewart August 18, 2022 20:19
Comment on lines 147 to 148
*/
RESULT_ENSURE_NE(OSSL_PROVIDER_load(NULL, "legacy"), NULL);
Copy link
Contributor

Choose a reason for hiding this comment

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

These will return a pretty generic "S2N_ERR_SAFETY". Do you want a more specific error code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should be ok since the operation is only doing one things (loading provider). We dont really have insight into why it might fail to load so I am not sure there is much to add other than "failed to load".

Copy link
Contributor

Choose a reason for hiding this comment

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

But the customer will be calling s2n_init, not s2n_libcrypot_init. s2n_init does a lot of things :)

@toidiu toidiu requested a review from lrstewart August 18, 2022 21:45
@toidiu toidiu enabled auto-merge (squash) August 18, 2022 23:51
@toidiu toidiu requested a review from lrstewart August 22, 2022 17:14
@toidiu toidiu requested a review from maddeleine August 23, 2022 23:15
@toidiu toidiu merged commit fb1e57c into aws:main Aug 24, 2022
@toidiu toidiu mentioned this pull request Aug 24, 2022
10 tasks
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.

4 participants