tor-commits March 2020

tor-commits@lists.torproject.org

21 participants
1780 discussions

[tor/maint-0.3.5] When parsing, reject >1024-bit RSA private keys sooner.
by nickm＠torproject.org 18 Mar '20

18 Mar '20

commit 9e1085c924133d7b73c7b0a42282fb13b34f28b8 Author: Nick Mathewson <nickm(a)torproject.org> Date: Wed Feb 5 11:11:35 2020 -0500 When parsing, reject >1024-bit RSA private keys sooner. Private-key validation is fairly expensive for long keys in openssl, so we need to avoid it sooner. --- src/feature/dirparse/parsecommon.c | 3 ++- 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 | 11 +++++++++-- 5 files changed, 49 insertions(+), 11 deletions(-) diff --git a/src/feature/dirparse/parsecommon.c b/src/feature/dirparse/parsecommon.c index 632f5adff..5b753f0f2 100644 --- a/src/feature/dirparse/parsecommon.c +++ b/src/feature/dirparse/parsecommon.c @@ -389,7 +389,8 @@ get_next_token(memarea_t *area, RET_ERR("Couldn't parse public key."); } else if (!strcmp(tok->object_type, "RSA PRIVATE KEY")) { /* private key */ tok->key = crypto_pk_new(); - if (crypto_pk_read_private_key_from_string(tok->key, obstart, eol-obstart)) + if (crypto_pk_read_private_key1024_from_string(tok->key, + obstart, eol-obstart)) RET_ERR("Couldn't parse private key."); } else { /* If it's something else, try to base64-decode it */ int r; diff --git a/src/lib/crypt_ops/crypto_rsa.c b/src/lib/crypt_ops/crypto_rsa.c index c9189b0df..8fd8a8aa7 100644 --- a/src/lib/crypt_ops/crypto_rsa.c +++ b/src/lib/crypt_ops/crypto_rsa.c @@ -490,7 +490,7 @@ crypto_pk_write_private_key_to_string(crypto_pk_t *env, static int crypto_pk_read_from_string_generic(crypto_pk_t *env, const char *src, size_t len, int severity, - bool private_key) + bool private_key, int max_bits) { if (len == (size_t)-1) // "-1" indicates "use the length of the string." len = strlen(src); @@ -510,7 +510,7 @@ crypto_pk_read_from_string_generic(crypto_pk_t *env, const char *src, } crypto_pk_t *pk = private_key - ? crypto_pk_asn1_decode_private((const char*)buf, n) + ? crypto_pk_asn1_decode_private((const char*)buf, n, max_bits) : crypto_pk_asn1_decode((const char*)buf, n); if (! pk) { log_fn(severity, LD_CRYPTO, @@ -539,7 +539,8 @@ int crypto_pk_read_public_key_from_string(crypto_pk_t *env, const char *src, size_t len) { - return crypto_pk_read_from_string_generic(env, src, len, LOG_INFO, false); + return crypto_pk_read_from_string_generic(env, src, len, LOG_INFO, false, + -1); } /** Read a PEM-encoded private key from the len-byte string src @@ -550,7 +551,21 @@ int crypto_pk_read_private_key_from_string(crypto_pk_t *env, const char *src, ssize_t len) { - return crypto_pk_read_from_string_generic(env, src, len, LOG_INFO, true); + return crypto_pk_read_from_string_generic(env, src, len, LOG_INFO, true, + -1); +} + +/** + * As crypto_pk_read_private_key_from_string(), but reject any key + * with a modulus longer than 1024 bits before doing any expensive + * validation on it. + */ +int +crypto_pk_read_private_key1024_from_string(crypto_pk_t *env, + const char *src, ssize_t len) +{ + return crypto_pk_read_from_string_generic(env, src, len, LOG_INFO, true, + 1024); } /** If a file is longer than this, we won't try to decode its private key */ @@ -578,7 +593,7 @@ crypto_pk_read_private_key_from_filename(crypto_pk_t *env, } int rv = crypto_pk_read_from_string_generic(env, buf, (ssize_t)st.st_size, - LOG_WARN, true); + LOG_WARN, true, -1); if (rv < 0) { log_warn(LD_CRYPTO, "Unable to decode private key from file %s", escaped(keyfile)); @@ -662,7 +677,7 @@ crypto_pk_base64_decode_private(const char *str, size_t len) goto out; } - pk = crypto_pk_asn1_decode_private(der, der_len); + pk = crypto_pk_asn1_decode_private(der, der_len, -1); out: memwipe(der, 0, len+1); diff --git a/src/lib/crypt_ops/crypto_rsa.h b/src/lib/crypt_ops/crypto_rsa.h index c1ea767f8..6d9cc8d30 100644 --- a/src/lib/crypt_ops/crypto_rsa.h +++ b/src/lib/crypt_ops/crypto_rsa.h @@ -61,6 +61,8 @@ int crypto_pk_read_public_key_from_string(crypto_pk_t *env, const char *src, size_t len); int crypto_pk_read_private_key_from_string(crypto_pk_t *env, const char *s, ssize_t len); +int crypto_pk_read_private_key1024_from_string(crypto_pk_t *env, + const char *src, ssize_t len); int crypto_pk_write_private_key_to_filename(crypto_pk_t *env, const char *fname); @@ -95,7 +97,8 @@ int crypto_pk_asn1_encode(const crypto_pk_t *pk, char *dest, size_t dest_len); crypto_pk_t *crypto_pk_asn1_decode(const char *str, size_t len); int crypto_pk_asn1_encode_private(const crypto_pk_t *pk, char *dest, size_t dest_len); -crypto_pk_t *crypto_pk_asn1_decode_private(const char *str, size_t len); +crypto_pk_t *crypto_pk_asn1_decode_private(const char *str, size_t len, + int max_bits); int crypto_pk_get_fingerprint(crypto_pk_t *pk, char *fp_out,int add_space); int crypto_pk_get_hashed_fingerprint(crypto_pk_t *pk, char *fp_out); void crypto_add_spaces_to_fp(char *out, size_t outlen, const char *in); diff --git a/src/lib/crypt_ops/crypto_rsa_nss.c b/src/lib/crypt_ops/crypto_rsa_nss.c index ad2ad38b6..4cf699067 100644 --- a/src/lib/crypt_ops/crypto_rsa_nss.c +++ b/src/lib/crypt_ops/crypto_rsa_nss.c @@ -679,9 +679,12 @@ crypto_pk_asn1_encode_private(const crypto_pk_t *pk, /** Given a buffer containing the DER representation of the * private key str, decode and return the result on success, or NULL * on failure. + * + * If max_bits is nonnegative, reject any key longer than max_bits + * without performing any expensive validation on it. */ crypto_pk_t * -crypto_pk_asn1_decode_private(const char *str, size_t len) +crypto_pk_asn1_decode_private(const char *str, size_t len, int max_bits) { tor_assert(str); tor_assert(len < INT_MAX); @@ -731,6 +734,15 @@ crypto_pk_asn1_decode_private(const char *str, size_t len) output = NULL; } + if (output) { + const int bits = SECKEY_PublicKeyStrengthInBits(output->pubkey); + if (max_bits > 0 && bits > max_bits) { + log_info(LD_CRYPTO, "Private key longer than expected."); + crypto_pk_free(output); + output = NULL; + } + } + if (slot) PK11_FreeSlot(slot); diff --git a/src/lib/crypt_ops/crypto_rsa_openssl.c b/src/lib/crypt_ops/crypto_rsa_openssl.c index fbdc76ccd..022a0dc09 100644 --- a/src/lib/crypt_ops/crypto_rsa_openssl.c +++ b/src/lib/crypt_ops/crypto_rsa_openssl.c @@ -566,9 +566,12 @@ crypto_pk_asn1_encode_private(const crypto_pk_t *pk, char *dest, /** Decode an ASN.1-encoded private key from str; return the result on * success and NULL on failure. + * + * If max_bits is nonnegative, reject any key longer than max_bits + * without performing any expensive validation on it. */ crypto_pk_t * -crypto_pk_asn1_decode_private(const char *str, size_t len) +crypto_pk_asn1_decode_private(const char *str, size_t len, int max_bits) { RSA *rsa; unsigned char *buf; @@ -578,7 +581,11 @@ crypto_pk_asn1_decode_private(const char *str, size_t len) rsa = d2i_RSAPrivateKey(NULL, &cp, len); tor_free(buf); if (!rsa) { - crypto_openssl_log_errors(LOG_WARN,"decoding public key"); + crypto_openssl_log_errors(LOG_WARN,"decoding private key"); + return NULL; + } + if (max_bits >= 0 && RSA_bits(rsa) > max_bits) { + log_info(LD_CRYPTO, "Private key longer than expected."); return NULL; } crypto_pk_t *result = crypto_new_pk_from_openssl_rsa_(rsa);

1 0

[tor/maint-0.3.5] Revise TROVE-2020-002 fix to work on older OpenSSL versions.
by nickm＠torproject.org 18 Mar '20

18 Mar '20

commit be064f77b93bda370e4165e6ad6da17324835c9e Author: Nick Mathewson <nickm(a)torproject.org> Date: Sat Mar 14 13:38:53 2020 -0400 Revise TROVE-2020-002 fix to work on older OpenSSL versions. Although OpenSSL before 1.1.1 is no longer supported, it's possible that somebody is still using it with 0.3.5, so we probably shouldn't break it with this fix. --- src/lib/crypt_ops/crypto_rsa_openssl.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/lib/crypt_ops/crypto_rsa_openssl.c b/src/lib/crypt_ops/crypto_rsa_openssl.c index 022a0dc09..39b7aaf0c 100644 --- a/src/lib/crypt_ops/crypto_rsa_openssl.c +++ b/src/lib/crypt_ops/crypto_rsa_openssl.c @@ -584,7 +584,11 @@ 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 log_info(LD_CRYPTO, "Private key longer than expected."); return NULL; }

1 0

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

18 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.3.5] Merge branch 'trove_2020_002_035' into maint-0.3.5
by nickm＠torproject.org 18 Mar '20

18 Mar '20

commit fe3d8ec38e3c4c2a992280c2847cdd8e05f81d36 Merge: 089e57d22 f958b537a Author: Nick Mathewson <nickm(a)torproject.org> Date: Tue Mar 17 15:21:48 2020 -0400 Merge branch 'trove_2020_002_035' into maint-0.3.5 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.3.5] Extract key length check into a new function, and check more fields.
by nickm＠torproject.org 18 Mar '20

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

1 0

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

18 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.3.5] Fix memory leak in crypto_pk_asn1_decode_private.
by nickm＠torproject.org 18 Mar '20

18 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.3.5] Add off-by-one checks for key length.
by nickm＠torproject.org 18 Mar '20

18 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.1] When parsing tokens, reject early on spurious keys.
by nickm＠torproject.org 18 Mar '20

18 Mar '20

1 0

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

18 Mar '20

commit 1a375c3b193f73e73e7c9c640dccdf1eb027234b Merge: 7afb95d3e d0bce65ce Author: Nick Mathewson <nickm(a)torproject.org> Date: Wed Feb 5 12:06:24 2020 -0500 Merge branch 'trove_2020_002_035' into trove_2020_002_041 Resolved Conflicts: src/feature/dirparse/parsecommon.c 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 | 11 +++++++++-- 6 files changed, 63 insertions(+), 11 deletions(-) diff --cc src/feature/dirparse/parsecommon.c index 036a51689,e8269f7ec..6c2c4b06b --- a/src/feature/dirparse/parsecommon.c +++ b/src/feature/dirparse/parsecommon.c @@@ -392,25 -383,28 +392,32 @@@ get_next_token(memarea_t *area if (next - *s > MAX_UNPARSED_OBJECT_SIZE) RET_ERR("Couldn't parse object: missing footer or object much too big."); + { + int r; + size_t maxsize = base64_decode_maxsize(next-*s); + tok->object_body = ALLOC(maxsize); + r = base64_decode(tok->object_body, maxsize, *s, next-*s); + if (r<0) + RET_ERR("Malformed object: bad base64-encoded data"); + tok->object_size = r; + } + 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)) + tok->key = crypto_pk_asn1_decode(tok->object_body, tok->object_size); + if (! tok->key) 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)) + tok->key = crypto_pk_asn1_decode_private(tok->object_body, - tok->object_size); ++ tok->object_size, ++ 1024); + if (! tok->key) RET_ERR("Couldn't parse private key."); - } else { /* If it's something else, try to base64-decode it */ - int r; - tok->object_body = ALLOC(next-*s); /* really, this is too much RAM. */ - r = base64_decode(tok->object_body, next-*s, *s, next-*s); - if (r<0) - RET_ERR("Malformed object: bad base64-encoded data"); - tok->object_size = r; } *s = eol;

1 0

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

tor-commits March 2020