commit f367453cb5a9ea5cb0d5664425256d7abbcbb407 Author: Nick Mathewson nickm@torproject.org Date: Tue Jun 27 12:01:46 2017 -0400
Mark descriptors as undownloadable when dirserv_add_() rejects them
As of ac2f6b608a18a8595f62384788196d7c3f2875fd in 0.2.1.19-alpha, Sebastian fixed bug 888 by marking descriptors as "impossible" by digest if they got rejected during the router_load_routers_from_string() phase. This fix stopped clients and relays from downloading the same thing over and over.
But we never made the same change for descriptors rejected during dirserv_add_{descriptor,extrainfo}. Instead, we tried to notice in advance that we'd reject them with dirserv_would_reject().
This notice-in-advance check stopped working once we added key-pinning and didn't make a corresponding key-pinning change to dirserv_would_reject() [since a routerstatus_t doesn't include an ed25519 key].
So as a fix, let's make the dirserv_add_*() functions mark digests as undownloadable when they are rejected.
Fixes bug 22349; I am calling this a fix on 0.2.1.19-alpha, though you could also argue for it being a fix on 0.2.7.2-alpha. --- changes/bug22349 | 9 +++++++++ src/or/dirserv.c | 56 ++++++++++++++++++++++++++++++++++++++++++++------------ 2 files changed, 53 insertions(+), 12 deletions(-)
diff --git a/changes/bug22349 b/changes/bug22349 new file mode 100644 index 000000000..bb43404bf --- /dev/null +++ b/changes/bug22349 @@ -0,0 +1,9 @@ + o Minor bugfixes (directory authority): + - When a directory authority rejects a descriptor or extrainfo with + a given digest, mark that digest as undownloadable, so that we + do not attempt to download it again over and over. We previously + tried to avoid downloading such descriptors by other means, but + we didn't notice if we accidentally downloaded one anyway. This + behavior became problematic in 0.2.7.2-alpha, when authorities + began pinning Ed25519 keys. Fixes ticket + 22349; bugfix on 0.2.1.19-alpha. diff --git a/src/or/dirserv.c b/src/or/dirserv.c index 72441081c..da34c196f 100644 --- a/src/or/dirserv.c +++ b/src/or/dirserv.c @@ -658,8 +658,8 @@ dirserv_add_descriptor(routerinfo_t *ri, const char **msg, const char *source) control_event_or_authdir_new_descriptor("REJECTED", ri->cache_info.signed_descriptor_body, desclen, *msg); - routerinfo_free(ri); - return ROUTER_AUTHDIR_REJECTS; + r = ROUTER_AUTHDIR_REJECTS; + goto fail; }
/* Check whether this descriptor is semantically identical to the last one @@ -679,8 +679,8 @@ dirserv_add_descriptor(routerinfo_t *ri, const char **msg, const char *source) control_event_or_authdir_new_descriptor("DROPPED", ri->cache_info.signed_descriptor_body, desclen, *msg); - routerinfo_free(ri); - return ROUTER_IS_ALREADY_KNOWN; + r = ROUTER_IS_ALREADY_KNOWN; + goto fail; }
/* Do keypinning again ... this time, to add the pin if appropriate */ @@ -703,8 +703,8 @@ dirserv_add_descriptor(routerinfo_t *ri, const char **msg, const char *source) "its key did not match an older RSA/Ed25519 keypair", router_describe(ri), source); *msg = "Looks like your keypair does not match its older value."; - routerinfo_free(ri); - return ROUTER_AUTHDIR_REJECTS; + r = ROUTER_AUTHDIR_REJECTS; + goto fail; }
/* Make a copy of desc, since router_add_to_routerlist might free @@ -742,6 +742,20 @@ dirserv_add_descriptor(routerinfo_t *ri, const char **msg, const char *source) tor_free(desc); tor_free(nickname); return r; + fail: + { + const char *desc_digest = ri->cache_info.signed_descriptor_digest; + download_status_t *dls = + router_get_dl_status_by_descriptor_digest(desc_digest); + if (dls) { + log_info(LD_GENERAL, "Marking router with descriptor %s as rejected, " + "and therefore undownloadable", + hex_str(desc_digest, DIGEST_LEN)); + download_status_mark_impossible(dls); + } + routerinfo_free(ri); + } + return r; }
/** As dirserv_add_descriptor, but for an extrainfo_t <b>ei</b>. */ @@ -750,6 +764,7 @@ dirserv_add_extrainfo(extrainfo_t *ei, const char **msg) { routerinfo_t *ri; int r; + was_router_added_t rv; tor_assert(msg); *msg = NULL;
@@ -758,8 +773,8 @@ dirserv_add_extrainfo(extrainfo_t *ei, const char **msg) 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); - return ROUTER_BAD_EI; + rv = ROUTER_BAD_EI; + goto fail; }
/* If it's too big, refuse it now. Otherwise we'll cache it all over the @@ -771,17 +786,34 @@ dirserv_add_extrainfo(extrainfo_t *ei, const char **msg) (int)ei->cache_info.signed_descriptor_len, MAX_EXTRAINFO_UPLOAD_SIZE); *msg = "Extrainfo document was too large"; - extrainfo_free(ei); - return ROUTER_BAD_EI; + rv = ROUTER_BAD_EI; + goto fail; }
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; + if (r<0) { + extrainfo_free(ei); + return ROUTER_IS_ALREADY_KNOWN; + } + rv = ROUTER_BAD_EI; + goto fail; } router_add_extrainfo_to_routerlist(ei, msg, 0, 0); return ROUTER_ADDED_SUCCESSFULLY; + fail: + { + const char *d = ei->cache_info.signed_descriptor_digest; + signed_descriptor_t *sd = router_get_by_extrainfo_digest((char*)d); + if (sd) { + log_info(LD_GENERAL, "Marking extrainfo with descriptor %s as " + "rejected, and therefore undownloadable", + hex_str((char*)d,DIGEST_LEN)); + download_status_mark_impossible(&sd->ei_dl_status); + } + extrainfo_free(ei); + } + return rv; }
/** Remove all descriptors whose nicknames or fingerprints no longer