commit 93fd924bdb8d47a8ee4074dfffaf568320372165 Author: Taylor Yu catalyst@torproject.org Date: Wed Oct 17 15:39:55 2018 -0500

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. --- changes/bug27800 | 4 ++++ src/or/nodelist.c | 37 +++++++++++++++++++++++++++++++++---- src/test/test_nodelist.c | 36 ++++++++++++++++++++++++++++-------- 3 files changed, 65 insertions(+), 12 deletions(-)

diff --git a/changes/bug27800 b/changes/bug27800 new file mode 100644 index 000000000..63d5dbc68 --- /dev/null +++ b/changes/bug27800 @@ -0,0 +1,4 @@ + 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. diff --git a/src/or/nodelist.c b/src/or/nodelist.c index 212606d2f..82e092628 100644 --- a/src/or/nodelist.c +++ b/src/or/nodelist.c @@ -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; }

diff --git a/src/test/test_nodelist.c b/src/test/test_nodelist.c index a873003d7..094e93471 100644 --- a/src/test/test_nodelist.c +++ b/src/test/test_nodelist.c @@ -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) \