Commit 435f31ae authored by Nick Mathewson's avatar Nick Mathewson 🦀
Browse files

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.
parent c4742b89

changes/ticket40081

0 → 100644
+6 −0
Original line number Diff line number Diff line 
  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.

src/core/or/channel.c

+6 −38
Original line number Diff line number Diff line
@@ -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);

}



/**

src/core/or/channel.h

+3 −6
Original line number Diff line number Diff line
@@ -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);

src/core/or/channeltls.c

+10 −22
Original line number Diff line number Diff line
@@ -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:

    /* 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;

        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();

    }

  }

  /* else return 0 for tlschan->conn == NULL */



  return answer;

}

src/core/or/circuitbuild.c

+2 −0
Original line number Diff line number Diff line
@@ -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);