Commit 93fd924b authored by Taylor Yu's avatar Taylor Yu
Browse files

Log more info for duplicate ed25519 IDs 

Occasionally, key pinning doesn't catch a relay that shares an ed25519
ID with another relay.  Log the identity fingerprints and the shared
ed25519 ID when this happens, instead of making a BUG() warning.

Fixes bug 27800; bugfix on 0.3.2.1-alpha.
parent 6b2ef2c5

changes/bug27800

0 → 100644
+4 −0
Original line number Diff line number Diff line 
  o Minor bugfixes (directory authority):

    - Log additional info when we get a relay that shares an ed25519

      ID with a different relay, instead making a BUG() warning.

      Fixes bug 27800; bugfix on 0.3.2.1-alpha.

src/or/nodelist.c

+33 −4
Original line number Diff line number Diff line
@@ -43,6 +43,7 @@ 
#include "or.h"

#include "address.h"

#include "address_set.h"

#include "backtrace.h"

#include "bridges.h"

#include "config.h"

#include "control.h"
@@ -63,6 +64,7 @@ 
#include "routerparse.h"

#include "routerset.h"

#include "torcert.h"

#include "util_format.h"



#include <string.h>
@@ -260,6 +262,20 @@ node_remove_from_ed25519_map(node_t *node) 
  return rv;

}



/** Helper function to log details of duplicated ed2559_ids */

static void

node_log_dup_ed_id(const node_t *old, const node_t *node, const char *ed_id)

{

  char *s;

  char *olddesc = tor_strdup(node_describe(old));



  tor_asprintf(&s, "Reused ed25519_id %s: old %s new %s", ed_id,

               olddesc, node_describe(node));

  log_backtrace(LOG_NOTICE, LD_DIR, s);

  tor_free(olddesc);

  tor_free(s);

}



/** If <b>node</b> has an ed25519 id, and it is not already in the ed25519 id

 * map, set its ed25519_id field, and add it to the ed25519 map.

 */
@@ -281,11 +297,24 @@ node_add_to_ed25519_map(node_t *node) 
  node_t *old;

  memcpy(&node->ed25519_id, key, sizeof(node->ed25519_id));

  old = HT_FIND(nodelist_ed_map, &the_nodelist->nodes_by_ed_id, node);

  if (BUG(old)) {

    /* XXXX order matters here, and this may mean that authorities aren't

     * pinning. */

    if (old != node)

  if (old) {

    char ed_id[BASE32_BUFSIZE(sizeof(key->pubkey))];



    base32_encode(ed_id, sizeof(ed_id), (const char *)key->pubkey,

                  sizeof(key->pubkey));

    if (BUG(old == node)) {

      /* Actual bug: all callers of this function call

       * node_remove_from_ed25519_map first. */

      log_err(LD_BUG,

              "Unexpectedly found deleted node with ed25519_id %s", ed_id);

    } else {

      /* Distinct nodes sharing a ed25519 id, possibly due to relay

       * misconfiguration.  The key pinning might not catch this,

       * possibly due to downloading a missing descriptor during

       * consensus voting. */

      node_log_dup_ed_id(old, node, ed_id);

      memset(&node->ed25519_id, 0, sizeof(node->ed25519_id));

    }

    return 0;

  }

src/test/test_nodelist.c

+28 −8
Original line number Diff line number Diff line
@@ -11,6 +11,7 @@ 
#include "nodelist.h"

#include "torcert.h"

#include "test.h"

#include "log_test_helpers.h"



/** Test the case when node_get_by_id() returns NULL,

 * node_get_verbose_nickname_by_id should return the base 16 encoding
@@ -118,9 +119,10 @@ mock_networkstatus_get_latest_consensus_by_flavor(consensus_flavor_t f) 
static void

test_nodelist_ed_id(void *arg)

{

  routerstatus_t *rs[4];

  microdesc_t *md[4];

  routerinfo_t *ri[4];

#define N_NODES 5

  routerstatus_t *rs[N_NODES];

  microdesc_t *md[N_NODES];

  routerinfo_t *ri[N_NODES];

  networkstatus_t *ns;

  int i;

  (void)arg;
@@ -137,7 +139,7 @@ test_nodelist_ed_id(void *arg) 
  /* Make a bunch of dummy objects that we can play around with.  Only set the

     necessary fields */



  for (i = 0; i < 4; ++i) {

  for (i = 0; i < N_NODES; ++i) {

    rs[i] = tor_malloc_zero(sizeof(*rs[i]));

    md[i] = tor_malloc_zero(sizeof(*md[i]));

    ri[i] = tor_malloc_zero(sizeof(*ri[i]));
@@ -154,7 +156,7 @@ test_nodelist_ed_id(void *arg) 
    memcpy(&ri[i]->cache_info.signing_key_cert->signing_key,

           md[i]->ed25519_identity_pkey, sizeof(ed25519_public_key_t));



    if (i != 3)

    if (i < 3)

      smartlist_add(ns->routerstatus_list, rs[i]);

  }
@@ -184,13 +186,30 @@ test_nodelist_ed_id(void *arg) 


  /* Register the 4th by ri only -- we never put it into the networkstatus,

   * so it has to be independent */

  n = nodelist_set_routerinfo(ri[3], &ri_old);

  tt_ptr_op(n, OP_EQ, node_get_by_ed25519_id(md[3]->ed25519_identity_pkey));

  node_t *n3 = nodelist_set_routerinfo(ri[3], &ri_old);

  tt_ptr_op(n3, OP_EQ, node_get_by_ed25519_id(md[3]->ed25519_identity_pkey));

  tt_ptr_op(ri_old, OP_EQ, NULL);

  tt_int_op(4, OP_EQ, smartlist_len(nodelist_get_list()));



  /* Register the 5th by ri only, and rewrite its ed25519 pubkey to be

   * the same as the 4th, to test the duplicate ed25519 key logging in

   * nodelist.c */

  memcpy(md[4]->ed25519_identity_pkey, md[3]->ed25519_identity_pkey,

         sizeof(ed25519_public_key_t));

  memcpy(&ri[4]->cache_info.signing_key_cert->signing_key,

         md[3]->ed25519_identity_pkey, sizeof(ed25519_public_key_t));



  setup_capture_of_logs(LOG_NOTICE);

  node_t *n4 = nodelist_set_routerinfo(ri[4], &ri_old);

  tt_ptr_op(ri_old, OP_EQ, NULL);

  tt_int_op(5, OP_EQ, smartlist_len(nodelist_get_list()));

  tt_ptr_op(n4, OP_NE, node_get_by_ed25519_id(md[3]->ed25519_identity_pkey));

  tt_ptr_op(n3, OP_EQ, node_get_by_ed25519_id(md[3]->ed25519_identity_pkey));

  expect_log_msg_containing("Reused ed25519_id");



 done:

  for (i = 0; i < 4; ++i) {

  teardown_capture_of_logs();

  for (i = 0; i < N_NODES; ++i) {

    tor_free(rs[i]);

    tor_free(md[i]->ed25519_identity_pkey);

    tor_free(md[i]);
@@ -201,6 +220,7 @@ test_nodelist_ed_id(void *arg) 
  networkstatus_vote_free(ns);

  UNMOCK(networkstatus_get_latest_consensus);

  UNMOCK(networkstatus_get_latest_consensus_by_flavor);

#undef N_NODES

}



#define NODE(name, flags) \