Commit 84a5bd48 authored by Nick Mathewson's avatar Nick Mathewson 🦋
Browse files

Merge remote-tracking branch 'tor-gitlab/mr/103' into maint-0.3.5

parents ed6a3282 435f31ae
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.
......@@ -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);
}
/**
......
......@@ -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);
......
......@@ -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;
}
......
......@@ -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);
......
......@@ -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
};
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment