Merge remote-tracking branch 'mikeperry/209-path-bias-changes'

changes/bug7157

@@ -0,0 +1,26 @@
+
+ o Minor features:
+   - Alter the Path Bias log messages to be more descriptive in terms
+     of reporting timeouts and other statistics.
+   - Create three levels of Path Bias log messages, as opposed to just
+     two. These are configurable via consensus as well as via torrc
+     options PathBiasNoticeRate, PathBiasWarnRate, PathBiasExtremeRate.
+     The default values are 0.70, 0.50, and 0.30 respectively.
+   - Separate the log message levels from the decision to drop guards,
+     which also is available via torrc option PathBiasDropGuards.
+     PathBiasDropGuards defaults to 0 (off).
+   - Deprecate PathBiasDisableRate in favor of PathBiasDropGuards
+     in combination with PathBiasExtremeRate.
+   - Increase the default values for PathBiasScaleThreshold and
+     PathBiasCircThreshold from 200 and 20 to 300 and 150, respectively.
+   - Add in circuit usage accounting to path bias. If we try to use a
+     built circuit but fail for any reason, it counts as path bias.
+     Certain classes of circuits where the adversary gets to pick your
+     destination node are exempt from this accounting. Usage accounting
+     can be specifically disabled via consensus parameter or torrc.
+   - Convert all internal path bias state to double-precision floating
+     point, to avoid roundoff error and other issues.
+   - Only record path bias information for circuits that have completed
+     *two* hops. Assuming end-to-end tagging is the attack vector, this
+     makes us more resilient to ambient circuit failure without any 
+     detection capability loss.

doc/tor.1.txt

@@ -1222,28 +1222,43 @@ The following options are useful only for clients (that is, if
 
 **PathBiasNoticeRate** __NUM__ +
 
-**PathBiasDisableRate** __NUM__ +
+**PathBiasWarnRate** __NUM__ +
+
+**PathBiasExtremeRate** __NUM__ +
+
+**PathBiasDropGuards** __NUM__ +
 
 **PathBiasScaleThreshold** __NUM__ +
 
-**PathBiasScaleFactor** __NUM__::
+**PathBiasMultFactor** __NUM__ +
+
+**PathBiasScaleFactor** __NUM__ +
+
+**PathBiasUseCloseCounts** __NUM__::
     These options override the default behavior of Tor's (**currently
     experimental**) path bias detection algorithm. To try to find broken or
     misbehaving guard nodes, Tor looks for nodes where more than a certain
-    fraction of circuits through that node fail after the first hop.  The
-    PathBiasCircThreshold option controls how many circuits we need to build
-    through a guard before we make these checks.  The PathBiasNoticeRate and
-    PathBiasDisableRate options control what fraction of circuits must
-    succeed through a guard so we won't warn about it or disable it,
-    respectively.  When we have seen more than PathBiasScaleThreshold
-    circuits through a guard, we divide our observations by
-    PathBiasScaleFactor, so that new observations don't get swamped by old
-    ones. +
+    fraction of circuits through that guard fail to get built. If
+    PathBiasUseCloseCounts is set to 1 (the default), usage-based accounting is
+    performed, and circuits that fail to carry streams are also counted as
+    failures. +
+ +
+    The PathBiasCircThreshold option controls how many circuits we need to build
+    through a guard before we make these checks.  The PathBiasNoticeRate,
+    PathBiasWarnRate and PathBiasExtremeRate options control what fraction of
+    circuits must succeed through a guard so we won't write log messages.
+    If less than PathBiasExtremeRate circuits succeed *and* PathBiasDropGuards
+    is set to 1, we disable use of that guard. +
+ +
+    When we have seen more than PathBiasScaleThreshold
+    circuits through a guard, we scale our observations by
+    PathBiasMultFactor/PathBiasScaleFactor, so that new observations don't get
+    swamped by old ones. +
  +
     By default, or if a negative value is provided for one of these options,
     Tor uses reasonable defaults from the networkstatus consensus document.
-    If no defaults are available there, these options default to 20, .70,
-    0.0, 200, and 4 respectively.
+    If no defaults are available there, these options default to 150, .70,
+    .50, .30, 0, 300, 1, and 2 respectively.
 
 **ClientUseIPv6** **0**|**1**::
     If this option is set to 1, Tor might connect to entry nodes over

src/or/circuitbuild.c

@@ -59,8 +59,11 @@ static crypt_path_t *onion_next_hop_in_cpath(crypt_path_t *cpath);
 static int onion_extend_cpath(origin_circuit_t *circ);
 static int count_acceptable_nodes(smartlist_t *routers);
 static int onion_append_hop(crypt_path_t **head_ptr, extend_info_t *choice);
-static int entry_guard_inc_first_hop_count(entry_guard_t *guard);
-static void pathbias_count_success(origin_circuit_t *circ);
+static int entry_guard_inc_circ_attempt_count(entry_guard_t *guard);
+static void pathbias_count_build_success(origin_circuit_t *circ);
+static void pathbias_count_successful_close(origin_circuit_t *circ);
+static void pathbias_count_collapse(origin_circuit_t *circ);
+static void pathbias_count_unusable(origin_circuit_t *circ);
 
 /** This function tries to get a channel to the specified endpoint,
  * and then calls command_setup_channel() to give it the right
@@ -731,13 +734,17 @@ circuit_send_next_onion_skin(origin_circuit_t *circ)
         }
       }
 
-      pathbias_count_success(circ);
+      pathbias_count_build_success(circ);
       circuit_rep_hist_note_result(circ);
       circuit_has_opened(circ); /* do other actions as necessary */
 
       /* We're done with measurement circuits here. Just close them */
-      if (circ->base_.purpose == CIRCUIT_PURPOSE_C_MEASURE_TIMEOUT)
+      if (circ->base_.purpose == CIRCUIT_PURPOSE_C_MEASURE_TIMEOUT) {
+        /* If a measurement circ ever gets back to us, consider it
+         * succeeded for path bias */
+        circ->path_state = PATH_STATE_USE_SUCCEEDED;
         circuit_mark_for_close(TO_CIRCUIT(circ), END_CIRC_REASON_FINISHED);
+      }
       return 0;
     }
 
@@ -980,12 +987,12 @@ circuit_init_cpath_crypto(crypt_path_t *cpath, const char *key_data,
   return 0;
 }
 
-/** The minimum number of first hop completions before we start
+/** The minimum number of circuit attempts before we start
   * thinking about warning about path bias and dropping guards */
 static int
 pathbias_get_min_circs(const or_options_t *options)
 {
-#define DFLT_PATH_BIAS_MIN_CIRC 20
+#define DFLT_PATH_BIAS_MIN_CIRC 150
   if (options->PathBiasCircThreshold >= 5)
     return options->PathBiasCircThreshold;
   else
@@ -994,10 +1001,11 @@ pathbias_get_min_circs(const or_options_t *options)
                                    5, INT32_MAX);
 }
 
+/** The circuit success rate below which we issue a notice */
 static double
 pathbias_get_notice_rate(const or_options_t *options)
 {
-#define DFLT_PATH_BIAS_NOTICE_PCT 40
+#define DFLT_PATH_BIAS_NOTICE_PCT 70
   if (options->PathBiasNoticeRate >= 0.0)
     return options->PathBiasNoticeRate;
   else
@@ -1006,23 +1014,61 @@ pathbias_get_notice_rate(const or_options_t *options)
 }
 
 /* XXXX024 I'd like to have this be static again, but entrynodes.c needs it. */
+/** The circuit success rate below which we issue a warn */
+static double
+pathbias_get_warn_rate(const or_options_t *options)
+{
+#define DFLT_PATH_BIAS_WARN_PCT 50
+  if (options->PathBiasWarnRate >= 0.0)
+    return options->PathBiasWarnRate;
+  else
+    return networkstatus_get_param(NULL, "pb_warnpct",
+                                   DFLT_PATH_BIAS_WARN_PCT, 0, 100)/100.0;
+}
+
+/* XXXX024 I'd like to have this be static again, but entrynodes.c needs it. */
+/**
+ * The extreme rate is the rate at which we would drop the guard,
+ * if pb_dropguard is also set. Otherwise we just warn.
+ */
 double
-pathbias_get_disable_rate(const or_options_t *options)
+pathbias_get_extreme_rate(const or_options_t *options)
+{
+#define DFLT_PATH_BIAS_EXTREME_PCT 30
+  if (options->PathBiasExtremeRate >= 0.0)
+    return options->PathBiasExtremeRate;
+  else
+    return networkstatus_get_param(NULL, "pb_extremepct",
+                                   DFLT_PATH_BIAS_EXTREME_PCT, 0, 100)/100.0;
+}
+
+/* XXXX024 I'd like to have this be static again, but entrynodes.c needs it. */
+/**
+ * If 1, we actually disable use of guards that fall below
+ * the extreme_pct.
+ */
+int
+pathbias_get_dropguards(const or_options_t *options)
 {
-// XXX: This needs tuning based on use + experimentation before we set it
-#define DFLT_PATH_BIAS_DISABLE_PCT 0
-  if (options->PathBiasDisableRate >= 0.0)
-    return options->PathBiasDisableRate;
+#define DFLT_PATH_BIAS_DROP_GUARDS 0
+  if (options->PathBiasDropGuards >= 0)
+    return options->PathBiasDropGuards;
   else
-    return networkstatus_get_param(NULL, "pb_disablepct",
-                                   DFLT_PATH_BIAS_DISABLE_PCT, 0, 100)/100.0;
+    return networkstatus_get_param(NULL, "pb_dropguards",
+                                   DFLT_PATH_BIAS_DROP_GUARDS, 0, 1);
 }
 
+/**
+ * This is the number of circuits at which we scale our
+ * counts by mult_factor/scale_factor. Note, this count is
+ * not exact, as we only perform the scaling in the event
+ * of no integer truncation.
+ */
 static int
 pathbias_get_scale_threshold(const or_options_t *options)
 {
-#define DFLT_PATH_BIAS_SCALE_THRESHOLD 200
-  if (options->PathBiasScaleThreshold >= 2)
+#define DFLT_PATH_BIAS_SCALE_THRESHOLD 300
+  if (options->PathBiasScaleThreshold >= 10)
     return options->PathBiasScaleThreshold;
   else
     return networkstatus_get_param(NULL, "pb_scalecircs",
@@ -1030,6 +1076,13 @@ pathbias_get_scale_threshold(const or_options_t *options)
                                    INT32_MAX);
 }
 
+/**
+ * The scale factor is the denominator for our scaling
+ * of circuit counts for our path bias window. 
+ *
+ * Note that our use of doubles for the path bias state
+ * file means that powers of 2 work best here.
+ */
 static int
 pathbias_get_scale_factor(const or_options_t *options)
 {
@@ -1041,40 +1094,116 @@ pathbias_get_scale_factor(const or_options_t *options)
                                 DFLT_PATH_BIAS_SCALE_FACTOR, 1, INT32_MAX);
 }
 
+/**
+ * The mult factor is the numerator for our scaling
+ * of circuit counts for our path bias window. It
+ * allows us to scale by fractions.
+ */
+static int
+pathbias_get_mult_factor(const or_options_t *options)
+{
+#define DFLT_PATH_BIAS_MULT_FACTOR 1
+  if (options->PathBiasMultFactor >= 1)
+    return options->PathBiasMultFactor;
+  else
+    return networkstatus_get_param(NULL, "pb_multfactor",
+                                DFLT_PATH_BIAS_MULT_FACTOR, 1,
+                                pathbias_get_scale_factor(options));
+}
+
+/**
+ * If this parameter is set to a true value (default), we use the
+ * successful_circuits_closed. Otherwise, we use the success_count.
+ */
+static int
+pathbias_use_close_counts(const or_options_t *options)
+{
+#define DFLT_PATH_BIAS_USE_CLOSE_COUNTS 1
+  if (options->PathBiasUseCloseCounts >= 0)
+    return options->PathBiasUseCloseCounts;
+  else
+    return networkstatus_get_param(NULL, "pb_useclosecounts",
+                                DFLT_PATH_BIAS_USE_CLOSE_COUNTS, 0, 1);
+}
+
+/**
+ * Convert a Guard's path state to string.
+ */
 static const char *
 pathbias_state_to_string(path_state_t state)
 {
   switch (state) {
     case PATH_STATE_NEW_CIRC:
       return "new";
-    case PATH_STATE_DID_FIRST_HOP:
-      return "first hop";
-    case PATH_STATE_SUCCEEDED:
-      return "succeeded";
+    case PATH_STATE_BUILD_ATTEMPTED:
+      return "build attempted";
+    case PATH_STATE_BUILD_SUCCEEDED:
+      return "build succeeded";
+    case PATH_STATE_USE_SUCCEEDED:
+      return "use succeeded";
+    case PATH_STATE_USE_FAILED:
+      return "use failed";
   }
 
   return "unknown";
 }
 
 /**
- * Check our circuit state to see if this is a successful first hop.
- * If so, record it in the current guard's path bias first_hop count.
+ * This function decides if a circuit has progressed far enough to count
+ * as a circuit "attempt". As long as end-to-end tagging is possible,
+ * we assume the adversary will use it over hop-to-hop failure. Therefore,
+ * we only need to account bias for the last hop. This should make us
+ * much more resilient to ambient circuit failure, and also make that
+ * failure easier to measure (we only need to measure Exit failure rates).
+ */
+static int
+pathbias_is_new_circ_attempt(origin_circuit_t *circ)
+{
+#define N2N_TAGGING_IS_POSSIBLE
+#ifdef N2N_TAGGING_IS_POSSIBLE
+  /* cpath is a circular list. We want circs with more than one hop,
+   * and the second hop must be waiting for keys still (it's just
+   * about to get them). */
+  return circ->cpath->next != circ->cpath &&
+         circ->cpath->next->state == CPATH_STATE_AWAITING_KEYS;
+#else
+  /* If tagging attacks are no longer possible, we probably want to
+   * count bias from the first hop. However, one could argue that
+   * timing-based tagging is still more useful than per-hop failure.
+   * In which case, we'd never want to use this.
+   */
+  return circ->cpath->state == CPATH_STATE_AWAITING_KEYS;
+#endif
+}
+
+/**
+ * Decide if the path bias code should count a circuit.
  *
- * Also check for several potential error cases for bug #6475.
+ * @returns 1 if we should count it, 0 otherwise.
  */
 static int
-pathbias_count_first_hop(origin_circuit_t *circ)
+pathbias_should_count(origin_circuit_t *circ)
 {
-#define FIRST_HOP_NOTICE_INTERVAL (600)
-  static ratelim_t first_hop_notice_limit =
-    RATELIM_INIT(FIRST_HOP_NOTICE_INTERVAL);
+#define PATHBIAS_COUNT_INTERVAL (600)
+  static ratelim_t count_limit =
+    RATELIM_INIT(PATHBIAS_COUNT_INTERVAL);
   char *rate_msg = NULL;
 
   /* We can't do path bias accounting without entry guards.
-   * Testing and controller circuits also have no guards. */
+   * Testing and controller circuits also have no guards.
+   *
+   * We also don't count server-side rends, because their
+   * endpoint could be chosen maliciously.
+   * Similarly, we can't count client-side intro attempts,
+   * because clients can be manipulated into connecting to
+   * malicious intro points. */
   if (get_options()->UseEntryGuards == 0 ||
           circ->base_.purpose == CIRCUIT_PURPOSE_TESTING ||
-          circ->base_.purpose == CIRCUIT_PURPOSE_CONTROLLER) {
+          circ->base_.purpose == CIRCUIT_PURPOSE_CONTROLLER ||
+          circ->base_.purpose == CIRCUIT_PURPOSE_S_CONNECT_REND ||
+          circ->base_.purpose == CIRCUIT_PURPOSE_S_REND_JOINED ||
+          (circ->base_.purpose >= CIRCUIT_PURPOSE_C_INTRODUCING &&
+           circ->base_.purpose <= CIRCUIT_PURPOSE_C_INTRODUCE_ACKED)) {
     return 0;
   }
 
@@ -1084,8 +1213,7 @@ pathbias_count_first_hop(origin_circuit_t *circ)
     /* Check for inconsistency */
     if (circ->build_state->desired_path_len != 1 ||
         !circ->build_state->onehop_tunnel) {
-      if ((rate_msg = rate_limit_log(&first_hop_notice_limit,
-              approx_time()))) {
+      if ((rate_msg = rate_limit_log(&count_limit, approx_time()))) {
         log_notice(LD_BUG,
                "One-hop circuit has length %d. Path state is %s. "
                "Circuit is a %s currently %s.%s",
@@ -1101,10 +1229,31 @@ pathbias_count_first_hop(origin_circuit_t *circ)
     return 0;
   }
 
-  if (circ->cpath->state == CPATH_STATE_AWAITING_KEYS) {
+  return 1;
+}
+
+/**
+ * Check our circuit state to see if this is a successful circuit attempt.
+ * If so, record it in the current guard's path bias circ_attempt count.
+ *
+ * Also check for several potential error cases for bug #6475.
+ */
+static int
+pathbias_count_circ_attempt(origin_circuit_t *circ)
+{
+#define CIRC_ATTEMPT_NOTICE_INTERVAL (600)
+  static ratelim_t circ_attempt_notice_limit =
+    RATELIM_INIT(CIRC_ATTEMPT_NOTICE_INTERVAL);
+  char *rate_msg = NULL;
+
+  if (!pathbias_should_count(circ)) {
+    return 0;
+  }
+
+  if (pathbias_is_new_circ_attempt(circ)) {
     /* Help track down the real cause of bug #6475: */
-    if (circ->has_opened && circ->path_state != PATH_STATE_DID_FIRST_HOP) {
-      if ((rate_msg = rate_limit_log(&first_hop_notice_limit,
+    if (circ->has_opened && circ->path_state != PATH_STATE_BUILD_ATTEMPTED) {
+      if ((rate_msg = rate_limit_log(&circ_attempt_notice_limit,
                                      approx_time()))) {
         log_info(LD_BUG,
                 "Opened circuit is in strange path state %s. "
@@ -1117,22 +1266,28 @@ pathbias_count_first_hop(origin_circuit_t *circ)
       }
     }
 
-    /* Don't count cannibalized circs for path bias */
+    /* Don't re-count cannibalized circs.. */
     if (!circ->has_opened) {
-      entry_guard_t *guard;
+      entry_guard_t *guard = NULL;
+
+      if (circ->cpath && circ->cpath->extend_info) {
+        guard = entry_guard_get_by_id_digest(
+                  circ->cpath->extend_info->identity_digest);
+      } else if (circ->base_.n_chan) {
+        guard =
+          entry_guard_get_by_id_digest(circ->base_.n_chan->identity_digest);
+      }
 
-      guard =
-        entry_guard_get_by_id_digest(circ->base_.n_chan->identity_digest);
       if (guard) {
         if (circ->path_state == PATH_STATE_NEW_CIRC) {
-          circ->path_state = PATH_STATE_DID_FIRST_HOP;
+          circ->path_state = PATH_STATE_BUILD_ATTEMPTED;
 
-          if (entry_guard_inc_first_hop_count(guard) < 0) {
+          if (entry_guard_inc_circ_attempt_count(guard) < 0) {
             /* Bogus guard; we already warned. */
             return -END_CIRC_REASON_TORPROTOCOL;
           }
         } else {
-          if ((rate_msg = rate_limit_log(&first_hop_notice_limit,
+          if ((rate_msg = rate_limit_log(&circ_attempt_notice_limit,
                   approx_time()))) {
             log_info(LD_BUG,
                    "Unopened circuit has strange path state %s. "
@@ -1145,9 +1300,9 @@ pathbias_count_first_hop(origin_circuit_t *circ)
           }
         }
       } else {
-        if ((rate_msg = rate_limit_log(&first_hop_notice_limit,
+        if ((rate_msg = rate_limit_log(&circ_attempt_notice_limit,
                 approx_time()))) {
-          log_info(LD_BUG,
+          log_info(LD_CIRC,
               "Unopened circuit has no known guard. "
               "Circuit is a %s currently %s.%s",
               circuit_purpose_to_string(circ->base_.purpose),
@@ -1157,22 +1312,6 @@ pathbias_count_first_hop(origin_circuit_t *circ)
         }
       }
     }
-  } else {
-    /* Help track down the real cause of bug #6475: */
-    if (circ->path_state == PATH_STATE_NEW_CIRC) {
-      if ((rate_msg = rate_limit_log(&first_hop_notice_limit,
-                approx_time()))) {
-        log_info(LD_BUG,
-            "A %s circuit is in cpath state %d (opened: %d). "
-            "Circuit is a %s currently %s.%s",
-            pathbias_state_to_string(circ->path_state),
-            circ->cpath->state, circ->has_opened,
-            circuit_purpose_to_string(circ->base_.purpose),
-            circuit_state_to_string(circ->base_.state),
-            rate_msg);
-        tor_free(rate_msg);
-      }
-    }
   }
 
   return 0;
@@ -1186,7 +1325,7 @@ pathbias_count_first_hop(origin_circuit_t *circ)
  * Also check for several potential error cases for bug #6475.
  */
 static void
-pathbias_count_success(origin_circuit_t *circ)
+pathbias_count_build_success(origin_circuit_t *circ)
 {
 #define SUCCESS_NOTICE_INTERVAL (600)
   static ratelim_t success_notice_limit =
@@ -1194,49 +1333,25 @@ pathbias_count_success(origin_circuit_t *circ)
   char *rate_msg = NULL;
   entry_guard_t *guard = NULL;
 
-  /* We can't do path bias accounting without entry guards.
-   * Testing and controller circuits also have no guards. */
-  if (get_options()->UseEntryGuards == 0 ||
-          circ->base_.purpose == CIRCUIT_PURPOSE_TESTING ||
-          circ->base_.purpose == CIRCUIT_PURPOSE_CONTROLLER) {
-    return;
-  }
-
-  /* Ignore one hop circuits */
-  if (circ->build_state->onehop_tunnel ||
-      circ->build_state->desired_path_len == 1) {
-    /* Check for consistency */
-    if (circ->build_state->desired_path_len != 1 ||
-        !circ->build_state->onehop_tunnel) {
-      if ((rate_msg = rate_limit_log(&success_notice_limit,
-              approx_time()))) {
-        log_notice(LD_BUG,
-               "One-hop circuit has length %d. Path state is %s. "
-               "Circuit is a %s currently %s.%s",
-               circ->build_state->desired_path_len,
-               pathbias_state_to_string(circ->path_state),
-               circuit_purpose_to_string(circ->base_.purpose),
-               circuit_state_to_string(circ->base_.state),
-               rate_msg);
-        tor_free(rate_msg);
-      }
-      tor_fragile_assert();
-    }
+  if (!pathbias_should_count(circ)) {
     return;
   }
 
-  /* Don't count cannibalized/reused circs for path bias */
+  /* Don't count cannibalized/reused circs for path bias
+   * build success.. They get counted under use success */
   if (!circ->has_opened) {
-    guard =
-      entry_guard_get_by_id_digest(circ->base_.n_chan->identity_digest);
+    if (circ->cpath && circ->cpath->extend_info) {
+      guard = entry_guard_get_by_id_digest(
+                circ->cpath->extend_info->identity_digest);
+    }
 
     if (guard) {
-      if (circ->path_state == PATH_STATE_DID_FIRST_HOP) {
-        circ->path_state = PATH_STATE_SUCCEEDED;
-        guard->circuit_successes++;
+      if (circ->path_state == PATH_STATE_BUILD_ATTEMPTED) {
+        circ->path_state = PATH_STATE_BUILD_SUCCEEDED;
+        guard->circ_successes++;
 
-        log_info(LD_PROTOCOL, "Got success count %u/%u for guard %s=%s",
-                 guard->circuit_successes, guard->first_hops,
+        log_info(LD_CIRC, "Got success count %f/%f for guard %s=%s",
+                 guard->circ_successes, guard->circ_attempts,
                  guard->nickname, hex_str(guard->identity, DIGEST_LEN));
       } else {
         if ((rate_msg = rate_limit_log(&success_notice_limit,
@@ -1252,10 +1367,10 @@ pathbias_count_success(origin_circuit_t *circ)
         }
       }
 
-      if (guard->first_hops < guard->circuit_successes) {
-        log_notice(LD_BUG, "Unexpectedly high circuit_successes (%u/%u) "
+      if (guard->circ_attempts < guard->circ_successes) {
+        log_notice(LD_BUG, "Unexpectedly high successes counts (%f/%f) "
                  "for guard %s=%s",
-                 guard->circuit_successes, guard->first_hops,
+                 guard->circ_successes, guard->circ_attempts,
                  guard->nickname, hex_str(guard->identity, DIGEST_LEN));
       }
     /* In rare cases, CIRCUIT_PURPOSE_TESTING can get converted to
@@ -1264,7 +1379,7 @@ pathbias_count_success(origin_circuit_t *circ)
     } else if (circ->base_.purpose != CIRCUIT_PURPOSE_C_MEASURE_TIMEOUT) {
       if ((rate_msg = rate_limit_log(&success_notice_limit,
               approx_time()))) {
-        log_info(LD_BUG,
+        log_info(LD_CIRC,
             "Completed circuit has no known guard. "
             "Circuit is a %s currently %s.%s",
             circuit_purpose_to_string(circ->base_.purpose),
@@ -1274,7 +1389,7 @@ pathbias_count_success(origin_circuit_t *circ)
       }
     }
   } else {
-    if (circ->path_state != PATH_STATE_SUCCEEDED) {
+    if (circ->path_state < PATH_STATE_BUILD_SUCCEEDED) {
       if ((rate_msg = rate_limit_log(&success_notice_limit,
               approx_time()))) {
         log_info(LD_BUG,
@@ -1290,65 +1405,381 @@ pathbias_count_success(origin_circuit_t *circ)
   }
 }
 
+/**
+ * Check if a circuit was used and/or closed successfully.
+ *
+ * If we attempted to use the circuit to carry a stream but failed
+ * for whatever reason, or if the circuit mysteriously died before
+ * we could attach any streams, record these two cases.
+ *
+ * If we *have* successfully used the circuit, or it appears to
+ * have been closed by us locally, count it as a success.
+ */
+void
+pathbias_check_close(origin_circuit_t *ocirc, int reason)
+{
+  circuit_t *circ = &ocirc->base_;
+
+  if (!pathbias_should_count(ocirc)) {
+    return;
+  }
+
+  if (ocirc->path_state == PATH_STATE_BUILD_SUCCEEDED) {
+    if (circ->timestamp_dirty) {
+      /* Any circuit where there were attempted streams but no successful
+       * streams could be bias */
+      log_info(LD_CIRC,
+            "Circuit %d closed without successful use for reason %d. "
+            "Circuit purpose %d currently %d,%s. Len %d.",
+            ocirc->global_identifier,
+            reason, circ->purpose, ocirc->has_opened,
+            circuit_state_to_string(circ->state),
+            ocirc->build_state->desired_path_len);
+      pathbias_count_unusable(ocirc);
+    } else {
+      if (reason & END_CIRC_REASON_FLAG_REMOTE) {
+        /* Unused remote circ close reasons all could be bias */
+        log_info(LD_CIRC,
+            "Circuit %d remote-closed without successful use for reason %d. "
+            "Circuit purpose %d currently %d,%s. Len %d.",
+            ocirc->global_identifier,
+            reason, circ->purpose, ocirc->has_opened,
+            circuit_state_to_string(circ->state),
+            ocirc->build_state->desired_path_len);
+        pathbias_count_collapse(ocirc);
+      } else if ((reason & ~END_CIRC_REASON_FLAG_REMOTE)
+                  == END_CIRC_REASON_CHANNEL_CLOSED &&
+                 circ->n_chan &&
+                 circ->n_chan->reason_for_closing
+                  != CHANNEL_CLOSE_REQUESTED) {
+        /* If we didn't close the channel ourselves, it could be bias */
+        /* XXX: Only count bias if the network is live?
+         * What about clock jumps/suspends? */
+        log_info(LD_CIRC,
+            "Circuit %d's channel closed without successful use for reason "
+            "%d, channel reason %d. Circuit purpose %d currently %d,%s. Len "
+            "%d.", ocirc->global_identifier,
+            reason, circ->n_chan->reason_for_closing,
+            circ->purpose, ocirc->has_opened,
+            circuit_state_to_string(circ->state),
+            ocirc->build_state->desired_path_len);
+        pathbias_count_collapse(ocirc);
+      } else {
+        pathbias_count_successful_close(ocirc);
+      }
+    }
+  } else if (ocirc->path_state == PATH_STATE_USE_SUCCEEDED) {
+    pathbias_count_successful_close(ocirc);
+  }
+}
+
+/**
+ * Count a successfully closed circuit.
+ */
+static void
+pathbias_count_successful_close(origin_circuit_t *circ)
+{
+  entry_guard_t *guard = NULL;
+  if (!pathbias_should_count(circ)) {
+    return;
+  }
+
+  if (circ->cpath && circ->cpath->extend_info) {
+    guard = entry_guard_get_by_id_digest(
+              circ->cpath->extend_info->identity_digest);
+  }
+
+  if (guard) {
+    /* In the long run: circuit_success ~= successful_circuit_close +
+     *                                     circ_failure + stream_failure */
+    guard->successful_circuits_closed++;
+    entry_guards_changed();
+  } else if (circ->base_.purpose != CIRCUIT_PURPOSE_C_MEASURE_TIMEOUT) {
+   /* In rare cases, CIRCUIT_PURPOSE_TESTING can get converted to
+    * CIRCUIT_PURPOSE_C_MEASURE_TIMEOUT and have no guards here.
+    * No need to log that case. */
+    log_info(LD_CIRC,
+        "Successfully closed circuit has no known guard. "
+        "Circuit is a %s currently %s",
+        circuit_purpose_to_string(circ->base_.purpose),
+        circuit_state_to_string(circ->base_.state));
+  }
+}
+
+/**
+ * Count a circuit that fails after it is built, but before it can
+ * carry any traffic.
+ *
+ * This is needed because there are ways to destroy a
+ * circuit after it has successfully completed. Right now, this is
+ * used for purely informational/debugging purposes.
+ */
+static void
+pathbias_count_collapse(origin_circuit_t *circ)
+{
+  entry_guard_t *guard = NULL;
+  if (!pathbias_should_count(circ)) {
+    return;
+  }
+
+  if (circ->cpath && circ->cpath->extend_info) {
+    guard = entry_guard_get_by_id_digest(
+              circ->cpath->extend_info->identity_digest);
+  }
+
+  if (guard) {
+    guard->collapsed_circuits++;
+    entry_guards_changed();
+  } else if (circ->base_.purpose != CIRCUIT_PURPOSE_C_MEASURE_TIMEOUT) {
+   /* In rare cases, CIRCUIT_PURPOSE_TESTING can get converted to
+    * CIRCUIT_PURPOSE_C_MEASURE_TIMEOUT and have no guards here.
+    * No need to log that case. */
+    log_info(LD_CIRC,
+        "Destroyed circuit has no known guard. "
+        "Circuit is a %s currently %s",
+        circuit_purpose_to_string(circ->base_.purpose),
+        circuit_state_to_string(circ->base_.state));
+  }
+}
+
+static void
+pathbias_count_unusable(origin_circuit_t *circ)
+{
+  entry_guard_t *guard = NULL;
+  if (!pathbias_should_count(circ)) {
+    return;
+  }
+
+  if (circ->cpath && circ->cpath->extend_info) {
+    guard = entry_guard_get_by_id_digest(
+              circ->cpath->extend_info->identity_digest);
+  }
+
+  if (guard) {
+    guard->unusable_circuits++;
+    entry_guards_changed();
+  } else if (circ->base_.purpose != CIRCUIT_PURPOSE_C_MEASURE_TIMEOUT) {
+   /* In rare cases, CIRCUIT_PURPOSE_TESTING can get converted to
+    * CIRCUIT_PURPOSE_C_MEASURE_TIMEOUT and have no guards here.
+    * No need to log that case. */
+    log_info(LD_CIRC,
+        "Stream-failing circuit has no known guard. "
+        "Circuit is a %s currently %s",
+        circuit_purpose_to_string(circ->base_.purpose),
+        circuit_state_to_string(circ->base_.state));
+  }
+}
+
+/**
+ * Count timeouts for path bias log messages.
+ *
+ * These counts are purely informational.
+ */
+void
+pathbias_count_timeout(origin_circuit_t *circ)
+{
+  entry_guard_t *guard = NULL;
+
+  if (!pathbias_should_count(circ)) {
+    return;
+  }
+
+  /* For hidden service circs, they can actually be used
+   * successfully and then time out later (because
+   * the other side declines to use them). */
+  if (circ->path_state == PATH_STATE_USE_SUCCEEDED) {
+    return;
+  }
+
+  if (circ->cpath && circ->cpath->extend_info) {
+    guard = entry_guard_get_by_id_digest(
+              circ->cpath->extend_info->identity_digest);
+  }
+
+  if (guard) {
+    guard->timeouts++;
+    entry_guards_changed();
+  }
+}
+
+/**
+ * Return the number of circuits counted as successfully closed for
+ * this guard.
+ * 
+ * Also add in the currently open circuits to give them the benefit
+ * of the doubt.
+ */
+double
+pathbias_get_closed_count(entry_guard_t *guard)
+{
+  circuit_t *circ = global_circuitlist;
+  int open_circuits = 0;
+
+  /* Count currently open circuits. Give them the benefit of the doubt */
+  for ( ; circ; circ = circ->next) {
+    origin_circuit_t *ocirc = NULL;
+    if (!CIRCUIT_IS_ORIGIN(circ) || /* didn't originate here */
+        circ->marked_for_close) /* already counted */
+      continue;
+
+    ocirc = TO_ORIGIN_CIRCUIT(circ);
+
+    if (!ocirc->cpath || !ocirc->cpath->extend_info)
+      continue;
+
+    if (ocirc->path_state >= PATH_STATE_BUILD_SUCCEEDED &&
+        fast_memeq(guard->identity,
+                ocirc->cpath->extend_info->identity_digest,
+                DIGEST_LEN)) {
+      open_circuits++;
+    }
+  }
+
+  return guard->successful_circuits_closed + open_circuits;
+}
+
+/**
+ * This function checks the consensus parameters to decide
+ * if it should return guard->circ_successes or
+ * guard->successful_circuits_closed.
+ */
+double
+pathbias_get_success_count(entry_guard_t *guard)
+{
+  if (pathbias_use_close_counts(get_options())) {
+    return pathbias_get_closed_count(guard);
+  } else {
+    return guard->circ_successes;
+  }
+}
+
 /** Increment the number of times we successfully extended a circuit to
  * 'guard', first checking if the failure rate is high enough that we should
  * eliminate the guard.  Return -1 if the guard looks no good; return 0 if the
  * guard looks fine. */
 static int
-entry_guard_inc_first_hop_count(entry_guard_t *guard)
+entry_guard_inc_circ_attempt_count(entry_guard_t *guard)
 {
   const or_options_t *options = get_options();
 
   entry_guards_changed();
 
-  if (guard->first_hops > (unsigned)pathbias_get_min_circs(options)) {
+  if (guard->circ_attempts > pathbias_get_min_circs(options)) {
     /* Note: We rely on the < comparison here to allow us to set a 0
      * rate and disable the feature entirely. If refactoring, don't
      * change to <= */
-    if (guard->circuit_successes/((double)guard->first_hops)
-        < pathbias_get_disable_rate(options)) {
-
-      /* This message is currently disabled by default. */
-      log_warn(LD_PROTOCOL,
-               "Extremely low circuit success rate %u/%u for guard %s=%s. "
-               "This indicates either an overloaded guard, an attack, or "
-               "a bug.",
-               guard->circuit_successes, guard->first_hops, guard->nickname,
-               hex_str(guard->identity, DIGEST_LEN));
-
-      guard->path_bias_disabled = 1;
-      guard->bad_since = approx_time();
-      return -1;
-    } else if (guard->circuit_successes/((double)guard->first_hops)
-               < pathbias_get_notice_rate(options)
-               && !guard->path_bias_notice) {
-      guard->path_bias_notice = 1;
-      log_notice(LD_PROTOCOL,
-                 "Low circuit success rate %u/%u for guard %s=%s.",
-                 guard->circuit_successes, guard->first_hops, guard->nickname,
-                 hex_str(guard->identity, DIGEST_LEN));
+    if (pathbias_get_success_count(guard)/guard->circ_attempts
+        < pathbias_get_extreme_rate(options)) {
+      /* Dropping is currently disabled by default. */
+      if (pathbias_get_dropguards(options)) {
+        if (!guard->path_bias_disabled) {
+          log_warn(LD_CIRC,
+                 "Your Guard %s=%s is failing an extremely large amount of "
+                 "circuits. To avoid potential route manipluation attacks, "
+                 "Tor has disabled use of this guard. "
+                 "Success counts are %ld/%ld. %ld circuits completed, %ld "
+                 "were unusable, %ld collapsed, and %ld timed out. For "
+                 "reference, your timeout cutoff is %ld seconds.",
+                 guard->nickname, hex_str(guard->identity, DIGEST_LEN),
+                 tor_lround(pathbias_get_closed_count(guard)),
+                 tor_lround(guard->circ_attempts),
+                 tor_lround(guard->circ_successes),
+                 tor_lround(guard->unusable_circuits),
+                 tor_lround(guard->collapsed_circuits),
+                 tor_lround(guard->timeouts),
+                 tor_lround(circ_times.close_ms/1000));
+          guard->path_bias_disabled = 1;
+          guard->bad_since = approx_time();
+          return -1;
+        }
+      } else if (!guard->path_bias_extreme) {
+        guard->path_bias_extreme = 1;
+        log_warn(LD_CIRC,
+                 "Your Guard %s=%s is failing an extremely large amount of "
+                 "circuits. This could indicate a route manipulation attack, "
+                 "extreme network overload, or a bug. "
+                 "Success counts are %ld/%ld. %ld circuits completed, %ld "
+                 "were unusable, %ld collapsed, and %ld timed out. For "
+                 "reference, your timeout cutoff is %ld seconds.",
+                 guard->nickname, hex_str(guard->identity, DIGEST_LEN),
+                 tor_lround(pathbias_get_closed_count(guard)),
+                 tor_lround(guard->circ_attempts),
+                 tor_lround(guard->circ_successes),
+                 tor_lround(guard->unusable_circuits),
+                 tor_lround(guard->collapsed_circuits),
+                 tor_lround(guard->timeouts),
+                 tor_lround(circ_times.close_ms/1000));
+      }
+    } else if (pathbias_get_success_count(guard)/((double)guard->circ_attempts)
+               < pathbias_get_warn_rate(options)) {
+      if (!guard->path_bias_warned) {
+        guard->path_bias_warned = 1;
+        log_warn(LD_CIRC,
+                 "Your Guard %s=%s is failing a very large amount of "
+                 "circuits. Most likely this means the Tor network is "
+                 "overloaded, but it could also mean an attack against "
+                 "you or the potentially the guard itself. "
+                 "Success counts are %ld/%ld. %ld circuits completed, %ld "
+                 "were unusable, %ld collapsed, and %ld timed out. For "
+                 "reference, your timeout cutoff is %ld seconds.",
+                 guard->nickname, hex_str(guard->identity, DIGEST_LEN),
+                 tor_lround(pathbias_get_closed_count(guard)),
+                 tor_lround(guard->circ_attempts),
+                 tor_lround(guard->circ_successes),
+                 tor_lround(guard->unusable_circuits),
+                 tor_lround(guard->collapsed_circuits),
+                 tor_lround(guard->timeouts),
+                 tor_lround(circ_times.close_ms/1000));
+      }
+    } else if (pathbias_get_success_count(guard)/((double)guard->circ_attempts)
+               < pathbias_get_notice_rate(options)) {
+      if (!guard->path_bias_noticed) {
+        guard->path_bias_noticed = 1;
+        log_notice(LD_CIRC,
+                   "Your Guard %s=%s is failing more circuits than usual. "
+                   "Most likely this means the Tor network is overloaded. "
+                   "Success counts are %ld/%ld. %ld circuits completed, %ld "
+                   "were unusable, %ld collapsed, and %ld timed out. For "
+                   "reference, your timeout cutoff is %ld seconds.",
+                   guard->nickname, hex_str(guard->identity, DIGEST_LEN),
+                   tor_lround(pathbias_get_closed_count(guard)),
+                   tor_lround(guard->circ_attempts),
+                   tor_lround(guard->circ_successes),
+                   tor_lround(guard->unusable_circuits),
+                   tor_lround(guard->collapsed_circuits),
+                   tor_lround(guard->timeouts),
+                   tor_lround(circ_times.close_ms/1000));
+      }
     }
   }
 
   /* If we get a ton of circuits, just scale everything down */
-  if (guard->first_hops > (unsigned)pathbias_get_scale_threshold(options)) {
+  if (guard->circ_attempts > pathbias_get_scale_threshold(options)) {
     const int scale_factor = pathbias_get_scale_factor(options);
-    /* For now, only scale if there will be no rounding error...
-     * XXX024: We want to switch to a real moving average for 0.2.4. */
-    if ((guard->first_hops % scale_factor) == 0 &&
-        (guard->circuit_successes % scale_factor) == 0) {
-      log_info(LD_PROTOCOL,
-               "Scaling pathbias counts to (%u/%u)/%d for guard %s=%s",
-               guard->circuit_successes, guard->first_hops,
-               scale_factor, guard->nickname, hex_str(guard->identity,
-               DIGEST_LEN));
-      guard->first_hops /= scale_factor;
-      guard->circuit_successes /= scale_factor;
-    }
+    const int mult_factor = pathbias_get_mult_factor(options);
+    log_info(LD_CIRC,
+             "Scaling pathbias counts to (%f/%f)*(%d/%d) for guard %s=%s",
+             guard->circ_successes, guard->circ_attempts,
+             mult_factor, scale_factor, guard->nickname,
+             hex_str(guard->identity, DIGEST_LEN));
+
+    guard->circ_attempts *= mult_factor;
+    guard->circ_successes *= mult_factor;
+    guard->timeouts *= mult_factor;
+    guard->successful_circuits_closed *= mult_factor;
+    guard->collapsed_circuits *= mult_factor;
+    guard->unusable_circuits *= mult_factor;
+
+    guard->circ_attempts /= scale_factor;
+    guard->circ_successes /= scale_factor;
+    guard->timeouts /= scale_factor;
+    guard->successful_circuits_closed /= scale_factor;
+    guard->collapsed_circuits /= scale_factor;
+    guard->unusable_circuits /= scale_factor;
   }
-  guard->first_hops++;
-  log_info(LD_PROTOCOL, "Got success count %u/%u for guard %s=%s",
-           guard->circuit_successes, guard->first_hops, guard->nickname,
+  guard->circ_attempts++;
+  log_info(LD_CIRC, "Got success count %f/%f for guard %s=%s",
+           guard->circ_successes, guard->circ_attempts, guard->nickname,
            hex_str(guard->identity, DIGEST_LEN));
   return 0;
 }
@@ -1371,7 +1802,7 @@ circuit_finish_handshake(origin_circuit_t *circ, uint8_t reply_type,
   crypt_path_t *hop;
   int rv;
 
-  if ((rv = pathbias_count_first_hop(circ)) < 0)
+  if ((rv = pathbias_count_circ_attempt(circ)) < 0)
     return rv;
 
   if (circ->cpath->state == CPATH_STATE_AWAITING_KEYS) {
@@ -2062,6 +2493,11 @@ circuit_extend_to_new_exit(origin_circuit_t *circ, extend_info_t *exit)
     circuit_mark_for_close(TO_CIRCUIT(circ), -err_reason);
     return -1;
   }
+
+  /* Set timestamp_dirty, so we can check it for path use bias */
+  if (!circ->base_.timestamp_dirty)
+    circ->base_.timestamp_dirty = time(NULL);
+
   return 0;
 }
 

src/or/circuitbuild.h

@@ -53,7 +53,10 @@ const char *build_state_get_exit_nickname(cpath_build_state_t *state);
 
 const node_t *choose_good_entry_server(uint8_t purpose,
                                        cpath_build_state_t *state);
-double pathbias_get_disable_rate(const or_options_t *options);
+double pathbias_get_extreme_rate(const or_options_t *options);
+int pathbias_get_dropguards(const or_options_t *options);
+void pathbias_count_timeout(origin_circuit_t *circ);
+void pathbias_check_close(origin_circuit_t *circ, int reason);
 
 #endif
 

src/or/circuitlist.c

@@ -1038,8 +1038,13 @@ circuit_unlink_all_from_channel(channel_t *chan, int reason)
   for (circ = global_circuitlist; circ; circ = circ->next) {
     int mark = 0;
     if (circ->n_chan == chan) {
-        circuit_set_n_circid_chan(circ, 0, NULL);
-        mark = 1;
+      circuit_set_n_circid_chan(circ, 0, NULL);
+      mark = 1;
+
+      /* If we didn't request this closure, pass the remote
+       * bit to mark_for_close. */
+      if (chan->reason_for_closing != CHANNEL_CLOSE_REQUESTED)
+        reason |= END_CIRC_REASON_FLAG_REMOTE;
     }
     if (! CIRCUIT_IS_ORIGIN(circ)) {
       or_circuit_t *or_circ = TO_OR_CIRCUIT(circ);
@@ -1347,7 +1352,10 @@ circuit_mark_for_close_(circuit_t *circ, int reason, int line,
     }
     reason = END_CIRC_REASON_NONE;
   }
+
   if (CIRCUIT_IS_ORIGIN(circ)) {
+    pathbias_check_close(TO_ORIGIN_CIRCUIT(circ), reason);
+
     /* We don't send reasons when closing circuits at the origin. */
     reason = END_CIRC_REASON_NONE;
   }

src/or/circuituse.c

@@ -663,6 +663,8 @@ circuit_expire_building(void)
       circuit_mark_for_close(victim, END_CIRC_REASON_MEASUREMENT_EXPIRED);
     else
       circuit_mark_for_close(victim, END_CIRC_REASON_TIMEOUT);
+
+    pathbias_count_timeout(TO_ORIGIN_CIRCUIT(victim));
   }
 }
 
@@ -1158,6 +1160,18 @@ circuit_has_opened(origin_circuit_t *circ)
 {
   control_event_circuit_status(circ, CIRC_EVENT_BUILT, 0);
 
+  /* Cannibalized circuits count as used for path bias.
+   * (PURPOSE_GENERAL circs especially, since they are
+   * marked dirty and often go unused after preemptive
+   * building). */
+  // XXX: Cannibalized now use RELAY_EARLY, which is visible
+  // to taggers end-to-end! We really need to probe these instead.
+  // Don't forget to remove this check once that's done!
+  if (circ->has_opened &&
+      circ->build_state->desired_path_len > DEFAULT_ROUTE_LEN) {
+    circ->path_state = PATH_STATE_USE_SUCCEEDED;
+  }
+
   /* Remember that this circuit has finished building. Now if we start
    * it building again later (e.g. by extending it), we will know not
    * to consider its build time. */
@@ -1400,6 +1414,25 @@ circuit_launch_by_extend_info(uint8_t purpose,
                build_state_get_exit_nickname(circ->build_state), purpose,
                circuit_purpose_to_string(purpose));
 
+      if ((purpose == CIRCUIT_PURPOSE_S_CONNECT_REND ||
+           purpose == CIRCUIT_PURPOSE_C_INTRODUCING) &&
+          circ->path_state == PATH_STATE_BUILD_SUCCEEDED) {
+        /* Path bias: Cannibalized rends pre-emptively count as a
+         * successfully used circ. We don't wait until the extend,
+         * because the rend point could be malicious.
+         *
+         * Same deal goes for client side introductions. Clients
+         * can be manipulated to connect repeatedly to them
+         * (especially web clients).
+         *
+         * If we decide to probe the initial portion of these circs,
+         * (up to the adversaries final hop), we need to remove this.
+         */
+        circ->path_state = PATH_STATE_USE_SUCCEEDED;
+        /* This must be called before the purpose change */
+        pathbias_check_close(circ, END_CIRC_REASON_FINISHED);
+      }
+
       circuit_change_purpose(TO_CIRCUIT(circ), purpose);
       /* Reset the start date of this circ, else expire_building
        * will see it and think it's been trying to build since it

src/or/config.c

@@ -314,11 +314,16 @@ static config_var_t option_vars_[] = {
   VPORT(ORPort,                      LINELIST, NULL),
   V(OutboundBindAddress,         LINELIST,   NULL),
 
+  OBSOLETE("PathBiasDisableRate"),
   V(PathBiasCircThreshold,       INT,      "-1"),
   V(PathBiasNoticeRate,          DOUBLE,   "-1"),
-  V(PathBiasDisableRate,         DOUBLE,   "-1"),
+  V(PathBiasWarnRate,            DOUBLE,   "-1"),
+  V(PathBiasExtremeRate,         DOUBLE,   "-1"),
   V(PathBiasScaleThreshold,      INT,      "-1"),
   V(PathBiasScaleFactor,         INT,      "-1"),
+  V(PathBiasMultFactor,          INT,      "-1"),
+  V(PathBiasDropGuards,          AUTOBOOL, "0"),
+  V(PathBiasUseCloseCounts,      AUTOBOOL, "1"),
 
   OBSOLETE("PathlenCoinWeight"),
   V(PerConnBWBurst,              MEMUNIT,  "0"),

src/or/connection_edge.c

@@ -2186,6 +2186,27 @@ connection_ap_handshake_socks_reply(entry_connection_t *conn, char *reply,
      status==SOCKS5_SUCCEEDED ? STREAM_EVENT_SUCCEEDED : STREAM_EVENT_FAILED,
                               endreason);
 
+  /* Flag this stream's circuit as having completed a stream successfully
+   * (for path bias) */
+  if (status == SOCKS5_SUCCEEDED ||
+      endreason == END_STREAM_REASON_RESOLVEFAILED ||
+      endreason == END_STREAM_REASON_CONNECTREFUSED ||
+      endreason == END_STREAM_REASON_CONNRESET ||
+      endreason == END_STREAM_REASON_NOROUTE ||
+      endreason == END_STREAM_REASON_RESOURCELIMIT) {
+    if (!conn->edge_.on_circuit ||
+       !CIRCUIT_IS_ORIGIN(conn->edge_.on_circuit)) {
+      // DNS remaps can trigger this. So can failed hidden service
+      // lookups.
+      log_info(LD_BUG,
+               "No origin circuit for successful SOCKS stream %lu. Reason: "
+               "%d", ENTRY_TO_CONN(conn)->global_identifier, endreason);
+    } else {
+      TO_ORIGIN_CIRCUIT(conn->edge_.on_circuit)->path_state
+          = PATH_STATE_USE_SUCCEEDED;
+    }
+  }
+
   if (conn->socks_request->has_finished) {
     log_warn(LD_BUG, "(Harmless.) duplicate calls to "
              "connection_ap_handshake_socks_reply.");
@@ -2453,6 +2474,10 @@ connection_exit_begin_conn(cell_t *cell, circuit_t *circ)
     assert_circuit_ok(circ);
 
     connection_exit_connect(n_stream);
+
+    /* For path bias: This circuit was used successfully */
+    origin_circ->path_state = PATH_STATE_USE_SUCCEEDED;
+
     tor_free(address);
     return 0;
   }

src/or/entrynodes.c

@@ -1100,7 +1100,8 @@ entry_guards_parse_state(or_state_t *state, int set, char **msg)
       digestmap_set(added_by, d, tor_strdup(line->value+HEX_DIGEST_LEN+1));
     } else if (!strcasecmp(line->key, "EntryGuardPathBias")) {
       const or_options_t *options = get_options();
-      unsigned hop_cnt, success_cnt;
+      double hop_cnt, success_cnt, timeouts, collapsed, successful_closed,
+               unusable;
 
       if (!node) {
         *msg = tor_strdup("Unable to parse entry nodes: "
@@ -1108,25 +1109,48 @@ entry_guards_parse_state(or_state_t *state, int set, char **msg)
         break;
       }
 
-      if (tor_sscanf(line->value, "%u %u", &success_cnt, &hop_cnt) != 2) {
-        log_warn(LD_GENERAL, "Unable to parse guard path bias info: "
-                 "Misformated EntryGuardPathBias %s", escaped(line->value));
-        continue;
+      /* First try 3 params, then 2. */
+      /* In the long run: circuit_success ~= successful_circuit_close +
+       *                                     collapsed_circuits +
+       *                                     unusable_circuits */
+      if (tor_sscanf(line->value, "%lf %lf %lf %lf %lf %lf",
+                  &hop_cnt, &success_cnt, &successful_closed,
+                  &collapsed, &unusable, &timeouts) != 6) {
+        int old_success, old_hops;
+        if (tor_sscanf(line->value, "%u %u", &old_success, &old_hops) != 2) {
+          continue;
+        }
+        log_info(LD_GENERAL, "Reading old-style EntryGuardPathBias %s",
+                 escaped(line->value));
+
+        success_cnt = old_success;
+        successful_closed = old_success;
+        hop_cnt = old_hops;
+        timeouts = 0;
+        collapsed = 0;
+        unusable = 0;
       }
 
-      node->first_hops = hop_cnt;
-      node->circuit_successes = success_cnt;
-      log_info(LD_GENERAL, "Read %u/%u path bias for node %s",
-               node->circuit_successes, node->first_hops, node->nickname);
+      node->circ_attempts = hop_cnt;
+      node->circ_successes = success_cnt;
+
+      node->successful_circuits_closed = successful_closed;
+      node->timeouts = timeouts;
+      node->collapsed_circuits = collapsed;
+      node->unusable_circuits = unusable;
+
+      log_info(LD_GENERAL, "Read %f/%f path bias for node %s",
+               node->circ_successes, node->circ_attempts, node->nickname);
       /* Note: We rely on the < comparison here to allow us to set a 0
        * rate and disable the feature entirely. If refactoring, don't
        * change to <= */
-      if (node->circuit_successes/((double)node->first_hops)
-          < pathbias_get_disable_rate(options)) {
+      if (pathbias_get_success_count(node)/node->circ_attempts
+            < pathbias_get_extreme_rate(options) &&
+          pathbias_get_dropguards(options)) {
         node->path_bias_disabled = 1;
         log_info(LD_GENERAL,
-                 "Path bias is too high (%u/%u); disabling node %s",
-                 node->circuit_successes, node->first_hops, node->nickname);
+                 "Path bias is too high (%f/%f); disabling node %s",
+                 node->circ_successes, node->circ_attempts, node->nickname);
       }
 
     } else {
@@ -1250,11 +1274,16 @@ entry_guards_update_state(or_state_t *state)
                      d, e->chosen_by_version, t);
         next = &(line->next);
       }
-      if (e->first_hops) {
+      if (e->circ_attempts > 0) {
         *next = line = tor_malloc_zero(sizeof(config_line_t));
         line->key = tor_strdup("EntryGuardPathBias");
-        tor_asprintf(&line->value, "%u %u",
-                     e->circuit_successes, e->first_hops);
+        /* In the long run: circuit_success ~= successful_circuit_close +
+         *                                     collapsed_circuits +
+         *                                     unusable_circuits */
+        tor_asprintf(&line->value, "%f %f %f %f %f %f",
+                     e->circ_attempts, e->circ_successes,
+                     pathbias_get_closed_count(e), e->collapsed_circuits,
+                     e->unusable_circuits, e->timeouts);
         next = &(line->next);
       }
 

src/or/entrynodes.h

@@ -31,8 +31,12 @@ typedef struct entry_guard_t {
                                   * router, 1 if we have. */
   unsigned int can_retry : 1; /**< Should we retry connecting to this entry,
                                * in spite of having it marked as unreachable?*/
-  unsigned int path_bias_notice : 1; /**< Did we alert the user about path bias
+  unsigned int path_bias_noticed : 1; /**< Did we alert the user about path
+                                       * bias for this node already? */
+  unsigned int path_bias_warned : 1; /**< Did we alert the user about path bias
                                       * for this node already? */
+  unsigned int path_bias_extreme : 1; /**< Did we alert the user about path
+                                       * bias for this node already? */
   unsigned int path_bias_disabled : 1; /**< Have we disabled this node because
                                         * of path bias issues? */
   unsigned int is_dir_cache : 1; /**< Is this node a directory cache? */
@@ -45,9 +49,18 @@ typedef struct entry_guard_t {
   time_t last_attempted; /**< 0 if we can connect to this guard, or the time
                           * at which we last failed to connect to it. */
 
-  unsigned first_hops; /**< Number of first hops this guard has completed */
-  unsigned circuit_successes; /**< Number of successfully built circuits using
+  double circ_attempts; /**< Number of circuits this guard has "attempted" */
+  double circ_successes; /**< Number of successfully built circuits using
                                * this guard as first hop. */
+  double successful_circuits_closed; /**< Number of circuits that carried
+                                        * streams successfully. */
+  double collapsed_circuits; /**< Number of fully built circuits that were
+                                 * remotely closed before any streams were
+                                 * attempted. */
+  double unusable_circuits; /**< Number of circuits for which streams were
+                                *  attempted, but none succeeded. */
+  double timeouts; /**< Number of 'right-censored' circuit timeouts for this
+                       * guard. */
 } entry_guard_t;
 
 entry_guard_t *entry_guard_get_by_id_digest(const char *digest);
@@ -100,5 +113,8 @@ int find_transport_by_bridge_addrport(const tor_addr_t *addr, uint16_t port,
 
 int validate_pluggable_transports_config(void);
 
+double pathbias_get_closed_count(entry_guard_t *gaurd);
+double pathbias_get_success_count(entry_guard_t *guard);
+
 #endif
 

src/or/or.h

@@ -2803,12 +2803,26 @@ typedef enum {
     /** This circuit is "new". It has not yet completed a first hop
      * or been counted by the path bias code. */
     PATH_STATE_NEW_CIRC = 0,
-    /** This circuit has completed a first hop, and has been counted by
+    /** This circuit has completed one/two hops, and has been counted by
      * the path bias logic. */
-    PATH_STATE_DID_FIRST_HOP = 1,
-    /** This circuit has been completely built, and has been counted as
-     * successful by the path bias logic. */
-    PATH_STATE_SUCCEEDED = 2,
+    PATH_STATE_BUILD_ATTEMPTED = 1,
+    /** This circuit has been completely built */
+    PATH_STATE_BUILD_SUCCEEDED = 2,
+    /** Did any SOCKS streams or hidserv introductions actually succeed on
+      * this circuit?
+      *
+      * Note: If we ever implement end-to-end stream timing through test
+      * stream probes (#5707), we must *not* set this for those probes
+      * (or any other automatic streams) because the adversary could
+      * just tag at a later point.
+      */
+    PATH_STATE_USE_SUCCEEDED = 3,
+
+    /**
+     * This is a special state to indicate that we got a corrupted
+     * relay cell on a circuit and we don't intend to probe it.
+     */
+    PATH_STATE_USE_FAILED = 4,
 } path_state_t;
 
 /** An origin_circuit_t holds data necessary to build and use a circuit.
@@ -2846,7 +2860,7 @@ typedef struct origin_circuit_t {
 
   /** Kludge to help us prevent the warn in bug #6475 and eventually
    * debug why we are not seeing first hops in some cases. */
-  path_state_t path_state : 2;
+  path_state_t path_state : 3;
 
   /** Set iff this is a hidden-service circuit which has timed out
    * according to our current circuit-build timeout, but which has
@@ -3850,9 +3864,13 @@ typedef struct {
    */
   int PathBiasCircThreshold;
   double PathBiasNoticeRate;
-  double PathBiasDisableRate;
+  double PathBiasWarnRate;
+  double PathBiasExtremeRate;
+  int PathBiasDropGuards;
   int PathBiasScaleThreshold;
   int PathBiasScaleFactor;
+  int PathBiasMultFactor;
+  int PathBiasUseCloseCounts;
   /** @} */
 
   int IPv6Exit; /**< Do we support exiting to IPv6 addresses? */

src/or/relay.c

@@ -693,6 +693,26 @@ connection_ap_process_end_not_open(
   edge_connection_t *edge_conn = ENTRY_TO_EDGE_CONN(conn);
   (void) layer_hint; /* unused */
 
+  if (rh->length > 0) {
+    if (reason == END_STREAM_REASON_TORPROTOCOL ||
+        reason == END_STREAM_REASON_INTERNAL ||
+        reason == END_STREAM_REASON_DESTROY) {
+      /* All three of these reasons could mean a failed tag
+       * hit the exit and it complained. Do not probe.
+       * Fail the circuit. */
+      circ->path_state = PATH_STATE_USE_FAILED;
+      return -END_CIRC_REASON_TORPROTOCOL;
+    } else {
+      /* Path bias: If we get a valid reason code from the exit,
+       * it wasn't due to tagging.
+       *
+       * We rely on recognized+digest being strong enough to make
+       * tags unlikely to allow us to get tagged, yet 'recognized'
+       * reason codes here. */
+      circ->path_state = PATH_STATE_USE_SUCCEEDED;
+    }
+  }
+
   if (rh->length > 0 && edge_reason_is_retriable(reason) &&
       /* avoid retry if rend */
       !connection_edge_is_rendezvous_stream(edge_conn)) {

src/or/rendclient.c

@@ -361,6 +361,10 @@ rend_client_introduction_acked(origin_circuit_t *circ,
 #endif
   tor_assert(circ->rend_data);
 
+  /* For path bias: This circuit was used successfully. Valid
+   * nacks and acks count. */
+  circ->path_state = PATH_STATE_USE_SUCCEEDED;
+
   if (request_len == 0) {
     /* It's an ACK; the introduction point relayed our introduction request. */
     /* Locate the rend circ which is waiting to hear about this ack,
@@ -858,6 +862,13 @@ rend_client_rendezvous_acked(origin_circuit_t *circ, const uint8_t *request,
   /* Set timestamp_dirty, because circuit_expire_building expects it
    * to specify when a circuit entered the _C_REND_READY state. */
   circ->base_.timestamp_dirty = time(NULL);
+
+  /* From a path bias point of view, this circuit is now successfully used.
+   * Waiting any longer opens us up to attacks from Bob. He could induce
+   * Alice to attempt to connect to his hidden service and never reply
+   * to her rend requests */
+  circ->path_state = PATH_STATE_USE_SUCCEEDED;
+
   /* XXXX This is a pretty brute-force approach. It'd be better to
    * attach only the connections that are waiting on this circuit, rather
    * than trying to attach them all. See comments bug 743. */

src/or/rendservice.c

@@ -1384,6 +1384,9 @@ rend_service_introduce(origin_circuit_t *circuit, const uint8_t *request,
     goto err;
   memcpy(cpath->handshake_digest, keys, DIGEST_LEN);
 
+  /* For path bias: This intro circuit was used successfully */
+  circuit->path_state = PATH_STATE_USE_SUCCEEDED;
+
   goto done;
 
  log_error:
@@ -2581,6 +2584,11 @@ rend_service_rendezvous_has_opened(origin_circuit_t *circuit)
   tor_assert(!(circuit->build_state->onehop_tunnel));
 #endif
   tor_assert(circuit->rend_data);
+
+  /* Declare the circuit dirty to avoid reuse, and for path-bias */
+  if (!circuit->base_.timestamp_dirty)
+    circuit->base_.timestamp_dirty = time(NULL);
+
   hop = circuit->build_state->service_pending_final_cpath_ref->cpath;
 
   base16_encode(hexcookie,9,circuit->rend_data->rend_cookie,4);