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.

diff --git a/changes/ticket40081 b/changes/ticket40081
new file mode 100644 (file)
index 0000000..683ae33
--- /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 3886906875211147e092ba78cabf3519c57e645d..b3a2d7122b4c23a31ada0ed3b229bce1f8e21c02 100644 (file)
--- 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 97aa0003370e1389adb25320d393b0f69b98fa5b..78e4d90ea53f02b1ed6a04065ae9e9e7893366d1 100644 (file)
--- 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 f874e39946490716cfe21f99b6ca2f96e7ab9062..299ab8857669acb01f856948591a1b8f738ace71 100644 (file)
--- 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 f3a5791d6c43a3afb3c5c058e1b5bcb8bfd29e74..1a5025cd710c1d9545394ec10dbb0c072c3f2a32 100644 (file)
--- 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 e55b9b07508635dbd4be994832359938eb2dc839..afb7db813ccf01e16c3129cfde400f6da30c7f59 100644 (file)
--- 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
 };
-