From 993087fd1d66fd558cf78bb855b286019e70d415 Mon Sep 17 00:00:00 2001 From: Nevine Ebeid <66388554+nebeid@users.noreply.github.com> Date: Wed, 23 Oct 2024 15:23:09 -0400 Subject: [PATCH] Remove retries on PCT failure in EC and RSA key generation. (#1944) FIPS review: The module should enter an error state if PCT fails in EC or RSA key generation, so there should be no retries and it aborts. This is to avoid that other threads would continue to use the module. (cherry picked from commit 90d2a34ead8d51c3a97be33f1e6eff751707807c on main, PR #1938) --- crypto/fipsmodule/ec/ec_key.c | 10 ++--- crypto/fipsmodule/rsa/rsa_impl.c | 64 +++++++++++++------------------- crypto/internal.h | 6 --- crypto/rsa_extra/rsa_test.cc | 32 ---------------- 4 files changed, 29 insertions(+), 83 deletions(-) diff --git a/crypto/fipsmodule/ec/ec_key.c b/crypto/fipsmodule/ec/ec_key.c index 730729ee3c..85e13c9666 100644 --- a/crypto/fipsmodule/ec/ec_key.c +++ b/crypto/fipsmodule/ec/ec_key.c @@ -516,18 +516,16 @@ int EC_KEY_generate_key(EC_KEY *key) { int EC_KEY_generate_key_fips(EC_KEY *eckey) { int ret = 0; - int num_attempts = 0; + // We have to verify both |EC_KEY_generate_key| and |EC_KEY_check_fips| both // succeed before updating the indicator state, so we lock the state here. FIPS_service_indicator_lock_state(); boringssl_ensure_ecc_self_test(); - do { - ret = EC_KEY_generate_key(eckey); - ret &= EC_KEY_check_fips(eckey); - num_attempts++; - } while ((ret == 0) && (num_attempts < MAX_KEYGEN_ATTEMPTS)); + if (EC_KEY_generate_key(eckey) && EC_KEY_check_fips(eckey)) { + ret = 1; + } FIPS_service_indicator_unlock_state(); if (ret) { diff --git a/crypto/fipsmodule/rsa/rsa_impl.c b/crypto/fipsmodule/rsa/rsa_impl.c index 69023bc5e5..e88f14c7af 100644 --- a/crypto/fipsmodule/rsa/rsa_impl.c +++ b/crypto/fipsmodule/rsa/rsa_impl.c @@ -1187,50 +1187,36 @@ static int RSA_generate_key_ex_maybe_fips(RSA *rsa, int bits, RSA *tmp = NULL; uint32_t err; int ret = 0; - int failures; - int num_attempts = 0; + // |rsa_generate_key_impl|'s 2^-20 failure probability is too high at scale, + // so we run the FIPS algorithm four times, bringing it down to 2^-80. We + // should just adjust the retry limit, but FIPS 186-4 prescribes that value + // and thus results in unnecessary complexity. + int failures = 0; do { - // The inner do-while loop can be considered as one invocation of RSA - // key generation: - // |rsa_generate_key_impl|'s 2^-20 failure probability is too high at scale, - // so we run the FIPS algorithm four times, bringing it down to 2^-80. We - // should just adjust the retry limit, but FIPS 186-4 prescribes that value - // and thus results in unnecessary complexity. - failures = 0; - do { - ERR_clear_error(); - // Generate into scratch space, to avoid leaving partial work on failure. - tmp = RSA_new(); - if (tmp == NULL) { - goto out; - } - - if (rsa_generate_key_impl(tmp, bits, e_value, cb)) { - break; - } + ERR_clear_error(); + // Generate into scratch space, to avoid leaving partial work on failure. + tmp = RSA_new(); + if (tmp == NULL) { + goto out; + } - err = ERR_peek_error(); - RSA_free(tmp); - tmp = NULL; - failures++; - - // Only retry on |RSA_R_TOO_MANY_ITERATIONS|. This is so a caller-induced - // failure in |BN_GENCB_call| is still fatal. - } while (failures < 4 && ERR_GET_LIB(err) == ERR_LIB_RSA && - ERR_GET_REASON(err) == RSA_R_TOO_MANY_ITERATIONS); - - // Perform PCT test in the case of FIPS - if (tmp) { - if (check_fips && !RSA_check_fips(tmp)) { - RSA_free(tmp); - tmp = NULL; - } + if (rsa_generate_key_impl(tmp, bits, e_value, cb)) { + break; } - num_attempts++; - } while ((tmp == NULL) && (num_attempts < MAX_KEYGEN_ATTEMPTS)); - if (tmp == NULL) { + err = ERR_peek_error(); + RSA_free(tmp); + tmp = NULL; + failures++; + + // Only retry on |RSA_R_TOO_MANY_ITERATIONS|. This is so a caller-induced + // failure in |BN_GENCB_call| is still fatal. + } while (failures < 4 && ERR_GET_LIB(err) == ERR_LIB_RSA && + ERR_GET_REASON(err) == RSA_R_TOO_MANY_ITERATIONS); + + // Perform PCT test in the case of FIPS + if (tmp == NULL || (check_fips && !RSA_check_fips(tmp))) { goto out; } diff --git a/crypto/internal.h b/crypto/internal.h index 97afbf008c..891da2931d 100644 --- a/crypto/internal.h +++ b/crypto/internal.h @@ -1259,12 +1259,6 @@ static inline uint64_t CRYPTO_subc_u64(uint64_t x, uint64_t y, uint64_t borrow, // FIPS functions. -#if defined(AWSLC_FIPS) -#define MAX_KEYGEN_ATTEMPTS 5 -#else -#define MAX_KEYGEN_ATTEMPTS 1 -#endif - #if defined(BORINGSSL_FIPS) // BORINGSSL_FIPS_abort is called when a FIPS power-on or continuous test diff --git a/crypto/rsa_extra/rsa_test.cc b/crypto/rsa_extra/rsa_test.cc index 66de5bcc1f..e2a3a0a683 100644 --- a/crypto/rsa_extra/rsa_test.cc +++ b/crypto/rsa_extra/rsa_test.cc @@ -1282,38 +1282,6 @@ TEST(RSADeathTest, KeygenFailAndDie) { } #endif -// This is not a death test. It's similar to the test KeygenFailOnce -// in the non-FIPS case, with the following differences: -// - the callback gets called again in the outer loop, -// ADD_FAILURE() is removed. -// - On subsequent retries (in FIPS, MAX_KEYGEN_ATTEMPTS > 1), -// |RSA_generate_key_ex| succeeds. -TEST(RSATest, KeygenFailOnceThenSucceed) { - bssl::UniquePtr rsa(RSA_new()); - ASSERT_TRUE(rsa); - - // Cause only the first iteration of RSA key generation to fail. - bool failed = false; - BN_GENCB cb; - BN_GENCB_set(&cb, - [](int event, int n, BN_GENCB *cb_ptr) -> int { - bool *failed_ptr = static_cast(cb_ptr->arg); - if (*failed_ptr) { - return 1; - } - *failed_ptr = true; - return 0; - }, - &failed); - - // Although key generation internally retries, the external behavior of - // |BN_GENCB| is preserved. - bssl::UniquePtr e(BN_new()); - ASSERT_TRUE(e); - ASSERT_TRUE(BN_set_word(e.get(), RSA_F4)); - EXPECT_TRUE(RSA_generate_key_ex(rsa.get(), 2048, e.get(), &cb)); -} - #endif