From 786554f8f4e8c75bb18c5f91f69b7a328c177618 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Fri, 12 May 2023 01:08:47 -0400 Subject: [PATCH] Check public components in freeze_private_key We currently don't enforce rsa_check_public_key invariants on private key operations, only public key operations and RSA_check_key. This means it was actually possible, in some corner cases, to perform operations with oversized e or n. Fix this. This gets us a bit closer to aligning the mess of RSA invariants. (RSA_check_key still performs some extra checks, but most of those should be redundant with the CRT self-check.) Update-Note: Manually constructed RSA private keys with invalid n or e will now fail private key operations. Such keys would always fail at public key operations (so the signatures would never verify). They also already failed RSA_check_key and parsing. The one incompatibility of note is keys with only n and d, constructed by reaching into the internal RSA struct, no longer work. Instead, use RSA_new_private_key_no_e. Conscrypt used to do this but has since been migrated to the new API. Bug: 316 Change-Id: I062fdad924b8698e257dab9760687e4b381c970d Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/59826 Reviewed-by: Adam Langley Commit-Queue: David Benjamin --- crypto/fipsmodule/rsa/rsa_impl.c | 7 ++++ crypto/rsa_extra/rsa_test.cc | 68 ++++++++++---------------------- 2 files changed, 27 insertions(+), 48 deletions(-) diff --git a/crypto/fipsmodule/rsa/rsa_impl.c b/crypto/fipsmodule/rsa/rsa_impl.c index b907ae405..928346657 100644 --- a/crypto/fipsmodule/rsa/rsa_impl.c +++ b/crypto/fipsmodule/rsa/rsa_impl.c @@ -79,6 +79,8 @@ int rsa_check_public_key(const RSA *rsa) { return 0; } + // TODO(davidben): 16384-bit RSA is huge. Can we bring this down to a limit of + // 8192-bit? unsigned n_bits = BN_num_bits(rsa->n); if (n_bits > 16 * 1024) { OPENSSL_PUT_ERROR(RSA, RSA_R_MODULUS_TOO_LARGE); @@ -176,6 +178,11 @@ static int freeze_private_key(RSA *rsa, BN_CTX *ctx) { goto err; } + // Check the public components are within DoS bounds. + if (!rsa_check_public_key(rsa)) { + goto err; + } + // Pre-compute various intermediate values, as well as copies of private // exponents with correct widths. Note that other threads may concurrently // read from |rsa->n|, |rsa->e|, etc., so any fixes must be in separate diff --git a/crypto/rsa_extra/rsa_test.cc b/crypto/rsa_extra/rsa_test.cc index 070e1f654..fc5d5f86e 100644 --- a/crypto/rsa_extra/rsa_test.cc +++ b/crypto/rsa_extra/rsa_test.cc @@ -584,51 +584,6 @@ TEST(RSATest, BadKey) { EXPECT_FALSE(key); } -TEST(RSATest, OnlyDGiven) { - static const char kN[] = - "00e77bbf3889d4ef36a9a25d4d69f3f632eb4362214c74517da6d6aeaa9bd09ac42b2662" - "1cd88f3a6eb013772fc3bf9f83914b6467231c630202c35b3e5808c659"; - static const char kE[] = "010001"; - static const char kD[] = - "0365db9eb6d73b53b015c40cd8db4de7dd7035c68b5ac1bf786d7a4ee2cea316eaeca21a" - "73ac365e58713195f2ae9849348525ca855386b6d028e437a9495a01"; - - bssl::UniquePtr key(RSA_new()); - ASSERT_TRUE(key); - ASSERT_TRUE(BN_hex2bn(&key->n, kN)); - ASSERT_TRUE(BN_hex2bn(&key->e, kE)); - ASSERT_TRUE(BN_hex2bn(&key->d, kD)); - - // Keys with only n, e, and d are functional. - EXPECT_TRUE(RSA_check_key(key.get())); - - const uint8_t kDummyHash[32] = {0}; - uint8_t buf[64]; - unsigned buf_len = sizeof(buf); - ASSERT_LE(RSA_size(key.get()), sizeof(buf)); - EXPECT_TRUE(RSA_sign(NID_sha256, kDummyHash, sizeof(kDummyHash), buf, - &buf_len, key.get())); - EXPECT_TRUE(RSA_verify(NID_sha256, kDummyHash, sizeof(kDummyHash), buf, - buf_len, key.get())); - - // Keys without the public exponent must continue to work when blinding is - // disabled to support Java's RSAPrivateKeySpec API. See - // https://bugs.chromium.org/p/boringssl/issues/detail?id=12. - bssl::UniquePtr key2(RSA_new()); - ASSERT_TRUE(key2); - ASSERT_TRUE(BN_hex2bn(&key2->n, kN)); - ASSERT_TRUE(BN_hex2bn(&key2->d, kD)); - key2->flags |= RSA_FLAG_NO_BLINDING; - - ASSERT_LE(RSA_size(key2.get()), sizeof(buf)); - EXPECT_TRUE(RSA_sign(NID_sha256, kDummyHash, sizeof(kDummyHash), buf, - &buf_len, key2.get())); - - // Verify the signature with |key|. |key2| has no public exponent. - EXPECT_TRUE(RSA_verify(NID_sha256, kDummyHash, sizeof(kDummyHash), buf, - buf_len, key.get())); -} - TEST(RSATest, ASN1) { // Test that private keys may be decoded. bssl::UniquePtr rsa( @@ -1258,6 +1213,25 @@ TEST(RSATest, LargeE) { /*dmp1=*/e, /*dmq1=*/e, iqmp)); EXPECT_FALSE(priv); + // Constructing such a key piecemeal also would not work. This was only + // possible with private APIs, so when |RSA| is opaque, this case will be + // impossible. + priv.reset(RSA_new()); + ASSERT_TRUE(priv); + priv->n = BN_dup(n); + ASSERT_TRUE(priv->n); + priv->e = BN_dup(d); // Swapped + ASSERT_TRUE(priv->e); + priv->d = BN_dup(e); + ASSERT_TRUE(priv->d); + + static const uint8_t kDigest[32] = {0}; + std::vector sig(RSA_size(priv.get())); + size_t len; + EXPECT_FALSE(RSA_sign_pss_mgf1(priv.get(), &len, sig.data(), sig.size(), + kDigest, sizeof(kDigest), EVP_sha256(), + EVP_sha256(), /*salt_len=*/32)); + // But the "large e" APIs tolerate it. pub.reset(RSA_new_public_key_large_e(n, /*e=*/d)); ASSERT_TRUE(pub); @@ -1266,9 +1240,7 @@ TEST(RSATest, LargeE) { ASSERT_TRUE(priv); // Test that operations work correctly. - static const uint8_t kDigest[32] = {0}; - std::vector sig(RSA_size(priv.get())); - size_t len; + sig.resize(RSA_size(priv.get())); ASSERT_TRUE(RSA_sign_pss_mgf1(priv.get(), &len, sig.data(), sig.size(), kDigest, sizeof(kDigest), EVP_sha256(), EVP_sha256(), /*salt_len=*/32));