From 86f0b806812da8a53c25061acca500e0dcfb1103 Mon Sep 17 00:00:00 2001 From: Yawning Angel Date: Tue, 14 Jun 2016 04:40:36 +0000 Subject: [PATCH 1/6] Bug 19406: OpenSSL changed the Thread API in 1.1.0 again. Instead of `ERR_remove_thread_state()` having a modified prototype, it now has the old prototype and a deprecation annotation. Since it's pointless to add extra complexity just to remain compatible with an old OpenSSL development snapshot, update the code to work with 1.1.0pre5 and later. --- src/common/crypto.c | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/src/common/crypto.c b/src/common/crypto.c index 933f1033f7..ca04e4ec3f 100644 --- a/src/common/crypto.c +++ b/src/common/crypto.c @@ -72,13 +72,18 @@ #define DISABLE_ENGINES #endif -#if OPENSSL_VERSION_NUMBER >= OPENSSL_VER(1,1,0,0,4) && \ +#if OPENSSL_VERSION_NUMBER >= OPENSSL_VER(1,1,0,0,5) && \ !defined(LIBRESSL_VERSION_NUMBER) -/* OpenSSL as of 1.1.0-pre4 has an "new" thread API, which doesn't require +/* OpenSSL as of 1.1.0pre4 has an "new" thread API, which doesn't require * seting up various callbacks. * - * Note: Yes, using OPENSSL_VER is naughty, but this was introduced in the - * pre-release series. + * OpenSSL 1.1.0pre4 has a messed up `ERR_remove_thread_state()` prototype, + * while the previous one was restored in pre5, and the function made a no-op + * (along with a deprecated annotation, which produces a compiler warning). + * + * While it is possible to support all three versions of the thread API, + * a version that existed only for one snapshot pre-release is kind of + * pointless, so let's not. */ #define NEW_THREAD_API #endif @@ -430,9 +435,7 @@ crypto_global_init(int useAccel, const char *accelName, const char *accelDir) void crypto_thread_cleanup(void) { -#ifdef NEW_THREAD_API - ERR_remove_thread_state(); -#else +#ifndef NEW_THREAD_API ERR_remove_thread_state(NULL); #endif } @@ -3193,9 +3196,7 @@ int crypto_global_cleanup(void) { EVP_cleanup(); -#ifdef NEW_THREAD_API - ERR_remove_thread_state(); -#else +#ifndef NEW_THREAD_API ERR_remove_thread_state(NULL); #endif ERR_free_strings(); From b563a3a09dd94892454210e82e46b62b947c5061 Mon Sep 17 00:00:00 2001 From: Yawning Angel Date: Tue, 14 Jun 2016 06:14:28 +0000 Subject: [PATCH 2/6] 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 ca04e4ec3f..614f9b5de9 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 env 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 env, 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= 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 4ffc672546..7d070c54cd 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 ed68bdf52c..8e957c2540 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 #include @@ -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); } From 6ddef1f7e0fe36f6e0b87873dfef5809dd593539 Mon Sep 17 00:00:00 2001 From: Yawning Angel Date: Tue, 14 Jun 2016 06:22:19 +0000 Subject: [PATCH 3/6] Bug 19406: OpenSSL removed SSL_R_RECORD_TOO_LARGE in 1.1.0. This is a logging onlu change, we were suppressing the severity down to INFO when it occured (treating it as "Mostly harmless"). Now it is no more. --- src/common/tortls.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/common/tortls.c b/src/common/tortls.c index 7d070c54cd..b68f5dfcdf 100644 --- a/src/common/tortls.c +++ b/src/common/tortls.c @@ -228,7 +228,9 @@ tor_tls_log_one_error(tor_tls_t *tls, unsigned long err, case SSL_R_HTTP_REQUEST: case SSL_R_HTTPS_PROXY_REQUEST: case SSL_R_RECORD_LENGTH_MISMATCH: +#ifndef OPENSSL_1_1_API case SSL_R_RECORD_TOO_LARGE: +#endif case SSL_R_UNKNOWN_PROTOCOL: case SSL_R_UNSUPPORTED_PROTOCOL: severity = LOG_INFO; From c5e2f7b9448cd0b7739e8515d0931b47fd208b9a Mon Sep 17 00:00:00 2001 From: Yawning Angel Date: Tue, 14 Jun 2016 06:24:13 +0000 Subject: [PATCH 4/6] Bug 19406: Fix the unit tests to work with OpenSSL 1.1.x Just as it says on the tin. Don't need to fully disable any tests and reduce coverage either. Yay me. --- src/test/test_tortls.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/test/test_tortls.c b/src/test/test_tortls.c index 973e727b46..b9b74a1e96 100644 --- a/src/test/test_tortls.c +++ b/src/test/test_tortls.c @@ -385,10 +385,12 @@ test_tortls_log_one_error(void *ignored) LOG_WARN, 0, NULL); expect_log_severity(LOG_INFO); +#ifndef OPENSSL_1_1_API mock_clean_saved_logs(); tor_tls_log_one_error(tls, ERR_PACK(1, 2, SSL_R_RECORD_TOO_LARGE), LOG_WARN, 0, NULL); expect_log_severity(LOG_INFO); +#endif mock_clean_saved_logs(); tor_tls_log_one_error(tls, ERR_PACK(1, 2, SSL_R_UNKNOWN_PROTOCOL), @@ -683,7 +685,7 @@ test_tortls_get_my_client_auth_key(void *ignored) crypto_pk_t *ret; crypto_pk_t *expected; tor_tls_context_t *ctx; - RSA *k = tor_malloc_zero(sizeof(RSA)); + RSA *k = RSA_new(); ctx = tor_malloc_zero(sizeof(tor_tls_context_t)); expected = crypto_new_pk_from_rsa_(k); @@ -698,8 +700,8 @@ test_tortls_get_my_client_auth_key(void *ignored) tt_assert(ret == expected); done: + RSA_free(k); tor_free(expected); - tor_free(k); tor_free(ctx); } From b217e4ac65532742ffd9527738303ca14ae3c6c0 Mon Sep 17 00:00:00 2001 From: Yawning Angel Date: Tue, 14 Jun 2016 06:37:03 +0000 Subject: [PATCH 5/6] Bug 19406: Add a changes file. --- changes/bug19406 | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 changes/bug19406 diff --git a/changes/bug19406 b/changes/bug19406 new file mode 100644 index 0000000000..e8b661b512 --- /dev/null +++ b/changes/bug19406 @@ -0,0 +1,4 @@ + o Minor features (build): + - Tor now again builds with the recent OpenSSL 1.1 development branch + (tested against 1.1.0-pre5 and 1.1.0-pre6-dev). + From 71aacbe427e2d0c2b970bdc81db4f96c506dd7f3 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 14 Jun 2016 12:17:02 -0400 Subject: [PATCH 6/6] Suppress the Wredundant-decls warning in another set of openssl headers --- src/tools/tor-gencert.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/src/tools/tor-gencert.c b/src/tools/tor-gencert.c index c05066722a..5f2cd3a92d 100644 --- a/src/tools/tor-gencert.c +++ b/src/tools/tor-gencert.c @@ -13,6 +13,20 @@ #include #endif +#ifdef __GNUC__ +#define GCC_VERSION (__GNUC__ * 100 + __GNUC_MINOR__) +#endif + +#if __GNUC__ && GCC_VERSION >= 402 +#if GCC_VERSION >= 406 +#pragma GCC diagnostic push +#endif +/* Some versions of OpenSSL declare X509_STORE_CTX_set_verify_cb twice in + * x509.h and x509_vfy.h. Suppress the GCC warning so we can build with + * -Wredundant-decl. */ +#pragma GCC diagnostic ignored "-Wredundant-decls" +#endif + #include #include #include @@ -20,6 +34,14 @@ #include #include +#if __GNUC__ && GCC_VERSION >= 402 +#if GCC_VERSION >= 406 +#pragma GCC diagnostic pop +#else +#pragma GCC diagnostic warning "-Wredundant-decls" +#endif +#endif + #include #if 0 #include