March 2020 - tor-commits - lists.torproject.org

[tor/maint-0.4.2] Fix memory leak in crypto_pk_asn1_decode_private.
by nickm＠torproject.org 19 Mar '20

19 Mar '20

commit 29c9675bdeb5a63564e9a76dcd7162bef884b240 Author: Nick Mathewson <nickm(a)torproject.org> Date: Sat Mar 14 14:17:33 2020 -0400 Fix memory leak in crypto_pk_asn1_decode_private. (Deep, deep thanks to Taylor for reminding me to test this!) --- src/lib/crypt_ops/crypto_rsa_openssl.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/lib/crypt_ops/crypto_rsa_openssl.c b/src/lib/crypt_ops/crypto_rsa_openssl.c index 39b7aaf0c..0db25f347 100644 --- a/src/lib/crypt_ops/crypto_rsa_openssl.c +++ b/src/lib/crypt_ops/crypto_rsa_openssl.c @@ -590,6 +590,7 @@ crypto_pk_asn1_decode_private(const char *str, size_t len, int max_bits) if (max_bits >= 0 && rsa->n && BN_num_bits(rsa->n) > max_bits) { #endif log_info(LD_CRYPTO, "Private key longer than expected."); + RSA_free(rsa); return NULL; } crypto_pk_t *result = crypto_new_pk_from_openssl_rsa_(rsa);

1 0

[tor/maint-0.4.2] When parsing tokens, reject early on spurious keys.
by nickm＠torproject.org 19 Mar '20

19 Mar '20

commit f160212ee855b6899063f2e9355abd59105d6a04 Author: Nick Mathewson <nickm(a)torproject.org> Date: Wed Feb 5 11:57:31 2020 -0500 When parsing tokens, reject early on spurious keys. --- src/feature/dirparse/parsecommon.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/feature/dirparse/parsecommon.c b/src/feature/dirparse/parsecommon.c index 5b753f0f2..e8269f7ec 100644 --- a/src/feature/dirparse/parsecommon.c +++ b/src/feature/dirparse/parsecommon.c @@ -384,10 +384,16 @@ get_next_token(memarea_t *area, RET_ERR("Couldn't parse object: missing footer or object much too big."); if (!strcmp(tok->object_type, "RSA PUBLIC KEY")) { /* If it's a public key */ + if (o_syn != NEED_KEY && o_syn != NEED_KEY_1024 && o_syn != OBJ_OK) { + RET_ERR("Unexpected public key."); + } tok->key = crypto_pk_new(); if (crypto_pk_read_public_key_from_string(tok->key, obstart, eol-obstart)) RET_ERR("Couldn't parse public key."); } else if (!strcmp(tok->object_type, "RSA PRIVATE KEY")) { /* private key */ + if (o_syn != NEED_SKEY_1024 && o_syn != OBJ_OK) { + RET_ERR("Unexpected private key."); + } tok->key = crypto_pk_new(); if (crypto_pk_read_private_key1024_from_string(tok->key, obstart, eol-obstart))

1 0

[tor/maint-0.4.2] Use >= consistently with max_bits.
by nickm＠torproject.org 19 Mar '20

19 Mar '20

commit f958b537abc1285dd627c03f091dc94a5d17995a Author: Nick Mathewson <nickm(a)torproject.org> Date: Tue Mar 17 10:09:58 2020 -0400 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 4cf699067..7abf6716f 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;

1 0

[tor/maint-0.4.2] Add off-by-one checks for key length.
by nickm＠torproject.org 19 Mar '20

19 Mar '20

commit 2328c79a5fbc2f1995390dd08002244bc952246d Author: Nick Mathewson <nickm(a)torproject.org> Date: Tue Mar 17 10:07:54 2020 -0400 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 2373e5bf8..5af0cce13 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);

1 0

[tor/maint-0.4.2] Merge branch 'trove_2020_002_035' into trove_2020_002_041
by nickm＠torproject.org 19 Mar '20

19 Mar '20

commit 9163781039b96e859fc102f003a274e9716cc02d Merge: dd6e2277e f958b537a Author: Nick Mathewson <nickm(a)torproject.org> Date: Tue Mar 17 10:45:03 2020 -0400 Merge branch 'trove_2020_002_035' into trove_2020_002_041 src/lib/crypt_ops/crypto_rsa_nss.c | 2 +- src/lib/crypt_ops/crypto_rsa_openssl.c | 57 +++++++++++++++++++++++++++++++--- src/test/test_crypto.c | 15 +++++++++ 3 files changed, 68 insertions(+), 6 deletions(-)

1 0

[tor/maint-0.4.2] Merge branch 'maint-0.4.1' into maint-0.4.2
by nickm＠torproject.org 19 Mar '20

19 Mar '20

commit 85141a3a74efb0db29ff98667e83b06208d1c926 Merge: 4ee269941 57b1d0848 Author: Nick Mathewson <nickm(a)torproject.org> Date: Tue Mar 17 15:22:36 2020 -0400 Merge branch 'maint-0.4.1' into maint-0.4.2 changes/ticket33119 | 8 +++++ src/feature/dirparse/parsecommon.c | 9 ++++- src/lib/crypt_ops/crypto_rsa.c | 27 +++++++++++---- src/lib/crypt_ops/crypto_rsa.h | 5 ++- src/lib/crypt_ops/crypto_rsa_nss.c | 14 +++++++- src/lib/crypt_ops/crypto_rsa_openssl.c | 63 ++++++++++++++++++++++++++++++++-- src/test/test_crypto.c | 39 +++++++++++++++++++++ 7 files changed, 154 insertions(+), 11 deletions(-)

1 0

[tor/maint-0.4.2] Merge branch 'trove_2020_002_035' into trove_2020_002_041
by nickm＠torproject.org 19 Mar '20

19 Mar '20

commit dd6e2277e0d336f3d519f88d792b832d04e2c323 Merge: 1a375c3b1 29c9675bd Author: Nick Mathewson <nickm(a)torproject.org> Date: Sat Mar 14 14:20:51 2020 -0400 Merge branch 'trove_2020_002_035' into trove_2020_002_041 src/lib/crypt_ops/crypto_rsa_openssl.c | 5 +++++ src/test/test_crypto.c | 24 ++++++++++++++++++++++++ 2 files changed, 29 insertions(+)

1 0

[tor/maint-0.4.2] Port rsa_private_key_too_long() to work on OpenSSL 1.1.0.
by nickm＠torproject.org 19 Mar '20

19 Mar '20

commit 0526801ed4caba4393942e62def2cb35f0f298b3 Author: Nick Mathewson <nickm(a)torproject.org> Date: Wed Mar 18 08:19:48 2020 -0400 Port rsa_private_key_too_long() to work on OpenSSL 1.1.0. --- src/lib/crypt_ops/crypto_rsa_openssl.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/lib/crypt_ops/crypto_rsa_openssl.c b/src/lib/crypt_ops/crypto_rsa_openssl.c index 6f3ac6fde..17eae24cc 100644 --- a/src/lib/crypt_ops/crypto_rsa_openssl.c +++ b/src/lib/crypt_ops/crypto_rsa_openssl.c @@ -573,6 +573,8 @@ 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 + +#if OPENSSL_VERSION_NUMBER >= OPENSSL_V_SERIES(1,1,1) n = RSA_get0_n(rsa); e = RSA_get0_e(rsa); p = RSA_get0_p(rsa); @@ -581,6 +583,11 @@ rsa_private_key_too_long(RSA *rsa, int max_bits) dmp1 = RSA_get0_dmp1(rsa); dmq1 = RSA_get0_dmq1(rsa); iqmp = RSA_get0_iqmp(rsa); +#else + /* The accessors above did not exist in openssl 1.1.0. */ + p = q = dmp1 = dmq1 = iqmp = NULL; + RSA_get0_key(rsa, &n, &e, &d); +#endif if (RSA_bits(rsa) > max_bits) return true;

1 0

[tor/maint-0.4.2] Extract key length check into a new function, and check more fields.
by nickm＠torproject.org 19 Mar '20

19 Mar '20

commit 8abdb394893a1704f885278f5f5d7913cdf516c9 Author: Nick Mathewson <nickm(a)torproject.org> Date: Tue Mar 17 10:04:38 2020 -0400 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 0db25f347..6f3ac6fde 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 <string.h> +#include <stdbool.h> /** 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 <b>str</b>; 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;

1 0

[tor/maint-0.4.2] Merge branch 'trove_2020_002_041' into maint-0.4.1
by nickm＠torproject.org 19 Mar '20

19 Mar '20

commit 3c8a4b8fbdb42b2fa98ddec8b6d7ed3202201eb1 Merge: cec647ff3 916378103 Author: Nick Mathewson <nickm(a)torproject.org> Date: Tue Mar 17 15:22:02 2020 -0400 Merge branch 'trove_2020_002_041' into maint-0.4.1 changes/ticket33119 | 8 +++++ src/feature/dirparse/parsecommon.c | 9 ++++- src/lib/crypt_ops/crypto_rsa.c | 27 +++++++++++---- src/lib/crypt_ops/crypto_rsa.h | 5 ++- src/lib/crypt_ops/crypto_rsa_nss.c | 14 +++++++- src/lib/crypt_ops/crypto_rsa_openssl.c | 63 ++++++++++++++++++++++++++++++++-- src/test/test_crypto.c | 39 +++++++++++++++++++++ 7 files changed, 154 insertions(+), 11 deletions(-)

1 0