From 8abdb394893a1704f885278f5f5d7913cdf516c9 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 17 Mar 2020 10:04:38 -0400 Subject: Extract key length check into a new function, and check more fields. In the openssl that I have, it should be safe to only check the size of n. But if I'm wrong, or if other openssls work differently, we should check whether any of the fields are too large. Issue spotted by Teor. --- src/lib/crypt_ops/crypto_rsa_openssl.c | 57 +++++++++++++++++++++++++++++++--- 1 file changed, 52 insertions(+), 5 deletions(-) diff --git a/src/lib/crypt_ops/crypto_rsa_openssl.c b/src/lib/crypt_ops/crypto_rsa_openssl.c index 0db25f3473..6f3ac6fde6 100644 --- a/src/lib/crypt_ops/crypto_rsa_openssl.c +++ b/src/lib/crypt_ops/crypto_rsa_openssl.c @@ -33,6 +33,7 @@ ENABLE_GCC_WARNING(redundant-decls) #include "lib/encoding/binascii.h" #include +#include /** Declaration for crypto_pk_t structure. */ struct crypto_pk_t @@ -564,6 +565,56 @@ crypto_pk_asn1_encode_private(const crypto_pk_t *pk, char *dest, return len; } +/** Check whether any component of a private key is too large in a way that + * seems likely to make verification too expensive. Return true if it's too + * long, and false otherwise. */ +static bool +rsa_private_key_too_long(RSA *rsa, int max_bits) +{ + const BIGNUM *n, *e, *p, *q, *d, *dmp1, *dmq1, *iqmp; +#ifdef OPENSSL_1_1_API + n = RSA_get0_n(rsa); + e = RSA_get0_e(rsa); + p = RSA_get0_p(rsa); + q = RSA_get0_q(rsa); + d = RSA_get0_d(rsa); + dmp1 = RSA_get0_dmp1(rsa); + dmq1 = RSA_get0_dmq1(rsa); + iqmp = RSA_get0_iqmp(rsa); + + if (RSA_bits(rsa) > max_bits) + return true; +#else + n = rsa->n; + e = rsa->e; + p = rsa->p; + q = rsa->q; + d = rsa->d; + dmp1 = rsa->dmp1; + dmq1 = rsa->dmq1; + iqmp = rsa->iqmp; +#endif + + if (n && BN_num_bits(n) > max_bits) + return true; + if (e && BN_num_bits(e) > max_bits) + return true; + if (p && BN_num_bits(p) > max_bits) + return true; + if (q && BN_num_bits(q) > max_bits) + return true; + if (d && BN_num_bits(d) > max_bits) + return true; + if (dmp1 && BN_num_bits(dmp1) > max_bits) + return true; + if (dmq1 && BN_num_bits(dmq1) > max_bits) + return true; + if (iqmp && BN_num_bits(iqmp) > max_bits) + return true; + + return false; +} + /** Decode an ASN.1-encoded private key from str; return the result on * success and NULL on failure. * @@ -584,11 +635,7 @@ crypto_pk_asn1_decode_private(const char *str, size_t len, int max_bits) crypto_openssl_log_errors(LOG_WARN,"decoding private key"); return NULL; } -#ifdef OPENSSL_1_1_API - if (max_bits >= 0 && RSA_bits(rsa) > max_bits) { -#else - if (max_bits >= 0 && rsa->n && BN_num_bits(rsa->n) > max_bits) { -#endif + if (max_bits >= 0 && rsa_private_key_too_long(rsa, max_bits)) { log_info(LD_CRYPTO, "Private key longer than expected."); RSA_free(rsa); return NULL; -- cgit v1.2.3-54-g00ecf From 2328c79a5fbc2f1995390dd08002244bc952246d Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 17 Mar 2020 10:07:54 -0400 Subject: Add off-by-one checks for key length. --- src/test/test_crypto.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/src/test/test_crypto.c b/src/test/test_crypto.c index 2373e5bf86..5af0cce130 100644 --- a/src/test/test_crypto.c +++ b/src/test/test_crypto.c @@ -1505,6 +1505,21 @@ test_crypto_pk_bad_size(void *arg) pk2 = crypto_pk_asn1_decode_private(buf, n, 1020); tt_assert(! pk2); + /* Set the max bit count one bit smaller: we should refuse to decode the + key.*/ + pk2 = crypto_pk_asn1_decode_private(buf, n, 1023); + tt_assert(! pk2); + + /* Correct size: should work. */ + pk2 = crypto_pk_asn1_decode_private(buf, n, 1024); + tt_assert(pk2); + crypto_pk_free(pk2); + + /* One bit larger: should work. */ + pk2 = crypto_pk_asn1_decode_private(buf, n, 1025); + tt_assert(pk2); + crypto_pk_free(pk2); + /* Set the max bit count larger: it should decode fine. */ pk2 = crypto_pk_asn1_decode_private(buf, n, 2048); tt_assert(pk2); -- cgit v1.2.3-54-g00ecf From f958b537abc1285dd627c03f091dc94a5d17995a Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 17 Mar 2020 10:09:58 -0400 Subject: Use >= consistently with max_bits. --- src/lib/crypt_ops/crypto_rsa_nss.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/lib/crypt_ops/crypto_rsa_nss.c b/src/lib/crypt_ops/crypto_rsa_nss.c index 4cf6990670..7abf6716f0 100644 --- a/src/lib/crypt_ops/crypto_rsa_nss.c +++ b/src/lib/crypt_ops/crypto_rsa_nss.c @@ -736,7 +736,7 @@ crypto_pk_asn1_decode_private(const char *str, size_t len, int max_bits) if (output) { const int bits = SECKEY_PublicKeyStrengthInBits(output->pubkey); - if (max_bits > 0 && bits > max_bits) { + if (max_bits >= 0 && bits > max_bits) { log_info(LD_CRYPTO, "Private key longer than expected."); crypto_pk_free(output); output = NULL; -- cgit v1.2.3-54-g00ecf