[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
[tor-commits] [tor/maint-0.4.4] Remove channel_is_canonical_is_reliable()
commit 435f31aed34e9b58d1e8d9f460e6d2e3c6714fbc
Author: Nick Mathewson <nickm@xxxxxxxxxxxxxx>
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
};
-
_______________________________________________
tor-commits mailing list
tor-commits@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-commits