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 <agl@google.com> Commit-Queue: David Benjamin <davidben@google.com>
diff --git a/crypto/fipsmodule/rsa/rsa_impl.c b/crypto/fipsmodule/rsa/rsa_impl.c index b907ae4..9283466 100644 --- a/crypto/fipsmodule/rsa/rsa_impl.c +++ b/crypto/fipsmodule/rsa/rsa_impl.c
@@ -79,6 +79,8 @@ 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 @@ 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 070e1f6..fc5d5f8 100644 --- a/crypto/rsa_extra/rsa_test.cc +++ b/crypto/rsa_extra/rsa_test.cc
@@ -584,51 +584,6 @@ 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<RSA> 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<RSA> 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> rsa( @@ -1258,6 +1213,25 @@ /*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<uint8_t> 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 @@ ASSERT_TRUE(priv); // Test that operations work correctly. - static const uint8_t kDigest[32] = {0}; - std::vector<uint8_t> 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));