Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Only abort when RSA PWCT fail in FIPS #2020

Merged
merged 2 commits into from
Dec 5, 2024
Merged

Only abort when RSA PWCT fail in FIPS #2020

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cc73164
Only abort when RSA PWCT fail in FIPS
samuel40791765 Nov 27, 2024
734133d
Fix CI failures
samuel40791765 Nov 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 9 additions & 7 deletions crypto/fipsmodule/rsa/rsa_impl.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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;
}

Expand Down
93 changes: 15 additions & 78 deletions crypto/rsa_extra/rsa_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand All @@ -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());
Expand Down Expand Up @@ -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) {
smittals2 marked this conversation as resolved.
Show resolved Hide resolved
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

Expand Down
4 changes: 4 additions & 0 deletions tests/ci/run_fips_tests.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down
Loading