commit b563a3a09dd94892454210e82e46b62b947c5061 Author: Yawning Angel <yawning@schwanenlied.me> Date: Tue Jun 14 06:14:28 2016 +0000 Bug 19406: OpenSSL made RSA and DH opaque in 1.1.0. There's accessors to get at things, but it ends up being rather cumbersome. The only place where behavior should change is that the code will fail instead of attempting to generate a new DH key if our internal sanity check fails. Like the previous commit, this probably breaks snapshots prior to pre5. --- src/common/crypto.c | 161 ++++++++++++++++++++++++++++++++++++++++------- src/common/tortls.c | 4 ++ src/tools/tor-checkkey.c | 11 +++- 3 files changed, 152 insertions(+), 24 deletions(-) diff --git a/src/common/crypto.c b/src/common/crypto.c index ca04e4e..614f9b5 100644 --- a/src/common/crypto.c +++ b/src/common/crypto.c @@ -94,11 +94,6 @@ /** Largest strong entropy request */ #define MAX_STRONGEST_RAND_SIZE 256 -/** Macro: is k a valid RSA public or private key? */ -#define PUBLIC_KEY_OK(k) ((k) && (k)->key && (k)->key->n) -/** Macro: is k a valid RSA private key? */ -#define PRIVATE_KEY_OK(k) ((k) && (k)->key && (k)->key->p) - #ifndef NEW_THREAD_API /** A number of preallocated mutexes for use by OpenSSL. */ static tor_mutex_t **openssl_mutexes_ = NULL; @@ -440,6 +435,24 @@ crypto_thread_cleanup(void) #endif } +/** used internally: quicly validate a crypto_pk_t object as a private key. + * Return 1 iff the public key is valid, 0 if obviously invalid. + */ +static int +crypto_pk_private_ok(const crypto_pk_t *k) +{ +#ifdef OPENSSL_1_1_API + if (!k || !k->key) + return 0; + + BIGNUM *p, *q; + RSA_get0_factors(k->key, &p, &q); + return p != NULL; /* XXX/yawning: Should we check q? */ +#else + return k && k->key && k->key->p; +#endif +} + /** used by tortls.c: wrap an RSA* in a crypto_pk_t. */ crypto_pk_t * crypto_new_pk_from_rsa_(RSA *rsa) @@ -802,7 +815,7 @@ crypto_pk_write_private_key_to_filename(crypto_pk_t *env, char *s; int r; - tor_assert(PRIVATE_KEY_OK(env)); + tor_assert(crypto_pk_private_ok(env)); if (!(bio = BIO_new(BIO_s_mem()))) return -1; @@ -844,7 +857,7 @@ int crypto_pk_key_is_private(const crypto_pk_t *key) { tor_assert(key); - return PRIVATE_KEY_OK(key); + return crypto_pk_private_ok(key); } /** Return true iff <b>env</b> contains a public key whose public exponent @@ -856,7 +869,15 @@ crypto_pk_public_exponent_ok(crypto_pk_t *env) tor_assert(env); tor_assert(env->key); - return BN_is_word(env->key->e, 65537); + BIGNUM *e; + +#ifdef OPENSSL_1_1_API + BIGNUM *n, *d; + RSA_get0_key(env->key, &n, &e, &d); +#else + e = env->key->e; +#endif + return BN_is_word(e, 65537); } /** Compare the public-key components of a and b. Return less than 0 @@ -877,12 +898,27 @@ crypto_pk_cmp_keys(const crypto_pk_t *a, const crypto_pk_t *b) if (an_argument_is_null) return result; - tor_assert(PUBLIC_KEY_OK(a)); - tor_assert(PUBLIC_KEY_OK(b)); - result = BN_cmp((a->key)->n, (b->key)->n); + BIGNUM *a_n, *a_e; + BIGNUM *b_n, *b_e; + +#ifdef OPENSSL_1_1_API + BIGNUM *a_d, *b_d; + RSA_get0_key(a->key, &a_n, &a_e, &a_d); + RSA_get0_key(b->key, &b_n, &b_e, &b_d); +#else + a_n = a->key->n; + a_e = a->key->e; + b_n = b->key->n; + b_e = b->key->e; +#endif + + tor_assert(a_n != NULL && a_e != NULL); + tor_assert(b_n != NULL && b_e != NULL); + + result = BN_cmp(a_n, b_n); if (result) return result; - return BN_cmp((a->key)->e, (b->key)->e); + return BN_cmp(a_e, b_e); } /** Compare the public-key components of a and b. Return non-zero iff @@ -913,9 +949,20 @@ crypto_pk_num_bits(crypto_pk_t *env) { tor_assert(env); tor_assert(env->key); - tor_assert(env->key->n); +#ifdef OPENSSL_1_1_API + /* It's so stupid that there's no other way to check that n is valid + * before calling RSA_bits(). + */ + BIGNUM *n, *e, *d; + RSA_get0_key(env->key, &n, &e, &d); + tor_assert(n != NULL); + + return RSA_bits(env->key); +#else + tor_assert(env->key->n); return BN_num_bits(env->key->n); +#endif } /** Increase the reference count of <b>env</b>, and return it. @@ -940,7 +987,7 @@ crypto_pk_copy_full(crypto_pk_t *env) tor_assert(env); tor_assert(env->key); - if (PRIVATE_KEY_OK(env)) { + if (crypto_pk_private_ok(env)) { new_key = RSAPrivateKey_dup(env->key); privatekey = 1; } else { @@ -1009,7 +1056,7 @@ crypto_pk_private_decrypt(crypto_pk_t *env, char *to, tor_assert(env->key); tor_assert(fromlen<INT_MAX); tor_assert(tolen >= crypto_pk_keysize(env)); - if (!env->key->p) + if (!crypto_pk_key_is_private(env)) /* Not a private key */ return -1; @@ -1115,7 +1162,7 @@ crypto_pk_private_sign(const crypto_pk_t *env, char *to, size_t tolen, tor_assert(to); tor_assert(fromlen < INT_MAX); tor_assert(tolen >= crypto_pk_keysize(env)); - if (!env->key->p) + if (!crypto_pk_key_is_private(env)) /* Not a private key */ return -1; @@ -2108,25 +2155,35 @@ crypto_validate_dh_params(const BIGNUM *p, const BIGNUM *g) DH *dh = NULL; int ret = -1; - /* Copy into a temporary DH object. */ + /* Copy into a temporary DH object, just so that DH_check() can be called. */ if (!(dh = DH_new())) goto out; +#ifdef OPENSSL_1_1_API + BIGNUM *dh_p, *dh_g; + if (!(dh_p = BN_dup(p))) + goto out; + if (!(dh_g = BN_dup(g))) + goto out; + if (!DH_set0_pqg(dh, dh_p, NULL, dh_g)) + goto out; +#else if (!(dh->p = BN_dup(p))) goto out; if (!(dh->g = BN_dup(g))) goto out; +#endif /* Perform the validation. */ int codes = 0; if (!DH_check(dh, &codes)) goto out; - if (BN_is_word(dh->g, DH_GENERATOR_2)) { + if (BN_is_word(g, DH_GENERATOR_2)) { /* Per https://wiki.openssl.org/index.php/Diffie-Hellman_parameters * * OpenSSL checks the prime is congruent to 11 when g = 2; while the * IETF's primes are congruent to 23 when g = 2. */ - BN_ULONG residue = BN_mod_word(dh->p, 24); + BN_ULONG residue = BN_mod_word(p, 24); if (residue == 11 || residue == 23) codes &= ~DH_NOT_SUITABLE_GENERATOR; } @@ -2257,6 +2314,30 @@ crypto_dh_new(int dh_type) if (!(res->dh = DH_new())) goto err; +#ifdef OPENSSL_1_1_API + BIGNUM *dh_p = NULL, *dh_g = NULL; + + if (dh_type == DH_TYPE_TLS) { + dh_p = BN_dup(dh_param_p_tls); + } else { + dh_p = BN_dup(dh_param_p); + } + if (!dh_p) + goto err; + + dh_g = BN_dup(dh_param_g); + if (!dh_g) { + BN_free(dh_p); + goto err; + } + + if (!DH_set0_pqg(res->dh, dh_p, NULL, dh_g)) { + goto err; + } + + if (!DH_set_length(res->dh, DH_PRIVATE_KEY_BITS)) + goto err; +#else if (dh_type == DH_TYPE_TLS) { if (!(res->dh->p = BN_dup(dh_param_p_tls))) goto err; @@ -2269,6 +2350,7 @@ crypto_dh_new(int dh_type) goto err; res->dh->length = DH_PRIVATE_KEY_BITS; +#endif return res; err: @@ -2305,11 +2387,26 @@ crypto_dh_get_bytes(crypto_dh_t *dh) int crypto_dh_generate_public(crypto_dh_t *dh) { +#ifndef OPENSSL_1_1_API again: +#endif if (!DH_generate_key(dh->dh)) { crypto_log_errors(LOG_WARN, "generating DH key"); return -1; } +#ifdef OPENSSL_1_1_API + /* OpenSSL 1.1.x doesn't appear to let you regenerate a DH key, without + * recreating the DH object. I have no idea what sort of aliasing madness + * can occur here, so do the check, and just bail on failure. + */ + BIGNUM *pub_key, *priv_key; + DH_get0_key(dh->dh, &pub_key, &priv_key); + if (tor_check_dh_key(LOG_WARN, pub_key)<0) { + log_warn(LD_CRYPTO, "Weird! Our own DH key was invalid. I guess once-in-" + "the-universe chances really do happen. Treating as a failure."); + return -1; + } +#else if (tor_check_dh_key(LOG_WARN, dh->dh->pub_key)<0) { log_warn(LD_CRYPTO, "Weird! Our own DH key was invalid. I guess once-in-" "the-universe chances really do happen. Trying again."); @@ -2319,6 +2416,7 @@ crypto_dh_generate_public(crypto_dh_t *dh) dh->dh->pub_key = dh->dh->priv_key = NULL; goto again; } +#endif return 0; } @@ -2331,13 +2429,30 @@ crypto_dh_get_public(crypto_dh_t *dh, char *pubkey, size_t pubkey_len) { int bytes; tor_assert(dh); - if (!dh->dh->pub_key) { + + BIGNUM *dh_pub; + +#ifdef OPENSSL_1_1_API + BIGNUM *dh_priv; + DH_get0_key(dh->dh, &dh_pub, &dh_priv); +#else + dh_pub = dh->dh->pub_key; +#endif + + if (!dh_pub) { if (crypto_dh_generate_public(dh)<0) return -1; + else { +#ifdef OPENSSL_1_1_API + DH_get0_key(dh->dh, &dh_pub, &dh_priv); +#else + dh_pub = dh->dh->pub_key; +#endif + } } - tor_assert(dh->dh->pub_key); - bytes = BN_num_bytes(dh->dh->pub_key); + tor_assert(dh_pub); + bytes = BN_num_bytes(dh_pub); tor_assert(bytes >= 0); if (pubkey_len < (size_t)bytes) { log_warn(LD_CRYPTO, @@ -2347,7 +2462,7 @@ crypto_dh_get_public(crypto_dh_t *dh, char *pubkey, size_t pubkey_len) } memset(pubkey, 0, pubkey_len); - BN_bn2bin(dh->dh->pub_key, (unsigned char*)(pubkey+(pubkey_len-bytes))); + BN_bn2bin(dh_pub, (unsigned char*)(pubkey+(pubkey_len-bytes))); return 0; } diff --git a/src/common/tortls.c b/src/common/tortls.c index 4ffc672..7d070c5 100644 --- a/src/common/tortls.c +++ b/src/common/tortls.c @@ -904,7 +904,11 @@ tor_tls_cert_is_valid(int severity, cert_key = X509_get_pubkey(cert->cert); if (check_rsa_1024 && cert_key) { RSA *rsa = EVP_PKEY_get1_RSA(cert_key); +#ifdef OPENSSL_1_1_API + if (rsa && RSA_bits(rsa) == 1024) +#else if (rsa && BN_num_bits(rsa->n) == 1024) +#endif key_ok = 1; if (rsa) RSA_free(rsa); diff --git a/src/tools/tor-checkkey.c b/src/tools/tor-checkkey.c index ed68bdf..8e957c2 100644 --- a/src/tools/tor-checkkey.c +++ b/src/tools/tor-checkkey.c @@ -9,6 +9,7 @@ #include "torlog.h" #include "util.h" #include "compat.h" +#include "compat_openssl.h" #include <openssl/bn.h> #include <openssl/rsa.h> @@ -70,7 +71,15 @@ main(int c, char **v) printf("%s\n",digest); } else { rsa = crypto_pk_get_rsa_(env); - str = BN_bn2hex(rsa->n); + + BIGNUM *rsa_n; +#ifdef OPENSSL_1_1_API + BIGNUM *rsa_e, *rsa_d; + RSA_get0_key(rsa, &rsa_n, &rsa_e, &rsa_d); +#else + rsa_n = rsa->n; +#endif + str = BN_bn2hex(rsa_n); printf("%s\n", str); }