commit 69df16e3765383bd59fd2fbedbb5301f3b58ab14 Author: Nick Mathewson <nickm@torproject.org> Date: Thu Jan 8 11:20:26 2015 -0500 Rewrite the logic for deciding when to drop old/superseded certificates Fixes bug 11454, where we would keep around a superseded descriptor if the descriptor replacing it wasn't at least a week later. Bugfix on 0.2.1.8-alpha. Fixes bug 11457, where a certificate with a publication time in the future could make us discard existing (and subsequent!) certificates with correct publication times. Bugfix on 0.2.0.3-alpha. --- changes/bug11454 | 6 ++++ changes/bug11457 | 5 ++++ src/common/container.h | 11 ++++++++ src/or/routerlist.c | 73 ++++++++++++++++++++++++++++++------------------ 4 files changed, 68 insertions(+), 27 deletions(-) diff --git a/changes/bug11454 b/changes/bug11454 new file mode 100644 index 0000000..da793de --- /dev/null +++ b/changes/bug11454 @@ -0,0 +1,6 @@ + o Minor bugfixes: + - Remove any old authority certificates that have been superseded + for at least two days. Previously, we would keep superseded + certificates until they expired, if they were published close + in time to the certificate that superseded them. + Fixes bug 11454; bugfix on 0.2.1.8-alpha. diff --git a/changes/bug11457 b/changes/bug11457 new file mode 100644 index 0000000..a8b76f6 --- /dev/null +++ b/changes/bug11457 @@ -0,0 +1,5 @@ + o Minor bugfixes: + - If an authority operator accidentally makes a signing certificate with + a future publication time, do not discard its real signing + certificates. Fixes bug 11457; bugfix on 0.2.0.3-alpha. + diff --git a/src/common/container.h b/src/common/container.h index 377cdf5..c704833 100644 --- a/src/common/container.h +++ b/src/common/container.h @@ -243,6 +243,17 @@ char *smartlist_join_strings2(smartlist_t *sl, const char *join, STMT_END /** Helper: While in a SMARTLIST_FOREACH loop over the list <b>sl</b> indexed + * with the variable <b>var</b>, remove the current element in a way that + * won't confuse the loop. */ +#define SMARTLIST_DEL_CURRENT_KEEPORDER(sl, var) \ + STMT_BEGIN \ + smartlist_del_keeporder(sl, var ## _sl_idx); \ + --var ## _sl_idx; \ + --var ## _sl_len; \ + STMT_END + + +/** Helper: While in a SMARTLIST_FOREACH loop over the list <b>sl</b> indexed * with the variable <b>var</b>, replace the current element with <b>val</b>. * Does not deallocate the current value of <b>var</b>. */ diff --git a/src/or/routerlist.c b/src/or/routerlist.c index 6cb052c..7112282 100644 --- a/src/or/routerlist.c +++ b/src/or/routerlist.c @@ -449,6 +449,19 @@ trusted_dirs_flush_certs_to_disk(void) trusted_dir_servers_certs_changed = 0; } +static int +compare_certs_by_pubdates(const void **_a, const void **_b) +{ + const authority_cert_t *cert1 = *_a, *cert2=*_b; + + if (cert1->cache_info.published_on < cert2->cache_info.published_on) + return -1; + else if (cert1->cache_info.published_on > cert2->cache_info.published_on) + return 1; + else + return 0; +} + /** Remove all expired v3 authority certificates that have been superseded for * more than 48 hours or, if not expired, that were published more than 7 days * before being superseded. (If the most recent cert was published more than 48 @@ -459,38 +472,44 @@ trusted_dirs_remove_old_certs(void) { time_t now = time(NULL); #define DEAD_CERT_LIFETIME (2*24*60*60) -#define OLD_CERT_LIFETIME (7*24*60*60) +#define SUPERSEDED_CERT_LIFETIME (2*24*60*60) if (!trusted_dir_certs) return; DIGESTMAP_FOREACH(trusted_dir_certs, key, cert_list_t *, cl) { - authority_cert_t *newest = NULL; - SMARTLIST_FOREACH(cl->certs, authority_cert_t *, cert, - if (!newest || (cert->cache_info.published_on > - newest->cache_info.published_on)) - newest = cert); - if (newest) { - const time_t newest_published = newest->cache_info.published_on; - SMARTLIST_FOREACH_BEGIN(cl->certs, authority_cert_t *, cert) { - int expired; - time_t cert_published; - if (newest == cert) - continue; - /* resolve spurious clang shallow analysis null pointer errors */ - tor_assert(cert); - expired = now > cert->expires; - cert_published = cert->cache_info.published_on; - /* Store expired certs for 48 hours after a newer arrives; + /* Sort the list from first-published to last-published */ + smartlist_sort(cl->certs, compare_certs_by_pubdates); + + SMARTLIST_FOREACH_BEGIN(cl->certs, authority_cert_t *, cert) { + if (cert_sl_idx == smartlist_len(cl->certs) - 1) { + /* This is the most recently published cert. Keep it. */ + continue; + } + authority_cert_t *next_cert = smartlist_get(cl->certs, cert_sl_idx+1); + const time_t next_cert_published = next_cert->cache_info.published_on; + if (next_cert_published > now) { + /* All later certs are published in the future. Keep everything + * we didn't discard. */ + break; + } + int should_remove = 0; + if (cert->expires + DEAD_CERT_LIFETIME < now) { + /* Certificate has been expired for at least DEAD_CERT_LIFETIME. + * Remove it. */ + should_remove = 1; + } else if (next_cert_published + SUPERSEDED_CERT_LIFETIME < now) { + /* Certificate has been superseded for OLD_CERT_LIFETIME. + * Remove it. */ - if (expired ? - (newest_published + DEAD_CERT_LIFETIME < now) : - (cert_published + OLD_CERT_LIFETIME < newest_published)) { - SMARTLIST_DEL_CURRENT(cl->certs, cert); - authority_cert_free(cert); - trusted_dir_servers_certs_changed = 1; - } - } SMARTLIST_FOREACH_END(cert); - } + should_remove = 1; + } + if (should_remove) { + SMARTLIST_DEL_CURRENT_KEEPORDER(cl->certs, cert); + authority_cert_free(cert); + trusted_dir_servers_certs_changed = 1; + } + } SMARTLIST_FOREACH_END(cert); + } DIGESTMAP_FOREACH_END; #undef DEAD_CERT_LIFETIME #undef OLD_CERT_LIFETIME