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 <[email protected]>
aws · Jun 6, 2023 · 382847c · 382847c
1 parent 56a897c
commit 382847c
Show file tree

Hide file tree

Showing 6 changed files with 76 additions and 89 deletions.
diff --git 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
@@ -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,23 +74,15 @@ 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;
 }
 
 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
@@ -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
@@ -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
@@ -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
@@ -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;