Merge branch 'bug7802' of ssh://git-rw.torproject.org/mikeperry/tor

changes/bug7802

+  o Minor features:
+    - Path Use Bias: Perform separate accounting for successful circuit use.
+      Separate statistics on stream attempt versus success rates are kept
+      for each guard. Configurable thresholds are provided to determine
+      when to emit log messages or disable use of guards that fail too
+      many stream attempts.
+  o Minor bugfixes:
+    - Remove a source of rounding error during path bias count scaling.
+    - Don't count cannibalized circuits as used for path bias until we
+      actually try to use them.
+    - Fix circuit_package_relay_cell warning message about n_chan==NULL.

doc/tor.1.txt

@@ -1242,16 +1242,11 @@ The following options are useful only for clients (that is, if
 
 **PathBiasMultFactor** __NUM__ +
 
-**PathBiasScaleFactor** __NUM__ +
-
-**PathBiasUseCloseCounts** __NUM__::
+**PathBiasScaleFactor** __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 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. +
+    fraction of circuits through that guard fail to get built.
  +
     The PathBiasCircThreshold option controls how many circuits we need to build
     through a guard before we make these checks.  The PathBiasNoticeRate,
@@ -1270,6 +1265,28 @@ The following options are useful only for clients (that is, if
     If no defaults are available there, these options default to 150, .70,
     .50, .30, 0, 300, 1, and 2 respectively.
 
+**PathBiasUseThreshold** __NUM__ +
+
+**PathBiasNoticeUseRate** __NUM__ +
+
+**PathBiasExtremeUseRate** __NUM__ +
+
+**PathBiasScaleUseThreshold** __NUM__::
+    Similar to the above options, these options override the default behavior
+    of Tor's (**currently experimental**) path use bias detection algorithm.
+ +
+    Where as the path bias parameters govern thresholds for successfully
+    building circuits, these four path use bias parameters govern thresholds
+    only for circuit usage. Circuits which receive no stream usage
+    are not counted by this detection algorithm. A used circuit is considered
+    successful if it is capable of carrying streams or otherwise receiving
+    well-formed responses to RELAY cells.
+ +
+    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, .90,
+    .70, and 100, respectively.
+
 **ClientUseIPv6** **0**|**1**::
     If this option is set to 1, Tor might connect to entry nodes over
     IPv6. Note that clients configured with an IPv6 address in a

src/or/circuitbuild.h

@@ -58,10 +58,13 @@ 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_extreme_rate(const or_options_t *options);
+double pathbias_get_extreme_use_rate(const or_options_t *options);
 int pathbias_get_dropguards(const or_options_t *options);
 void pathbias_count_timeout(origin_circuit_t *circ);
 int pathbias_check_close(origin_circuit_t *circ, int reason);
 int pathbias_check_probe_response(circuit_t *circ, const cell_t *cell);
+void pathbias_count_use_attempt(origin_circuit_t *circ);
+void pathbias_mark_use_success(origin_circuit_t *circ);
 
 #endif
 

src/or/circuituse.c

@@ -668,18 +668,6 @@ circuit_expire_building(void)
           circuit_build_times_set_timeout(&circ_times);
         }
       }
-
-      if (TO_ORIGIN_CIRCUIT(victim)->has_opened &&
-          victim->purpose != CIRCUIT_PURPOSE_PATH_BIAS_TESTING) {
-        /* For path bias: we want to let these guys live for a while
-         * so we get a chance to test them. */
-        log_info(LD_CIRC,
-                 "Allowing cannibalized circuit %d time to finish building "
-                 "as a pathbias testing circ.",
-                 TO_ORIGIN_CIRCUIT(victim)->global_identifier);
-        circuit_change_purpose(victim, CIRCUIT_PURPOSE_PATH_BIAS_TESTING);
-        continue; /* It now should have a longer timeout next time */
-      }
     }
 
     /* If this is a hidden service client circuit which is far enough
@@ -1090,7 +1078,10 @@ circuit_expire_old_circuits_clientside(void)
                 "purpose %d)",
                 circ->n_circ_id, (long)(now.tv_sec - circ->timestamp_dirty),
                 circ->purpose);
-      circuit_mark_for_close(circ, END_CIRC_REASON_FINISHED);
+      /* Don't do this magic for testing circuits. Their death is governed
+       * by circuit_expire_building */
+      if (circ->purpose != CIRCUIT_PURPOSE_PATH_BIAS_TESTING)
+        circuit_mark_for_close(circ, END_CIRC_REASON_FINISHED);
     } else if (!circ->timestamp_dirty && circ->state == CIRCUIT_STATE_OPEN) {
       if (timercmp(&circ->timestamp_began, &cutoff, <)) {
         if (circ->purpose == CIRCUIT_PURPOSE_C_GENERAL ||
@@ -1517,7 +1508,7 @@ circuit_launch_by_extend_info(uint8_t purpose,
          * 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);
       }
@@ -2037,6 +2028,8 @@ connection_ap_handshake_attach_chosen_circuit(entry_connection_t *conn,
   if (!circ->base_.timestamp_dirty)
     circ->base_.timestamp_dirty = time(NULL);
 
+  pathbias_count_use_attempt(circ);
+
   link_apconn_to_circ(conn, circ, cpath);
   tor_assert(conn->socks_request);
   if (conn->socks_request->command == SOCKS_COMMAND_CONNECT) {
@@ -2163,6 +2156,11 @@ connection_ap_handshake_attach_circuit(entry_connection_t *conn)
        * feasibility, at this point.
        */
       rendcirc->base_.timestamp_dirty = time(NULL);
+
+      /* We've also attempted to use them. If they fail, we need to
+       * probe them for path bias */
+      pathbias_count_use_attempt(rendcirc);
+
       link_apconn_to_circ(conn, rendcirc, NULL);
       if (connection_ap_handshake_send_begin(conn) < 0)
         return 0; /* already marked, let them fade away */
@@ -2214,6 +2212,10 @@ connection_ap_handshake_attach_circuit(entry_connection_t *conn)
         case 0: /* success */
           rendcirc->base_.timestamp_dirty = time(NULL);
           introcirc->base_.timestamp_dirty = time(NULL);
+
+          pathbias_count_use_attempt(introcirc);
+          pathbias_count_use_attempt(rendcirc);
+
           assert_circuit_ok(TO_CIRCUIT(rendcirc));
           assert_circuit_ok(TO_CIRCUIT(introcirc));
           return 0;

src/or/config.c

@@ -324,7 +324,12 @@ static config_var_t option_vars_[] = {
   V(PathBiasScaleFactor,         INT,      "-1"),
   V(PathBiasMultFactor,          INT,      "-1"),
   V(PathBiasDropGuards,          AUTOBOOL, "0"),
-  V(PathBiasUseCloseCounts,      AUTOBOOL, "1"),
+  OBSOLETE("PathBiasUseCloseCounts"),
+
+  V(PathBiasUseThreshold,       INT,      "-1"),
+  V(PathBiasNoticeUseRate,          DOUBLE,   "-1"),
+  V(PathBiasExtremeUseRate,         DOUBLE,   "-1"),
+  V(PathBiasScaleUseThreshold,      INT,      "-1"),
 
   OBSOLETE("PathlenCoinWeight"),
   V(PerConnBWBurst,              MEMUNIT,  "0"),

src/or/connection_edge.c

@@ -37,6 +37,7 @@
 #include "router.h"
 #include "routerlist.h"
 #include "routerset.h"
+#include "circuitbuild.h"
 
 #ifdef HAVE_LINUX_TYPES_H
 #include <linux/types.h>
@@ -636,6 +637,16 @@ connection_ap_expire_beginning(void)
     }
     if (circ->purpose == CIRCUIT_PURPOSE_C_REND_JOINED) {
       if (seconds_idle >= options->SocksTimeout) {
+        /* Path bias: We need to probe the circuit to ensure validity.
+         * Roll its state back if it succeeded so that we do so upon close. */
+        if (TO_ORIGIN_CIRCUIT(circ)->path_state == PATH_STATE_USE_SUCCEEDED) {
+          log_info(LD_CIRC,
+                   "Rolling back pathbias use state to 'attempted' for timed "
+                   "out rend circ %d",
+                   TO_ORIGIN_CIRCUIT(circ)->global_identifier);
+          TO_ORIGIN_CIRCUIT(circ)->path_state = PATH_STATE_USE_ATTEMPTED;
+        }
+
         log_fn(severity, LD_REND,
                "Rend stream is %d seconds late. Giving up on address"
                " '%s.onion'.",
@@ -805,6 +816,15 @@ connection_ap_detach_retriable(entry_connection_t *conn,
   control_event_stream_status(conn, STREAM_EVENT_FAILED_RETRIABLE, reason);
   ENTRY_TO_CONN(conn)->timestamp_lastread = time(NULL);
 
+  /* Path bias: We need to probe the circuit to ensure validity.
+   * Roll its state back if it succeeded so that we do so upon close. */
+  if (circ->path_state == PATH_STATE_USE_SUCCEEDED) {
+    log_info(LD_CIRC,
+             "Rolling back pathbias use state to 'attempted' for detached "
+             "circuit %d", circ->global_identifier);
+    circ->path_state = PATH_STATE_USE_ATTEMPTED;
+  }
+
   if (conn->pending_optimistic_data) {
     generic_buffer_set_to_copy(&conn->sending_optimistic_data,
                                conn->pending_optimistic_data);
@@ -2205,8 +2225,10 @@ connection_ap_handshake_socks_reply(entry_connection_t *conn, char *reply,
                U64_PRINTF_ARG(ENTRY_TO_CONN(conn)->global_identifier),
                endreason);
     } else {
-      TO_ORIGIN_CIRCUIT(conn->edge_.on_circuit)->path_state
-          = PATH_STATE_USE_SUCCEEDED;
+      // XXX: Hrmm. It looks like optimistic data can't go through this
+      // codepath, but someone should probably test it and make sure.
+      // We don't want to mark optimistically opened streams as successful.
+      pathbias_mark_use_success(TO_ORIGIN_CIRCUIT(conn->edge_.on_circuit));
     }
   }
 
@@ -2480,7 +2502,7 @@ connection_exit_begin_conn(cell_t *cell, circuit_t *circ)
     connection_exit_connect(n_stream);
 
     /* For path bias: This circuit was used successfully */
-    origin_circ->path_state = PATH_STATE_USE_SUCCEEDED;
+    pathbias_mark_use_success(origin_circ);
 
     tor_free(address);
     return 0;

src/or/entrynodes.c

@@ -1098,6 +1098,40 @@ entry_guards_parse_state(or_state_t *state, int set, char **msg)
         continue;
       }
       digestmap_set(added_by, d, tor_strdup(line->value+HEX_DIGEST_LEN+1));
+    } else if (!strcasecmp(line->key, "EntryGuardPathUseBias")) {
+      const or_options_t *options = get_options();
+      double use_cnt, success_cnt;
+
+      if (!node) {
+        *msg = tor_strdup("Unable to parse entry nodes: "
+               "EntryGuardPathUseBias without EntryGuard");
+        break;
+      }
+
+      if (tor_sscanf(line->value, "%lf %lf",
+                     &use_cnt, &success_cnt) != 2) {
+        log_info(LD_GENERAL, "Malformed path use bias line for node %s",
+                 node->nickname);
+        continue;
+      }
+
+      node->use_attempts = use_cnt;
+      node->use_successes = success_cnt;
+
+      log_info(LD_GENERAL, "Read %f/%f path use bias for node %s",
+               node->use_successes, node->use_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 (pathbias_get_use_success_count(node)/node->use_attempts
+            < pathbias_get_extreme_use_rate(options) &&
+          pathbias_get_dropguards(options)) {
+        node->path_bias_disabled = 1;
+        log_info(LD_GENERAL,
+                 "Path use bias is too high (%f/%f); disabling node %s",
+                 node->circ_successes, node->circ_attempts, node->nickname);
+      }
     } else if (!strcasecmp(line->key, "EntryGuardPathBias")) {
       const or_options_t *options = get_options();
       double hop_cnt, success_cnt, timeouts, collapsed, successful_closed,
@@ -1144,7 +1178,7 @@ entry_guards_parse_state(or_state_t *state, int set, char **msg)
       /* 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 (pathbias_get_success_count(node)/node->circ_attempts
+      if (pathbias_get_close_success_count(node)/node->circ_attempts
             < pathbias_get_extreme_rate(options) &&
           pathbias_get_dropguards(options)) {
         node->path_bias_disabled = 1;
@@ -1282,10 +1316,20 @@ entry_guards_update_state(or_state_t *state)
          *                                     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,
+                     pathbias_get_close_success_count(e),
+                     e->collapsed_circuits,
                      e->unusable_circuits, e->timeouts);
         next = &(line->next);
       }
+      if (e->use_attempts > 0) {
+        *next = line = tor_malloc_zero(sizeof(config_line_t));
+        line->key = tor_strdup("EntryGuardPathUseBias");
+
+        tor_asprintf(&line->value, "%f %f",
+                     e->use_attempts,
+                     pathbias_get_use_success_count(e));
+        next = &(line->next);
+      }
 
   } SMARTLIST_FOREACH_END(e);
   if (!get_options()->AvoidDiskWrites)

src/or/entrynodes.h

@@ -61,6 +61,9 @@ typedef struct entry_guard_t {
                                 *  attempted, but none succeeded. */
   double timeouts; /**< Number of 'right-censored' circuit timeouts for this
                        * guard. */
+  double use_attempts; /**< Number of circuits we tried to use with streams */
+  double use_successes; /**< Number of successfully used circuits using
+                               * this guard as first hop. */
 } entry_guard_t;
 
 entry_guard_t *entry_guard_get_by_id_digest(const char *digest);
@@ -113,8 +116,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);
+double pathbias_get_close_success_count(entry_guard_t *guard);
+double pathbias_get_use_success_count(entry_guard_t *guard);
 
 #endif
 

src/or/or.h

@@ -2839,6 +2839,15 @@ typedef enum {
     PATH_STATE_BUILD_ATTEMPTED = 1,
     /** This circuit has been completely built */
     PATH_STATE_BUILD_SUCCEEDED = 2,
+    /** Did we try to attach any SOCKS streams or hidserv introductions to
+      * 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_ATTEMPTED = 3,
     /** Did any SOCKS streams or hidserv introductions actually succeed on
       * this circuit?
       *
@@ -2847,13 +2856,20 @@ typedef enum {
       * (or any other automatic streams) because the adversary could
       * just tag at a later point.
       */
-    PATH_STATE_USE_SUCCEEDED = 3,
+    PATH_STATE_USE_SUCCEEDED = 4,
 
     /**
      * 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_USE_FAILED = 5,
+
+    /**
+     * This is a special state to indicate that we already counted
+     * the circuit. Used to guard against potential state machine
+     * violations.
+     */
+    PATH_STATE_ALREADY_COUNTED = 6,
 } path_state_t;
 
 /** An origin_circuit_t holds data necessary to build and use a circuit.
@@ -2998,7 +3014,6 @@ typedef struct origin_circuit_t {
    * ISO_STREAM. */
   uint64_t associated_isolated_stream_global_id;
   /**@}*/
-
 } origin_circuit_t;
 
 struct onion_queue_t;
@@ -3913,7 +3928,16 @@ typedef struct {
   int PathBiasScaleThreshold;
   int PathBiasScaleFactor;
   int PathBiasMultFactor;
-  int PathBiasUseCloseCounts;
+  /** @} */
+
+  /**
+   * Parameters for path-bias use detection
+   * @{
+   */
+  int PathBiasUseThreshold;
+  double PathBiasNoticeUseRate;
+  double PathBiasExtremeUseRate;
+  int PathBiasScaleUseThreshold;
   /** @} */
 
   int IPv6Exit; /**< Do we support exiting to IPv6 addresses? */

src/or/relay.c

@@ -730,7 +730,7 @@ connection_ap_process_end_not_open(
        * 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;
+      pathbias_mark_use_success(circ);
     }
   }
 

src/or/rendclient.c

@@ -71,6 +71,9 @@ rend_client_send_establish_rendezvous(origin_circuit_t *circ)
    * and the rend cookie also means we've used the circ. */
   circ->base_.timestamp_dirty = time(NULL);
 
+  /* We've attempted to use this circuit. Probe it if we fail */
+  pathbias_count_use_attempt(circ);
+
   if (relay_send_command_from_edge(0, TO_CIRCUIT(circ),
                                    RELAY_COMMAND_ESTABLISH_RENDEZVOUS,
                                    circ->rend_data->rend_cookie,
@@ -316,6 +319,8 @@ rend_client_send_introduction(origin_circuit_t *introcirc,
    * state. */
   introcirc->base_.timestamp_dirty = time(NULL);
 
+  pathbias_count_use_attempt(introcirc);
+
   goto cleanup;
 
  perm_err:
@@ -395,7 +400,7 @@ rend_client_introduction_acked(origin_circuit_t *circ,
 
   /* For path bias: This circuit was used successfully. Valid
    * nacks and acks count. */
-  circ->path_state = PATH_STATE_USE_SUCCEEDED;
+  pathbias_mark_use_success(circ);
 
   if (request_len == 0) {
     /* It's an ACK; the introduction point relayed our introduction request. */
@@ -902,7 +907,7 @@ rend_client_rendezvous_acked(origin_circuit_t *circ, const uint8_t *request,
    * 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;
+  pathbias_mark_use_success(circ);
 
   /* XXXX This is a pretty brute-force approach. It'd be better to
    * attach only the connections that are waiting on this circuit, rather

src/or/rendservice.c

@@ -1384,9 +1384,6 @@ rend_service_introduce(origin_circuit_t *circuit, const uint8_t *request,
     goto err;
   memcpy(cpath->rend_circ_nonce, keys, DIGEST_LEN);
 
-  /* For path bias: This intro circuit was used successfully */
-  circuit->path_state = PATH_STATE_USE_SUCCEEDED;
-
   goto done;
 
  log_error:
@@ -2511,6 +2508,9 @@ rend_service_intro_has_opened(origin_circuit_t *circuit)
     goto err;
   }
 
+  /* We've attempted to use this circuit */
+  pathbias_count_use_attempt(circuit);
+
   goto done;
 
  err:
@@ -2558,6 +2558,10 @@ rend_service_intro_established(origin_circuit_t *circuit,
            "Received INTRO_ESTABLISHED cell on circuit %d for service %s",
            circuit->base_.n_circ_id, serviceid);
 
+  /* Getting a valid INTRODUCE_ESTABLISHED means we've successfully
+   * used the circ */
+  pathbias_mark_use_success(circuit);
+
   return 0;
  err:
   circuit_mark_for_close(TO_CIRCUIT(circuit), END_CIRC_REASON_TORPROTOCOL);
@@ -2589,6 +2593,9 @@ rend_service_rendezvous_has_opened(origin_circuit_t *circuit)
   if (!circuit->base_.timestamp_dirty)
     circuit->base_.timestamp_dirty = time(NULL);
 
+  /* This may be redundant */
+  pathbias_count_use_attempt(circuit);
+
   hop = circuit->build_state->service_pending_final_cpath_ref->cpath;
 
   base16_encode(hexcookie,9,circuit->rend_data->rend_cookie,4);
@@ -3061,7 +3068,8 @@ rend_services_introduce(void)
       if (intro->time_expiring + INTRO_POINT_EXPIRATION_GRACE_PERIOD > now) {
         /* This intro point has completely expired.  Remove it, and
          * mark the circuit for close if it's still alive. */
-        if (intro_circ != NULL) {
+        if (intro_circ != NULL &&
+            intro_circ->base_.purpose != CIRCUIT_PURPOSE_PATH_BIAS_TESTING) {
           circuit_mark_for_close(TO_CIRCUIT(intro_circ),
                                  END_CIRC_REASON_FINISHED);
         }