Validate RSA public keys more consistently.
https://boringssl-review.googlesource.com/c/boringssl/+/42504 aligned
RSA private key checks, but I missed the public key ones. We have two
different sets of RSA public key checks right now. One in the parser
just checks for e = 1 and even e. The other, when using the key, checks
for overly large e and n.
Align the two. Now parsing RSA public keys calls RSA_check_key and the
extra checks on e are added to RSA_check_key. Note RSA private key
parsing already called RSA_check_key. The consequences are:
First, RSA public keys with large n, large e, or n < e will be rejected
at parse time. Previously, they would be parsed but all operations on
them would fail. This aligns with our existing behavior for parsing
private keys.
Second, operations on RSA public keys with even e will fail. They
already failed to parse, but it was possible to manually construct such
a key. Previously, operations wouldn't explicitly fail, but they
wouldn't do anything useful because even exponents are not invertible.
(Encrypting would produce something undecryptable and the private key
would have a hard time reliably producing signatures we'd accept.) There
is no change to RSA private keys with even e. Those would already fail
the (e, d) consistency check and the fault check.
Third, operations on RSA public keys with e = 1 will fail. They already
failed to parse, but it was possible to manually construct such a key
and "verify" signatures or "encrypt" messages. However, with e = 1,
those operations are no-ops.
Finally, RSA private keys with e = d = 1 will be rejected at parse and
use. This is the only case that affects private keys because e = d = 1
are inverses, just pointless. Uses paired with RSA public key parsing
(e.g. our TLS library checks consistency with a certificate public key)
are not affected. Those already rejected such keys because we rejected
them in the public key parser. This CL aligns the private half.
This doesn't close https://crbug.com/boringssl/316, but we won't be able
to resolve that without a consistent story for what keys are valid.
Update-Note: See above.
Bug: 316
Change-Id: Ic27df18c4f48e5e3e57a17d6fe39399e2f8d5c68
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47524
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/crypto/fipsmodule/rsa/rsa_impl.c b/crypto/fipsmodule/rsa/rsa_impl.c
index 2f76e9e..6dd89b9 100644
--- a/crypto/fipsmodule/rsa/rsa_impl.c
+++ b/crypto/fipsmodule/rsa/rsa_impl.c
@@ -79,9 +79,8 @@
return 0;
}
- unsigned rsa_bits = BN_num_bits(rsa->n);
-
- if (rsa_bits > 16 * 1024) {
+ unsigned n_bits = BN_num_bits(rsa->n);
+ if (n_bits > 16 * 1024) {
OPENSSL_PUT_ERROR(RSA, RSA_R_MODULUS_TOO_LARGE);
return 0;
}
@@ -96,17 +95,21 @@
// [2] https://www.imperialviolet.org/2012/03/17/rsados.html
// [3] https://msdn.microsoft.com/en-us/library/aa387685(VS.85).aspx
static const unsigned kMaxExponentBits = 33;
-
- if (BN_num_bits(rsa->e) > kMaxExponentBits) {
+ unsigned e_bits = BN_num_bits(rsa->e);
+ if (e_bits > kMaxExponentBits ||
+ // Additionally reject e = 1 or even e. e must be odd to be relatively
+ // prime with phi(n).
+ e_bits < 2 ||
+ !BN_is_odd(rsa->e)) {
OPENSSL_PUT_ERROR(RSA, RSA_R_BAD_E_VALUE);
return 0;
}
- // Verify |n > e|. Comparing |rsa_bits| to |kMaxExponentBits| is a small
+ // Verify |n > e|. Comparing |n_bits| to |kMaxExponentBits| is a small
// shortcut to comparing |n| and |e| directly. In reality, |kMaxExponentBits|
// is much smaller than the minimum RSA key size that any application should
// accept.
- if (rsa_bits <= kMaxExponentBits) {
+ if (n_bits <= kMaxExponentBits) {
OPENSSL_PUT_ERROR(RSA, RSA_R_KEY_SIZE_TOO_SMALL);
return 0;
}
diff --git a/crypto/rsa_extra/rsa_asn1.c b/crypto/rsa_extra/rsa_asn1.c
index 3cc6a9c..58fd69a 100644
--- a/crypto/rsa_extra/rsa_asn1.c
+++ b/crypto/rsa_extra/rsa_asn1.c
@@ -102,8 +102,7 @@
return NULL;
}
- if (!BN_is_odd(ret->e) ||
- BN_num_bits(ret->e) < 2) {
+ if (!RSA_check_key(ret)) {
OPENSSL_PUT_ERROR(RSA, RSA_R_BAD_RSA_PARAMETERS);
RSA_free(ret);
return NULL;
