diff --git a/changes/ticket40081 b/changes/ticket40081 new file mode 100644 index 0000000000..683ae33518 --- /dev/null +++ b/changes/ticket40081 @@ -0,0 +1,6 @@ + o Minor features (security): + - Channels using obsolete versions of the Tor link protocol are no + longer allowed to circumvent address-canonicity checks. + (This is only a minor issue, since such channels have no way to + set ed25519 keys, and therefore should always be rejected.) + Closes ticket 40081. diff --git a/src/core/or/channel.c b/src/core/or/channel.c index 18940bd81f..553d6a6bf4 100644 --- a/src/core/or/channel.c +++ b/src/core/or/channel.c @@ -772,10 +772,9 @@ channel_check_for_duplicates(void) connections_to_relay++; total_relay_connections++; - if (chan->is_canonical(chan, 0)) total_canonical++; + if (chan->is_canonical(chan)) total_canonical++; - if (!chan->is_canonical_to_peer && chan->is_canonical(chan, 0) - && chan->is_canonical(chan, 1)) { + if (!chan->is_canonical_to_peer && chan->is_canonical(chan)) { total_half_canonical++; } } @@ -2419,21 +2418,9 @@ channel_get_for_extend(const char *rsa_id_digest, continue; } - /* Never return a non-canonical connection using a recent link protocol - * if the address is not what we wanted. - * - * The channel_is_canonical_is_reliable() function asks the lower layer - * if we should trust channel_is_canonical(). The below is from the - * comments of the old circuit_or_get_for_extend() and applies when - * the lower-layer transport is channel_tls_t. - * - * (For old link protocols, we can't rely on is_canonical getting - * set properly if we're talking to the right address, since we might - * have an out-of-date descriptor, and we will get no NETINFO cell to - * tell us about the right address.) - */ + /* Only return canonical connections or connections where the address + * is the address we wanted. */ if (!channel_is_canonical(chan) && - channel_is_canonical_is_reliable(chan) && !channel_matches_target_addr_for_extend(chan, target_addr)) { ++n_noncanonical; continue; @@ -2575,16 +2562,12 @@ channel_dump_statistics, (channel_t *chan, int severity)) /* Handle marks */ tor_log(severity, LD_GENERAL, - " * Channel %"PRIu64 " has these marks: %s %s %s " - "%s %s %s", + " * Channel %"PRIu64 " has these marks: %s %s %s %s %s", (chan->global_identifier), channel_is_bad_for_new_circs(chan) ? "bad_for_new_circs" : "!bad_for_new_circs", channel_is_canonical(chan) ? "canonical" : "!canonical", - channel_is_canonical_is_reliable(chan) ? - "is_canonical_is_reliable" : - "!is_canonical_is_reliable", channel_is_client(chan) ? "client" : "!client", channel_is_local(chan) ? @@ -2943,22 +2926,7 @@ channel_is_canonical(channel_t *chan) tor_assert(chan); tor_assert(chan->is_canonical); - return chan->is_canonical(chan, 0); -} - -/** - * Test if the canonical flag is reliable. - * - * This function asks if the lower layer thinks it's safe to trust the - * result of channel_is_canonical(). - */ -int -channel_is_canonical_is_reliable(channel_t *chan) -{ - tor_assert(chan); - tor_assert(chan->is_canonical); - - return chan->is_canonical(chan, 1); + return chan->is_canonical(chan); } /** diff --git a/src/core/or/channel.h b/src/core/or/channel.h index 2e2936a69a..b48bd8800d 100644 --- a/src/core/or/channel.h +++ b/src/core/or/channel.h @@ -350,12 +350,10 @@ struct channel_t { /** Check if the lower layer has queued writes */ int (*has_queued_writes)(channel_t *); /** - * If the second param is zero, ask the lower layer if this is - * 'canonical', for a transport-specific definition of canonical; if - * it is 1, ask if the answer to the preceding query is safe to rely - * on. + * Ask the lower layer if this is 'canonical', for a transport-specific + * definition of canonical. */ - int (*is_canonical)(channel_t *, int); + int (*is_canonical)(channel_t *); /** Check if this channel matches a specified extend_info_t */ int (*matches_extend_info)(channel_t *, extend_info_t *); /** Check if this channel matches a target address when extending */ @@ -728,7 +726,6 @@ int channel_has_queued_writes(channel_t *chan); int channel_is_bad_for_new_circs(channel_t *chan); void channel_mark_bad_for_new_circs(channel_t *chan); int channel_is_canonical(channel_t *chan); -int channel_is_canonical_is_reliable(channel_t *chan); int channel_is_client(const channel_t *chan); int channel_is_local(channel_t *chan); int channel_is_incoming(channel_t *chan); diff --git a/src/core/or/channeltls.c b/src/core/or/channeltls.c index efa8e2d891..5f3d038730 100644 --- a/src/core/or/channeltls.c +++ b/src/core/or/channeltls.c @@ -107,7 +107,7 @@ channel_tls_get_transport_name_method(channel_t *chan, char **transport_out); static const char * channel_tls_get_remote_descr_method(channel_t *chan, int flags); static int channel_tls_has_queued_writes_method(channel_t *chan); -static int channel_tls_is_canonical_method(channel_t *chan, int req); +static int channel_tls_is_canonical_method(channel_t *chan); static int channel_tls_matches_extend_info_method(channel_t *chan, extend_info_t *extend_info); @@ -645,12 +645,11 @@ channel_tls_has_queued_writes_method(channel_t *chan) /** * Tell the upper layer if we're canonical. * - * This implements the is_canonical method for channel_tls_t; if req is zero, - * it returns whether this is a canonical channel, and if it is one it returns - * whether that can be relied upon. + * This implements the is_canonical method for channel_tls_t: + * it returns whether this is a canonical channel. */ static int -channel_tls_is_canonical_method(channel_t *chan, int req) +channel_tls_is_canonical_method(channel_t *chan) { int answer = 0; channel_tls_t *tlschan = BASE_CHAN_TO_TLS(chan); @@ -658,24 +657,13 @@ channel_tls_is_canonical_method(channel_t *chan, int req) tor_assert(tlschan); 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_nonfatal_unreached_once(); - } + /* If this bit is set to 0, and link_proto is sufficiently old, then we + * can't actually _rely_ on this being a non-canonical channel. + * Nonetheless, we're going to believe that this is a non-canonical + * channel in this case, since nobody should be using these link protocols + * any more. */ + answer = tlschan->conn->is_canonical; } - /* else return 0 for tlschan->conn == NULL */ return answer; } diff --git a/src/core/or/circuitbuild.c b/src/core/or/circuitbuild.c index 4865d05d98..d4dda8db77 100644 --- a/src/core/or/circuitbuild.c +++ b/src/core/or/circuitbuild.c @@ -730,6 +730,8 @@ circuit_deliver_create_cell(circuit_t *circ, const create_cell_t *create_cell, goto error; } + tor_assert_nonfatal_once(circ->n_chan->is_canonical); + memset(&cell, 0, sizeof(cell_t)); r = relayed ? create_cell_format_relayed(&cell, create_cell) : create_cell_format(&cell, create_cell); diff --git a/src/test/test_channel.c b/src/test/test_channel.c index 5b13f1f979..01aa01b4db 100644 --- a/src/test/test_channel.c +++ b/src/test/test_channel.c @@ -44,7 +44,6 @@ static int dump_statistics_mock_matches = 0; static int test_close_called = 0; static int test_chan_should_be_canonical = 0; static int test_chan_should_match_target = 0; -static int test_chan_canonical_should_be_reliable = 0; static int test_chan_listener_close_fn_called = 0; static int test_chan_listener_fn_called = 0; @@ -337,14 +336,10 @@ scheduler_release_channel_mock(channel_t *ch) } static int -test_chan_is_canonical(channel_t *chan, int req) +test_chan_is_canonical(channel_t *chan) { tor_assert(chan); - if (req && test_chan_canonical_should_be_reliable) { - return 1; - } - if (test_chan_should_be_canonical) { return 1; } @@ -1357,6 +1352,9 @@ test_channel_for_extend(void *arg) /* Make it older than chan1. */ chan2->timestamp_created = chan1->timestamp_created - 1; + /* Say it's all canonical. */ + test_chan_should_be_canonical = 1; + /* Set channel identities and add it to the channel map. The last one to be * added is made the first one in the list so the lookup will always return * that one first. */ @@ -1451,8 +1449,8 @@ test_channel_for_extend(void *arg) chan2->is_bad_for_new_circs = 0; /* Non canonical channels. */ + test_chan_should_be_canonical = 0; test_chan_should_match_target = 0; - test_chan_canonical_should_be_reliable = 1; ret_chan = channel_get_for_extend(digest, &ed_id, &addr, &msg, &launch); tt_assert(!ret_chan); tt_str_op(msg, OP_EQ, "Connections all too old, or too non-canonical. "