Merge remote-tracking branch 'catalyst-github/bug25756'

changes/bug25756

@@ -0,0 +1,7 @@
+  o Minor bugfixes (error reporting):
+    - Improve tolerance for directory authorities with skewed clocks.
+      Previously, an authority with a clock more than 60 seconds ahead
+      could cause a client with a correct clock to warn that the
+      client's clock was behind.  Now the clocks of a majority of
+      directory authorities have to be ahead of the client before this
+      warning will occur.  Fixes bug 25756; bugfix on 0.2.2.25-alpha.

src/or/connection.c

@@ -5234,10 +5234,10 @@ connection_free_all(void)
  * that we had more faith in and therefore the warning level should have higher
  * severity.
  */
-void
+MOCK_IMPL(void,
-clock_skew_warning(const connection_t *conn, long apparent_skew, int trusted,
+clock_skew_warning, (const connection_t *conn, long apparent_skew, int trusted,
                      log_domain_mask_t domain, const char *received,
-                   const char *source)
+                     const char *source))
 {
   char dbuf[64];
   char *ext_source = NULL, *warn = NULL;

src/or/connection.h

@@ -254,9 +254,10 @@ void assert_connection_ok(connection_t *conn, time_t now);
 int connection_or_nonopen_was_started_here(or_connection_t *conn);
 void connection_dump_buffer_mem_stats(int severity);
 
-void clock_skew_warning(const connection_t *conn, long apparent_skew,
+MOCK_DECL(void, clock_skew_warning,
-                        int trusted, log_domain_mask_t domain,
+          (const connection_t *conn, long apparent_skew, int trusted,
-                        const char *received, const char *source);
+           log_domain_mask_t domain, const char *received,
+           const char *source));
 
 /** Check if a connection is on the way out so the OOS handler doesn't try
  * to kill more than it needs. */

src/or/networkstatus.c

@@ -1733,6 +1733,57 @@ handle_missing_protocol_warning(const networkstatus_t *c,
     handle_missing_protocol_warning_impl(c, 1);
 }
 
+/**
+ * Check whether we received a consensus that appears to be coming
+ * from the future.  Because we implicitly trust the directory
+ * authorities' idea of the current time, we produce a warning if we
+ * get an early consensus.
+ *
+ * If we got a consensus that is time stamped far in the past, that
+ * could simply have come from a stale cache.  Possible ways to get a
+ * consensus from the future can include:
+ *
+ * - enough directory authorities have wrong clocks
+ * - directory authorities collude to produce misleading time stamps
+ * - our own clock is wrong (this is by far the most likely)
+ *
+ * We neglect highly improbable scenarios that involve actual time
+ * travel.
+ */
+STATIC void
+warn_early_consensus(const networkstatus_t *c, const char *flavor,
+                     time_t now)
+{
+  char tbuf[ISO_TIME_LEN+1];
+  char dbuf[64];
+  long delta = now - c->valid_after;
+  char *flavormsg = NULL;
+
+/** If a consensus appears more than this many seconds before it could
+ * possibly be a sufficiently-signed consensus, declare that our clock
+ * is skewed. */
+#define EARLY_CONSENSUS_NOTICE_SKEW 60
+
+  /* We assume that if a majority of dirauths have accurate clocks,
+   * the earliest that a dirauth with a skewed clock could possibly
+   * publish a sufficiently-signed consensus is (valid_after -
+   * dist_seconds).  Before that time, the skewed dirauth would be
+   * unable to obtain enough authority signatures for the consensus to
+   * be valid. */
+  if (now >= c->valid_after - c->dist_seconds - EARLY_CONSENSUS_NOTICE_SKEW)
+    return;
+
+  format_iso_time(tbuf, c->valid_after);
+  format_time_interval(dbuf, sizeof(dbuf), delta);
+  log_warn(LD_GENERAL, "Our clock is %s behind the time published in the "
+           "consensus network status document (%s UTC).  Tor needs an "
+           "accurate clock to work correctly. Please check your time and "
+           "date settings!", dbuf, tbuf);
+  tor_asprintf(&flavormsg, "%s flavor consensus", flavor);
+  clock_skew_warning(NULL, delta, 1, LD_GENERAL, flavormsg, "CONSENSUS");
+  tor_free(flavormsg);
+}
+
 /** Try to replace the current cached v3 networkstatus with the one in
  * <b>consensus</b>.  If we don't have enough certificates to validate it,
  * store it in consensus_waiting_for_certs and launch a certificate fetch.
@@ -2035,25 +2086,7 @@ networkstatus_set_current_consensus(const char *consensus,
     write_str_to_file(consensus_fname, consensus, 0);
   }
 
-/** If a consensus appears more than this many seconds before its declared
+  warn_early_consensus(c, flavor, now);
- * valid-after time, declare that our clock is skewed. */
-#define EARLY_CONSENSUS_NOTICE_SKEW 60
-
-  if (now < c->valid_after - EARLY_CONSENSUS_NOTICE_SKEW) {
-    char tbuf[ISO_TIME_LEN+1];
-    char dbuf[64];
-    long delta = now - c->valid_after;
-    char *flavormsg = NULL;
-    format_iso_time(tbuf, c->valid_after);
-    format_time_interval(dbuf, sizeof(dbuf), delta);
-    log_warn(LD_GENERAL, "Our clock is %s behind the time published in the "
-             "consensus network status document (%s UTC).  Tor needs an "
-             "accurate clock to work correctly. Please check your time and "
-             "date settings!", dbuf, tbuf);
-    tor_asprintf(&flavormsg, "%s flavor consensus", flavor);
-    clock_skew_warning(NULL, delta, 1, LD_GENERAL, flavormsg, "CONSENSUS");
-    tor_free(flavormsg);
-  }
 
   /* We got a new consesus. Reset our md fetch fail cache */
   microdesc_reset_outdated_dirservers_list();

src/or/networkstatus.h

@@ -151,6 +151,8 @@ void vote_routerstatus_free_(vote_routerstatus_t *rs);
 #ifdef TOR_UNIT_TESTS
 STATIC int networkstatus_set_current_consensus_from_ns(networkstatus_t *c,
                                                 const char *flavor);
+STATIC void warn_early_consensus(const networkstatus_t *c, const char *flavor,
+                                 time_t now);
 extern networkstatus_t *current_ns_consensus;
 extern networkstatus_t *current_md_consensus;
 #endif /* defined(TOR_UNIT_TESTS) */

src/test/log_test_helpers.h

@@ -85,6 +85,10 @@ void mock_dump_saved_logs(void);
   assert_log_predicate(!mock_saved_log_has_message(str), \
                 "expected log to not contain " # str);
 
+#define expect_no_log_msg_containing(str) \
+  assert_log_predicate(!mock_saved_log_has_message_containing(str), \
+                "expected log to not contain " # str);
+
 #define expect_log_severity(severity) \
   assert_log_predicate(mock_saved_log_has_severity(severity), \
                 "expected log to contain severity " # severity);

src/test/test_routerlist.c

@@ -37,7 +37,7 @@
 #include "test_dir_common.h"
 #include "log_test_helpers.h"
 
-void construct_consensus(char **consensus_text_md);
+void construct_consensus(char **consensus_text_md, time_t now);
 
 static authority_cert_t *mock_cert;
 
@@ -136,7 +136,7 @@ test_routerlist_launch_descriptor_downloads(void *arg)
 }
 
 void
-construct_consensus(char **consensus_text_md)
+construct_consensus(char **consensus_text_md, time_t now)
 {
   networkstatus_t *vote = NULL;
   networkstatus_t *v1 = NULL, *v2 = NULL, *v3 = NULL;
@@ -144,7 +144,6 @@ construct_consensus(char **consensus_text_md)
   authority_cert_t *cert1=NULL, *cert2=NULL, *cert3=NULL;
   crypto_pk_t *sign_skey_1=NULL, *sign_skey_2=NULL, *sign_skey_3=NULL;
   crypto_pk_t *sign_skey_leg=NULL;
-  time_t now = time(NULL);
   smartlist_t *votes = NULL;
   int n_vrs;
 
@@ -259,7 +258,7 @@ test_router_pick_directory_server_impl(void *arg)
   rs = router_pick_directory_server_impl(V3_DIRINFO, (const int) 0, NULL);
   tt_ptr_op(rs, OP_EQ, NULL);
 
-  construct_consensus(&consensus_text_md);
+  construct_consensus(&consensus_text_md, now);
   tt_assert(consensus_text_md);
   con_md = networkstatus_parse_vote_from_string(consensus_text_md, NULL,
                                                 NS_TYPE_CONSENSUS);
@@ -453,6 +452,7 @@ test_directory_guard_fetch_with_no_dirinfo(void *arg)
   int retval;
   char *consensus_text_md = NULL;
   or_options_t *options = get_options_mutable();
+  time_t now = time(NULL);
 
   (void) arg;
 
@@ -496,7 +496,7 @@ test_directory_guard_fetch_with_no_dirinfo(void *arg)
   conn->requested_resource = tor_strdup("ns");
 
   /* Construct a consensus */
-  construct_consensus(&consensus_text_md);
+  construct_consensus(&consensus_text_md, now);
   tt_assert(consensus_text_md);
 
   /* Place the consensus in the dirconn */
@@ -507,7 +507,7 @@ test_directory_guard_fetch_with_no_dirinfo(void *arg)
   args.body_len = strlen(consensus_text_md);
 
   /* Update approx time so that the consensus is considered live */
-  update_approx_time(time(NULL)+1010);
+  update_approx_time(now+1010);
 
   setup_capture_of_logs(LOG_DEBUG);
 
@@ -599,11 +599,167 @@ test_routerlist_router_is_already_dir_fetching(void *arg)
 #undef TEST_ADDR_STR
 #undef TEST_DIR_PORT
 
+static long mock_apparent_skew = 0;
+
+/** Store apparent_skew and assert that the other arguments are as
+ * expected. */
+static void
+mock_clock_skew_warning(const connection_t *conn, long apparent_skew,
+                        int trusted, log_domain_mask_t domain,
+                        const char *received, const char *source)
+{
+  (void)conn;
+  mock_apparent_skew = apparent_skew;
+  tt_int_op(trusted, OP_EQ, 1);
+  tt_int_op(domain, OP_EQ, LD_GENERAL);
+  tt_str_op(received, OP_EQ, "microdesc flavor consensus");
+  tt_str_op(source, OP_EQ, "CONSENSUS");
+ done:
+  ;
+}
+
+/** Do common setup for test_timely_consensus() and
+ * test_early_consensus().  Call networkstatus_set_current_consensus()
+ * on a constructed consensus and with an appropriately-modified
+ * approx_time.  Callers expect presence or absence of appropriate log
+ * messages and control events. */
+static int
+test_skew_common(void *arg, time_t now, unsigned long *offset)
+{
+  char *consensus = NULL;
+  int retval = 0;
+
+  *offset = strtoul(arg, NULL, 10);
+
+  /* Initialize the SRV subsystem */
+  MOCK(get_my_v3_authority_cert, get_my_v3_authority_cert_m);
+  mock_cert = authority_cert_parse_from_string(AUTHORITY_CERT_1, NULL);
+  sr_init(0);
+  UNMOCK(get_my_v3_authority_cert);
+
+  construct_consensus(&consensus, now);
+  tt_assert(consensus);
+
+  update_approx_time(now + *offset);
+
+  mock_apparent_skew = 0;
+  /* Caller will call UNMOCK() */
+  MOCK(clock_skew_warning, mock_clock_skew_warning);
+  /* Caller will call teardown_capture_of_logs() */
+  setup_capture_of_logs(LOG_WARN);
+  retval = networkstatus_set_current_consensus(consensus, "microdesc", 0,
+                                               NULL);
+
+ done:
+  tor_free(consensus);
+  return retval;
+}
+
+/** Test non-early consensus */
+static void
+test_timely_consensus(void *arg)
+{
+  time_t now = time(NULL);
+  unsigned long offset = 0;
+  int retval = 0;
+
+  retval = test_skew_common(arg, now, &offset);
+  (void)offset;
+  expect_no_log_msg_containing("behind the time published in the consensus");
+  tt_int_op(retval, OP_EQ, 0);
+  tt_int_op(mock_apparent_skew, OP_EQ, 0);
+ done:
+  teardown_capture_of_logs();
+  UNMOCK(clock_skew_warning);
+}
+
+/** Test early consensus  */
+static void
+test_early_consensus(void *arg)
+{
+  time_t now = time(NULL);
+  unsigned long offset = 0;
+  int retval = 0;
+
+  retval = test_skew_common(arg, now, &offset);
+  /* Can't use expect_single_log_msg() because of unrecognized authorities */
+  expect_log_msg_containing("behind the time published in the consensus");
+  tt_int_op(retval, OP_EQ, 0);
+  /* This depends on construct_consensus() setting valid_after=now+1000 */
+  tt_int_op(mock_apparent_skew, OP_EQ, offset - 1000);
+ done:
+  teardown_capture_of_logs();
+  UNMOCK(clock_skew_warning);
+}
+
+/** Test warn_early_consensus(), expecting no warning  */
+static void
+test_warn_early_consensus_no(const networkstatus_t *c, time_t now,
+                             long offset)
+{
+  mock_apparent_skew = 0;
+  setup_capture_of_logs(LOG_WARN);
+  warn_early_consensus(c, "microdesc", now + offset);
+  expect_no_log_msg_containing("behind the time published in the consensus");
+  tt_int_op(mock_apparent_skew, OP_EQ, 0);
+ done:
+  teardown_capture_of_logs();
+}
+
+/** Test warn_early_consensus(), expecting a warning */
+static void
+test_warn_early_consensus_yes(const networkstatus_t *c, time_t now,
+                              long offset)
+{
+  mock_apparent_skew = 0;
+  setup_capture_of_logs(LOG_WARN);
+  warn_early_consensus(c, "microdesc", now + offset);
+  /* Can't use expect_single_log_msg() because of unrecognized authorities */
+  expect_log_msg_containing("behind the time published in the consensus");
+  tt_int_op(mock_apparent_skew, OP_EQ, offset);
+ done:
+  teardown_capture_of_logs();
+}
+
+/**
+ * Test warn_early_consensus() directly, checking both the non-warning
+ * case (consensus is not early) and the warning case (consensus is
+ * early).  Depends on EARLY_CONSENSUS_NOTICE_SKEW=60.
+ */
+static void
+test_warn_early_consensus(void *arg)
+{
+  networkstatus_t *c = NULL;
+  time_t now = time(NULL);
+
+  (void)arg;
+  c = tor_malloc_zero(sizeof *c);
+  c->valid_after = now;
+  c->dist_seconds = 300;
+  mock_apparent_skew = 0;
+  MOCK(clock_skew_warning, mock_clock_skew_warning);
+  test_warn_early_consensus_no(c, now, 60);
+  test_warn_early_consensus_no(c, now, 0);
+  test_warn_early_consensus_no(c, now, -60);
+  test_warn_early_consensus_no(c, now, -360);
+  test_warn_early_consensus_yes(c, now, -361);
+  test_warn_early_consensus_yes(c, now, -600);
+  UNMOCK(clock_skew_warning);
+  tor_free(c);
+}
+
 #define NODE(name, flags) \
   { #name, test_routerlist_##name, (flags), NULL, NULL }
 #define ROUTER(name,flags) \
   { #name, test_router_##name, (flags), NULL, NULL }
 
+#define TIMELY(name, arg)                                     \
+  { name, test_timely_consensus, TT_FORK, &passthrough_setup, \
+    (char *)(arg) }
+#define EARLY(name, arg) \
+  { name, test_early_consensus, TT_FORK, &passthrough_setup, \
+    (char *)(arg) }
+
 struct testcase_t routerlist_tests[] = {
   NODE(initiate_descriptor_downloads, 0),
   NODE(launch_descriptor_downloads, 0),
@@ -611,6 +767,13 @@ struct testcase_t routerlist_tests[] = {
   ROUTER(pick_directory_server_impl, TT_FORK),
   { "directory_guard_fetch_with_no_dirinfo",
     test_directory_guard_fetch_with_no_dirinfo, TT_FORK, NULL, NULL },
+  /* These depend on construct_consensus() setting
+   * valid_after=now+1000 and dist_seconds=250 */
+  TIMELY("timely_consensus1", "1010"),
+  TIMELY("timely_consensus2", "1000"),
+  TIMELY("timely_consensus3", "690"),
+  EARLY("early_consensus1", "689"),
+  { "warn_early_consensus", test_warn_early_consensus, 0, NULL, NULL },
   END_OF_TESTCASES
 };