February 2016 - tor-commits - lists.torproject.org

[tor/master] Validate the DH parameters for correctness.
by nickm＠torproject.org 06 Feb '16

06 Feb '16

commit c625ab9f5a36a99f9dff2fa842d2e43d23ad1908 Author: Yawning Angel <yawning(a)schwanenlied.me> Date: Tue Feb 2 22:03:48 2016 +0000 Validate the DH parameters for correctness. We use sensible parameters taken from common sources, and no longer have dynamic DH groups as an option, but it feels prudent to have OpenSSL validate p and g at initialization time. --- changes/bug18221 | 3 ++ src/common/crypto.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++------ 2 files changed, 74 insertions(+), 8 deletions(-) diff --git a/changes/bug18221 b/changes/bug18221 new file mode 100644 index 0000000..afc2404 --- /dev/null +++ b/changes/bug18221 @@ -0,0 +1,3 @@ + o Minor features (crypto): + - Validate the Diffie-Hellman hard coded parameters and ensure that + p is a safe prime, and g is suitable. Closes ticket 18221. diff --git a/src/common/crypto.c b/src/common/crypto.c index a42c461..431ff30 100644 --- a/src/common/crypto.c +++ b/src/common/crypto.c @@ -2084,6 +2084,71 @@ static BIGNUM *dh_param_p_tls = NULL; /** Shared G parameter for our DH key exchanges. */ static BIGNUM *dh_param_g = NULL; +/** Validate a given set of Diffie-Hellman parameters. This is moderately + * computationally expensive (milliseconds), so should only be called when + * the DH parameters change. Returns 0 on success, * -1 on failure. + */ +static int +crypto_validate_dh_params(const BIGNUM *p, const BIGNUM *g) +{ + DH *dh = NULL; + int ret = -1; + + /* Copy into a temporary DH object. */ + if (!(dh = DH_new())) + goto out; + if (!(dh->p = BN_dup(p))) + goto out; + if (!(dh->g = BN_dup(g))) + goto out; + + /* Perform the validation. */ + int codes = 0; + if (!DH_check(dh, &codes)) + goto out; + if (BN_is_word(dh->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); + if (residue == 11 || residue == 23) + codes &= ~DH_NOT_SUITABLE_GENERATOR; + } + if (codes != 0) /* Specifics on why the params suck is irrelevant. */ + goto out; + + /* Things are probably not evil. */ + ret = 0; + + out: + if (dh) + DH_free(dh); + return ret; +} + +/** Set the global Diffie-Hellman generator, used for both TLS and internal + * DH stuff. + */ +static void +crypto_set_dh_generator(void) +{ + BIGNUM *generator; + int r; + + if (dh_param_g) + return; + + generator = BN_new(); + tor_assert(generator); + + r = BN_set_word(generator, DH_GENERATOR); + tor_assert(r); + + dh_param_g = generator; +} + /** Set the global TLS Diffie-Hellman modulus. Use the Apache mod_ssl DH * modulus. */ void @@ -2116,6 +2181,8 @@ crypto_set_tls_dh_prime(void) tor_assert(tls_prime); dh_param_p_tls = tls_prime; + crypto_set_dh_generator(); + tor_assert(0 == crypto_validate_dh_params(dh_param_p_tls, dh_param_g)); } /** Initialize dh_param_p and dh_param_g if they are not already @@ -2123,18 +2190,13 @@ crypto_set_tls_dh_prime(void) static void init_dh_param(void) { - BIGNUM *circuit_dh_prime, *generator; + BIGNUM *circuit_dh_prime; int r; if (dh_param_p && dh_param_g) return; circuit_dh_prime = BN_new(); - generator = BN_new(); - tor_assert(circuit_dh_prime && generator); - - /* Set our generator for all DH parameters */ - r = BN_set_word(generator, DH_GENERATOR); - tor_assert(r); + tor_assert(circuit_dh_prime); /* This is from rfc2409, section 6.2. It's a safe prime, and supposedly it equals: @@ -2150,7 +2212,8 @@ init_dh_param(void) /* Set the new values as the global DH parameters. */ dh_param_p = circuit_dh_prime; - dh_param_g = generator; + crypto_set_dh_generator(); + tor_assert(0 == crypto_validate_dh_params(dh_param_p, dh_param_g)); if (!dh_param_p_tls) { crypto_set_tls_dh_prime();

1 0

[tor/master] Merge remote-tracking branch 'yawning/bug18221'
by nickm＠torproject.org 06 Feb '16

06 Feb '16

commit 0f5f6b8a412615026603954b1760006723026da1 Merge: b645e2f c625ab9 Author: Nick Mathewson <nickm(a)torproject.org> Date: Sat Feb 6 15:30:22 2016 -0500 Merge remote-tracking branch 'yawning/bug18221' changes/bug18221 | 3 ++ src/common/crypto.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++------ 2 files changed, 74 insertions(+), 8 deletions(-)

1 0

[tor/master] Merge remote-tracking branch 'alec/dead_code_removal'
by nickm＠torproject.org 06 Feb '16

06 Feb '16

commit b645e2f2b0113b143078a4fb6c227280b17fcb00 Merge: 928380e 6852868 Author: Nick Mathewson <nickm(a)torproject.org> Date: Sat Feb 6 15:08:49 2016 -0500 Merge remote-tracking branch 'alec/dead_code_removal' src/or/main.c | 38 -------------------------------------- 1 file changed, 38 deletions(-)

1 0

[tor/master] Removed dead code in main.c
by nickm＠torproject.org 06 Feb '16

06 Feb '16

commit 6852868b4a1769d960dd95672716f39350ba5043 Author: Alec Heifetz <aheifetz(a)mit.edu> Date: Sat Feb 6 14:41:31 2016 -0500 Removed dead code in main.c --- src/or/main.c | 38 -------------------------------------- 1 file changed, 38 deletions(-) diff --git a/src/or/main.c b/src/or/main.c index bd4f7ea..e406070 100644 --- a/src/or/main.c +++ b/src/or/main.c @@ -191,32 +191,6 @@ int quiet_level = 0; * ****************************************************************************/ -#if 0 && defined(USE_BUFFEREVENTS) -static void -free_old_inbuf(connection_t *conn) -{ - if (! conn->inbuf) - return; - - tor_assert(conn->outbuf); - tor_assert(buf_datalen(conn->inbuf) == 0); - tor_assert(buf_datalen(conn->outbuf) == 0); - buf_free(conn->inbuf); - buf_free(conn->outbuf); - conn->inbuf = conn->outbuf = NULL; - - if (conn->read_event) { - event_del(conn->read_event); - tor_event_free(conn->read_event); - } - if (conn->write_event) { - event_del(conn->read_event); - tor_event_free(conn->write_event); - } - conn->read_event = conn->write_event = NULL; -} -#endif - #if defined(_WIN32) && defined(USE_BUFFEREVENTS) /** Remove the kernel-space send and receive buffers for s. For use * with IOCP only. */ @@ -946,18 +920,6 @@ conn_close_if_marked(int i) * would make much more sense to react in * connection_handle_read_impl, or to just stop reading in * mark_and_flush */ -#if 0 -#define MARKED_READING_RATE 180 - static ratelim_t marked_read_lim = RATELIM_INIT(MARKED_READING_RATE); - char *m; - if ((m = rate_limit_log(&marked_read_lim, now))) { - log_warn(LD_BUG, "Marked connection (fd %d, type %s, state %s) " - "is still reading; that shouldn't happen.%s", - (int)conn->s, conn_type_to_string(conn->type), - conn_state_to_string(conn->type, conn->state), m); - tor_free(m); - } -#endif conn->read_blocked_on_bw = 1; connection_stop_reading(conn); }

1 0

[tor/master] Credit the patch
by nickm＠torproject.org 06 Feb '16

06 Feb '16

commit 928380e23176040a1c1c04fc517e0808f028901c Author: Nick Mathewson <nickm(a)torproject.org> Date: Sat Feb 6 14:12:48 2016 -0500 Credit the patch --- changes/bug18258 | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/changes/bug18258 b/changes/bug18258 index d82b217..00e1224 100644 --- a/changes/bug18258 +++ b/changes/bug18258 @@ -1,7 +1,8 @@ o Code simplification and refactoring: - Remove specialized code for non-inplace AES_CTR. 99% of our AES is inplace, so there's no need to have a separate implementation - for the non-inplace code. Closes ticket 18258. + for the non-inplace code. Closes ticket 18258. Patch from + Malek. o New requirements: - Tor no longer supports versions of OpenSSL with a broken

1 0

[tor/master] Fix spaces.
by nickm＠torproject.org 06 Feb '16

06 Feb '16

commit 31a27729b9f290f46ad2f323907dc14a9bff006e Author: Nick Mathewson <nickm(a)torproject.org> Date: Sat Feb 6 14:00:24 2016 -0500 Fix spaces. --- src/common/aes.c | 1 + src/common/aes.h | 1 + src/common/crypto.c | 1 + src/test/test_tortls.c | 2 +- 4 files changed, 4 insertions(+), 1 deletion(-) diff --git a/src/common/aes.c b/src/common/aes.c index 44b9921..fd20433 100644 --- a/src/common/aes.c +++ b/src/common/aes.c @@ -380,3 +380,4 @@ aes_set_iv(aes_cnt_cipher_t *cipher, const char *iv) } #endif + diff --git a/src/common/aes.h b/src/common/aes.h index c1e5c53..bd04565 100644 --- a/src/common/aes.h +++ b/src/common/aes.h @@ -23,3 +23,4 @@ int evaluate_evp_for_aes(int force_value); int evaluate_ctr_for_aes(void); #endif + diff --git a/src/common/crypto.c b/src/common/crypto.c index 5714eea..372869d 100644 --- a/src/common/crypto.c +++ b/src/common/crypto.c @@ -3155,3 +3155,4 @@ crypto_global_cleanup(void) } /** @} */ + diff --git a/src/test/test_tortls.c b/src/test/test_tortls.c index 71b3863..138485c 100644 --- a/src/test/test_tortls.c +++ b/src/test/test_tortls.c @@ -1612,7 +1612,7 @@ test_tortls_block_renegotiation(void *ignored) tt_assert(!(tls->ssl->s3->flags & SSL3_FLAGS_ALLOW_UNSAFE_LEGACY_RENEGOTIATION)); #endif - + done: tor_free(tls->ssl->s3); tor_free(tls->ssl);

1 0

[tor/master] Removed aes_crypt, left only aes_crypt_inplace. Removed should_use_openssl_CTR, was used for openssl 1.0.0 bug.
by nickm＠torproject.org 06 Feb '16

06 Feb '16

commit a9cd291753b022aab1a07525f19e113c9da2cd0e Author: Malek <malek(a)mit.edu> Date: Sat Feb 6 12:05:32 2016 -0500 Removed aes_crypt, left only aes_crypt_inplace. Removed should_use_openssl_CTR, was used for openssl 1.0.0 bug. --- src/common/aes.c | 159 ++++++++-------------------------------------------- src/common/aes.h | 3 - src/common/crypto.c | 7 ++- 3 files changed, 27 insertions(+), 142 deletions(-) diff --git a/src/common/aes.c b/src/common/aes.c index 89c99c1..44b9921 100644 --- a/src/common/aes.c +++ b/src/common/aes.c @@ -101,18 +101,6 @@ aes_cipher_free(aes_cnt_cipher_t *cipher_) EVP_CIPHER_CTX_free(cipher); } void -aes_crypt(aes_cnt_cipher_t *cipher_, const char *input, size_t len, - char *output) -{ - int outl; - EVP_CIPHER_CTX *cipher = (EVP_CIPHER_CTX *) cipher_; - - tor_assert(len < INT_MAX); - - EVP_EncryptUpdate(cipher, (unsigned char*)output, - &outl, (const unsigned char *)input, (int)len); -} -void aes_crypt_inplace(aes_cnt_cipher_t *cipher_, char *data, size_t len) { int outl; @@ -181,10 +169,6 @@ struct aes_cnt_cipher { * we're testing it or because we have hardware acceleration configured */ static int should_use_EVP = 0; -/** True iff we have tested the counter-mode implementation and found that it - * doesn't have the counter-mode bug from OpenSSL 1.0.0. */ -static int should_use_openssl_CTR = 0; - /** Check whether we should use the EVP interface for AES. If force_val * is nonnegative, we use use EVP iff it is true. Otherwise, we use EVP * if there is an engine enabled for aes-ecb. */ @@ -249,13 +233,9 @@ evaluate_ctr_for_aes(void) if (fast_memneq(output, encrypt_zero, 16)) { /* Counter mode is buggy */ - log_notice(LD_CRYPTO, "This OpenSSL has a buggy version of counter mode; " - "not using it."); - } else { - /* Counter mode is okay */ - log_info(LD_CRYPTO, "This OpenSSL has a good implementation of counter " - "mode; using it."); - should_use_openssl_CTR = 1; + log_err(LD_CRYPTO, "This OpenSSL has a buggy version of counter mode; " + "quitting tor."); + exit(1); } return 0; } @@ -266,29 +246,6 @@ evaluate_ctr_for_aes(void) #define COUNTER(c, n) ((c)->counter ## n) #endif -/** - * Helper function: set cipher's internal buffer to the encrypted - * value of the current counter. - */ -static inline void -aes_fill_buf_(aes_cnt_cipher_t *cipher) -{ - /* We don't currently use OpenSSL's counter mode implementation because: - * 1) some versions have known bugs - * 2) its attitude towards IVs is not our own - * 3) changing the counter position was not trivial, last time I looked. - * None of these issues are insurmountable in principle. - */ - - if (cipher->using_evp) { - int outl=16, inl=16; - EVP_EncryptUpdate(&cipher->key.evp, cipher->buf, &outl, - cipher->ctr_buf.buf, inl); - } else { - AES_encrypt(cipher->ctr_buf.buf, cipher->buf, &cipher->key.aes); - } -} - static void aes_set_key(aes_cnt_cipher_t *cipher, const char *key, int key_bits); static void aes_set_iv(aes_cnt_cipher_t *cipher, const char *iv); @@ -341,10 +298,7 @@ aes_set_key(aes_cnt_cipher_t *cipher, const char *key, int key_bits) cipher->pos = 0; - if (should_use_openssl_CTR) - memset(cipher->buf, 0, sizeof(cipher->buf)); - else - aes_fill_buf_(cipher); + memset(cipher->buf, 0, sizeof(cipher->buf)); } /** Release storage held by cipher @@ -380,63 +334,6 @@ evp_block128_fn(const uint8_t in[16], EVP_EncryptUpdate(ctx, out, &outl, in, inl); } -/** Encrypt len bytes from input, storing the result in - * output. Uses the key in cipher, and advances the counter - * by len bytes as it encrypts. - */ -void -aes_crypt(aes_cnt_cipher_t *cipher, const char *input, size_t len, - char *output) -{ - if (should_use_openssl_CTR) { - if (cipher->using_evp) { - /* In openssl 1.0.0, there's an if'd out EVP_aes_128_ctr in evp.h. If - * it weren't disabled, it might be better just to use that. - */ - CRYPTO_ctr128_encrypt((const unsigned char *)input, - (unsigned char *)output, - len, - &cipher->key.evp, - cipher->ctr_buf.buf, - cipher->buf, - &cipher->pos, - evp_block128_fn); - } else { - AES_ctr128_encrypt((const unsigned char *)input, - (unsigned char *)output, - len, - &cipher->key.aes, - cipher->ctr_buf.buf, - cipher->buf, - &cipher->pos); - } - return; - } else { - int c = cipher->pos; - if (PREDICT_UNLIKELY(!len)) return; - - while (1) { - do { - if (len-- == 0) { cipher->pos = c; return; } - *(output++) = *(input++) ^ cipher->buf[c]; - } while (++c != 16); - cipher->pos = c = 0; - if (PREDICT_UNLIKELY(! ++COUNTER(cipher, 0))) { - if (PREDICT_UNLIKELY(! ++COUNTER(cipher, 1))) { - if (PREDICT_UNLIKELY(! ++COUNTER(cipher, 2))) { - ++COUNTER(cipher, 3); - UPDATE_CTR_BUF(cipher, 3); - } - UPDATE_CTR_BUF(cipher, 2); - } - UPDATE_CTR_BUF(cipher, 1); - } - UPDATE_CTR_BUF(cipher, 0); - aes_fill_buf_(cipher); - } - } -} - /** Encrypt len bytes from input, storing the results in place. * Uses the key in cipher, and advances the counter by len bytes * as it encrypts. @@ -444,32 +341,26 @@ aes_crypt(aes_cnt_cipher_t *cipher, const char *input, size_t len, void aes_crypt_inplace(aes_cnt_cipher_t *cipher, char *data, size_t len) { - if (should_use_openssl_CTR) { - aes_crypt(cipher, data, len, data); - return; + if (cipher->using_evp) { + /* In openssl 1.0.0, there's an if'd out EVP_aes_128_ctr in evp.h. If + * it weren't disabled, it might be better just to use that. + */ + CRYPTO_ctr128_encrypt((const unsigned char *)data, + (unsigned char *)data, + len, + &cipher->key.evp, + cipher->ctr_buf.buf, + cipher->buf, + &cipher->pos, + evp_block128_fn); } else { - int c = cipher->pos; - if (PREDICT_UNLIKELY(!len)) return; - - while (1) { - do { - if (len-- == 0) { cipher->pos = c; return; } - *(data++) ^= cipher->buf[c]; - } while (++c != 16); - cipher->pos = c = 0; - if (PREDICT_UNLIKELY(! ++COUNTER(cipher, 0))) { - if (PREDICT_UNLIKELY(! ++COUNTER(cipher, 1))) { - if (PREDICT_UNLIKELY(! ++COUNTER(cipher, 2))) { - ++COUNTER(cipher, 3); - UPDATE_CTR_BUF(cipher, 3); - } - UPDATE_CTR_BUF(cipher, 2); - } - UPDATE_CTR_BUF(cipher, 1); - } - UPDATE_CTR_BUF(cipher, 0); - aes_fill_buf_(cipher); - } + AES_ctr128_encrypt((const unsigned char *)data, + (unsigned char *)data, + len, + &cipher->key.aes, + cipher->ctr_buf.buf, + cipher->buf, + &cipher->pos); } } @@ -486,10 +377,6 @@ aes_set_iv(aes_cnt_cipher_t *cipher, const char *iv) #endif cipher->pos = 0; memcpy(cipher->ctr_buf.buf, iv, 16); - - if (!should_use_openssl_CTR) - aes_fill_buf_(cipher); } #endif - diff --git a/src/common/aes.h b/src/common/aes.h index 5500db7..c1e5c53 100644 --- a/src/common/aes.h +++ b/src/common/aes.h @@ -17,12 +17,9 @@ typedef struct aes_cnt_cipher aes_cnt_cipher_t; aes_cnt_cipher_t* aes_new_cipher(const char *key, const char *iv); void aes_cipher_free(aes_cnt_cipher_t *cipher); -void aes_crypt(aes_cnt_cipher_t *cipher, const char *input, size_t len, - char *output); void aes_crypt_inplace(aes_cnt_cipher_t *cipher, char *data, size_t len); int evaluate_evp_for_aes(int force_value); int evaluate_ctr_for_aes(void); #endif - diff --git a/src/common/crypto.c b/src/common/crypto.c index bc659b1..956ec88 100644 --- a/src/common/crypto.c +++ b/src/common/crypto.c @@ -1509,7 +1509,8 @@ crypto_cipher_encrypt(crypto_cipher_t *env, char *to, tor_assert(to); tor_assert(fromlen < SIZE_T_CEILING); - aes_crypt(env->cipher, from, fromlen, to); + memcpy(to, from, fromlen); + aes_crypt_inplace(env->cipher, to, fromlen); return 0; } @@ -1526,7 +1527,8 @@ crypto_cipher_decrypt(crypto_cipher_t *env, char *to, tor_assert(to); tor_assert(fromlen < SIZE_T_CEILING); - aes_crypt(env->cipher, from, fromlen, to); + memcpy(to, from, fromlen); + aes_crypt_inplace(env->cipher, to, fromlen); return 0; } @@ -3154,4 +3156,3 @@ crypto_global_cleanup(void) } /** @} */ -

1 0

[tor/master] changes file for bug 18258.
by nickm＠torproject.org 06 Feb '16

06 Feb '16

commit ef5f35971f37387d04c9cc42048b7ea24a9bcaec Author: Nick Mathewson <nickm(a)torproject.org> Date: Sat Feb 6 13:51:46 2016 -0500 changes file for bug 18258. --- changes/bug18258 | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/changes/bug18258 b/changes/bug18258 new file mode 100644 index 0000000..d82b217 --- /dev/null +++ b/changes/bug18258 @@ -0,0 +1,11 @@ + o Code simplification and refactoring: + - Remove specialized code for non-inplace AES_CTR. 99% of our AES + is inplace, so there's no need to have a separate implementation + for the non-inplace code. Closes ticket 18258. + + o New requirements: + - Tor no longer supports versions of OpenSSL with a broken + implementation of counter mode. (This bug was present in OpenSSL + 1.0.0, and was fixed in OpenSSL 1.0.0a.) Tor still detects, but + no longer runs with, these versions. +

1 0

[tor/master] Merge branch 'cleaned_aes_crypt'
by nickm＠torproject.org 06 Feb '16

06 Feb '16

commit 03371e3d3cfe6c3a1859b316c24061a0d7e5d1bd Merge: f3a0de0 ef5f359 Author: Nick Mathewson <nickm(a)torproject.org> Date: Sat Feb 6 13:54:09 2016 -0500 Merge branch 'cleaned_aes_crypt' changes/bug18258 | 11 ++++ src/common/aes.c | 159 ++++++++-------------------------------------------- src/common/aes.h | 3 - src/common/crypto.c | 7 ++- 4 files changed, 38 insertions(+), 142 deletions(-)

1 0

[tor/master] add a changes file
by nickm＠torproject.org 06 Feb '16

06 Feb '16

commit f3a0de0c2c7ac80ca77ccec49e625ea44986751e Author: Nick Mathewson <nickm(a)torproject.org> Date: Sat Feb 6 12:37:56 2016 -0500 add a changes file --- changes/bug18259 | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/changes/bug18259 b/changes/bug18259 new file mode 100644 index 0000000..1084b8f --- /dev/null +++ b/changes/bug18259 @@ -0,0 +1,4 @@ + o Code simplification and refactoring: + - Simplify return types for some crypto functions that can't + actually fail. Patch from Hassan Alsibyani. Closes ticket + 18259.

1 0