Remove retries on PCT failure in EC and RSA key generation. (#1938)

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.
aws · Oct 23, 2024 · 90d2a34 · 90d2a34
1 parent a3df396
commit 90d2a34
Show file tree

Hide file tree

Showing 4 changed files with 29 additions and 83 deletions.
diff --git a/crypto/fipsmodule/ec/ec_key.c b/crypto/fipsmodule/ec/ec_key.c
@@ -523,18 +523,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
@@ -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
@@ -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
@@ -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(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<bool *>(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<BIGNUM> 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