From 382847c565c5a0b1e2ae3f556fcad639a6486efc Mon Sep 17 00:00:00 2001 From: James Mayclin Date: Tue, 6 Jun 2023 12:42:27 -0700 Subject: [PATCH] fix: ossl3 legacy provider mem leak (#4033) We use a legacy provider to support RC4 when using openssl3. This approach was leaking memory. As an easy way of solving it, this commit uses Lindsay's approach of just disabling RC4 (so no legacy provider) when using openssl3. Co-authored-by: Lindsay Stewart --- codebuild/spec/buildspec_omnibus.yml | 23 +++++++++ crypto/s2n_stream_cipher_rc4.c | 72 +++++++--------------------- docs/USAGE-GUIDE.md | 6 +++ tests/integrationv2/providers.py | 24 ++++++---- tests/unit/s2n_rc4_test.c | 29 ++++++----- tls/s2n_cipher_suites.c | 11 +---- 6 files changed, 76 insertions(+), 89 deletions(-) diff --git a/codebuild/spec/buildspec_omnibus.yml b/codebuild/spec/buildspec_omnibus.yml index d6dda6d0c46..88d8eaea5be 100644 --- a/codebuild/spec/buildspec_omnibus.yml +++ b/codebuild/spec/buildspec_omnibus.yml @@ -48,6 +48,17 @@ batch: variables: TESTS: sidetrail + - identifier: s2nValgrindOpenSSL3 + buildspec: codebuild/spec/buildspec_ubuntu.yml + env: + privileged-mode: true + compute-type: BUILD_GENERAL1_LARGE + image: 024603541914.dkr.ecr.us-west-2.amazonaws.com/docker:ubuntu18codebuild + variables: + TESTS: valgrind + GCC_VERSION: 9 + S2N_LIBCRYPTO: openssl-3.0 + BUILD_S2N: true - identifier: s2nValgrindOpenSSL102Gcc6Fips buildspec: codebuild/spec/buildspec_ubuntu.yml @@ -122,6 +133,18 @@ batch: BUILD_S2N: 'true' S2N_COVERAGE: 'true' + - identifier: s2nAsanOpenssl3 + buildspec: codebuild/spec/buildspec_ubuntu.yml + env: + privileged-mode: true + compute-type: BUILD_GENERAL1_LARGE + image: 024603541914.dkr.ecr.us-west-2.amazonaws.com/docker:ubuntu18codebuild + variables: + TESTS: asan + GCC_VERSION: '6' + S2N_LIBCRYPTO: 'openssl-3.0' + BUILD_S2N: 'true' + - identifier: s2nAsanOpenssl102 buildspec: codebuild/spec/buildspec_ubuntu.yml env: diff --git a/crypto/s2n_stream_cipher_rc4.c b/crypto/s2n_stream_cipher_rc4.c index 8514f41b332..30be7e6a432 100644 --- a/crypto/s2n_stream_cipher_rc4.c +++ b/crypto/s2n_stream_cipher_rc4.c @@ -21,60 +21,28 @@ #include "utils/s2n_blob.h" #include "utils/s2n_safety.h" -#if S2N_OPENSSL_VERSION_AT_LEAST(3, 0, 0) - #include "openssl/provider.h" -DEFINE_POINTER_CLEANUP_FUNC(OSSL_LIB_CTX *, OSSL_LIB_CTX_free); -#endif - -static EVP_CIPHER *s2n_rc4_cipher = NULL; - -S2N_RESULT s2n_rc4_init() -{ - /* In Openssl-3.0, RC4 is only available from the "legacy" provider, - * which is not loaded in the default library context. - */ -#if defined(S2N_LIBCRYPTO_SUPPORTS_EVP_RC4) && S2N_OPENSSL_VERSION_AT_LEAST(3, 0, 0) - DEFER_CLEANUP(OSSL_LIB_CTX *lib_ctx = OSSL_LIB_CTX_new(), OSSL_LIB_CTX_free_pointer); - RESULT_ENSURE_REF(lib_ctx); - RESULT_ENSURE_REF(OSSL_PROVIDER_load(lib_ctx, "legacy")); - s2n_rc4_cipher = EVP_CIPHER_fetch(lib_ctx, "rc4", "provider=legacy"); - RESULT_ENSURE_REF(s2n_rc4_cipher); -#endif - return S2N_RESULT_OK; -} - -S2N_RESULT s2n_rc4_cleanup() +static const EVP_CIPHER *s2n_evp_rc4() { -#if S2N_OPENSSL_VERSION_AT_LEAST(3, 0, 0) - EVP_CIPHER_free(s2n_rc4_cipher); +#ifdef S2N_LIBCRYPTO_SUPPORTS_EVP_RC4 + return EVP_rc4(); +#else + return NULL; #endif - return S2N_RESULT_OK; } -static S2N_RESULT s2n_get_rc4_cipher(const EVP_CIPHER **cipher) +static uint8_t s2n_stream_cipher_rc4_available() { - RESULT_ENSURE_REF(cipher); - *cipher = NULL; if (s2n_is_in_fips_mode()) { - *cipher = NULL; - } else if (s2n_rc4_cipher) { - *cipher = s2n_rc4_cipher; -#if S2N_LIBCRYPTO_SUPPORTS_EVP_RC4 - } else { - *cipher = EVP_rc4(); -#endif + return 0; } - RESULT_ENSURE(*cipher, S2N_ERR_UNIMPLEMENTED); - return S2N_RESULT_OK; -} - -static uint8_t s2n_stream_cipher_rc4_available() -{ - const EVP_CIPHER *cipher = NULL; - if (s2n_result_is_ok(s2n_get_rc4_cipher(&cipher)) && cipher) { - return 1; + /* RC4 MIGHT be available in Openssl-3.0, depending on whether or not the + * "legacy" provider is loaded. However, for simplicity, assume that RC4 + * is unavailable. + */ + if (S2N_OPENSSL_VERSION_AT_LEAST(3, 0, 0)) { + return 0; } - return 0; + return (s2n_evp_rc4() ? 1 : 0); } static int s2n_stream_cipher_rc4_encrypt(struct s2n_session_key *key, struct s2n_blob *in, struct s2n_blob *out) @@ -106,11 +74,7 @@ static int s2n_stream_cipher_rc4_decrypt(struct s2n_session_key *key, struct s2n static int s2n_stream_cipher_rc4_set_encryption_key(struct s2n_session_key *key, struct s2n_blob *in) { POSIX_ENSURE_EQ(in->size, 16); - - const EVP_CIPHER *evp_rc4 = NULL; - POSIX_GUARD_RESULT(s2n_get_rc4_cipher(&evp_rc4)); - - POSIX_GUARD_OSSL(EVP_EncryptInit_ex(key->evp_cipher_ctx, evp_rc4, NULL, in->data, NULL), S2N_ERR_KEY_INIT); + POSIX_GUARD_OSSL(EVP_EncryptInit_ex(key->evp_cipher_ctx, s2n_evp_rc4(), NULL, in->data, NULL), S2N_ERR_KEY_INIT); return S2N_SUCCESS; } @@ -118,11 +82,7 @@ static int s2n_stream_cipher_rc4_set_encryption_key(struct s2n_session_key *key, static int s2n_stream_cipher_rc4_set_decryption_key(struct s2n_session_key *key, struct s2n_blob *in) { POSIX_ENSURE_EQ(in->size, 16); - - const EVP_CIPHER *evp_rc4 = NULL; - POSIX_GUARD_RESULT(s2n_get_rc4_cipher(&evp_rc4)); - - POSIX_GUARD_OSSL(EVP_DecryptInit_ex(key->evp_cipher_ctx, evp_rc4, NULL, in->data, NULL), S2N_ERR_KEY_INIT); + POSIX_GUARD_OSSL(EVP_DecryptInit_ex(key->evp_cipher_ctx, s2n_evp_rc4(), NULL, in->data, NULL), S2N_ERR_KEY_INIT); return S2N_SUCCESS; } diff --git a/docs/USAGE-GUIDE.md b/docs/USAGE-GUIDE.md index 290f8634c6d..ae1f5d549fd 100644 --- a/docs/USAGE-GUIDE.md +++ b/docs/USAGE-GUIDE.md @@ -179,6 +179,12 @@ cd ../../ # root of project make ``` +### Available algorithms +Not all algorithms are available from all versions of Openssl: +* ChaChaPoly is not supported before Openssl-1.1.1 +* RSA-PSS is not supported before Openssl-1.1.1 +* RC4 is not supported with Openssl-3.0 or later. + ## Building s2n-tls with LibreSSL To build s2n-tls with LibreSSL, do the following: diff --git a/tests/integrationv2/providers.py b/tests/integrationv2/providers.py index 8eaac632c22..03b24ecf490 100644 --- a/tests/integrationv2/providers.py +++ b/tests/integrationv2/providers.py @@ -167,16 +167,22 @@ def supports_protocol(cls, protocol, with_cert=None): @classmethod def supports_cipher(cls, cipher, with_curve=None): - # Disable chacha20 tests in unsupported libcryptos - if any([ - libcrypto in get_flag(S2N_PROVIDER_VERSION) - for libcrypto in [ - "openssl-1.0.2", - "libressl" - ] - ]) and "CHACHA20" in cipher.name: - return False + # Disable chacha20 and RC4 tests in libcryptos that don't support those + # algorithms + unsupported_configurations = { + "CHACHA20": ["openssl-1.0.2", "libressl"], + "RC4": ["openssl-3"], + } + for unsupported_cipher, unsupported_libcryptos in unsupported_configurations.items(): + # the queried cipher has some libcrypto's that don't support it + # e.g. "RC4" in "TLS_ECDHE_RSA_WITH_RC4_128_SHA" + if unsupported_cipher in cipher.name: + current_libcrypto = get_flag(S2N_PROVIDER_VERSION) + for lc in unsupported_libcryptos: + # e.g. "openssl-3" in "openssl-3.0.7" + if lc in current_libcrypto: + return False return True @classmethod diff --git a/tests/unit/s2n_rc4_test.c b/tests/unit/s2n_rc4_test.c index b65de079e26..31fab3d1c35 100644 --- a/tests/unit/s2n_rc4_test.c +++ b/tests/unit/s2n_rc4_test.c @@ -20,6 +20,7 @@ #include "crypto/s2n_cipher.h" #include "crypto/s2n_fips.h" #include "crypto/s2n_hmac.h" +#include "crypto/s2n_openssl.h" #include "s2n_test.h" #include "stuffer/s2n_stuffer.h" #include "testlib/s2n_testlib.h" @@ -32,6 +33,16 @@ int main(int argc, char **argv) { BEGIN_TEST(); + /* Test Openssl-3.0 does not support RC4 */ + if (S2N_OPENSSL_VERSION_AT_LEAST(3, 0, 0)) { + EXPECT_FALSE(s2n_rc4.is_available()); + } + + /* Test FIPS does not support RC4 */ + if (s2n_is_in_fips_mode()) { + EXPECT_FALSE(s2n_rc4.is_available()); + } + struct s2n_connection *conn; uint8_t mac_key[] = "sample mac key"; uint8_t rc4_key[] = "123456789012345"; @@ -55,17 +66,6 @@ int main(int argc, char **argv) conn->server = conn->secure; conn->client = conn->secure; - /* Make sure that RC4 is available when expected */ -#if defined(S2N_LIBCRYPTO_SUPPORTS_EVP_RC4) - if (s2n_is_in_fips_mode()) { - EXPECT_FALSE(s2n_rc4.is_available()); - } else { - EXPECT_TRUE(s2n_rc4.is_available()); - } -#else - EXPECT_FALSE(s2n_rc4.is_available()); -#endif - /* test the RC4 cipher with a SHA1 hash */ conn->secure->cipher_suite->record_alg = &s2n_record_alg_rc4_sha; EXPECT_SUCCESS(conn->secure->cipher_suite->record_alg->cipher->init(&conn->secure->server_key)); @@ -128,11 +128,10 @@ int main(int argc, char **argv) EXPECT_SUCCESS(conn->secure->cipher_suite->record_alg->cipher->destroy_key(&conn->secure->server_key)); EXPECT_SUCCESS(conn->secure->cipher_suite->record_alg->cipher->destroy_key(&conn->secure->client_key)); - EXPECT_SUCCESS(s2n_connection_free(conn)); } else { - EXPECT_FAILURE_WITH_ERRNO(conn->secure->cipher_suite->record_alg->cipher->set_decryption_key(&conn->secure->client_key, &key_iv), S2N_ERR_UNIMPLEMENTED); - EXPECT_FAILURE_WITH_ERRNO(conn->secure->cipher_suite->record_alg->cipher->set_encryption_key(&conn->secure->server_key, &key_iv), S2N_ERR_UNIMPLEMENTED); + EXPECT_FAILURE(conn->secure->cipher_suite->record_alg->cipher->set_decryption_key(&conn->secure->client_key, &key_iv)); + EXPECT_FAILURE(conn->secure->cipher_suite->record_alg->cipher->set_encryption_key(&conn->secure->server_key, &key_iv)); } - + EXPECT_SUCCESS(s2n_connection_free(conn)); END_TEST(); } diff --git a/tls/s2n_cipher_suites.c b/tls/s2n_cipher_suites.c index d20325270b3..2ea36324938 100644 --- a/tls/s2n_cipher_suites.c +++ b/tls/s2n_cipher_suites.c @@ -971,9 +971,6 @@ int s2n_crypto_disable_init(void) /* Determines cipher suite availability and selects record algorithms */ int s2n_cipher_suites_init(void) { - /* RC4 requires some extra setup */ - POSIX_GUARD_RESULT(s2n_rc4_init()); - const int num_cipher_suites = s2n_array_len(s2n_all_cipher_suites); for (int i = 0; i < num_cipher_suites; i++) { struct s2n_cipher_suite *cur_suite = s2n_all_cipher_suites[i]; @@ -1054,10 +1051,6 @@ S2N_RESULT s2n_cipher_suites_cleanup(void) * cleanup in later versions */ #endif } - - /* RC4 requires some extra cleanup */ - RESULT_GUARD(s2n_rc4_cleanup()); - return S2N_RESULT_OK; } @@ -1179,7 +1172,7 @@ bool s2n_cipher_suite_uses_chacha20_alg(struct s2n_cipher_suite *cipher_suite) return cipher_suite && cipher_suite->record_alg && cipher_suite->record_alg->cipher == &s2n_chacha20_poly1305; } -/* Iff the server has enabled allow_chacha20_boosting and the client has a chacha20 cipher suite as its most +/* Iff the server has enabled allow_chacha20_boosting and the client has a chacha20 cipher suite as its most * preferred cipher suite, then we have mutual chacha20 boosting support. */ static S2N_RESULT s2n_validate_chacha20_boosting(const struct s2n_cipher_preferences *cipher_preferences, const uint8_t *wire, @@ -1254,7 +1247,7 @@ static int s2n_set_cipher_as_server(struct s2n_connection *conn, uint8_t *wire, * other cipher suites. * * If no mutually supported cipher suites are found, we choose one with a version - * too high for the current connection (higher_vers_match). + * too high for the current connection (higher_vers_match). */ for (size_t i = 0; i < cipher_preferences->count; i++) { const uint8_t *ours = cipher_preferences->suites[i]->iana_value;