commit 02a5835c27780e45f705fc1c044b9c471b929dbe Author: Mike Perry mikeperry-git@torproject.org Date: Thu Apr 20 15:00:46 2017 -0400

Fix issues from dgoulet's code review.

https://gitlab.com/dgoulet/tor/merge_requests/24 --- src/or/channelpadding.c | 73 +++++++++++++++++++++++++++++++------------------ src/or/channelpadding.h | 3 +- src/or/channeltls.c | 20 ++++++++------ src/or/command.c | 29 +++++++------------- src/or/connection_or.c | 2 +- src/or/relay.c | 4 +-- 6 files changed, 73 insertions(+), 58 deletions(-)

diff --git a/src/or/channelpadding.c b/src/or/channelpadding.c index a84994a..2370827 100644 --- a/src/or/channelpadding.c +++ b/src/or/channelpadding.c @@ -47,6 +47,28 @@ static int consensus_nf_pad_before_usage; /** Should we pad relay-to-relay connections? */ static int consensus_nf_pad_relays;

+#define TOR_MSEC_PER_SEC 1000 +#define TOR_USEC_PER_MSEC 1000 + +/** + * How often do we get called by the connection housekeeping (ie: once + * per second) */ +#define TOR_HOUSEKEEPING_CALLBACK_MSEC 1000 +/** + * Additional extra time buffer on the housekeeping callback, since + * it can be delayed. This extra slack is used to decide if we should + * schedule a timer or wait for the next callback. */ +#define TOR_HOUSEKEEPING_CALLBACK_SLACK_MSEC 100 + +/** + * This macro tells us if either end of the channel is connected to a client. + * (If we're not a server, we're definitely a client. If the channel thinks + * its a client, use that. Then finally verify in the consensus). + */ +#define CHANNEL_IS_CLIENT(chan, options) \ + (!public_server_mode((options)) || (chan)->is_client || \ + !connection_or_digest_is_known_relay((chan)->identity_digest)) + /** * This function is called to update cached consensus parameters every time * there is a consensus update. This allows us to move the consensus param @@ -64,7 +86,7 @@ channelpadding_new_consensus_params(networkstatus_t *ns) DFLT_NETFLOW_INACTIVE_KEEPALIVE_LOW, DFLT_NETFLOW_INACTIVE_KEEPALIVE_MIN, DFLT_NETFLOW_INACTIVE_KEEPALIVE_MAX); - consensus_nf_ito_high = networkstatus_get_param(NULL, "nf_ito_high", + consensus_nf_ito_high = networkstatus_get_param(ns, "nf_ito_high", DFLT_NETFLOW_INACTIVE_KEEPALIVE_HIGH, consensus_nf_ito_low, DFLT_NETFLOW_INACTIVE_KEEPALIVE_MAX); @@ -74,13 +96,13 @@ channelpadding_new_consensus_params(networkstatus_t *ns) #define DFLT_NETFLOW_REDUCED_KEEPALIVE_MIN 0 #define DFLT_NETFLOW_REDUCED_KEEPALIVE_MAX 60000 consensus_nf_ito_low_reduced = - networkstatus_get_param(NULL, "nf_ito_low_reduced", + networkstatus_get_param(ns, "nf_ito_low_reduced", DFLT_NETFLOW_REDUCED_KEEPALIVE_LOW, DFLT_NETFLOW_REDUCED_KEEPALIVE_MIN, DFLT_NETFLOW_REDUCED_KEEPALIVE_MAX);

consensus_nf_ito_high_reduced = - networkstatus_get_param(NULL, "nf_ito_high_reduced", + networkstatus_get_param(ns, "nf_ito_high_reduced", DFLT_NETFLOW_REDUCED_KEEPALIVE_HIGH, consensus_nf_ito_low_reduced, DFLT_NETFLOW_REDUCED_KEEPALIVE_MAX); @@ -89,7 +111,7 @@ channelpadding_new_consensus_params(networkstatus_t *ns) #define CONNTIMEOUT_RELAYS_MIN 60 #define CONNTIMEOUT_RELAYS_MAX (7*24*60*60) // 1 week consensus_nf_conntimeout_relays = - networkstatus_get_param(NULL, "nf_conntimeout_relays", + networkstatus_get_param(ns, "nf_conntimeout_relays", CONNTIMEOUT_RELAYS_DFLT, CONNTIMEOUT_RELAYS_MIN, CONNTIMEOUT_RELAYS_MAX); @@ -98,16 +120,16 @@ channelpadding_new_consensus_params(networkstatus_t *ns) #define CIRCTIMEOUT_CLIENTS_MIN 60 #define CIRCTIMEOUT_CLIENTS_MAX (24*60*60) // 24 hours consensus_nf_conntimeout_clients = - networkstatus_get_param(NULL, "nf_conntimeout_clients", + networkstatus_get_param(ns, "nf_conntimeout_clients", CIRCTIMEOUT_CLIENTS_DFLT, CIRCTIMEOUT_CLIENTS_MIN, CIRCTIMEOUT_CLIENTS_MAX);

consensus_nf_pad_before_usage = - networkstatus_get_param(NULL, "nf_pad_before_usage", 1, 0, 1); + networkstatus_get_param(ns, "nf_pad_before_usage", 1, 0, 1);

consensus_nf_pad_relays = - networkstatus_get_param(NULL, "nf_pad_relays", 0, 0, 1); + networkstatus_get_param(ns, "nf_pad_relays", 0, 0, 1); }

/** @@ -214,10 +236,10 @@ channelpadding_update_padding_for_channel(channel_t *chan,

// We should not allow malicious relays to disable or reduce padding for // us as clients. In fact, we should only accept this cell at all if we're - // operating as a relay. Brides should not accept it from relays, either + // operating as a relay. Bridges should not accept it from relays, either // (only from their clients). if ((get_options()->BridgeRelay && - connection_or_digest_is_known_relay(chan->identity_digest)) || + connection_or_digest_is_known_relay(chan->identity_digest)) || !get_options()->ORPort_set) { static ratelim_t relay_limit = RATELIM_INIT(600);

@@ -238,7 +260,7 @@ channelpadding_update_padding_for_channel(channel_t *chan,

/* Max must not be lower than ito_low_ms */ chan->padding_timeout_high_ms = MAX(chan->padding_timeout_low_ms, - pad_vars->ito_high_ms); + pad_vars->ito_high_ms);

log_fn(LOG_INFO,LD_OR, "Negotiated padding=%d, lo=%d, hi=%d on "U64_FORMAT, @@ -271,7 +293,7 @@ channelpadding_send_disable_command(channel_t *chan) channelpadding_negotiate_set_command(&disable, CHANNELPADDING_COMMAND_STOP);

if (channelpadding_negotiate_encode(cell.payload, CELL_PAYLOAD_SIZE, - &disable) < 0) + &disable) < 0) return -1;

if (chan->write_cell(chan, &cell) == 1) @@ -305,7 +327,7 @@ channelpadding_send_enable_command(channel_t *chan, uint16_t low_timeout, channelpadding_negotiate_set_ito_high_ms(&enable, high_timeout);

if (channelpadding_negotiate_encode(cell.payload, CELL_PAYLOAD_SIZE, - &enable) < 0) + &enable) < 0) return -1;

if (chan->write_cell(chan, &cell) == 1) @@ -388,9 +410,9 @@ channelpadding_send_padding_callback(tor_timer_t *timer, void *args, if (chan && CHANNEL_CAN_HANDLE_CELLS(chan)) { /* Hrmm.. It might be nice to have an equivalent to assert_connection_ok * for channels. Then we could get rid of the channeltls dependency */ - tor_assert(BASE_CHAN_TO_TLS(chan)->conn->base_.magic == + tor_assert(TO_CONN(BASE_CHAN_TO_TLS(chan)->conn)->magic == OR_CONNECTION_MAGIC); - assert_connection_ok(&BASE_CHAN_TO_TLS(chan)->conn->base_, approx_time()); + assert_connection_ok(TO_CONN(BASE_CHAN_TO_TLS(chan)->conn), approx_time());

channelpadding_send_padding_cell_for_callback(chan); } else { @@ -421,8 +443,8 @@ channelpadding_schedule_padding(channel_t *chan, int in_ms) return CHANNELPADDING_PADDING_SENT; }

- timeout.tv_sec = in_ms/1000; - timeout.tv_usec = (in_ms%1000)*1000; + timeout.tv_sec = in_ms/TOR_MSEC_PER_SEC; + timeout.tv_usec = (in_ms%TOR_USEC_PER_MSEC)*TOR_USEC_PER_MSEC;

if (!chan->timer_handle) { chan->timer_handle = channel_handle_new(chan); @@ -467,7 +489,7 @@ channelpadding_compute_time_until_pad_for_netflow(channel_t *chan)

if (!chan->next_padding_time_ms) { /* If the below line or crypto_rand_int() shows up on a profile, - * we can avoid getting a timeout until we're at least nt_ito_lo + * we can avoid getting a timeout until we're at least nf_ito_lo * from a timeout window. That will prevent us from setting timers * on connections that were active up to 1.5 seconds ago. * Idle connections should only call this once every 5.5s on average @@ -479,7 +501,7 @@ channelpadding_compute_time_until_pad_for_netflow(channel_t *chan) return CHANNELPADDING_TIME_DISABLED;

chan->next_padding_time_ms = padding_timeout - + chan->timestamp_xfer_ms; + + chan->timestamp_xfer_ms; }

/* If the next padding time is beyond the maximum possible consensus value, @@ -499,11 +521,13 @@ channelpadding_compute_time_until_pad_for_netflow(channel_t *chan) }

/* If the timeout will expire before the next time we're called (1000ms - from now, plus some slack), then calcualte the number of milliseconds + from now, plus some slack), then calculate the number of milliseconds from now which we should send padding, so we can schedule a callback then. */ - if (long_now + 1100 >= chan->next_padding_time_ms) { + if (long_now + + (TOR_HOUSEKEEPING_CALLBACK_MSEC + TOR_HOUSEKEEPING_CALLBACK_SLACK_MSEC) + >= chan->next_padding_time_ms) { int64_t ms_until_pad_for_netflow = chan->next_padding_time_ms - long_now; if (ms_until_pad_for_netflow < 0) { @@ -540,9 +564,7 @@ channelpadding_get_channel_idle_timeout(const channel_t *chan, unsigned int timeout;

/* Non-canonical and client channels only last for 3-4.5 min when idle */ - if (!is_canonical || !public_server_mode(options) || - chan->is_client || - !connection_or_digest_is_known_relay(chan->identity_digest)) { + if (!is_canonical || CHANNEL_IS_CLIENT(chan, options)) { #define CONNTIMEOUT_CLIENTS_BASE 180 // 3 to 4.5 min timeout = CONNTIMEOUT_CLIENTS_BASE + crypto_rand_int(CONNTIMEOUT_CLIENTS_BASE/2); @@ -562,7 +584,7 @@ channelpadding_get_channel_idle_timeout(const channel_t *chan, * set. */ if (options->ReducedConnectionPadding - && !options->CircuitsAvailableTimeout) { + && !options->CircuitsAvailableTimeout) { timeout /= 2; }

@@ -688,8 +710,7 @@ channelpadding_decide_to_pad_channel(channel_t *chan) if (!chan->has_queued_writes(chan)) { int is_client_channel = 0;

- if (!public_server_mode(options) || chan->is_client || - !connection_or_digest_is_known_relay(chan->identity_digest)) { + if (CHANNEL_IS_CLIENT(chan, options)) { is_client_channel = 1; }

diff --git a/src/or/channelpadding.h b/src/or/channelpadding.h index e08f7a2..2708ee9 100644 --- a/src/or/channelpadding.h +++ b/src/or/channelpadding.h @@ -24,7 +24,8 @@ typedef enum { channelpadding_decision_t channelpadding_decide_to_pad_channel(channel_t *chan); int channelpadding_update_padding_for_channel(channel_t *, - const channelpadding_negotiate_t *); + const channelpadding_negotiate_t + *chan);

void channelpadding_disable_padding_on_channel(channel_t *chan); void channelpadding_reduce_padding_on_channel(channel_t *chan); diff --git a/src/or/channeltls.c b/src/or/channeltls.c index 4f2663a..7f5667b 100644 --- a/src/or/channeltls.c +++ b/src/or/channeltls.c @@ -1114,13 +1114,13 @@ channel_tls_handle_cell(cell_t *cell, or_connection_t *conn) entry_guards_note_internet_connectivity(get_guard_selection_info()); rep_hist_padding_count_read(PADDING_TYPE_TOTAL);

- if (chan->base_.currently_padding) + if (TLS_CHAN_TO_BASE(chan)->currently_padding) rep_hist_padding_count_read(PADDING_TYPE_ENABLED_TOTAL);

switch (cell->command) { case CELL_PADDING: rep_hist_padding_count_read(PADDING_TYPE_CELL); - if (chan->base_.currently_padding) + if (TLS_CHAN_TO_BASE(chan)->currently_padding) rep_hist_padding_count_read(PADDING_TYPE_ENABLED_CELL); ++stats_n_padding_cells_processed; /* do nothing */ @@ -1591,11 +1591,11 @@ channel_tls_process_versions_cell(var_cell_t *cell, channel_tls_t *chan)

/* We set this after sending the verions cell. */ /*XXXXX symbolic const.*/ - chan->base_.wide_circ_ids = + TLS_CHAN_TO_BASE(chan)->wide_circ_ids = chan->conn->link_proto >= MIN_LINK_PROTO_FOR_WIDE_CIRC_IDS; - chan->conn->wide_circ_ids = chan->base_.wide_circ_ids; + chan->conn->wide_circ_ids = TLS_CHAN_TO_BASE(chan)->wide_circ_ids;

- chan->base_.padding_enabled = + TLS_CHAN_TO_BASE(chan)->padding_enabled = chan->conn->link_proto >= MIN_LINK_PROTO_FOR_CHANNEL_PADDING;

if (send_certs) { @@ -1758,7 +1758,7 @@ channel_tls_process_netinfo_cell(cell_t *cell, channel_tls_t *chan)

if (!get_options()->BridgeRelay && me && get_uint32(my_addr_ptr) == htonl(me->addr)) { - chan->base_.is_canonical_to_peer = 1; + TLS_CHAN_TO_BASE(chan)->is_canonical_to_peer = 1; }

} else if (my_addr_type == RESOLVED_TYPE_IPV6 && my_addr_len == 16) { @@ -1767,7 +1767,7 @@ channel_tls_process_netinfo_cell(cell_t *cell, channel_tls_t *chan) if (!get_options()->BridgeRelay && me && !tor_addr_is_null(&me->ipv6_addr) && tor_addr_eq(&my_apparent_addr, &me->ipv6_addr)) { - chan->base_.is_canonical_to_peer = 1; + TLS_CHAN_TO_BASE(chan)->is_canonical_to_peer = 1; } }

@@ -1800,12 +1800,14 @@ channel_tls_process_netinfo_cell(cell_t *cell, channel_tls_t *chan) --n_other_addrs; }

- if (me && !chan->base_.is_canonical_to_peer && chan->conn->is_canonical) { + if (me && !TLS_CHAN_TO_BASE(chan)->is_canonical_to_peer && + channel_is_canonical(TLS_CHAN_TO_BASE(chan))) { log_info(LD_OR, "We made a connection to a relay at %s (fp=%s) but we think " "they will not consider this connection canonical. They " "think we are at %s, but we think its %s.", - safe_str(chan->base_.get_remote_descr(&chan->base_, 0)), + safe_str(TLS_CHAN_TO_BASE(chan)->get_remote_descr(TLS_CHAN_TO_BASE(chan), + 0)), safe_str(hex_str(chan->conn->identity_digest, DIGEST_LEN)), safe_str(tor_addr_is_null(&my_apparent_addr) ? "<none>" : fmt_and_decorate_addr(&my_apparent_addr)), diff --git a/src/or/command.c b/src/or/command.c index cebb5bf..5f3b667 100644 --- a/src/or/command.c +++ b/src/or/command.c @@ -326,17 +326,18 @@ command_process_create_cell(cell_t *cell, channel_t *chan) return; }

+ if (connection_or_digest_is_known_relay(chan->identity_digest)) { + rep_hist_note_circuit_handshake_requested(create_cell->handshake_type); + // Needed for chutney: Sometimes relays aren't in the consensus yet, and + // get marked as clients. This resets their channels once they appear. + // Probably useful for normal operation wrt relay flapping, too. + chan->is_client = 0; + } else { + channel_mark_client(chan); + } + if (create_cell->handshake_type != ONION_HANDSHAKE_TYPE_FAST) { /* hand it off to the cpuworkers, and then return. */ - if (connection_or_digest_is_known_relay(chan->identity_digest)) { - rep_hist_note_circuit_handshake_requested(create_cell->handshake_type); - // Needed for chutney: Sometimes relays aren't in the consensus yet, and - // get marked as clients. This resets their channels once they appear. - // Probably useful for normal operation wrt relay flapping, too. - chan->is_client = 0; - } else { - channel_mark_client(chan); - }

if (assign_onionskin_to_cpuworker(circ, create_cell) < 0) { log_debug(LD_GENERAL,"Failed to hand off onionskin. Closing."); @@ -352,16 +353,6 @@ command_process_create_cell(cell_t *cell, channel_t *chan) int len; created_cell_t created_cell;

- /* If this is a create_fast, this might be a client. Let's check. */ - if (connection_or_digest_is_known_relay(chan->identity_digest)) { - // Needed for chutney: Sometimes relays aren't in the consensus yet, and - // get marked as clients. This resets their channels once they appear. - // Probably useful for normal operation wrt relay flapping, too. - chan->is_client = 0; - } else { - channel_mark_client(chan); - } - memset(&created_cell, 0, sizeof(created_cell)); len = onion_skin_server_handshake(ONION_HANDSHAKE_TYPE_FAST, create_cell->onionskin, diff --git a/src/or/connection_or.c b/src/or/connection_or.c index c77e364..0ee3fce 100644 --- a/src/or/connection_or.c +++ b/src/or/connection_or.c @@ -1976,7 +1976,7 @@ connection_or_write_cell_to_buf(const cell_t *cell, or_connection_t *conn) if (conn->chan) { channel_timestamp_active(TLS_CHAN_TO_BASE(conn->chan));

- if (conn->chan->base_.currently_padding) { + if (TLS_CHAN_TO_BASE(conn->chan)->currently_padding) { rep_hist_padding_count_write(PADDING_TYPE_ENABLED_TOTAL); if (cell->command == CELL_PADDING) rep_hist_padding_count_write(PADDING_TYPE_ENABLED_CELL); diff --git a/src/or/relay.c b/src/or/relay.c index 18b68ff..886ddad 100644 --- a/src/or/relay.c +++ b/src/or/relay.c @@ -260,8 +260,8 @@ circuit_update_channel_usage(circuit_t *circ, cell_t *cell) if (BUG(!or_circ->p_chan)) return;

- if (!or_circ->p_chan->is_client || - (or_circ->p_chan->is_client && circ->n_chan)) { + if (!channel_is_client(or_circ->p_chan) || + (channel_is_client(or_circ->p_chan) && circ->n_chan)) { if (cell->command == CELL_RELAY_EARLY) { if (or_circ->p_chan->channel_usage < CHANNEL_USED_FOR_FULL_CIRCS) { or_circ->p_chan->channel_usage = CHANNEL_USED_FOR_FULL_CIRCS;