From 44da47d3c1e4809d5aaf91cd708c6f02b01a5396 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 17 May 2016 12:53:12 -0400 Subject: [PATCH 1/6] Move extra_info_digest256 into signed_descriptor_t This patch includes no semantic changes; it's just a field movement. It's prerequisite for a fix to 19017/17150. --- src/or/or.h | 4 ++-- src/or/router.c | 6 +++--- src/or/routerlist.c | 4 ++-- src/or/routerparse.c | 2 +- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/or/or.h b/src/or/or.h index 431927c7e7..e0f2eb416a 100644 --- a/src/or/or.h +++ b/src/or/or.h @@ -1993,6 +1993,8 @@ typedef struct signed_descriptor_t { time_t published_on; /** For routerdescs only: digest of the corresponding extrainfo. */ char extra_info_digest[DIGEST_LEN]; + /** For routerdescs only: A SHA256-digest of the extrainfo (if any) */ + char extra_info_digest256[DIGEST256_LEN]; /** For routerdescs only: Status of downloading the corresponding * extrainfo. */ download_status_t ei_dl_status; @@ -2024,8 +2026,6 @@ typedef int16_t country_t; /** Information about another onion router in the network. */ typedef struct { signed_descriptor_t cache_info; - /** A SHA256-digest of the extrainfo (if any) */ - char extra_info_digest256[DIGEST256_LEN]; char *nickname; /**< Human-readable OR name. */ uint32_t addr; /**< IPv4 address of OR, in host order. */ diff --git a/src/or/router.c b/src/or/router.c index 841f6fde1b..3943643f9b 100644 --- a/src/or/router.c +++ b/src/or/router.c @@ -2028,7 +2028,7 @@ router_build_fresh_descriptor(routerinfo_t **r, extrainfo_t **e) memcpy(ri->cache_info.extra_info_digest, ei->cache_info.signed_descriptor_digest, DIGEST_LEN); - memcpy(ri->extra_info_digest256, + memcpy(ri->cache_info.extra_info_digest256, ei->digest256, DIGEST256_LEN); } else { @@ -2543,9 +2543,9 @@ router_dump_router_to_string(routerinfo_t *router, char extra_info_digest[HEX_DIGEST_LEN+1]; base16_encode(extra_info_digest, sizeof(extra_info_digest), router->cache_info.extra_info_digest, DIGEST_LEN); - if (!tor_digest256_is_zero(router->extra_info_digest256)) { + if (!tor_digest256_is_zero(router->cache_info.extra_info_digest256)) { char d256_64[BASE64_DIGEST256_LEN+1]; - digest256_to_base64(d256_64, router->extra_info_digest256); + digest256_to_base64(d256_64, router->cache_info.extra_info_digest256); tor_asprintf(&extra_info_line, "extra-info-digest %s %s\n", extra_info_digest, d256_64); } else { diff --git a/src/or/routerlist.c b/src/or/routerlist.c index 79a5bb3910..3d85d86f20 100644 --- a/src/or/routerlist.c +++ b/src/or/routerlist.c @@ -4926,9 +4926,9 @@ routerinfo_incompatible_with_extrainfo(const routerinfo_t *ri, /* Set digest256_matches to 1 if the digest is correct, or if no * digest256 was in the ri. */ digest256_matches = tor_memeq(ei->digest256, - ri->extra_info_digest256, DIGEST256_LEN); + ri->cache_info.extra_info_digest256, DIGEST256_LEN); digest256_matches |= - tor_mem_is_zero(ri->extra_info_digest256, DIGEST256_LEN); + tor_mem_is_zero(ri->cache_info.extra_info_digest256, DIGEST256_LEN); /* The identity must match exactly to have been generated at the same time * by the same router. */ diff --git a/src/or/routerparse.c b/src/or/routerparse.c index 6b6e21d5d0..bc6c35fe09 100644 --- a/src/or/routerparse.c +++ b/src/or/routerparse.c @@ -1597,7 +1597,7 @@ router_parse_entry_from_string(const char *s, const char *end, } if (tok->n_args >= 2) { - if (digest256_from_base64(router->extra_info_digest256, tok->args[1]) + if (digest256_from_base64(router->cache_info.extra_info_digest256, tok->args[1]) < 0) { log_warn(LD_DIR, "Invalid extra info digest256 %s", escaped(tok->args[1])); From 7d1eb0d57086c9630f7797916d9d2c948117a7e1 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 17 May 2016 12:57:03 -0400 Subject: [PATCH 2/6] When making sure digest256 matches in ei, look at sd, not ri. The routerinfo we pass to routerinfo_incompatible_with_extrainfo is the latest routerinfo for the relay. The signed_descriptor_t, on the other hand, is the signed_descriptor_t that corresponds to the extrainfo. That means we should be checking the digest256 match with that signed_descriptor_t, not with the routerinfo. Fixes bug 17150 (and 19017); bugfix on 0.2.7.2-alpha. --- changes/bug17150 | 6 ++++++ src/or/routerlist.c | 4 ++-- 2 files changed, 8 insertions(+), 2 deletions(-) create mode 100644 changes/bug17150 diff --git a/changes/bug17150 b/changes/bug17150 new file mode 100644 index 0000000000..3fb78213d9 --- /dev/null +++ b/changes/bug17150 @@ -0,0 +1,6 @@ + o Minor bugfixes (directory warnings): + - When fetching extrainfo documents, compare their SHA256 digests + with the routerinfo that led us to fetch them, rather than + with the most recent routerinfo. Otherwise we generate many + spurious warnings about mismatches. Fixes bug 17150; bugfix + on 0.2.7.2-alpha. diff --git a/src/or/routerlist.c b/src/or/routerlist.c index 3d85d86f20..2e347b0692 100644 --- a/src/or/routerlist.c +++ b/src/or/routerlist.c @@ -4926,9 +4926,9 @@ routerinfo_incompatible_with_extrainfo(const routerinfo_t *ri, /* Set digest256_matches to 1 if the digest is correct, or if no * digest256 was in the ri. */ digest256_matches = tor_memeq(ei->digest256, - ri->cache_info.extra_info_digest256, DIGEST256_LEN); + sd->extra_info_digest256, DIGEST256_LEN); digest256_matches |= - tor_mem_is_zero(ri->cache_info.extra_info_digest256, DIGEST256_LEN); + tor_mem_is_zero(sd->extra_info_digest256, DIGEST256_LEN); /* The identity must match exactly to have been generated at the same time * by the same router. */ From 64748f2f987a28815f056dfe3a8a739f46d9157f Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 17 May 2016 13:08:34 -0400 Subject: [PATCH 3/6] Fix documentation for routerinfo_incompatible_with_extrainfo --- src/or/routerlist.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/src/or/routerlist.c b/src/or/routerlist.c index 2e347b0692..140fe218c3 100644 --- a/src/or/routerlist.c +++ b/src/or/routerlist.c @@ -4896,13 +4896,21 @@ router_differences_are_cosmetic(const routerinfo_t *r1, const routerinfo_t *r2) return 1; } -/** Check whether ri (a.k.a. sd) is a router compatible with the - * extrainfo document - * ei. If no router is compatible with ei, ei should be +/** Check whether sd describes a router descriptor compatible with the + * extrainfo document ei. + * + * ri (which must also be provided) is the full routerinfo corresponding + * to the same router -- but note that it might not refer to the same specific + * descriptor as sd. + * + * If no router is compatible with ei, ei should be * dropped. Return 0 for "compatible", return 1 for "reject, and inform * whoever uploaded ei, and return -1 for "reject silently.". If * msg is present, set *msg to a description of the * incompatibility (if any). + * + * Set the extrainfo_is_bogus field in sd if the digests matched + * but the extrainfo was nonetheless incompatible. **/ int routerinfo_incompatible_with_extrainfo(const routerinfo_t *ri, From 8acfac7375e7a0692193434704984d7eb507faf1 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 17 May 2016 13:14:04 -0400 Subject: [PATCH 4/6] Copy the signing_key_cert field into signed_descriptor_t We need this field to be in signed_descriptor_t so that routerinfo_incompatible_with_extrainfo can work correctly (#17150). But I don't want to move it completely in this patch, since a great deal of the code that messes with it has been in flux since 0.2.7, when this ticket was opened. I should open another ticket about removing the field from routerinfo_t and extrainfo_t later on. This patch fixes no actual behavior. --- src/or/or.h | 8 ++++++-- src/or/router.c | 3 +++ src/or/routerlist.c | 2 ++ src/or/routerparse.c | 2 ++ 4 files changed, 13 insertions(+), 2 deletions(-) diff --git a/src/or/or.h b/src/or/or.h index e0f2eb416a..ffff6d631a 100644 --- a/src/or/or.h +++ b/src/or/or.h @@ -1995,6 +1995,8 @@ typedef struct signed_descriptor_t { char extra_info_digest[DIGEST_LEN]; /** For routerdescs only: A SHA256-digest of the extrainfo (if any) */ char extra_info_digest256[DIGEST256_LEN]; + /** Certificate for ed25519 signing key. */ + struct tor_cert_st *signing_key_cert; /** For routerdescs only: Status of downloading the corresponding * extrainfo. */ download_status_t ei_dl_status; @@ -2043,7 +2045,8 @@ typedef struct { crypto_pk_t *identity_pkey; /**< Public RSA key for signing. */ /** Public curve25519 key for onions */ curve25519_public_key_t *onion_curve25519_pkey; - /** Certificate for ed25519 signing key */ + /** Certificate for ed25519 signing key + * (XXXX duplicated in cache_info.) */ struct tor_cert_st *signing_key_cert; /** What's the earliest expiration time on all the certs in this * routerinfo? */ @@ -2115,7 +2118,8 @@ typedef struct extrainfo_t { uint8_t digest256[DIGEST256_LEN]; /** The router's nickname. */ char nickname[MAX_NICKNAME_LEN+1]; - /** Certificate for ed25519 signing key */ + /** Certificate for ed25519 signing key + * (XXXX duplicated in cache_info.) */ struct tor_cert_st *signing_key_cert; /** True iff we found the right key for this extra-info, verified the * signature, and found it to be bad. */ diff --git a/src/or/router.c b/src/or/router.c index 3943643f9b..b3523ec718 100644 --- a/src/or/router.c +++ b/src/or/router.c @@ -1911,6 +1911,7 @@ router_build_fresh_descriptor(routerinfo_t **r, extrainfo_t **e) return -1; } ri->signing_key_cert = tor_cert_dup(get_master_signing_key_cert()); + ri->cache_info.signing_key_cert = tor_cert_dup(get_master_signing_key_cert()); get_platform_str(platform, sizeof(platform)); ri->platform = tor_strdup(platform); @@ -2003,6 +2004,8 @@ router_build_fresh_descriptor(routerinfo_t **r, extrainfo_t **e) strlcpy(ei->nickname, get_options()->Nickname, sizeof(ei->nickname)); ei->cache_info.published_on = ri->cache_info.published_on; ei->signing_key_cert = tor_cert_dup(get_master_signing_key_cert()); + ei->cache_info.signing_key_cert = tor_cert_dup(get_master_signing_key_cert()); + memcpy(ei->cache_info.identity_digest, ri->cache_info.identity_digest, DIGEST_LEN); if (extrainfo_dump_to_string(&ei->cache_info.signed_descriptor_body, diff --git a/src/or/routerlist.c b/src/or/routerlist.c index 140fe218c3..9b3b79444f 100644 --- a/src/or/routerlist.c +++ b/src/or/routerlist.c @@ -2670,6 +2670,7 @@ routerinfo_free(routerinfo_t *router) if (router->identity_pkey) crypto_pk_free(router->identity_pkey); tor_cert_free(router->signing_key_cert); + tor_cert_free(router->cache_info.signing_key_cert); if (router->declared_family) { SMARTLIST_FOREACH(router->declared_family, char *, s, tor_free(s)); smartlist_free(router->declared_family); @@ -2689,6 +2690,7 @@ extrainfo_free(extrainfo_t *extrainfo) if (!extrainfo) return; tor_cert_free(extrainfo->signing_key_cert); + tor_cert_free(extrainfo->cache_info.signing_key_cert); tor_free(extrainfo->cache_info.signed_descriptor_body); tor_free(extrainfo->pending_sig); diff --git a/src/or/routerparse.c b/src/or/routerparse.c index bc6c35fe09..531a95d5b8 100644 --- a/src/or/routerparse.c +++ b/src/or/routerparse.c @@ -1403,6 +1403,7 @@ router_parse_entry_from_string(const char *s, const char *end, goto err; } router->signing_key_cert = cert; /* makes sure it gets freed. */ + router->cache_info.signing_key_cert = tor_cert_dup(cert); if (cert->cert_type != CERT_TYPE_ID_SIGNING || ! cert->signing_key_included) { @@ -1778,6 +1779,7 @@ extrainfo_parse_entry_from_string(const char *s, const char *end, goto err; } extrainfo->signing_key_cert = cert; /* makes sure it gets freed. */ + extrainfo->cache_info.signing_key_cert = tor_cert_dup(cert); if (cert->cert_type != CERT_TYPE_ID_SIGNING || ! cert->signing_key_included) { log_warn(LD_DIR, "Invalid form for ed25519 cert"); From 49ff09aef27d2883b77008be56c29def2a6a8dff Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 17 May 2016 13:16:36 -0400 Subject: [PATCH 5/6] Fix another, more subtle, case of bug 17150. We need to make sure that the corresponding sd and ei match in their certificates. --- changes/bug17150 | 1 + src/or/routerlist.c | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/changes/bug17150 b/changes/bug17150 index 3fb78213d9..686cc34296 100644 --- a/changes/bug17150 +++ b/changes/bug17150 @@ -1,5 +1,6 @@ o Minor bugfixes (directory warnings): - When fetching extrainfo documents, compare their SHA256 digests + and Ed25519 signing key certificates with the routerinfo that led us to fetch them, rather than with the most recent routerinfo. Otherwise we generate many spurious warnings about mismatches. Fixes bug 17150; bugfix diff --git a/src/or/routerlist.c b/src/or/routerlist.c index 9b3b79444f..f75ec11aca 100644 --- a/src/or/routerlist.c +++ b/src/or/routerlist.c @@ -4949,7 +4949,7 @@ routerinfo_incompatible_with_extrainfo(const routerinfo_t *ri, goto err; /* different servers */ } - if (! tor_cert_opt_eq(ri->signing_key_cert, ei->signing_key_cert)) { + if (! tor_cert_opt_eq(sd->signing_key_cert, ei->signing_key_cert)) { if (msg) *msg = "Extrainfo signing key cert didn't match routerinfo"; goto err; /* different servers */ } From 00f74e0372a956f9db590e1cb2ddcfb265125023 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 17 May 2016 13:24:01 -0400 Subject: [PATCH 6/6] Improve API of routerinfo_incompatible_with_extrainfo() This API change makes it so that routerinfo_incompatible...() no longer takes a routerinfo_t, so that it's obvious that it should only look at fields from the signed_descriptor_t. This change should prevent a recurrence of #17150. --- src/or/dirserv.c | 9 ++++++--- src/or/router.c | 3 ++- src/or/routerlist.c | 21 ++++++++++----------- src/or/routerlist.h | 2 +- 4 files changed, 19 insertions(+), 16 deletions(-) diff --git a/src/or/dirserv.c b/src/or/dirserv.c index 01b08ca41b..ab77021184 100644 --- a/src/or/dirserv.c +++ b/src/or/dirserv.c @@ -691,12 +691,14 @@ dirserv_add_descriptor(routerinfo_t *ri, const char **msg, const char *source) static was_router_added_t dirserv_add_extrainfo(extrainfo_t *ei, const char **msg) { - const routerinfo_t *ri; + routerinfo_t *ri; int r; tor_assert(msg); *msg = NULL; - ri = router_get_by_id_digest(ei->cache_info.identity_digest); + /* Needs to be mutable so routerinfo_incompatible_with_extrainfo + * can mess with some of the flags in ri->cache_info. */ + ri = router_get_mutable_by_digest(ei->cache_info.identity_digest); if (!ri) { *msg = "No corresponding router descriptor for extra-info descriptor"; extrainfo_free(ei); @@ -716,7 +718,8 @@ dirserv_add_extrainfo(extrainfo_t *ei, const char **msg) return ROUTER_BAD_EI; } - if ((r = routerinfo_incompatible_with_extrainfo(ri, ei, NULL, msg))) { + if ((r = routerinfo_incompatible_with_extrainfo(ri->identity_pkey, ei, + &ri->cache_info, msg))) { extrainfo_free(ei); return r < 0 ? ROUTER_IS_ALREADY_KNOWN : ROUTER_BAD_EI; } diff --git a/src/or/router.c b/src/or/router.c index b3523ec718..37ce9e7b45 100644 --- a/src/or/router.c +++ b/src/or/router.c @@ -2072,7 +2072,8 @@ router_build_fresh_descriptor(routerinfo_t **r, extrainfo_t **e) ri->cache_info.signed_descriptor_digest); if (ei) { - tor_assert(! routerinfo_incompatible_with_extrainfo(ri, ei, NULL, NULL)); + tor_assert(! routerinfo_incompatible_with_extrainfo(ri->identity_pkey, ei, + &ri->cache_info, NULL)); } *r = ri; diff --git a/src/or/routerlist.c b/src/or/routerlist.c index f75ec11aca..0f1fac747f 100644 --- a/src/or/routerlist.c +++ b/src/or/routerlist.c @@ -2900,7 +2900,7 @@ extrainfo_insert,(routerlist_t *rl, extrainfo_t *ei, int warn_if_incompatible)) "Mismatch in digest in extrainfo map."); goto done; } - if (routerinfo_incompatible_with_extrainfo(ri, ei, sd, + if (routerinfo_incompatible_with_extrainfo(ri->identity_pkey, ei, sd, &compatibility_error_msg)) { char d1[HEX_DIGEST_LEN+1], d2[HEX_DIGEST_LEN+1]; r = (ri->cache_info.extrainfo_is_bogus) ? @@ -4901,9 +4901,9 @@ router_differences_are_cosmetic(const routerinfo_t *r1, const routerinfo_t *r2) /** Check whether sd describes a router descriptor compatible with the * extrainfo document ei. * - * ri (which must also be provided) is the full routerinfo corresponding - * to the same router -- but note that it might not refer to the same specific - * descriptor as sd. + * identity_pkey (which must also be provided) is RSA1024 identity key + * for the router. We use it to check the signature of the extrainfo document, + * if it has not already been checked. * * If no router is compatible with ei, ei should be * dropped. Return 0 for "compatible", return 1 for "reject, and inform @@ -4915,16 +4915,15 @@ router_differences_are_cosmetic(const routerinfo_t *r1, const routerinfo_t *r2) * but the extrainfo was nonetheless incompatible. **/ int -routerinfo_incompatible_with_extrainfo(const routerinfo_t *ri, +routerinfo_incompatible_with_extrainfo(const crypto_pk_t *identity_pkey, extrainfo_t *ei, signed_descriptor_t *sd, const char **msg) { int digest_matches, digest256_matches, r=1; - tor_assert(ri); + tor_assert(identity_pkey); + tor_assert(sd); tor_assert(ei); - if (!sd) - sd = (signed_descriptor_t*)&ri->cache_info; if (ei->bad_sig) { if (msg) *msg = "Extrainfo signature was bad, or signed with wrong key."; @@ -4942,7 +4941,7 @@ routerinfo_incompatible_with_extrainfo(const routerinfo_t *ri, /* The identity must match exactly to have been generated at the same time * by the same router. */ - if (tor_memneq(ri->cache_info.identity_digest, + if (tor_memneq(sd->identity_digest, ei->cache_info.identity_digest, DIGEST_LEN)) { if (msg) *msg = "Extrainfo nickname or identity did not match routerinfo"; @@ -4956,7 +4955,7 @@ routerinfo_incompatible_with_extrainfo(const routerinfo_t *ri, if (ei->pending_sig) { char signed_digest[128]; - if (crypto_pk_public_checksig(ri->identity_pkey, + if (crypto_pk_public_checksig(identity_pkey, signed_digest, sizeof(signed_digest), ei->pending_sig, ei->pending_sig_len) != DIGEST_LEN || tor_memneq(signed_digest, ei->cache_info.signed_descriptor_digest, @@ -4967,7 +4966,7 @@ routerinfo_incompatible_with_extrainfo(const routerinfo_t *ri, goto err; /* Bad signature, or no match. */ } - ei->cache_info.send_unencrypted = ri->cache_info.send_unencrypted; + ei->cache_info.send_unencrypted = sd->send_unencrypted; tor_free(ei->pending_sig); } diff --git a/src/or/routerlist.h b/src/or/routerlist.h index 200533fe91..bbe99f8173 100644 --- a/src/or/routerlist.h +++ b/src/or/routerlist.h @@ -187,7 +187,7 @@ void update_extrainfo_downloads(time_t now); void router_reset_descriptor_download_failures(void); int router_differences_are_cosmetic(const routerinfo_t *r1, const routerinfo_t *r2); -int routerinfo_incompatible_with_extrainfo(const routerinfo_t *ri, +int routerinfo_incompatible_with_extrainfo(const crypto_pk_t *ri, extrainfo_t *ei, signed_descriptor_t *sd, const char **msg);