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

[tor/release-0.2.4] NULL out conns on tlschans when freeing in case channel_run_cleanup() is late; fixes bug 9602
by arma＠torproject.org 10 Feb '14

10 Feb '14

commit 707c1e2e263fd34f70a5f780e77820d667ba2931 Author: Andrea Shepard <andrea(a)torproject.org> Date: Thu Feb 6 14:47:34 2014 -0800 NULL out conns on tlschans when freeing in case channel_run_cleanup() is late; fixes bug 9602 --- changes/bug9602 | 4 + src/or/channeltls.c | 193 +++++++++++++++++++++++++++++++----------------- src/or/connection.c | 16 ++++ src/or/connection_or.c | 5 ++ 4 files changed, 149 insertions(+), 69 deletions(-) diff --git a/changes/bug9602 b/changes/bug9602 new file mode 100644 index 0000000..9e9a3ca --- /dev/null +++ b/changes/bug9602 @@ -0,0 +1,4 @@ + o Bugfixes + - Null out orconn->chan->conn when closing orconn in case orconn is freed + before channel_run_cleanup() gets to orconn->chan, and handle the null + conn edge case correctly in channel_tls_t methods. Fixes bug #9602. diff --git a/src/or/channeltls.c b/src/or/channeltls.c index f751c0d..495f856 100644 --- a/src/or/channeltls.c +++ b/src/or/channeltls.c @@ -365,7 +365,7 @@ channel_tls_describe_transport_method(channel_t *chan) tor_assert(chan); - tlschan = BASE_CHAN_TO_TLS(chan); + tlschan = BASE_CHAN_TO_TLS(chan); if (tlschan->conn) { id = TO_CONN(tlschan->conn)->global_identifier; @@ -394,15 +394,18 @@ channel_tls_describe_transport_method(channel_t *chan) static int channel_tls_get_remote_addr_method(channel_t *chan, tor_addr_t *addr_out) { + int rv = 0; channel_tls_t *tlschan = BASE_CHAN_TO_TLS(chan); tor_assert(tlschan); tor_assert(addr_out); - tor_assert(tlschan->conn); - tor_addr_copy(addr_out, &(TO_CONN(tlschan->conn)->addr)); + if (tlschan->conn) { + tor_addr_copy(addr_out, &(TO_CONN(tlschan->conn)->addr)); + rv = 1; + } else tor_addr_make_unspec(addr_out); - return 1; + return rv; } /** @@ -426,41 +429,43 @@ channel_tls_get_remote_descr_method(channel_t *chan, int flags) char *addr_str; tor_assert(tlschan); - tor_assert(tlschan->conn); - - conn = TO_CONN(tlschan->conn); - switch (flags) { - case 0: - /* Canonical address with port*/ - tor_snprintf(buf, MAX_DESCR_LEN + 1, - "%s:%u", conn->address, conn->port); - answer = buf; - break; - case GRD_FLAG_ORIGINAL: - /* Actual address with port */ - addr_str = tor_dup_addr(&(tlschan->conn->real_addr)); - tor_snprintf(buf, MAX_DESCR_LEN + 1, - "%s:%u", addr_str, conn->port); - tor_free(addr_str); - answer = buf; - break; - case GRD_FLAG_ADDR_ONLY: - /* Canonical address, no port */ - strlcpy(buf, conn->address, sizeof(buf)); - answer = buf; - break; - case GRD_FLAG_ORIGINAL|GRD_FLAG_ADDR_ONLY: - /* Actual address, no port */ - addr_str = tor_dup_addr(&(tlschan->conn->real_addr)); - strlcpy(buf, addr_str, sizeof(buf)); - tor_free(addr_str); - answer = buf; - break; - - default: - /* Something's broken in channel.c */ - tor_assert(1); + if (tlschan->conn) { + conn = TO_CONN(tlschan->conn); + switch (flags) { + case 0: + /* Canonical address with port*/ + tor_snprintf(buf, MAX_DESCR_LEN + 1, + "%s:%u", conn->address, conn->port); + answer = buf; + break; + case GRD_FLAG_ORIGINAL: + /* Actual address with port */ + addr_str = tor_dup_addr(&(tlschan->conn->real_addr)); + tor_snprintf(buf, MAX_DESCR_LEN + 1, + "%s:%u", addr_str, conn->port); + tor_free(addr_str); + answer = buf; + break; + case GRD_FLAG_ADDR_ONLY: + /* Canonical address, no port */ + strlcpy(buf, conn->address, sizeof(buf)); + answer = buf; + break; + case GRD_FLAG_ORIGINAL|GRD_FLAG_ADDR_ONLY: + /* Actual address, no port */ + addr_str = tor_dup_addr(&(tlschan->conn->real_addr)); + strlcpy(buf, addr_str, sizeof(buf)); + tor_free(addr_str); + answer = buf; + break; + default: + /* Something's broken in channel.c */ + tor_assert(1); + } + } else { + strlcpy(buf, "(No connection)", sizeof(buf)); + answer = buf; } return answer; @@ -480,9 +485,16 @@ channel_tls_has_queued_writes_method(channel_t *chan) channel_tls_t *tlschan = BASE_CHAN_TO_TLS(chan); tor_assert(tlschan); - tor_assert(tlschan->conn); + if (!(tlschan->conn)) { + log_info(LD_CHANNEL, + "something called has_queued_writes on a tlschan " + "(%p with ID " U64_FORMAT " but no conn", + chan, U64_PRINTF_ARG(chan->global_identifier)); + } - outbuf_len = connection_get_outbuf_len(TO_CONN(tlschan->conn)); + outbuf_len = (tlschan->conn != NULL) ? + connection_get_outbuf_len(TO_CONN(tlschan->conn)) : + 0; return (outbuf_len > 0); } @@ -502,24 +514,26 @@ channel_tls_is_canonical_method(channel_t *chan, int req) channel_tls_t *tlschan = BASE_CHAN_TO_TLS(chan); tor_assert(tlschan); - tor_assert(tlschan->conn); - switch (req) { - case 0: - answer = tlschan->conn->is_canonical; - break; - case 1: - /* - * Is the is_canonical bit reliable? In protocols version 2 and up - * we get the canonical address from a NETINFO cell, but in older - * versions it might be based on an obsolete descriptor. - */ - answer = (tlschan->conn->link_proto >= 2); - break; - default: - /* This shouldn't happen; channel.c is broken if it does */ - tor_assert(1); + if (tlschan->conn) { + switch (req) { + case 0: + answer = tlschan->conn->is_canonical; + break; + case 1: + /* + * Is the is_canonical bit reliable? In protocols version 2 and up + * we get the canonical address from a NETINFO cell, but in older + * versions it might be based on an obsolete descriptor. + */ + answer = (tlschan->conn->link_proto >= 2); + break; + default: + /* This shouldn't happen; channel.c is broken if it does */ + tor_assert(1); + } } + /* else return 0 for tlschan->conn == NULL */ return answer; } @@ -540,6 +554,15 @@ channel_tls_matches_extend_info_method(channel_t *chan, tor_assert(tlschan); tor_assert(extend_info); + /* Never match if we have no conn */ + if (!(tlschan->conn)) { + log_info(LD_CHANNEL, + "something called matches_extend_info on a tlschan " + "(%p with ID " U64_FORMAT " but no conn", + chan, U64_PRINTF_ARG(chan->global_identifier)); + return 0; + } + return (tor_addr_eq(&(extend_info->addr), &(TO_CONN(tlschan->conn)->addr)) && (extend_info->port == TO_CONN(tlschan->conn)->port)); @@ -561,7 +584,15 @@ channel_tls_matches_target_method(channel_t *chan, tor_assert(tlschan); tor_assert(target); - tor_assert(tlschan->conn); + + /* Never match if we have no conn */ + if (!(tlschan->conn)) { + log_info(LD_CHANNEL, + "something called matches_target on a tlschan " + "(%p with ID " U64_FORMAT " but no conn", + chan, U64_PRINTF_ARG(chan->global_identifier)); + return 0; + } return tor_addr_eq(&(tlschan->conn->real_addr), target); } @@ -577,14 +608,22 @@ static int channel_tls_write_cell_method(channel_t *chan, cell_t *cell) { channel_tls_t *tlschan = BASE_CHAN_TO_TLS(chan); + int written = 0; tor_assert(tlschan); tor_assert(cell); - tor_assert(tlschan->conn); - connection_or_write_cell_to_buf(cell, tlschan->conn); + if (tlschan->conn) { + connection_or_write_cell_to_buf(cell, tlschan->conn); + ++written; + } else { + log_info(LD_CHANNEL, + "something called write_cell on a tlschan " + "(%p with ID " U64_FORMAT " but no conn", + chan, U64_PRINTF_ARG(chan->global_identifier)); + } - return 1; + return written; } /** @@ -600,18 +639,26 @@ channel_tls_write_packed_cell_method(channel_t *chan, { channel_tls_t *tlschan = BASE_CHAN_TO_TLS(chan); size_t cell_network_size = get_cell_network_size(chan->wide_circ_ids); + int written = 0; tor_assert(tlschan); tor_assert(packed_cell); - tor_assert(tlschan->conn); - connection_write_to_buf(packed_cell->body, cell_network_size, - TO_CONN(tlschan->conn)); + if (tlschan->conn) { + connection_write_to_buf(packed_cell->body, cell_network_size, + TO_CONN(tlschan->conn)); - /* This is where the cell is finished; used to be done from relay.c */ - packed_cell_free(packed_cell); + /* This is where the cell is finished; used to be done from relay.c */ + packed_cell_free(packed_cell); + ++written; + } else { + log_info(LD_CHANNEL, + "something called write_packed_cell on a tlschan " + "(%p with ID " U64_FORMAT " but no conn", + chan, U64_PRINTF_ARG(chan->global_identifier)); + } - return 1; + return written; } /** @@ -625,14 +672,22 @@ static int channel_tls_write_var_cell_method(channel_t *chan, var_cell_t *var_cell) { channel_tls_t *tlschan = BASE_CHAN_TO_TLS(chan); + int written = 0; tor_assert(tlschan); tor_assert(var_cell); - tor_assert(tlschan->conn); - connection_or_write_var_cell_to_buf(var_cell, tlschan->conn); + if (tlschan->conn) { + connection_or_write_var_cell_to_buf(var_cell, tlschan->conn); + ++written; + } else { + log_info(LD_CHANNEL, + "something called write_var_cell on a tlschan " + "(%p with ID " U64_FORMAT " but no conn", + chan, U64_PRINTF_ARG(chan->global_identifier)); + } - return 1; + return written; } /************************************************* diff --git a/src/or/connection.c b/src/or/connection.c index 062c971..4f74a1d 100644 --- a/src/or/connection.c +++ b/src/or/connection.c @@ -514,6 +514,22 @@ connection_free_(connection_t *conn) or_handshake_state_free(or_conn->handshake_state); or_conn->handshake_state = NULL; tor_free(or_conn->nickname); + if (or_conn->chan) { + /* Owww, this shouldn't happen, but... */ + log_info(LD_CHANNEL, + "Freeing orconn at %p, saw channel %p with ID " + U64_FORMAT " left un-NULLed", + or_conn, TLS_CHAN_TO_BASE(or_conn->chan), + U64_PRINTF_ARG( + TLS_CHAN_TO_BASE(or_conn->chan)->global_identifier)); + if (!(TLS_CHAN_TO_BASE(or_conn->chan)->state == CHANNEL_STATE_CLOSED || + TLS_CHAN_TO_BASE(or_conn->chan)->state == CHANNEL_STATE_ERROR)) { + channel_close_for_error(TLS_CHAN_TO_BASE(or_conn->chan)); + } + + or_conn->chan->conn = NULL; + or_conn->chan = NULL; + } } if (conn->type == CONN_TYPE_AP) { entry_connection_t *entry_conn = TO_ENTRY_CONN(conn); diff --git a/src/or/connection_or.c b/src/or/connection_or.c index 3d16e14..8e7cd9e 100644 --- a/src/or/connection_or.c +++ b/src/or/connection_or.c @@ -622,6 +622,11 @@ connection_or_about_to_close(or_connection_t *or_conn) /* Tell the controlling channel we're closed */ if (or_conn->chan) { channel_closed(TLS_CHAN_TO_BASE(or_conn->chan)); + /* + * NULL this out because the channel might hang around a little + * longer before channel_run_cleanup() gets it. + */ + or_conn->chan->conn = NULL; or_conn->chan = NULL; }

1 0

[tor/release-0.2.4] Make sure orconn->chan gets nulled out when channels exit from channel_free_all() too
by arma＠torproject.org 10 Feb '14

10 Feb '14

commit c330d63ff7614b2382dfa0e84da0b40ed6348ced Author: Andrea Shepard <andrea(a)torproject.org> Date: Sat Feb 8 14:05:51 2014 -0800 Make sure orconn->chan gets nulled out when channels exit from channel_free_all() too --- src/or/channeltls.c | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/src/or/channeltls.c b/src/or/channeltls.c index 495f856..d5428c1 100644 --- a/src/or/channeltls.c +++ b/src/or/channeltls.c @@ -53,6 +53,7 @@ static void channel_tls_common_init(channel_tls_t *tlschan); static void channel_tls_close_method(channel_t *chan); static const char * channel_tls_describe_transport_method(channel_t *chan); +static void channel_tls_free_method(channel_t *chan); static int channel_tls_get_remote_addr_method(channel_t *chan, tor_addr_t *addr_out); static const char * @@ -112,6 +113,7 @@ channel_tls_common_init(channel_tls_t *tlschan) chan->state = CHANNEL_STATE_OPENING; chan->close = channel_tls_close_method; chan->describe_transport = channel_tls_describe_transport_method; + chan->free = channel_tls_free_method; chan->get_remote_addr = channel_tls_get_remote_addr_method; chan->get_remote_descr = channel_tls_get_remote_descr_method; chan->has_queued_writes = channel_tls_has_queued_writes_method; @@ -384,6 +386,30 @@ channel_tls_describe_transport_method(channel_t *chan) } /** + * Free a channel_tls_t + * + * This is called by the generic channel layer when freeing a channel_tls_t; + * this happens either on a channel which has already reached + * CHANNEL_STATE_CLOSED or CHANNEL_STATE_ERROR from channel_run_cleanup() or + * on shutdown from channel_free_all(). In the latter case we might still + * have an orconn active (which connection_free_all() will get to later), + * so we should null out its channel pointer now. + */ + +static void +channel_tls_free_method(channel_t *chan) +{ + channel_tls_t *tlschan = BASE_CHAN_TO_TLS(chan); + + tor_assert(tlschan); + + if (tlschan->conn) { + tlschan->conn->chan = NULL; + tlschan->conn = NULL; + } +} + +/** * Get the remote address of a channel_tls_t * * This implements the get_remote_addr method for channel_tls_t; copy the

1 0

[tor/release-0.2.4] Survive fedora's openssl in our benchmarks
by arma＠torproject.org 10 Feb '14

10 Feb '14

commit 9bb34aa897e4ecac27a6f8d50a659803f73c6cb9 Author: Nick Mathewson <nickm(a)torproject.org> Date: Fri Feb 7 17:36:11 2014 -0500 Survive fedora's openssl in our benchmarks Apparently fedora currently has ECDH but not P224. This isn't a huge deal, since we no longer use OpenSSL's P224 ever (see #9780 and 72c1e5acfe1c6). But we shouldn't have segfaulting benchmarks really. Fixes bug 10835; bugfix on 0.2.4.8-alpha. --- changes/bug10835 | 4 ++++ src/test/bench.c | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/changes/bug10835 b/changes/bug10835 new file mode 100644 index 0000000..9df7bdd --- /dev/null +++ b/changes/bug10835 @@ -0,0 +1,4 @@ + o Minor bugfixes (testing): + - Fix a segmentation fault in our benchmark code when running with + Fedora's OpenSSL package, or any other OpenSSL that provides + ECDH but not P224. Fixes bug 10835; bugfix on 0.2.4.8-alpha. diff --git a/src/test/bench.c b/src/test/bench.c index 5a8d21d..706b8bc 100644 --- a/src/test/bench.c +++ b/src/test/bench.c @@ -440,6 +440,10 @@ bench_ecdh_impl(int nid, const char *name) ssize_t slen_a, slen_b; EC_KEY *dh_a = EC_KEY_new_by_curve_name(nid); EC_KEY *dh_b = EC_KEY_new_by_curve_name(nid); + if (!dh_a || !dh_b) { + puts("Skipping. (No implementation?)"); + return; + } EC_KEY_generate_key(dh_a); EC_KEY_generate_key(dh_b);

1 0

[tor/release-0.2.4] Some anti-forensics paranoia...
by arma＠torproject.org 10 Feb '14

10 Feb '14

commit 01132c93fdfd634475c6d56455efdfe1cff1fe83 Author: Florent Daigniere <nextgens(a)freenetproject.org> Date: Sun Feb 2 18:40:30 2014 +0000 Some anti-forensics paranoia... sed -i 's/BN_free/BN_clear_free/g' --- src/common/crypto.c | 20 ++++++++++---------- src/common/tortls.c | 2 +- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/src/common/crypto.c b/src/common/crypto.c index 940a756..925beb3 100644 --- a/src/common/crypto.c +++ b/src/common/crypto.c @@ -486,7 +486,7 @@ crypto_pk_generate_key_with_bits(crypto_pk_t *env, int bits) r = NULL; done: if (e) - BN_free(e); + BN_clear_free(e); if (r) RSA_free(r); } @@ -1922,7 +1922,7 @@ crypto_set_tls_dh_prime(const char *dynamic_dh_modulus_fname) /* If the space is occupied, free the previous TLS DH prime */ if (dh_param_p_tls) { - BN_free(dh_param_p_tls); + BN_clear_free(dh_param_p_tls); dh_param_p_tls = NULL; } @@ -2084,8 +2084,8 @@ crypto_dh_generate_public(crypto_dh_t *dh) log_warn(LD_CRYPTO, "Weird! Our own DH key was invalid. I guess once-in-" "the-universe chances really do happen. Trying again."); /* Free and clear the keys, so OpenSSL will actually try again. */ - BN_free(dh->dh->pub_key); - BN_free(dh->dh->priv_key); + BN_clear_free(dh->dh->pub_key); + BN_clear_free(dh->dh->priv_key); dh->dh->pub_key = dh->dh->priv_key = NULL; goto again; } @@ -2147,10 +2147,10 @@ tor_check_dh_key(int severity, BIGNUM *bn) log_fn(severity, LD_CRYPTO, "DH key must be at most p-2."); goto err; } - BN_free(x); + BN_clear_free(x); return 0; err: - BN_free(x); + BN_clear_free(x); s = BN_bn2hex(bn); log_fn(severity, LD_CRYPTO, "Rejecting insecure DH key [%s]", s); OPENSSL_free(s); @@ -2209,7 +2209,7 @@ crypto_dh_compute_secret(int severity, crypto_dh_t *dh, done: crypto_log_errors(LOG_WARN, "completing DH handshake"); if (pubkey_bn) - BN_free(pubkey_bn); + BN_clear_free(pubkey_bn); if (secret_tmp) { memwipe(secret_tmp, 0, secret_tmp_len); tor_free(secret_tmp); @@ -3118,11 +3118,11 @@ crypto_global_cleanup(void) ERR_free_strings(); if (dh_param_p) - BN_free(dh_param_p); + BN_clear_free(dh_param_p); if (dh_param_p_tls) - BN_free(dh_param_p_tls); + BN_clear_free(dh_param_p_tls); if (dh_param_g) - BN_free(dh_param_g); + BN_clear_free(dh_param_g); #ifndef DISABLE_ENGINES ENGINE_cleanup(); diff --git a/src/common/tortls.c b/src/common/tortls.c index 60444f1..886ee0d 100644 --- a/src/common/tortls.c +++ b/src/common/tortls.c @@ -694,7 +694,7 @@ tor_tls_create_certificate(crypto_pk_t *rsa, if (pkey) EVP_PKEY_free(pkey); if (serial_number) - BN_free(serial_number); + BN_clear_free(serial_number); if (name) X509_NAME_free(name); if (name_issuer)

1 0

[tor/release-0.2.4] changelog for 10793
by arma＠torproject.org 10 Feb '14

10 Feb '14

commit 9e2de8cecc97a9bc6d346d309a1bd6a800a34751 Author: Nick Mathewson <nickm(a)torproject.org> Date: Mon Feb 3 10:47:49 2014 -0500 changelog for 10793 --- changes/bug10793 | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/changes/bug10793 b/changes/bug10793 new file mode 100644 index 0000000..24c4025 --- /dev/null +++ b/changes/bug10793 @@ -0,0 +1,4 @@ + o Minor features (security): + - Always clear OpenSSL bignums before freeing them--even bignums + that don't contain secrets. Resolves ticket 10793. Patch by + Florent Daigniere.

1 0

[tor/release-0.2.4] Add a missing include
by arma＠torproject.org 10 Feb '14

10 Feb '14

commit 655adbf6677fde28ba37f029701083901eda7efd Author: Nick Mathewson <nickm(a)torproject.org> Date: Thu Feb 6 13:25:36 2014 -0500 Add a missing include --- src/or/status.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/or/status.c b/src/or/status.c index 4c42155..69f92ed 100644 --- a/src/or/status.c +++ b/src/or/status.c @@ -15,6 +15,7 @@ #include "circuitlist.h" #include "main.h" #include "hibernate.h" +#include "rephist.h" /** Return the total number of circuits. */ static int

1 0

[tor/release-0.2.4] Attribute bug 9602 to a version.
by arma＠torproject.org 10 Feb '14

10 Feb '14

commit a7e946596d6da9aca80456141b7fddbc198c217c Author: Nick Mathewson <nickm(a)torproject.org> Date: Fri Feb 7 10:38:00 2014 -0500 Attribute bug 9602 to a version. --- changes/bug9602 | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/changes/bug9602 b/changes/bug9602 index 9e9a3ca..2dc13c4 100644 --- a/changes/bug9602 +++ b/changes/bug9602 @@ -1,4 +1,5 @@ o Bugfixes - Null out orconn->chan->conn when closing orconn in case orconn is freed before channel_run_cleanup() gets to orconn->chan, and handle the null - conn edge case correctly in channel_tls_t methods. Fixes bug #9602. + conn edge case correctly in channel_tls_t methods. Fixes bug #9602; + bugfix on 0.2.4.4-alpha.

1 0

[tor/release-0.2.4] Merge remote-tracking branch 'public/bug9716_024' into maint-0.2.4
by arma＠torproject.org 10 Feb '14

10 Feb '14

commit b4e8d8dc0e6f942346895a5cff123db3aa1f94d5 Merge: 9e2de8c a2754d4 Author: Nick Mathewson <nickm(a)torproject.org> Date: Thu Feb 6 16:29:08 2014 -0500 Merge remote-tracking branch 'public/bug9716_024' into maint-0.2.4 changes/bug9716 | 4 ++++ src/or/connection.c | 23 ++++++++++++++++++++++- 2 files changed, 26 insertions(+), 1 deletion(-)

1 0

[tor/release-0.2.4] Merge branch 'maint-0.2.4' into release-0.2.4
by arma＠torproject.org 10 Feb '14

10 Feb '14

1 0

[tor/release-0.2.4] Merge remote-tracking branch 'andrea/bug9602' into maint-0.2.4
by arma＠torproject.org 10 Feb '14

10 Feb '14

commit 7f6aa780e3183f34b2fa771e17813018e6b28115 Merge: 9bb34aa c330d63 Author: Nick Mathewson <nickm(a)torproject.org> Date: Sun Feb 9 21:41:24 2014 -0500 Merge remote-tracking branch 'andrea/bug9602' into maint-0.2.4 src/or/channeltls.c | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+)

1 0