Only abort when RSA PWCT fail in FIPS (#2020)

We've been getting abort failures when building with FIPS mode against
Ruby's break tests. The issue happens to be related to the abort call we
do when calls to `RSA_generate_key_ex` fail.
In the original commit where this was introduced (6bdd4c3), it's
mentioned that "It's required that the FIPS module aborts when PCT
tests fail in `RSA_check_fips()`." Our current behavior fails regardless
of a regular RSA failure or a PWCT failure, which causes regular RSA
failures to unintentionally abort as well.
This changes aborting to only happen during failures in
`RSA_check_fips`. Our existing death tests were also expecting failures
during regular RSA failures rather than PWCT failures, so I've tweaked
the tests to account for that. Ruby's RSA break test passes successfully
with this change.

### Call-outs:
The new test is ran when the CFLAG `BORINGSSL_FIPS_BREAK_TESTS` is
set, so I've updated our test script to run tests when building with this
dimension. This leverages our existing break tests to test PWCT aborting
behavior.

### Testing:
New test dimension

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license and the ISC license.

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,28 @@ 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.
+// In the case of a FIPS build, expect abort() when PWCTs in
+// |RSA_generate_key_fips| fail.
 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);
-
-  ASSERT_DEATH_IF_SUPPORTED(RSA_generate_key_ex(rsa.get(), 2048, e.get(), &cb),"");
+  const char *const value = getenv("BORINGSSL_FIPS_BREAK_TEST");
+  if (!value || strcmp(value, "RSA_PWCT") != 0) {
+    GTEST_SKIP() << "Skipping BORINGSSL_FIPS_BREAK_TESTS RSA_PWCT Test.";
+  }
 
-  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

@@ -33,6 +33,10 @@ if static_linux_supported || static_openbsd_supported; then
 
   echo "Testing AWS-LC static breakable release build"
   run_build -DFIPS=1 -DCMAKE_C_FLAGS="-DBORINGSSL_FIPS_BREAK_TESTS"
+  export BORINGSSL_FIPS_BREAK_TEST="RSA_PWCT"
+  ${BUILD_ROOT}/crypto/crypto_test --gtest_filter="RSADeathTest.KeygenFailAndDie"
+  unset BORINGSSL_FIPS_BREAK_TEST
+
   cd $SRC_ROOT
   MODULE_HASH=$(./util/fipstools/test-break-kat.sh |\
                     (egrep "Hash of module was:.* ([a-f0-9]*)" || true))