Only abort when RSA PWCT fail in FIPS

crypto/fipsmodule/rsa/rsa_impl.c

@@ -1214,10 +1214,17 @@ static int RSA_generate_key_ex_maybe_fips(RSA *rsa, int bits,
     // 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);
+  if (tmp == NULL) {
+    goto out;
+  }
 
   // Perform PCT test in the case of FIPS
-  if (tmp == NULL || (check_fips && !RSA_check_fips(tmp))) {
-    goto out;
+  if(check_fips && !RSA_check_fips(tmp)) {
+    RSA_free(tmp);
+#if defined(AWSLC_FIPS)
+    BORINGSSL_FIPS_abort();
+#endif
+    return ret;
   }
 
   rsa_invalidate_key(rsa);
@@ -1241,11 +1248,6 @@ static int RSA_generate_key_ex_maybe_fips(RSA *rsa, int bits,
 
 out:
   RSA_free(tmp);
-#if defined(AWSLC_FIPS)
-  if (ret == 0) {
-    BORINGSSL_FIPS_abort();
-  }
-#endif
   return ret;
 }
 

crypto/rsa_extra/rsa_test.cc

@@ -695,8 +695,6 @@ TEST(RSATest, BadExponent) {
   ERR_clear_error();
 }
 
-#if !defined(AWSLC_FIPS)
-
 // Attempting to generate an excessively small key should fail.
 TEST(RSATest, GenerateSmallKey) {
   bssl::UniquePtr<RSA> rsa(RSA_new());
@@ -710,25 +708,6 @@ TEST(RSATest, GenerateSmallKey) {
       ErrorEquals(ERR_get_error(), ERR_LIB_RSA, RSA_R_KEY_SIZE_TOO_SMALL));
 }
 
-#else
-// AWSLCAndroidTestRunner does not take tests that do |ASSERT_DEATH| very well.
-// GTEST issue: https://github.com/google/googletest/issues/1496.
-#if !defined(OPENSSL_ANDROID)
-
-// Attempting to generate an excessively small key should fail.
-// In the case of a FIPS build, expect abort() when |RSA_generate_key_ex| fails.
-TEST(RSADeathTest, GenerateSmallKeyAndDie) {
-  bssl::UniquePtr<RSA> rsa(RSA_new());
-  ASSERT_TRUE(rsa);
-  bssl::UniquePtr<BIGNUM> e(BN_new());
-  ASSERT_TRUE(e);
-  ASSERT_TRUE(BN_set_word(e.get(), RSA_F4));
-
-  ASSERT_DEATH_IF_SUPPORTED(RSA_generate_key_ex(rsa.get(), 255, e.get(), nullptr), "");
-}
-#endif
-#endif
-
 // Attempting to generate an funny RSA key length should round down.
 TEST(RSATest, RoundKeyLengths) {
   bssl::UniquePtr<BIGNUM> e(BN_new());
@@ -1214,70 +1193,33 @@ TEST(RSATest, KeygenFailOnce) {
 }
 
 #else
+
 // AWSLCAndroidTestRunner does not take tests that do |ASSERT_DEATH| very well.
 // GTEST issue: https://github.com/google/googletest/issues/1496.
 #if !defined(OPENSSL_ANDROID)
 
-// In the case of a FIPS build, expect abort() when |RSA_generate_key_ex| fails.
-TEST(RSADeathTest, KeygenFailAndDie) {
-  bssl::UniquePtr<RSA> rsa(RSA_new());
-  ASSERT_TRUE(rsa);
-
-  // Cause RSA key generation after a prime has been generated, to test that
-  // |rsa| is left alone.
-  BN_GENCB cb;
-  BN_GENCB_set(&cb,
-               [](int event, int, BN_GENCB *) -> int { return event != 3; },
-               nullptr);
-
-  bssl::UniquePtr<BIGNUM> e(BN_new());
-  ASSERT_TRUE(e);
-  ASSERT_TRUE(BN_set_word(e.get(), RSA_F4));
-
-  // Key generation should fail.
-  ASSERT_DEATH_IF_SUPPORTED(RSA_generate_key_ex(rsa.get(), 2048, e.get(), &cb),"");
-
-  // Failed key generations do not leave garbage in |rsa|.
-  EXPECT_FALSE(rsa->n);
-  EXPECT_FALSE(rsa->e);
-  EXPECT_FALSE(rsa->d);
-  EXPECT_FALSE(rsa->p);
-  EXPECT_FALSE(rsa->q);
-  EXPECT_FALSE(rsa->dmp1);
-  EXPECT_FALSE(rsa->dmq1);
-  EXPECT_FALSE(rsa->iqmp);
-  EXPECT_FALSE(rsa->mont_n);
-  EXPECT_FALSE(rsa->mont_p);
-  EXPECT_FALSE(rsa->mont_q);
-  EXPECT_FALSE(rsa->d_fixed);
-  EXPECT_FALSE(rsa->dmp1_fixed);
-  EXPECT_FALSE(rsa->dmq1_fixed);
-  EXPECT_FALSE(rsa->iqmp_mont);
-  EXPECT_FALSE(rsa->private_key_frozen);
-
-  // Failed key generations leave the previous contents alone.
-  EXPECT_TRUE(RSA_generate_key_ex(rsa.get(), 2048, e.get(), nullptr));
-  uint8_t *der;
-  size_t der_len;
-  ASSERT_TRUE(RSA_private_key_to_bytes(&der, &der_len, rsa.get()));
-  bssl::UniquePtr<uint8_t> delete_der(der);
+static bool set_env(const std::string& name, const std::string& value) {
+#if defined(OPENSSL_WINDOWS)
+  return SetEnvironmentVariableA(name.c_str(), value.c_str()) != 0;
+#else
+  return setenv(name.c_str(), value.c_str(), 1) == 0;
+#endif
+}
 
-  ASSERT_DEATH_IF_SUPPORTED(RSA_generate_key_ex(rsa.get(), 2048, e.get(), &cb),"");
+// In the case of a FIPS build, expect abort() when |RSA_generate_key_fips|
+// fails.
+TEST(RSADeathTest, KeygenFailAndDie) {
+  ASSERT_TRUE(set_env("BORINGSSL_FIPS_BREAK_TEST", "RSA_PWCT"));
 
-  uint8_t *der2;
-  size_t der2_len;
-  ASSERT_TRUE(RSA_private_key_to_bytes(&der2, &der2_len, rsa.get()));
-  bssl::UniquePtr<uint8_t> delete_der2(der2);
-  EXPECT_EQ(Bytes(der, der_len), Bytes(der2, der2_len));
+  // Test that all supported key lengths abort when PWCTs fail.
+  for (const size_t bits : {2048, 3072, 4096}) {
+    SCOPED_TRACE(bits);
+    bssl::UniquePtr<RSA> rsa(RSA_new());
+    ASSERT_TRUE(rsa);
 
-  // Generating a key over an existing key works, despite any cached state.
-  EXPECT_TRUE(RSA_generate_key_ex(rsa.get(), 2048, e.get(), nullptr));
-  EXPECT_TRUE(RSA_check_key(rsa.get()));
-  uint8_t *der3;
-  size_t der3_len;
-  ASSERT_TRUE(RSA_private_key_to_bytes(&der3, &der3_len, rsa.get()));
-  bssl::UniquePtr<uint8_t> delete_der3(der3);
-  EXPECT_NE(Bytes(der, der_len), Bytes(der3, der3_len));
+    ASSERT_DEATH_IF_SUPPORTED(RSA_generate_key_fips(rsa.get(), bits, nullptr),
+                              "");
+  }
 }
 #endif
 

tests/ci/run_fips_tests.sh

@@ -32,7 +32,7 @@ if static_linux_supported || static_openbsd_supported; then
   fips_build_and_test -DCMAKE_BUILD_TYPE=Release
 
   echo "Testing AWS-LC static breakable release build"
-  run_build -DFIPS=1 -DCMAKE_C_FLAGS="-DBORINGSSL_FIPS_BREAK_TESTS"
+  fips_build_and_test -DFIPS=1 -DCMAKE_C_FLAGS="-DBORINGSSL_FIPS_BREAK_TESTS"
   cd $SRC_ROOT
   MODULE_HASH=$(./util/fipstools/test-break-kat.sh |\
                     (egrep "Hash of module was:.* ([a-f0-9]*)" || true))