Commit 69df16e3 authored by Nick Mathewson's avatar Nick Mathewson 🦀
Browse files

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.
parent 33df3e37

changes/bug11454

0 → 100644
+6 −0
Original line number Diff line number Diff line 
  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.

changes/bug11457

0 → 100644
+5 −0
Original line number Diff line number Diff line 
  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.

src/common/container.h

+11 −0
Original line number Diff line number Diff line
@@ -242,6 +242,17 @@ char *smartlist_join_strings2(smartlist_t *sl, const char *join, 
    --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>, 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>.

src/or/routerlist.c

+46 −27
Original line number Diff line number Diff line
@@ -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;

    /* 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) {

        int expired;

        time_t cert_published;

        if (newest == cert)

      if (cert_sl_idx == smartlist_len(cl->certs) - 1) {

        /* This is the most recently published cert.  Keep it. */

        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;

      }

      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);

        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