Commit 3c8a4b8f authored by Nick Mathewson's avatar Nick Mathewson 🐻
Browse files

Merge branch 'trove_2020_002_041' into maint-0.4.1

parents cec647ff 91637810
o Major bugfixes (security, denial-of-service):
- Fix a denial-of-service bug that could be used by anyone to consume a
bunch of CPU on any Tor relay or authority, or by directories to
consume a bunch of CPU on clients or hidden services. Because
of the potential for CPU consumption to introduce observable
timing patterns, we are treating this as a high-severity security
issue. Fixes bug 33119; bugfix on 0.2.1.5-alpha. We are also tracking
this issue as TROVE-2020-002.
...@@ -403,12 +403,19 @@ get_next_token(memarea_t *area, ...@@ -403,12 +403,19 @@ get_next_token(memarea_t *area,
} }
if (!strcmp(tok->object_type, "RSA PUBLIC KEY")) { /* If it's a public key */ 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_asn1_decode(tok->object_body, tok->object_size); tok->key = crypto_pk_asn1_decode(tok->object_body, tok->object_size);
if (! tok->key) if (! tok->key)
RET_ERR("Couldn't parse public key."); RET_ERR("Couldn't parse public key.");
} else if (!strcmp(tok->object_type, "RSA PRIVATE KEY")) { /* private 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_asn1_decode_private(tok->object_body, tok->key = crypto_pk_asn1_decode_private(tok->object_body,
tok->object_size); tok->object_size,
1024);
if (! tok->key) if (! tok->key)
RET_ERR("Couldn't parse private key."); RET_ERR("Couldn't parse private key.");
} }
......
...@@ -490,7 +490,7 @@ crypto_pk_write_private_key_to_string(crypto_pk_t *env, ...@@ -490,7 +490,7 @@ crypto_pk_write_private_key_to_string(crypto_pk_t *env,
static int static int
crypto_pk_read_from_string_generic(crypto_pk_t *env, const char *src, crypto_pk_read_from_string_generic(crypto_pk_t *env, const char *src,
size_t len, int severity, 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." if (len == (size_t)-1) // "-1" indicates "use the length of the string."
len = strlen(src); len = strlen(src);
...@@ -510,7 +510,7 @@ crypto_pk_read_from_string_generic(crypto_pk_t *env, const char *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_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); : crypto_pk_asn1_decode((const char*)buf, n);
if (! pk) { if (! pk) {
log_fn(severity, LD_CRYPTO, log_fn(severity, LD_CRYPTO,
...@@ -539,7 +539,8 @@ int ...@@ -539,7 +539,8 @@ int
crypto_pk_read_public_key_from_string(crypto_pk_t *env, crypto_pk_read_public_key_from_string(crypto_pk_t *env,
const char *src, size_t len) 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 <b>len</b>-byte string <b>src</b> /** Read a PEM-encoded private key from the <b>len</b>-byte string <b>src</b>
...@@ -550,7 +551,21 @@ int ...@@ -550,7 +551,21 @@ int
crypto_pk_read_private_key_from_string(crypto_pk_t *env, crypto_pk_read_private_key_from_string(crypto_pk_t *env,
const char *src, ssize_t len) 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 */ /** 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, ...@@ -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, 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) { if (rv < 0) {
log_warn(LD_CRYPTO, "Unable to decode private key from file %s", log_warn(LD_CRYPTO, "Unable to decode private key from file %s",
escaped(keyfile)); escaped(keyfile));
...@@ -662,7 +677,7 @@ crypto_pk_base64_decode_private(const char *str, size_t len) ...@@ -662,7 +677,7 @@ crypto_pk_base64_decode_private(const char *str, size_t len)
goto out; goto out;
} }
pk = crypto_pk_asn1_decode_private(der, der_len); pk = crypto_pk_asn1_decode_private(der, der_len, -1);
out: out:
memwipe(der, 0, len+1); memwipe(der, 0, len+1);
......
...@@ -61,6 +61,8 @@ int crypto_pk_read_public_key_from_string(crypto_pk_t *env, ...@@ -61,6 +61,8 @@ int crypto_pk_read_public_key_from_string(crypto_pk_t *env,
const char *src, size_t len); const char *src, size_t len);
int crypto_pk_read_private_key_from_string(crypto_pk_t *env, int crypto_pk_read_private_key_from_string(crypto_pk_t *env,
const char *s, ssize_t len); 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, int crypto_pk_write_private_key_to_filename(crypto_pk_t *env,
const char *fname); const char *fname);
...@@ -95,7 +97,8 @@ int crypto_pk_asn1_encode(const crypto_pk_t *pk, char *dest, size_t dest_len); ...@@ -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); crypto_pk_t *crypto_pk_asn1_decode(const char *str, size_t len);
int crypto_pk_asn1_encode_private(const crypto_pk_t *pk, int crypto_pk_asn1_encode_private(const crypto_pk_t *pk,
char *dest, size_t dest_len); 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_fingerprint(crypto_pk_t *pk, char *fp_out,int add_space);
int crypto_pk_get_hashed_fingerprint(crypto_pk_t *pk, char *fp_out); 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); void crypto_add_spaces_to_fp(char *out, size_t outlen, const char *in);
......
...@@ -679,9 +679,12 @@ crypto_pk_asn1_encode_private(const crypto_pk_t *pk, ...@@ -679,9 +679,12 @@ crypto_pk_asn1_encode_private(const crypto_pk_t *pk,
/** Given a buffer containing the DER representation of the /** Given a buffer containing the DER representation of the
* private key <b>str</b>, decode and return the result on success, or NULL * private key <b>str</b>, decode and return the result on success, or NULL
* on failure. * on failure.
*
* If <b>max_bits</b> is nonnegative, reject any key longer than max_bits
* without performing any expensive validation on it.
*/ */
crypto_pk_t * 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(str);
tor_assert(len < INT_MAX); tor_assert(len < INT_MAX);
...@@ -731,6 +734,15 @@ crypto_pk_asn1_decode_private(const char *str, size_t len) ...@@ -731,6 +734,15 @@ crypto_pk_asn1_decode_private(const char *str, size_t len)
output = NULL; 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) if (slot)
PK11_FreeSlot(slot); PK11_FreeSlot(slot);
......
...@@ -33,6 +33,7 @@ ENABLE_GCC_WARNING(redundant-decls) ...@@ -33,6 +33,7 @@ ENABLE_GCC_WARNING(redundant-decls)
#include "lib/encoding/binascii.h" #include "lib/encoding/binascii.h"
#include <string.h> #include <string.h>
#include <stdbool.h>
/** Declaration for crypto_pk_t structure. */ /** Declaration for crypto_pk_t structure. */
struct crypto_pk_t struct crypto_pk_t
...@@ -564,11 +565,64 @@ crypto_pk_asn1_encode_private(const crypto_pk_t *pk, char *dest, ...@@ -564,11 +565,64 @@ crypto_pk_asn1_encode_private(const crypto_pk_t *pk, char *dest,
return len; 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 /** Decode an ASN.1-encoded private key from <b>str</b>; return the result on
* success and NULL on failure. * success and NULL on failure.
*
* If <b>max_bits</b> is nonnegative, reject any key longer than max_bits
* without performing any expensive validation on it.
*/ */
crypto_pk_t * 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; RSA *rsa;
unsigned char *buf; unsigned char *buf;
...@@ -578,7 +632,12 @@ crypto_pk_asn1_decode_private(const char *str, size_t len) ...@@ -578,7 +632,12 @@ crypto_pk_asn1_decode_private(const char *str, size_t len)
rsa = d2i_RSAPrivateKey(NULL, &cp, len); rsa = d2i_RSAPrivateKey(NULL, &cp, len);
tor_free(buf); tor_free(buf);
if (!rsa) { 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_private_key_too_long(rsa, max_bits)) {
log_info(LD_CRYPTO, "Private key longer than expected.");
RSA_free(rsa);
return NULL; return NULL;
} }
crypto_pk_t *result = crypto_new_pk_from_openssl_rsa_(rsa); crypto_pk_t *result = crypto_new_pk_from_openssl_rsa_(rsa);
......
...@@ -1335,6 +1335,44 @@ test_crypto_pk_pem_encrypted(void *arg) ...@@ -1335,6 +1335,44 @@ test_crypto_pk_pem_encrypted(void *arg)
crypto_pk_free(pk); crypto_pk_free(pk);
} }
static void
test_crypto_pk_bad_size(void *arg)
{
(void)arg;
crypto_pk_t *pk1 = pk_generate(0);
crypto_pk_t *pk2 = NULL;
char buf[2048];
int n = crypto_pk_asn1_encode_private(pk1, buf, sizeof(buf));
tt_int_op(n, OP_GT, 0);
/* Set the max bit count smaller: we should refuse to decode the key.*/
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);
done:
crypto_pk_free(pk1);
crypto_pk_free(pk2);
}
static void static void
test_crypto_pk_invalid_private_key(void *arg) test_crypto_pk_invalid_private_key(void *arg)
{ {
...@@ -2998,6 +3036,7 @@ struct testcase_t crypto_tests[] = { ...@@ -2998,6 +3036,7 @@ struct testcase_t crypto_tests[] = {
{ "pk_fingerprints", test_crypto_pk_fingerprints, TT_FORK, NULL, NULL }, { "pk_fingerprints", test_crypto_pk_fingerprints, TT_FORK, NULL, NULL },
{ "pk_base64", test_crypto_pk_base64, TT_FORK, NULL, NULL }, { "pk_base64", test_crypto_pk_base64, TT_FORK, NULL, NULL },
{ "pk_pem_encrypted", test_crypto_pk_pem_encrypted, TT_FORK, NULL, NULL }, { "pk_pem_encrypted", test_crypto_pk_pem_encrypted, TT_FORK, NULL, NULL },
{ "pk_bad_size", test_crypto_pk_bad_size, 0, NULL, NULL },
{ "pk_invalid_private_key", test_crypto_pk_invalid_private_key, 0, { "pk_invalid_private_key", test_crypto_pk_invalid_private_key, 0,
NULL, NULL }, NULL, NULL },
CRYPTO_LEGACY(digests), CRYPTO_LEGACY(digests),
......
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment