Merge remote-tracking branch 'public/bug11454_11457'

4 files changed, 68 insertions, 27 deletions

diff --git a/changes/bug11454 b/changes/bug11454
new file mode 100644
index 0000000000..da793de868
--- /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 0000000000..a8b76f6d78
--- /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 6edc80f75b..d07697f019 100644
--- a/src/common/container.h
+++ b/src/common/container.h
@@ -247,6 +247,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 375732a310..f4f6200bbc 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