commit 435f31aed34e9b58d1e8d9f460e6d2e3c6714fbc Author: Nick Mathewson <nickm@torproject.org> Date: Mon Aug 3 11:25:37 2020 -0400 Remove channel_is_canonical_is_reliable() This function once served to let circuits continue to be built over version-1 link connections. But such connections are long-obsolete, and it's time to remove this check. Closes #40081. --- changes/ticket40081 | 6 ++++++ src/core/or/channel.c | 44 ++++++-------------------------------------- src/core/or/channel.h | 9 +++------ src/core/or/channeltls.c | 32 ++++++++++---------------------- src/core/or/circuitbuild.c | 2 ++ src/test/test_channel.c | 13 +++++-------- 6 files changed, 32 insertions(+), 74 deletions(-) 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 3886906875..b3a2d7122b 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++; } } @@ -2431,21 +2430,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; @@ -2587,16 +2574,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) ? @@ -2955,22 +2938,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 97aa000337..78e4d90ea5 100644 --- a/src/core/or/channel.h +++ b/src/core/or/channel.h @@ -351,12 +351,10 @@ struct channel_s { /** 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 */ @@ -733,7 +731,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 f874e39946..299ab88576 100644 --- a/src/core/or/channeltls.c +++ b/src/core/or/channeltls.c @@ -106,7 +106,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); @@ -643,12 +643,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); @@ -656,24 +655,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 f3a5791d6c..1a5025cd71 100644 --- a/src/core/or/circuitbuild.c +++ b/src/core/or/circuitbuild.c @@ -707,6 +707,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 e55b9b0750..afb7db813c 100644 --- a/src/test/test_channel.c +++ b/src/test/test_channel.c @@ -46,7 +46,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; @@ -357,14 +356,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; } @@ -1381,6 +1376,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. */ @@ -1475,8 +1473,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. " @@ -1567,4 +1565,3 @@ struct testcase_t channel_tests[] = { NULL, NULL }, END_OF_TESTCASES }; -