October 2018 - tor-commits - lists.torproject.org

[tor/master] test: Add test for closing intro circuits when storing a new descriptor
by nickm＠torproject.org 18 Oct '18

18 Oct '18

commit 8b2e72106ae87c8018d9bae25f826c7bd92a88e8 Author: David Goulet <dgoulet(a)torproject.org> Date: Fri Oct 12 10:30:33 2018 -0400 test: Add test for closing intro circuits when storing a new descriptor This is client side and related to 27471 for previous commit that fixes this issue. Signed-off-by: David Goulet <dgoulet(a)torproject.org> --- src/test/test_hs_client.c | 103 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 103 insertions(+) diff --git a/src/test/test_hs_client.c b/src/test/test_hs_client.c index 25cb991a7..91b3ed1ec 100644 --- a/src/test/test_hs_client.c +++ b/src/test/test_hs_client.c @@ -885,6 +885,107 @@ test_desc_has_arrived_cleanup(void *arg) UNMOCK(router_have_minimum_dir_info); } +static void +test_close_intro_circuits_new_desc(void *arg) +{ + int ret; + ed25519_keypair_t service_kp; + circuit_t *circ = NULL; + origin_circuit_t *ocirc = NULL; + hs_descriptor_t *desc1 = NULL, *desc2 = NULL; + + (void) arg; + + hs_init(); + + /* This is needed because of the client cache expiration timestamp is based + * on having a consensus. See cached_client_descriptor_has_expired(). */ + MOCK(networkstatus_get_live_consensus, + mock_networkstatus_get_live_consensus); + + /* Set consensus time */ + parse_rfc1123_time("Sat, 26 Oct 1985 13:00:00 UTC", + &mock_ns.valid_after); + parse_rfc1123_time("Sat, 26 Oct 1985 14:00:00 UTC", + &mock_ns.fresh_until); + parse_rfc1123_time("Sat, 26 Oct 1985 16:00:00 UTC", + &mock_ns.valid_until); + + /* Generate service keypair */ + tt_int_op(0, OP_EQ, ed25519_keypair_generate(&service_kp, 0)); + + /* Create and add to the global list a dummy client introduction circuits. + * We'll then make sure the hs_ident is attached to a dummy descriptor. */ + circ = dummy_origin_circuit_new(0); + tt_assert(circ); + circ->purpose = CIRCUIT_PURPOSE_C_INTRODUCING; + ocirc = TO_ORIGIN_CIRCUIT(circ); + + /* Build the first descriptor and cache it. */ + { + char *encoded; + desc1 = hs_helper_build_hs_desc_with_ip(&service_kp); + tt_assert(desc1); + ret = hs_desc_encode_descriptor(desc1, &service_kp, NULL, &encoded); + tt_int_op(ret, OP_EQ, 0); + tt_assert(encoded); + + /* Store it */ + ret = hs_cache_store_as_client(encoded, &service_kp.pubkey); + tt_int_op(ret, OP_EQ, 0); + tor_free(encoded); + tt_assert(hs_cache_lookup_as_client(&service_kp.pubkey)); + } + + /* We'll pick one introduction point and associate it with the circuit. */ + { + const hs_desc_intro_point_t *ip = + smartlist_get(desc1->encrypted_data.intro_points, 0); + tt_assert(ip); + ocirc->hs_ident = hs_ident_circuit_new(&service_kp.pubkey, + HS_IDENT_CIRCUIT_INTRO); + ed25519_pubkey_copy(&ocirc->hs_ident->intro_auth_pk, + &ip->auth_key_cert->signed_key); + } + + /* Before we are about to clean up the intro circuits, make sure it is + * actually there. */ + tt_assert(circuit_get_next_intro_circ(NULL, true)); + + /* Build the second descriptor for the same service and cache it. */ + { + char *encoded; + desc2 = hs_helper_build_hs_desc_with_ip(&service_kp); + tt_assert(desc2); + tt_mem_op(&desc1->plaintext_data.signing_pubkey, OP_EQ, + &desc2->plaintext_data.signing_pubkey, ED25519_PUBKEY_LEN); + /* To replace the existing descriptor, the revision counter needs to be + * bigger. */ + desc2->plaintext_data.revision_counter = + desc1->plaintext_data.revision_counter + 1; + + ret = hs_desc_encode_descriptor(desc2, &service_kp, NULL, &encoded); + tt_int_op(ret, OP_EQ, 0); + tt_assert(encoded); + + hs_cache_store_as_client(encoded, &service_kp.pubkey); + tt_int_op(ret, OP_EQ, 0); + tor_free(encoded); + tt_assert(hs_cache_lookup_as_client(&service_kp.pubkey)); + } + + /* Once stored, our intro circuit should be closed because it is related to + * an old introduction point that doesn't exists anymore. */ + tt_assert(!circuit_get_next_intro_circ(NULL, true)); + + done: + circuit_free(circ); + hs_descriptor_free(desc1); + hs_descriptor_free(desc2); + hs_free_all(); + UNMOCK(networkstatus_get_live_consensus); +} + struct testcase_t hs_client_tests[] = { { "e2e_rend_circuit_setup_legacy", test_e2e_rend_circuit_setup_legacy, TT_FORK, NULL, NULL }, @@ -902,6 +1003,8 @@ struct testcase_t hs_client_tests[] = { TT_FORK, NULL, NULL }, { "desc_has_arrived_cleanup", test_desc_has_arrived_cleanup, TT_FORK, NULL, NULL }, + { "close_intro_circuits_new_desc", test_close_intro_circuits_new_desc, + TT_FORK, NULL, NULL }, END_OF_TESTCASES };

1 0

[tor/master] Merge remote-tracking branch 'dgoulet/ticket27471_035_02'
by nickm＠torproject.org 18 Oct '18

18 Oct '18

commit 62401812c71856c6884c32a57d1edd16576643f1 Merge: 0a41d17c1 8b2e72106 Author: Nick Mathewson <nickm(a)torproject.org> Date: Thu Oct 18 13:01:41 2018 -0400 Merge remote-tracking branch 'dgoulet/ticket27471_035_02' changes/ticket27471 | 5 ++ src/core/or/circuitlist.c | 49 +++++++++++++++----- src/core/or/circuitlist.h | 3 +- src/feature/hs/hs_cache.c | 7 +++ src/feature/hs/hs_client.c | 32 +++++++++++++ src/feature/hs/hs_client.h | 1 + src/feature/rend/rendservice.c | 2 +- src/test/test_hs_client.c | 103 +++++++++++++++++++++++++++++++++++++++++ 8 files changed, 188 insertions(+), 14 deletions(-)

1 0

[translation/donatepages-messagespot] Update translations for donatepages-messagespot
by translation＠torproject.org 18 Oct '18

18 Oct '18

commit dcf081522408079d70e172ff361c5cbcaa194d9c Author: Translation commit bot <translation(a)torproject.org> Date: Thu Oct 18 16:45:23 2018 +0000 Update translations for donatepages-messagespot --- locale/he/LC_MESSAGES/messages.po | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/locale/he/LC_MESSAGES/messages.po b/locale/he/LC_MESSAGES/messages.po index cc4e117d6..82737d87d 100644 --- a/locale/he/LC_MESSAGES/messages.po +++ b/locale/he/LC_MESSAGES/messages.po @@ -249,7 +249,7 @@ msgstr "סכום בלתי תקף" #: tmp/cache_locale/c7/c763c19bb6abb9330294c550c8241bb3874e3b4e17fb6e7b15db26c60df8d5fe.php:137 msgid "$2 minimum donation" -msgstr "" +msgstr "תרומה מזערית $2" #: tmp/cache_locale/c7/c763c19bb6abb9330294c550c8241bb3874e3b4e17fb6e7b15db26c60df8d5fe.php:141 msgid "$ other"

1 0

[tor/master] Merge branch 'ticket27549_035_01_squashed'
by nickm＠torproject.org 18 Oct '18

18 Oct '18

commit 0a41d17c1585d262031a8b5885853881adaaf8fb Merge: e979a56bb 3a8f32067 Author: Nick Mathewson <nickm(a)torproject.org> Date: Thu Oct 18 10:16:30 2018 -0400 Merge branch 'ticket27549_035_01_squashed' changes/ticket27549 | 3 ++ src/feature/hs/hs_descriptor.c | 89 +++++++++++++++++++++++++++--------------- 2 files changed, 61 insertions(+), 31 deletions(-)

1 0

[tor/master] hs-v3: Consolidate descriptor cookie computation code
by nickm＠torproject.org 18 Oct '18

18 Oct '18

commit 3a8f32067d1cb4d5aee320fbd3d1c02541f4e112 Author: David Goulet <dgoulet(a)torproject.org> Date: Tue Sep 18 13:50:12 2018 -0400 hs-v3: Consolidate descriptor cookie computation code Both client and service had their own code for this. Consolidate into one place so we avoid duplication. Closes #27549 Signed-off-by: David Goulet <dgoulet(a)torproject.org> --- changes/ticket27549 | 3 ++ src/feature/hs/hs_descriptor.c | 89 +++++++++++++++++++++++++++--------------- 2 files changed, 61 insertions(+), 31 deletions(-) diff --git a/changes/ticket27549 b/changes/ticket27549 new file mode 100644 index 000000000..51d0f2475 --- /dev/null +++ b/changes/ticket27549 @@ -0,0 +1,3 @@ + o Code simplification and refactoring (hidden service v3): + - Consolidate the authorized client descriptor cookie computation code + from client and service into one function. Closes ticket 27549. diff --git a/src/feature/hs/hs_descriptor.c b/src/feature/hs/hs_descriptor.c index d0cdffdf1..1232182ea 100644 --- a/src/feature/hs/hs_descriptor.c +++ b/src/feature/hs/hs_descriptor.c @@ -1400,6 +1400,49 @@ encrypted_data_length_is_valid(size_t len) return 0; } +/* Build the KEYS component for the authorized client computation. The format + * of the construction is: + * + * SECRET_SEED = x25519(sk, pk) + * KEYS = KDF(subcredential | SECRET_SEED, 40) + * + * The keys_out parameter will points to the buffer containing the KEYS. The + * caller should wipe and free its content once done with it. This function + * can't fail. */ +static void +build_descriptor_cookie_keys(const uint8_t *subcredential, + size_t subcredential_len, + const curve25519_secret_key_t *sk, + const curve25519_public_key_t *pk, + uint8_t **keys_out) +{ + uint8_t secret_seed[CURVE25519_OUTPUT_LEN]; + uint8_t *keystream; + size_t keystream_len = HS_DESC_CLIENT_ID_LEN + HS_DESC_COOKIE_KEY_LEN; + crypto_xof_t *xof; + + tor_assert(subcredential); + tor_assert(sk); + tor_assert(pk); + tor_assert(keys_out); + + keystream = tor_malloc_zero(keystream_len); + + /* Calculate x25519(sk, pk) to get the secret seed. */ + curve25519_handshake(secret_seed, sk, pk); + + /* Calculate KEYS = KDF(subcredential | SECRET_SEED, 40) */ + xof = crypto_xof_new(); + crypto_xof_add_bytes(xof, subcredential, subcredential_len); + crypto_xof_add_bytes(xof, secret_seed, sizeof(secret_seed)); + crypto_xof_squeeze_bytes(xof, keystream, keystream_len); + crypto_xof_free(xof); + + memwipe(secret_seed, 0, sizeof(secret_seed)); + + *keys_out = keystream; +} + /* Decrypt the descriptor cookie given the descriptor, the auth client, * and the client secret key. On sucess, return 0 and a newly allocated * descriptor cookie descriptor_cookie_out. On error or if the client id @@ -1412,12 +1455,10 @@ decrypt_descriptor_cookie(const hs_descriptor_t *desc, uint8_t **descriptor_cookie_out) { int ret = -1; - uint8_t secret_seed[CURVE25519_OUTPUT_LEN]; - uint8_t keystream[HS_DESC_CLIENT_ID_LEN + HS_DESC_COOKIE_KEY_LEN]; - uint8_t *cookie_key = NULL; + uint8_t *keystream = NULL; uint8_t *descriptor_cookie = NULL; + const uint8_t *cookie_key = NULL; crypto_cipher_t *cipher = NULL; - crypto_xof_t *xof = NULL; tor_assert(desc); tor_assert(client); @@ -1429,16 +1470,11 @@ decrypt_descriptor_cookie(const hs_descriptor_t *desc, sizeof(*client_auth_sk))); tor_assert(!tor_mem_is_zero((char *) desc->subcredential, DIGEST256_LEN)); - /* Calculate x25519(client_x, hs_Y) */ - curve25519_handshake(secret_seed, client_auth_sk, - &desc->superencrypted_data.auth_ephemeral_pubkey); - - /* Calculate KEYS = KDF(subcredential | SECRET_SEED, 40) */ - xof = crypto_xof_new(); - crypto_xof_add_bytes(xof, desc->subcredential, DIGEST256_LEN); - crypto_xof_add_bytes(xof, secret_seed, sizeof(secret_seed)); - crypto_xof_squeeze_bytes(xof, keystream, sizeof(keystream)); - crypto_xof_free(xof); + /* Get the KEYS component to derive the CLIENT-ID and COOKIE-KEY. */ + build_descriptor_cookie_keys(desc->subcredential, DIGEST256_LEN, + client_auth_sk, + &desc->superencrypted_data.auth_ephemeral_pubkey, + &keystream); /* If the client id of auth client is not the same as the calculcated * client id, it means that this auth client is invaild according to the @@ -1464,8 +1500,8 @@ decrypt_descriptor_cookie(const hs_descriptor_t *desc, if (cipher) { crypto_cipher_free(cipher); } - memwipe(secret_seed, 0, sizeof(secret_seed)); memwipe(keystream, 0, sizeof(keystream)); + tor_free(keystream); return ret; } @@ -2864,11 +2900,9 @@ hs_desc_build_authorized_client(const uint8_t *subcredential, const uint8_t *descriptor_cookie, hs_desc_authorized_client_t *client_out) { - uint8_t secret_seed[CURVE25519_OUTPUT_LEN]; - uint8_t keystream[HS_DESC_CLIENT_ID_LEN + HS_DESC_COOKIE_KEY_LEN]; - uint8_t *cookie_key; + uint8_t *keystream = NULL; + const uint8_t *cookie_key; crypto_cipher_t *cipher; - crypto_xof_t *xof; tor_assert(client_auth_pk); tor_assert(auth_ephemeral_sk); @@ -2884,18 +2918,11 @@ hs_desc_build_authorized_client(const uint8_t *subcredential, tor_assert(!tor_mem_is_zero((char *) subcredential, DIGEST256_LEN)); - /* Calculate x25519(hs_y, client_X) */ - curve25519_handshake(secret_seed, - auth_ephemeral_sk, - client_auth_pk); - - /* Calculate KEYS = KDF(subcredential | SECRET_SEED, 40) */ - xof = crypto_xof_new(); - crypto_xof_add_bytes(xof, subcredential, DIGEST256_LEN); - crypto_xof_add_bytes(xof, secret_seed, sizeof(secret_seed)); - crypto_xof_squeeze_bytes(xof, keystream, sizeof(keystream)); - crypto_xof_free(xof); + /* Get the KEYS part so we can derive the CLIENT-ID and COOKIE-KEY. */ + build_descriptor_cookie_keys(subcredential, DIGEST256_LEN, + auth_ephemeral_sk, client_auth_pk, &keystream); + /* Extract the CLIENT-ID and COOKIE-KEY from the KEYS. */ memcpy(client_out->client_id, keystream, HS_DESC_CLIENT_ID_LEN); cookie_key = keystream + HS_DESC_CLIENT_ID_LEN; @@ -2910,8 +2937,8 @@ hs_desc_build_authorized_client(const uint8_t *subcredential, (const char *) descriptor_cookie, HS_DESC_DESCRIPTOR_COOKIE_LEN); - memwipe(secret_seed, 0, sizeof(secret_seed)); memwipe(keystream, 0, sizeof(keystream)); + tor_free(keystream); crypto_cipher_free(cipher); }

1 0

[tor/maint-0.3.4] Merge branch 'maint-0.3.3' into maint-0.3.4
by nickm＠torproject.org 18 Oct '18

18 Oct '18

commit d1eac7830fbd86d145c1892b8a7123615cc34658 Merge: 2000d04cb 93fd924bd Author: Nick Mathewson <nickm(a)torproject.org> Date: Thu Oct 18 09:12:58 2018 -0400 Merge branch 'maint-0.3.3' into maint-0.3.4 changes/bug27800 | 4 ++++ src/or/nodelist.c | 37 +++++++++++++++++++++++++++++++++---- src/test/test_nodelist.c | 36 ++++++++++++++++++++++++++++-------- 3 files changed, 65 insertions(+), 12 deletions(-)

1 0

[tor/maint-0.3.5] Merge branch 'bug27800-033' into tor-034
by nickm＠torproject.org 18 Oct '18

18 Oct '18

commit 4e7f65ee5dfc735cf78f3e78c33bad5ce882af9e Merge: 2000d04cb 93fd924bd Author: Taylor Yu <catalyst(a)torproject.org> Date: Wed Oct 17 15:42:58 2018 -0500 Merge branch 'bug27800-033' into tor-034 changes/bug27800 | 4 ++++ src/or/nodelist.c | 37 +++++++++++++++++++++++++++++++++---- src/test/test_nodelist.c | 36 ++++++++++++++++++++++++++++-------- 3 files changed, 65 insertions(+), 12 deletions(-)

1 0

[tor/maint-0.3.5] Log more info for duplicate ed25519 IDs
by nickm＠torproject.org 18 Oct '18

18 Oct '18

commit 93fd924bdb8d47a8ee4074dfffaf568320372165 Author: Taylor Yu <catalyst(a)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 node 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) \

1 0

[tor/maint-0.3.4] Log more info for duplicate ed25519 IDs
by nickm＠torproject.org 18 Oct '18

18 Oct '18

commit 93fd924bdb8d47a8ee4074dfffaf568320372165 Author: Taylor Yu <catalyst(a)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 node 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) \

1 0

[tor/maint-0.3.3] Log more info for duplicate ed25519 IDs
by nickm＠torproject.org 18 Oct '18

18 Oct '18

commit 93fd924bdb8d47a8ee4074dfffaf568320372165 Author: Taylor Yu <catalyst(a)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 node 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) \

1 0