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));
