From 954f263ed5eb451a0742f8888681e10e64dd193a Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Wed, 24 Oct 2012 17:34:18 -0700 Subject: Add the ability to count circuit timeouts for guards. This is purely for informational reasons for debugging. --- src/or/circuitbuild.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++++++ src/or/circuituse.c | 2 ++ src/or/entrynodes.c | 18 +++++++++----- src/or/entrynodes.h | 2 ++ src/or/or.h | 2 ++ 5 files changed, 85 insertions(+), 6 deletions(-) diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c index 73018740c2..c8c8db3967 100644 --- a/src/or/circuitbuild.c +++ b/src/or/circuitbuild.c @@ -1056,6 +1056,53 @@ pathbias_state_to_string(path_state_t state) return "unknown"; } +/** + * Decide if the path bias code should count a circuit. + * + * @returns 1 if we should count it, 0 otherwise. + */ +static int +pathbias_should_count(origin_circuit_t *circ) +{ +#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. */ + if (get_options()->UseEntryGuards == 0 || + circ->base_.purpose == CIRCUIT_PURPOSE_TESTING || + circ->base_.purpose == CIRCUIT_PURPOSE_CONTROLLER) { + return 0; + } + + /* Completely ignore one hop circuits */ + if (circ->build_state->onehop_tunnel || + circ->build_state->desired_path_len == 1) { + /* Check for inconsistency */ + if (circ->build_state->desired_path_len != 1 || + !circ->build_state->onehop_tunnel) { + 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", + 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(); + } + return 0; + } + + return 1; +} + + /** * 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. @@ -1290,6 +1337,26 @@ pathbias_count_success(origin_circuit_t *circ) } } +/** + * Count timeouts for path bias log messages. + * + * These counts are purely informational. + */ +void +pathbias_count_timeout(origin_circuit_t *circ) +{ + if(!pathbias_should_count(circ)) { + return; + } + entry_guard_t *guard = + entry_guard_get_by_id_digest(circ->base_.n_chan->identity_digest); + + if (guard) { + guard->timeouts++; + entry_guards_changed(); + } +} + /** 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 diff --git a/src/or/circuituse.c b/src/or/circuituse.c index e14f9d03ca..77822a36ad 100644 --- a/src/or/circuituse.c +++ b/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)); } } diff --git a/src/or/entrynodes.c b/src/or/entrynodes.c index 8712241f62..d9a06a6573 100644 --- a/src/or/entrynodes.c +++ b/src/or/entrynodes.c @@ -1021,7 +1021,7 @@ 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; + unsigned hop_cnt, success_cnt, timeouts; if (!node) { *msg = tor_strdup("Unable to parse entry nodes: " @@ -1029,14 +1029,20 @@ 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: " + /* First try 3 params, then 2. */ + if (tor_sscanf(line->value, "%u %u %u", &success_cnt, &hop_cnt, + &timeouts) != 3) { + timeouts = 0; + 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; + continue; + } } node->first_hops = hop_cnt; node->circuit_successes = success_cnt; + node->timeouts = timeouts; log_info(LD_GENERAL, "Read %u/%u path bias for node %s", node->circuit_successes, node->first_hops, node->nickname); /* Note: We rely on the < comparison here to allow us to set a 0 @@ -1173,8 +1179,8 @@ entry_guards_update_state(or_state_t *state) if (e->first_hops) { *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); + tor_asprintf(&line->value, "%u %u %u", + e->circuit_successes, e->first_hops, e->timeouts); next = &(line->next); } diff --git a/src/or/entrynodes.h b/src/or/entrynodes.h index 4d031c3593..b34744183c 100644 --- a/src/or/entrynodes.h +++ b/src/or/entrynodes.h @@ -47,6 +47,8 @@ typedef struct entry_guard_t { unsigned first_hops; /**< Number of first hops this guard has completed */ unsigned circuit_successes; /**< Number of successfully built circuits using * this guard as first hop. */ + unsigned timeouts; /**< Number of 'right-censored' timeouts + for this guard. */ } entry_guard_t; entry_guard_t *entry_guard_get_by_id_digest(const char *digest); diff --git a/src/or/or.h b/src/or/or.h index 195cb2b98f..59202104db 100644 --- a/src/or/or.h +++ b/src/or/or.h @@ -4067,6 +4067,8 @@ typedef struct { double close_ms; } circuit_build_times_t; +void pathbias_count_timeout(origin_circuit_t *circ); + /********************************* config.c ***************************/ /** An error from options_trial_assign() or options_init_from_string(). */ -- cgit v1.2.3-54-g00ecf From 248fbc3619664e1d9f4b16732ccbdb484939624d Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Wed, 24 Oct 2012 18:03:09 -0700 Subject: Update pathbias parameters to match Proposal 209. Needs manpage update and testing still.. --- src/or/circuitbuild.c | 77 +++++++++++++++++++++++++++++++++++++++------------ src/or/circuitbuild.h | 3 +- src/or/config.c | 5 +++- src/or/entrynodes.c | 5 ++-- src/or/or.h | 5 +++- 5 files changed, 72 insertions(+), 23 deletions(-) diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c index c8c8db3967..65c6492601 100644 --- a/src/or/circuitbuild.c +++ b/src/or/circuitbuild.c @@ -985,7 +985,7 @@ circuit_init_cpath_crypto(crypt_path_t *cpath, const char *key_data, 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 @@ -997,7 +997,7 @@ pathbias_get_min_circs(const or_options_t *options) 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 @@ -1007,22 +1007,45 @@ pathbias_get_notice_rate(const or_options_t *options) /* XXXX024 I'd like to have this be static again, but entrynodes.c needs it. */ double -pathbias_get_disable_rate(const or_options_t *options) +pathbias_get_warn_rate(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_WARN_PCT 50 + if (options->PathBiasWarnRate >= 0.0) + return options->PathBiasWarnRate; else - return networkstatus_get_param(NULL, "pb_disablepct", - DFLT_PATH_BIAS_DISABLE_PCT, 0, 100)/100.0; + 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. */ +double +pathbias_get_crit_rate(const or_options_t *options) +{ +#define DFLT_PATH_BIAS_CRIT_PCT 30 + if (options->PathBiasCritRate >= 0.0) + return options->PathBiasCritRate; + else + return networkstatus_get_param(NULL, "pb_critpct", + DFLT_PATH_BIAS_CRIT_PCT, 0, 100)/100.0; +} + +/* XXXX024 I'd like to have this be static again, but entrynodes.c needs it. */ +int +pathbias_get_dropguards(const or_options_t *options) +{ +#define DFLT_PATH_BIAS_DROP_GUARDS 0 + if (options->PathBiasDropGuards >= 0) + return options->PathBiasDropGuards; + else + return networkstatus_get_param(NULL, "pb_dropguards", + DFLT_PATH_BIAS_DROP_GUARDS, 0, 100)/100.0; } 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", @@ -1041,6 +1064,18 @@ pathbias_get_scale_factor(const or_options_t *options) DFLT_PATH_BIAS_SCALE_FACTOR, 1, INT32_MAX); } +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)-1); +} + static const char * pathbias_state_to_string(path_state_t state) { @@ -1373,7 +1408,7 @@ entry_guard_inc_first_hop_count(entry_guard_t *guard) * 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)) { + < pathbias_get_crit_rate(options)) { /* This message is currently disabled by default. */ log_warn(LD_PROTOCOL, @@ -1383,8 +1418,10 @@ entry_guard_inc_first_hop_count(entry_guard_t *guard) guard->circuit_successes, guard->first_hops, guard->nickname, hex_str(guard->identity, DIGEST_LEN)); - guard->path_bias_disabled = 1; - guard->bad_since = approx_time(); + if (pathbias_get_dropguards(options)) { + 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) @@ -1400,15 +1437,19 @@ entry_guard_inc_first_hop_count(entry_guard_t *guard) /* If we get a ton of circuits, just scale everything down */ if (guard->first_hops > (unsigned)pathbias_get_scale_threshold(options)) { const int scale_factor = pathbias_get_scale_factor(options); + const int mult_factor = pathbias_get_mult_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) { + if (((mult_factor*guard->first_hops) % scale_factor) == 0 && + ((mult_factor*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, + "Scaling pathbias counts to (%u/%u)*(%d/%d) for guard %s=%s", + guard->circuit_successes, guard->first_hops, mult_factor, scale_factor, guard->nickname, hex_str(guard->identity, DIGEST_LEN)); + guard->first_hops *= mult_factor; + guard->circuit_successes *= mult_factor; + guard->first_hops /= scale_factor; guard->circuit_successes /= scale_factor; } diff --git a/src/or/circuitbuild.h b/src/or/circuitbuild.h index 78575afcf2..52f9f7955d 100644 --- a/src/or/circuitbuild.h +++ b/src/or/circuitbuild.h @@ -53,7 +53,8 @@ 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_crit_rate(const or_options_t *options); +int pathbias_get_dropguards(const or_options_t *options); #endif diff --git a/src/or/config.c b/src/or/config.c index 75f6193352..fffe14c644 100644 --- a/src/or/config.c +++ b/src/or/config.c @@ -316,9 +316,12 @@ static config_var_t option_vars_[] = { V(PathBiasCircThreshold, INT, "-1"), V(PathBiasNoticeRate, DOUBLE, "-1"), - V(PathBiasDisableRate, DOUBLE, "-1"), + V(PathBiasWarnRate, DOUBLE, "-1"), + V(PathBiasCritRate, DOUBLE, "-1"), V(PathBiasScaleThreshold, INT, "-1"), V(PathBiasScaleFactor, INT, "-1"), + V(PathBiasMultFactor, INT, "-1"), + V(PathBiasDropGuards, BOOL, "0"), OBSOLETE("PathlenCoinWeight"), V(PerConnBWBurst, MEMUNIT, "0"), diff --git a/src/or/entrynodes.c b/src/or/entrynodes.c index d9a06a6573..faf5269a58 100644 --- a/src/or/entrynodes.c +++ b/src/or/entrynodes.c @@ -1048,8 +1048,9 @@ 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 (node->circuit_successes/((double)node->first_hops) - < pathbias_get_disable_rate(options)) { + if ((node->circuit_successes/((double)node->first_hops) + < pathbias_get_crit_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", diff --git a/src/or/or.h b/src/or/or.h index 59202104db..cef52018bb 100644 --- a/src/or/or.h +++ b/src/or/or.h @@ -3784,9 +3784,12 @@ typedef struct { */ int PathBiasCircThreshold; double PathBiasNoticeRate; - double PathBiasDisableRate; + double PathBiasWarnRate; + double PathBiasCritRate; + int PathBiasDropGuards; int PathBiasScaleThreshold; int PathBiasScaleFactor; + int PathBiasMultFactor; /** @} */ int IPv6Exit; /**< Do we support exiting to IPv6 addresses? */ -- cgit v1.2.3-54-g00ecf From 9bf5582e7368bf91e2b9bf9e88ee71d2ba51a335 Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Wed, 24 Oct 2012 18:15:41 -0700 Subject: Add log message checks for different rates. May want to squash this forward or back.. --- src/or/circuitbuild.c | 17 ++++++++++++++--- src/or/entrynodes.h | 6 +++++- 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c index 65c6492601..34234322e4 100644 --- a/src/or/circuitbuild.c +++ b/src/or/circuitbuild.c @@ -1408,7 +1408,8 @@ entry_guard_inc_first_hop_count(entry_guard_t *guard) * rate and disable the feature entirely. If refactoring, don't * change to <= */ if (guard->circuit_successes/((double)guard->first_hops) - < pathbias_get_crit_rate(options)) { + < pathbias_get_crit_rate(options) + && !guard->path_bias_crited) { /* This message is currently disabled by default. */ log_warn(LD_PROTOCOL, @@ -1417,21 +1418,31 @@ entry_guard_inc_first_hop_count(entry_guard_t *guard) "a bug.", guard->circuit_successes, guard->first_hops, guard->nickname, hex_str(guard->identity, DIGEST_LEN)); + guard->path_bias_crited = 1; if (pathbias_get_dropguards(options)) { guard->path_bias_disabled = 1; guard->bad_since = approx_time(); } return -1; + } else if (guard->circuit_successes/((double)guard->first_hops) + < pathbias_get_warn_rate(options) + && !guard->path_bias_warned) { + guard->path_bias_warned = 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)); } else if (guard->circuit_successes/((double)guard->first_hops) < pathbias_get_notice_rate(options) - && !guard->path_bias_notice) { - guard->path_bias_notice = 1; + && !guard->path_bias_noticed) { + guard->path_bias_noticed = 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 we get a ton of circuits, just scale everything down */ diff --git a/src/or/entrynodes.h b/src/or/entrynodes.h index b34744183c..6fdb3f7585 100644 --- a/src/or/entrynodes.h +++ b/src/or/entrynodes.h @@ -31,7 +31,11 @@ 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_crited : 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? */ -- cgit v1.2.3-54-g00ecf From ab9c83c949bd415271281f96c04ff598a9a3b7b5 Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Thu, 25 Oct 2012 13:42:37 -0700 Subject: Update Path Bias log messages to match Proposal 209. --- src/or/circuitbuild.c | 51 +++++++++++++++++++++++++++++++++++---------------- 1 file changed, 35 insertions(+), 16 deletions(-) diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c index 34234322e4..6f91fc37fc 100644 --- a/src/or/circuitbuild.c +++ b/src/or/circuitbuild.c @@ -1410,37 +1410,56 @@ entry_guard_inc_first_hop_count(entry_guard_t *guard) if (guard->circuit_successes/((double)guard->first_hops) < pathbias_get_crit_rate(options) && !guard->path_bias_crited) { - - /* 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_crited = 1; - + if (pathbias_get_dropguards(options)) { + /* This message is currently disabled by default. */ + log_warn(LD_PROTOCOL, + "Your Guard %s=%s is failing an extremely large amount of " + "circuits. Tor has disabled use of this guard. Success " + "counts are %d/%d, with %d timeouts. For reference, your " + "timeout cutoff is %ld.", + guard->nickname, hex_str(guard->identity, DIGEST_LEN), + guard->circuit_successes, guard->first_hops, guard->timeouts, + (long)circ_times.close_ms/1000); guard->path_bias_disabled = 1; guard->bad_since = approx_time(); + } else { + log_warn(LD_PROTOCOL, + "Your Guard %s=%s is failing an extremely large amount of " + "circuits. Success counts are %d/%d, with %d timeouts. " + "For reference, your timeout cutoff is %ld.", + guard->nickname, hex_str(guard->identity, DIGEST_LEN), + guard->circuit_successes, guard->first_hops, guard->timeouts, + (long)circ_times.close_ms/1000); } return -1; } else if (guard->circuit_successes/((double)guard->first_hops) < pathbias_get_warn_rate(options) && !guard->path_bias_warned) { guard->path_bias_warned = 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)); + log_warn(LD_PROTOCOL, + "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 %d/%d, with %d timeouts. For reference, your timeout " + "cutoff is %ld.", + guard->nickname, hex_str(guard->identity, DIGEST_LEN), + guard->circuit_successes, guard->first_hops, guard->timeouts, + (long)circ_times.close_ms/1000); } else if (guard->circuit_successes/((double)guard->first_hops) < pathbias_get_notice_rate(options) && !guard->path_bias_noticed) { guard->path_bias_noticed = 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)); + "Your Guard %s=%s is failing more circuits than usual. Most " + "likely this means the Tor network is overloaded. Success " + "counts are %d/%d, with %d timeouts. For reference, your " + "timeout cutoff is %ld.", + guard->nickname, hex_str(guard->identity, DIGEST_LEN), + guard->circuit_successes, guard->first_hops, guard->timeouts, + (long)circ_times.close_ms/1000); } } -- cgit v1.2.3-54-g00ecf From a54873648f226fc9b16a3c1c192b0bae6559dbbc Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Thu, 25 Oct 2012 14:05:44 -0700 Subject: Refactor pathbias functions to use pathbias_should_count. --- src/or/circuitbuild.c | 58 ++------------------------------------------------- 1 file changed, 2 insertions(+), 56 deletions(-) diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c index 6f91fc37fc..299ca29c0c 100644 --- a/src/or/circuitbuild.c +++ b/src/or/circuitbuild.c @@ -1152,34 +1152,7 @@ pathbias_count_first_hop(origin_circuit_t *circ) RATELIM_INIT(FIRST_HOP_NOTICE_INTERVAL); char *rate_msg = 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 0; - } - - /* Completely ignore one hop circuits */ - if (circ->build_state->onehop_tunnel || - circ->build_state->desired_path_len == 1) { - /* 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()))) { - 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 0; } @@ -1276,34 +1249,7 @@ 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; } -- cgit v1.2.3-54-g00ecf From 192996690cd7bc3ae6c159901a3a9a6232e74514 Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Thu, 25 Oct 2012 14:14:28 -0700 Subject: Fix spaces. --- src/or/circuitbuild.c | 6 ++---- src/or/entrynodes.h | 7 +++---- 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c index 299ca29c0c..4e9a9747fa 100644 --- a/src/or/circuitbuild.c +++ b/src/or/circuitbuild.c @@ -1137,7 +1137,6 @@ pathbias_should_count(origin_circuit_t *circ) return 1; } - /** * 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. @@ -1326,7 +1325,7 @@ pathbias_count_success(origin_circuit_t *circ) void pathbias_count_timeout(origin_circuit_t *circ) { - if(!pathbias_should_count(circ)) { + if (!pathbias_should_count(circ)) { return; } entry_guard_t *guard = @@ -1357,7 +1356,7 @@ entry_guard_inc_first_hop_count(entry_guard_t *guard) < pathbias_get_crit_rate(options) && !guard->path_bias_crited) { guard->path_bias_crited = 1; - + if (pathbias_get_dropguards(options)) { /* This message is currently disabled by default. */ log_warn(LD_PROTOCOL, @@ -1407,7 +1406,6 @@ entry_guard_inc_first_hop_count(entry_guard_t *guard) guard->circuit_successes, guard->first_hops, guard->timeouts, (long)circ_times.close_ms/1000); } - } /* If we get a ton of circuits, just scale everything down */ diff --git a/src/or/entrynodes.h b/src/or/entrynodes.h index 6fdb3f7585..dfabacb093 100644 --- a/src/or/entrynodes.h +++ b/src/or/entrynodes.h @@ -31,8 +31,8 @@ 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_noticed : 1; /**< Did we alert the user about path bias - * for this node already? */ + 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_crited : 1; /**< Did we alert the user about path bias @@ -51,8 +51,7 @@ typedef struct entry_guard_t { unsigned first_hops; /**< Number of first hops this guard has completed */ unsigned circuit_successes; /**< Number of successfully built circuits using * this guard as first hop. */ - unsigned timeouts; /**< Number of 'right-censored' timeouts - for this guard. */ + unsigned timeouts; /**< Number of 'right-censored' timeouts for this guard.*/ } entry_guard_t; entry_guard_t *entry_guard_get_by_id_digest(const char *digest); -- cgit v1.2.3-54-g00ecf From bb548134cd4fd7b4b330cea15111ff257859bba8 Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Wed, 31 Oct 2012 18:49:49 -0700 Subject: Update with code review changes from Nick. --- src/or/circuitbuild.c | 113 +++++++++++++++++++++++++++++++++----------------- src/or/circuitbuild.h | 3 +- src/or/config.c | 3 +- src/or/entrynodes.c | 2 +- src/or/entrynodes.h | 4 +- src/or/or.h | 4 +- 6 files changed, 83 insertions(+), 46 deletions(-) diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c index 4e9a9747fa..feb8e9cddc 100644 --- a/src/or/circuitbuild.c +++ b/src/or/circuitbuild.c @@ -994,6 +994,7 @@ 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) { @@ -1006,6 +1007,7 @@ 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 */ double pathbias_get_warn_rate(const or_options_t *options) { @@ -1018,18 +1020,26 @@ pathbias_get_warn_rate(const or_options_t *options) } /* 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_crit_rate(const or_options_t *options) +pathbias_get_extreme_rate(const or_options_t *options) { -#define DFLT_PATH_BIAS_CRIT_PCT 30 - if (options->PathBiasCritRate >= 0.0) - return options->PathBiasCritRate; +#define DFLT_PATH_BIAS_EXTREME_PCT 30 + if (options->PathBiasExtremeRate >= 0.0) + return options->PathBiasExtremeRate; else - return networkstatus_get_param(NULL, "pb_critpct", - DFLT_PATH_BIAS_CRIT_PCT, 0, 100)/100.0; + 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) { @@ -1041,6 +1051,12 @@ pathbias_get_dropguards(const or_options_t *options) DFLT_PATH_BIAS_DROP_GUARDS, 0, 100)/100.0; } +/** + * 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) { @@ -1053,6 +1069,12 @@ 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 + * we must be careful of the values we use here, as the + * code only scales in the event of no integer truncation. + */ static int pathbias_get_scale_factor(const or_options_t *options) { @@ -1064,6 +1086,11 @@ 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) { @@ -1076,6 +1103,9 @@ pathbias_get_mult_factor(const or_options_t *options) pathbias_get_scale_factor(options)-1); } +/** + * Convert a Guard's path state to string. + */ static const char * pathbias_state_to_string(path_state_t state) { @@ -1262,7 +1292,7 @@ pathbias_count_success(origin_circuit_t *circ) circ->path_state = PATH_STATE_SUCCEEDED; guard->circuit_successes++; - log_info(LD_PROTOCOL, "Got success count %u/%u for guard %s=%s", + log_info(LD_CIRC, "Got success count %u/%u for guard %s=%s", guard->circuit_successes, guard->first_hops, guard->nickname, hex_str(guard->identity, DIGEST_LEN)); } else { @@ -1353,26 +1383,29 @@ entry_guard_inc_first_hop_count(entry_guard_t *guard) * rate and disable the feature entirely. If refactoring, don't * change to <= */ if (guard->circuit_successes/((double)guard->first_hops) - < pathbias_get_crit_rate(options) - && !guard->path_bias_crited) { - guard->path_bias_crited = 1; - + < pathbias_get_extreme_rate(options)) { + /* Dropping is currently disabled by default. */ if (pathbias_get_dropguards(options)) { - /* This message is currently disabled by default. */ - log_warn(LD_PROTOCOL, + if (!guard->path_bias_disabled) { + log_warn(LD_CIRC, "Your Guard %s=%s is failing an extremely large amount of " - "circuits. Tor has disabled use of this guard. Success " + "circuits. To avoid potential route manipluation attacks, " + "Tor has disabled use of this guard. Success " "counts are %d/%d, with %d timeouts. For reference, your " "timeout cutoff is %ld.", guard->nickname, hex_str(guard->identity, DIGEST_LEN), guard->circuit_successes, guard->first_hops, guard->timeouts, (long)circ_times.close_ms/1000); - guard->path_bias_disabled = 1; - guard->bad_since = approx_time(); - } else { - log_warn(LD_PROTOCOL, + guard->path_bias_disabled = 1; + guard->bad_since = approx_time(); + } + } 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. Success counts are %d/%d, with %d timeouts. " + "circuits. This could indicate a route manipulation attack, " + "extreme network overload, or a bug. " + "Success counts are %d/%d, with %d timeouts. " "For reference, your timeout cutoff is %ld.", guard->nickname, hex_str(guard->identity, DIGEST_LEN), guard->circuit_successes, guard->first_hops, guard->timeouts, @@ -1380,10 +1413,10 @@ entry_guard_inc_first_hop_count(entry_guard_t *guard) } return -1; } else if (guard->circuit_successes/((double)guard->first_hops) - < pathbias_get_warn_rate(options) - && !guard->path_bias_warned) { - guard->path_bias_warned = 1; - log_warn(LD_PROTOCOL, + < 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 " @@ -1393,18 +1426,20 @@ entry_guard_inc_first_hop_count(entry_guard_t *guard) guard->nickname, hex_str(guard->identity, DIGEST_LEN), guard->circuit_successes, guard->first_hops, guard->timeouts, (long)circ_times.close_ms/1000); + } } else if (guard->circuit_successes/((double)guard->first_hops) - < pathbias_get_notice_rate(options) - && !guard->path_bias_noticed) { - guard->path_bias_noticed = 1; - log_notice(LD_PROTOCOL, - "Your Guard %s=%s is failing more circuits than usual. Most " - "likely this means the Tor network is overloaded. Success " - "counts are %d/%d, with %d timeouts. For reference, your " - "timeout cutoff is %ld.", - guard->nickname, hex_str(guard->identity, DIGEST_LEN), - guard->circuit_successes, guard->first_hops, guard->timeouts, - (long)circ_times.close_ms/1000); + < 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 %d/%d, with %d timeouts. For " + "reference, your timeout cutoff is %ld.", + guard->nickname, hex_str(guard->identity, DIGEST_LEN), + guard->circuit_successes, guard->first_hops, + guard->timeouts, (long)circ_times.close_ms/1000); + } } } @@ -1412,24 +1447,26 @@ entry_guard_inc_first_hop_count(entry_guard_t *guard) if (guard->first_hops > (unsigned)pathbias_get_scale_threshold(options)) { const int scale_factor = pathbias_get_scale_factor(options); const int mult_factor = pathbias_get_mult_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. */ + /* Only scale if there will be no rounding error for our scaling + * factors */ if (((mult_factor*guard->first_hops) % scale_factor) == 0 && ((mult_factor*guard->circuit_successes) % scale_factor) == 0) { - log_info(LD_PROTOCOL, + log_info(LD_CIRC, "Scaling pathbias counts to (%u/%u)*(%d/%d) for guard %s=%s", guard->circuit_successes, guard->first_hops, mult_factor, scale_factor, guard->nickname, hex_str(guard->identity, DIGEST_LEN)); guard->first_hops *= mult_factor; guard->circuit_successes *= mult_factor; + guard->timeouts *= mult_factor; guard->first_hops /= scale_factor; guard->circuit_successes /= scale_factor; + guard->timeouts /= scale_factor; } } guard->first_hops++; - log_info(LD_PROTOCOL, "Got success count %u/%u for guard %s=%s", + log_info(LD_CIRC, "Got success count %u/%u for guard %s=%s", guard->circuit_successes, guard->first_hops, guard->nickname, hex_str(guard->identity, DIGEST_LEN)); return 0; diff --git a/src/or/circuitbuild.h b/src/or/circuitbuild.h index 52f9f7955d..3338c9e8d2 100644 --- a/src/or/circuitbuild.h +++ b/src/or/circuitbuild.h @@ -53,8 +53,9 @@ 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_crit_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); #endif diff --git a/src/or/config.c b/src/or/config.c index fffe14c644..fb95642037 100644 --- a/src/or/config.c +++ b/src/or/config.c @@ -314,10 +314,11 @@ 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(PathBiasWarnRate, DOUBLE, "-1"), - V(PathBiasCritRate, DOUBLE, "-1"), + V(PathBiasExtremeRate, DOUBLE, "-1"), V(PathBiasScaleThreshold, INT, "-1"), V(PathBiasScaleFactor, INT, "-1"), V(PathBiasMultFactor, INT, "-1"), diff --git a/src/or/entrynodes.c b/src/or/entrynodes.c index faf5269a58..317a088470 100644 --- a/src/or/entrynodes.c +++ b/src/or/entrynodes.c @@ -1049,7 +1049,7 @@ entry_guards_parse_state(or_state_t *state, int set, char **msg) * rate and disable the feature entirely. If refactoring, don't * change to <= */ if ((node->circuit_successes/((double)node->first_hops) - < pathbias_get_crit_rate(options)) && + < pathbias_get_extreme_rate(options)) && pathbias_get_dropguards(options)) { node->path_bias_disabled = 1; log_info(LD_GENERAL, diff --git a/src/or/entrynodes.h b/src/or/entrynodes.h index dfabacb093..f087f900bd 100644 --- a/src/or/entrynodes.h +++ b/src/or/entrynodes.h @@ -35,8 +35,8 @@ typedef struct entry_guard_t { * 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_crited : 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? */ time_t bad_since; /**< 0 if this guard is currently usable, or the time at diff --git a/src/or/or.h b/src/or/or.h index cef52018bb..a5e96b22ee 100644 --- a/src/or/or.h +++ b/src/or/or.h @@ -3785,7 +3785,7 @@ typedef struct { int PathBiasCircThreshold; double PathBiasNoticeRate; double PathBiasWarnRate; - double PathBiasCritRate; + double PathBiasExtremeRate; int PathBiasDropGuards; int PathBiasScaleThreshold; int PathBiasScaleFactor; @@ -4070,8 +4070,6 @@ typedef struct { double close_ms; } circuit_build_times_t; -void pathbias_count_timeout(origin_circuit_t *circ); - /********************************* config.c ***************************/ /** An error from options_trial_assign() or options_init_from_string(). */ -- cgit v1.2.3-54-g00ecf From df4aeaa0d63ecf8f282f54c8178cf48fee10bad0 Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Wed, 31 Oct 2012 18:50:45 -0700 Subject: Update manpage for new PathBias torrc options. --- doc/tor.1.txt | 29 +++++++++++++++++++---------- 1 file changed, 19 insertions(+), 10 deletions(-) diff --git a/doc/tor.1.txt b/doc/tor.1.txt index d2687177bb..734bc64bf7 100644 --- a/doc/tor.1.txt +++ b/doc/tor.1.txt @@ -1173,28 +1173,37 @@ The following options are useful only for clients (that is, if **PathBiasNoticeRate** __NUM__ + -**PathBiasDisableRate** __NUM__ + +**PathBiasWarnRate** __NUM__ + + +**PathBiasExtremeRate** __NUM__ + + +**PathBiasDropGuards** __NUM__ + **PathBiasScaleThreshold** __NUM__ + +**PathBiasMultFactor** __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 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. + + 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 -- cgit v1.2.3-54-g00ecf From f215d1910517ad3d6bb0b7ba5280f9fa1a59c297 Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Wed, 31 Oct 2012 18:51:07 -0700 Subject: Add a changes file for bug7157. --- changes/bug7157 | 15 +++++++++++++++ 1 file changed, 15 insertions(+) create mode 100644 changes/bug7157 diff --git a/changes/bug7157 b/changes/bug7157 new file mode 100644 index 0000000000..fad3977bfa --- /dev/null +++ b/changes/bug7157 @@ -0,0 +1,15 @@ + + 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. -- cgit v1.2.3-54-g00ecf From ef1b830ef8d751172ebe577a3e8a754c89225394 Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Fri, 2 Nov 2012 12:36:08 -0700 Subject: Fix an assert crash and an incorrectly placed return. --- src/or/circuitbuild.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c index feb8e9cddc..eda36d3c44 100644 --- a/src/or/circuitbuild.c +++ b/src/or/circuitbuild.c @@ -1100,7 +1100,7 @@ pathbias_get_mult_factor(const or_options_t *options) else return networkstatus_get_param(NULL, "pb_multfactor", DFLT_PATH_BIAS_MULT_FACTOR, 1, - pathbias_get_scale_factor(options)-1); + pathbias_get_scale_factor(options)); } /** @@ -1398,6 +1398,7 @@ entry_guard_inc_first_hop_count(entry_guard_t *guard) (long)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; @@ -1411,7 +1412,6 @@ entry_guard_inc_first_hop_count(entry_guard_t *guard) guard->circuit_successes, guard->first_hops, guard->timeouts, (long)circ_times.close_ms/1000); } - return -1; } else if (guard->circuit_successes/((double)guard->first_hops) < pathbias_get_warn_rate(options)) { if (!guard->path_bias_warned) { -- cgit v1.2.3-54-g00ecf From da5c398d79c890966339558749662fa8ffabf480 Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Fri, 2 Nov 2012 12:37:26 -0700 Subject: Be explicit about units for timeout. --- src/or/circuitbuild.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c index eda36d3c44..215ca14398 100644 --- a/src/or/circuitbuild.c +++ b/src/or/circuitbuild.c @@ -1392,7 +1392,7 @@ entry_guard_inc_first_hop_count(entry_guard_t *guard) "circuits. To avoid potential route manipluation attacks, " "Tor has disabled use of this guard. Success " "counts are %d/%d, with %d timeouts. For reference, your " - "timeout cutoff is %ld.", + "timeout cutoff is %ld seconds.", guard->nickname, hex_str(guard->identity, DIGEST_LEN), guard->circuit_successes, guard->first_hops, guard->timeouts, (long)circ_times.close_ms/1000); @@ -1407,7 +1407,7 @@ entry_guard_inc_first_hop_count(entry_guard_t *guard) "circuits. This could indicate a route manipulation attack, " "extreme network overload, or a bug. " "Success counts are %d/%d, with %d timeouts. " - "For reference, your timeout cutoff is %ld.", + "For reference, your timeout cutoff is %ld seconds.", guard->nickname, hex_str(guard->identity, DIGEST_LEN), guard->circuit_successes, guard->first_hops, guard->timeouts, (long)circ_times.close_ms/1000); @@ -1422,7 +1422,7 @@ entry_guard_inc_first_hop_count(entry_guard_t *guard) "overloaded, but it could also mean an attack against " "you or the potentially the guard itself. Success counts " "are %d/%d, with %d timeouts. For reference, your timeout " - "cutoff is %ld.", + "cutoff is %ld seconds.", guard->nickname, hex_str(guard->identity, DIGEST_LEN), guard->circuit_successes, guard->first_hops, guard->timeouts, (long)circ_times.close_ms/1000); @@ -1435,7 +1435,7 @@ entry_guard_inc_first_hop_count(entry_guard_t *guard) "Your Guard %s=%s is failing more circuits than usual. " "Most likely this means the Tor network is overloaded. " "Success counts are %d/%d, with %d timeouts. For " - "reference, your timeout cutoff is %ld.", + "reference, your timeout cutoff is %ld seconds.", guard->nickname, hex_str(guard->identity, DIGEST_LEN), guard->circuit_successes, guard->first_hops, guard->timeouts, (long)circ_times.close_ms/1000); -- cgit v1.2.3-54-g00ecf From 412ae099cb656ab47fc8cbb408aa5f4cee956961 Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Sat, 17 Nov 2012 16:30:50 -0800 Subject: Prop 209: Add path bias counts for timeouts and other mechanisms. Turns out there's more than one way to block a tagged circuit. This seems to successfully handle all of the normal exit circuits. Hidden services need additional tweaks, still. --- src/or/circuitbuild.c | 194 ++++++++++++++++++++++++++++++++++++++++++----- src/or/circuitbuild.h | 6 ++ src/or/circuitlist.c | 37 +++++++++ src/or/config.c | 1 + src/or/connection_edge.c | 11 +++ src/or/entrynodes.c | 36 +++++++-- src/or/entrynodes.h | 10 ++- src/or/or.h | 8 ++ 8 files changed, 277 insertions(+), 26 deletions(-) diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c index 215ca14398..22f728972e 100644 --- a/src/or/circuitbuild.c +++ b/src/or/circuitbuild.c @@ -1103,6 +1103,21 @@ pathbias_get_mult_factor(const or_options_t *options) 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. */ @@ -1347,6 +1362,94 @@ pathbias_count_success(origin_circuit_t *circ) } } +/** + * Count a successfully closed circuit. + */ +void +pathbias_count_successful_close(origin_circuit_t *circ) +{ + entry_guard_t *guard = NULL; + if (!pathbias_should_count(circ)) { + return; + } + + guard = entry_guard_get_by_id_digest(circ->base_.n_chan->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_BUG, + "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. + */ +void +pathbias_count_collapse(origin_circuit_t *circ) +{ + entry_guard_t *guard = NULL; + if (!pathbias_should_count(circ)) { + return; + } + + guard = entry_guard_get_by_id_digest(circ->base_.n_chan->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_BUG, + "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)); + } +} + +void +pathbias_count_unusable(origin_circuit_t *circ) +{ + entry_guard_t *guard = NULL; + if (!pathbias_should_count(circ)) { + return; + } + + guard = entry_guard_get_by_id_digest(circ->base_.n_chan->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_BUG, + "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. * @@ -1367,6 +1470,44 @@ pathbias_count_timeout(origin_circuit_t *circ) } } +// XXX: DOCDOC +int +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) { + if (!CIRCUIT_IS_ORIGIN(circ) || /* didn't originate here */ + circ->marked_for_close) /* already counted */ + continue; + + if (TO_ORIGIN_CIRCUIT(circ)->path_state == PATH_STATE_SUCCEEDED && + (memcmp(guard->identity, circ->n_chan->identity_digest, DIGEST_LEN) + == 0)) { + open_circuits++; + } + } + + return guard->successful_circuits_closed + open_circuits; +} + +/** + * This function checks the consensus parameters to decide + * if it should return guard->circuit_successes or + * guard->successful_circuits_closed. + */ +static int +pathbias_get_success_count(entry_guard_t *guard) +{ + if (pathbias_use_close_counts(get_options())) { + return pathbias_get_closed_count(guard); + } else { + return guard->circuit_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 @@ -1382,7 +1523,7 @@ entry_guard_inc_first_hop_count(entry_guard_t *guard) /* 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) + if (pathbias_get_success_count(guard)/((double)guard->first_hops) < pathbias_get_extreme_rate(options)) { /* Dropping is currently disabled by default. */ if (pathbias_get_dropguards(options)) { @@ -1390,11 +1531,14 @@ entry_guard_inc_first_hop_count(entry_guard_t *guard) 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 %d/%d, with %d timeouts. For reference, your " - "timeout cutoff is %ld seconds.", + "Tor has disabled use of this guard. " + "Success counts are %d/%d. %d circuits completed, %d " + "were unusable, %d collapsed, and %d timed out. For " + "reference, your timeout cutoff is %ld seconds.", guard->nickname, hex_str(guard->identity, DIGEST_LEN), - guard->circuit_successes, guard->first_hops, guard->timeouts, + pathbias_get_closed_count(guard), guard->first_hops, + guard->circuit_successes, guard->unusable_circuits, + guard->collapsed_circuits, guard->timeouts, (long)circ_times.close_ms/1000); guard->path_bias_disabled = 1; guard->bad_since = approx_time(); @@ -1406,13 +1550,16 @@ entry_guard_inc_first_hop_count(entry_guard_t *guard) "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 %d/%d, with %d timeouts. " - "For reference, your timeout cutoff is %ld seconds.", + "Success counts are %d/%d. %d circuits completed, %d " + "were unusable, %d collapsed, and %d timed out. For " + "reference, your timeout cutoff is %ld seconds.", guard->nickname, hex_str(guard->identity, DIGEST_LEN), - guard->circuit_successes, guard->first_hops, guard->timeouts, + pathbias_get_closed_count(guard), guard->first_hops, + guard->circuit_successes, guard->unusable_circuits, + guard->collapsed_circuits, guard->timeouts, (long)circ_times.close_ms/1000); } - } else if (guard->circuit_successes/((double)guard->first_hops) + } else if (pathbias_get_success_count(guard)/((double)guard->first_hops) < pathbias_get_warn_rate(options)) { if (!guard->path_bias_warned) { guard->path_bias_warned = 1; @@ -1420,25 +1567,31 @@ entry_guard_inc_first_hop_count(entry_guard_t *guard) "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 %d/%d, with %d timeouts. For reference, your timeout " - "cutoff is %ld seconds.", + "you or the potentially the guard itself. " + "Success counts are %d/%d. %d circuits completed, %d " + "were unusable, %d collapsed, and %d timed out. For " + "reference, your timeout cutoff is %ld seconds.", guard->nickname, hex_str(guard->identity, DIGEST_LEN), - guard->circuit_successes, guard->first_hops, guard->timeouts, + pathbias_get_closed_count(guard), guard->first_hops, + guard->circuit_successes, guard->unusable_circuits, + guard->collapsed_circuits, guard->timeouts, (long)circ_times.close_ms/1000); } - } else if (guard->circuit_successes/((double)guard->first_hops) + } else if (pathbias_get_success_count(guard)/((double)guard->first_hops) < 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 %d/%d, with %d timeouts. For " + "Success counts are %d/%d. %d circuits completed, %d " + "were unusable, %d collapsed, and %d timed out. For " "reference, your timeout cutoff is %ld seconds.", guard->nickname, hex_str(guard->identity, DIGEST_LEN), - guard->circuit_successes, guard->first_hops, - guard->timeouts, (long)circ_times.close_ms/1000); + pathbias_get_closed_count(guard), guard->first_hops, + guard->circuit_successes, guard->unusable_circuits, + guard->collapsed_circuits, guard->timeouts, + (long)circ_times.close_ms/1000); } } } @@ -1456,13 +1609,20 @@ entry_guard_inc_first_hop_count(entry_guard_t *guard) guard->circuit_successes, guard->first_hops, mult_factor, scale_factor, guard->nickname, hex_str(guard->identity, DIGEST_LEN)); + guard->first_hops *= mult_factor; guard->circuit_successes *= mult_factor; guard->timeouts *= mult_factor; + guard->successful_circuits_closed *= mult_factor; + guard->collapsed_circuits *= mult_factor; + guard->unusable_circuits *= mult_factor; guard->first_hops /= scale_factor; guard->circuit_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++; diff --git a/src/or/circuitbuild.h b/src/or/circuitbuild.h index 3338c9e8d2..325a583edb 100644 --- a/src/or/circuitbuild.h +++ b/src/or/circuitbuild.h @@ -56,6 +56,12 @@ const node_t *choose_good_entry_server(uint8_t purpose, 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_count_successful_close(origin_circuit_t *circ); +void pathbias_count_collapse(origin_circuit_t *circ); +void pathbias_count_unusable(origin_circuit_t *circ); + +typedef struct entry_guard_t entry_guard_t; +int pathbias_get_closed_count(entry_guard_t *gaurd); #endif diff --git a/src/or/circuitlist.c b/src/or/circuitlist.c index 8f06c0679e..66cdbe10c7 100644 --- a/src/or/circuitlist.c +++ b/src/or/circuitlist.c @@ -1347,7 +1347,44 @@ circuit_mark_for_close_(circuit_t *circ, int reason, int line, } reason = END_CIRC_REASON_NONE; } + if (CIRCUIT_IS_ORIGIN(circ)) { + origin_circuit_t *ocirc = TO_ORIGIN_CIRCUIT(circ); + + if (ocirc->path_state == PATH_STATE_SUCCEEDED) { + int pathbias_is_normal_close = 1; + + /* FIXME: Is timestamp_dirty the right thing for these two checks? + * Should we use isolation_any_streams_attached instead? */ + if (!circ->timestamp_dirty) { + if (reason & END_CIRC_REASON_FLAG_REMOTE) { + /* Unused remote circ close reasons all could be bias */ + pathbias_is_normal_close = 0; + pathbias_count_collapse(ocirc); + } else if ((reason & ~END_CIRC_REASON_FLAG_REMOTE) + == END_CIRC_REASON_CHANNEL_CLOSED && + circ->n_chan->reason_for_closing + != CHANNEL_CLOSE_REQUESTED) { + /* If we didn't close the channel ourselves, it could be bias */ + /* FIXME: Only count bias if the network is live? + * What about clock jumps/suspends? */ + pathbias_is_normal_close = 0; + pathbias_count_collapse(ocirc); + } + } else if (circ->timestamp_dirty && !ocirc->any_streams_succeeded) { + /* Any circuit where there were attempted streams but no successful + * streams could be bias */ + /* FIXME: This may be better handled by limiting the number of retries + * per stream? */ + pathbias_is_normal_close = 0; + pathbias_count_unusable(ocirc); + } + + if (pathbias_is_normal_close) { + pathbias_count_successful_close(ocirc); + } + } + /* We don't send reasons when closing circuits at the origin. */ reason = END_CIRC_REASON_NONE; } diff --git a/src/or/config.c b/src/or/config.c index fb95642037..79725cbe03 100644 --- a/src/or/config.c +++ b/src/or/config.c @@ -323,6 +323,7 @@ static config_var_t option_vars_[] = { V(PathBiasScaleFactor, INT, "-1"), V(PathBiasMultFactor, INT, "-1"), V(PathBiasDropGuards, BOOL, "0"), + V(PathBiasUseCloseCounts, BOOL, "1"), OBSOLETE("PathlenCoinWeight"), V(PerConnBWBurst, MEMUNIT, "0"), diff --git a/src/or/connection_edge.c b/src/or/connection_edge.c index 95088c7c17..cb2afe1e86 100644 --- a/src/or/connection_edge.c +++ b/src/or/connection_edge.c @@ -2176,6 +2176,17 @@ 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) { + if(!conn->edge_.on_circuit || + !CIRCUIT_IS_ORIGIN(conn->edge_.on_circuit)) { + log_warn(LD_BUG, "No origin circuit for successful SOCKS stream"); + } else { + TO_ORIGIN_CIRCUIT(conn->edge_.on_circuit)->any_streams_succeeded = 1; + } + } + if (conn->socks_request->has_finished) { log_warn(LD_BUG, "(Harmless.) duplicate calls to " "connection_ap_handshake_socks_reply."); diff --git a/src/or/entrynodes.c b/src/or/entrynodes.c index 317a088470..1e64aaf985 100644 --- a/src/or/entrynodes.c +++ b/src/or/entrynodes.c @@ -1021,7 +1021,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, timeouts; + unsigned hop_cnt, success_cnt, timeouts, collapsed, successful_closed, + unusable; if (!node) { *msg = tor_strdup("Unable to parse entry nodes: " @@ -1030,19 +1031,33 @@ entry_guards_parse_state(or_state_t *state, int set, char **msg) } /* First try 3 params, then 2. */ - if (tor_sscanf(line->value, "%u %u %u", &success_cnt, &hop_cnt, - &timeouts) != 3) { - timeouts = 0; + // XXX: We want to convert this to floating point before merge + /* In the long run: circuit_success ~= successful_circuit_close + + * collapsed_circuits + + * unusable_circuits */ + if (tor_sscanf(line->value, "%u %u %u %u %u %u", + &hop_cnt, &success_cnt, &successful_closed, + &collapsed, &unusable, &timeouts) != 6) { 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; } + log_info(LD_GENERAL, "Reading old-style EntryGuardPathBias %s", + escaped(line->value)); + + successful_closed = success_cnt; + timeouts = 0; + collapsed = 0; + unusable = 0; } node->first_hops = hop_cnt; node->circuit_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 %u/%u path bias for node %s", node->circuit_successes, node->first_hops, node->nickname); /* Note: We rely on the < comparison here to allow us to set a 0 @@ -1180,8 +1195,13 @@ entry_guards_update_state(or_state_t *state) if (e->first_hops) { *next = line = tor_malloc_zero(sizeof(config_line_t)); line->key = tor_strdup("EntryGuardPathBias"); - tor_asprintf(&line->value, "%u %u %u", - e->circuit_successes, e->first_hops, e->timeouts); + /* In the long run: circuit_success ~= successful_circuit_close + + * collapsed_circuits + + * unusable_circuits */ + tor_asprintf(&line->value, "%u %u %u %u %u %u", + e->first_hops, e->circuit_successes, + pathbias_get_closed_count(e), e->collapsed_circuits, + e->unusable_circuits, e->timeouts); next = &(line->next); } diff --git a/src/or/entrynodes.h b/src/or/entrynodes.h index f087f900bd..b9c8052533 100644 --- a/src/or/entrynodes.h +++ b/src/or/entrynodes.h @@ -51,7 +51,15 @@ typedef struct entry_guard_t { unsigned first_hops; /**< Number of first hops this guard has completed */ unsigned circuit_successes; /**< Number of successfully built circuits using * this guard as first hop. */ - unsigned timeouts; /**< Number of 'right-censored' timeouts for this guard.*/ + unsigned successful_circuits_closed; /**< Number of circuits that carried + * streams successfully. */ + unsigned collapsed_circuits; /**< Number of fully built circuits that were + * remotely closed before any streams were + * attempted. */ + unsigned unusable_circuits; /**< Number of circuits for which streams were + * attempted, but none succeeded. */ + unsigned 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); diff --git a/src/or/or.h b/src/or/or.h index a5e96b22ee..f26fc394fa 100644 --- a/src/or/or.h +++ b/src/or/or.h @@ -2872,6 +2872,13 @@ typedef struct origin_circuit_t { */ unsigned int isolation_any_streams_attached : 1; + /** + * Did any SOCKS streams actually succeed on this circuit? + * + * XXX: We probably also need to set this for intro other hidserv circs.. + */ + unsigned int any_streams_succeeded : 1; + /** A bitfield of ISO_* flags for every isolation field such that this * circuit has had streams with more than one value for that field * attached to it. */ @@ -3790,6 +3797,7 @@ typedef struct { int PathBiasScaleThreshold; int PathBiasScaleFactor; int PathBiasMultFactor; + int PathBiasUseCloseCounts; /** @} */ int IPv6Exit; /**< Do we support exiting to IPv6 addresses? */ -- cgit v1.2.3-54-g00ecf From aa0e6e2c0337c5ddfc9f64ed593d8a59b3c4cb44 Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Sat, 17 Nov 2012 17:51:27 -0800 Subject: Prop 209: Add in hidserv path bias counts for usage. --- src/or/connection_edge.c | 4 ++++ src/or/or.h | 5 ++--- src/or/rendclient.c | 3 +++ src/or/rendservice.c | 3 +++ 4 files changed, 12 insertions(+), 3 deletions(-) diff --git a/src/or/connection_edge.c b/src/or/connection_edge.c index cb2afe1e86..a654b61711 100644 --- a/src/or/connection_edge.c +++ b/src/or/connection_edge.c @@ -2454,6 +2454,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->any_streams_succeeded = 1; + tor_free(address); return 0; } diff --git a/src/or/or.h b/src/or/or.h index f26fc394fa..8501235b7b 100644 --- a/src/or/or.h +++ b/src/or/or.h @@ -2873,9 +2873,8 @@ typedef struct origin_circuit_t { unsigned int isolation_any_streams_attached : 1; /** - * Did any SOCKS streams actually succeed on this circuit? - * - * XXX: We probably also need to set this for intro other hidserv circs.. + * Did any SOCKS streams or hidserv introductions actually succeed on + * this circuit? */ unsigned int any_streams_succeeded : 1; diff --git a/src/or/rendclient.c b/src/or/rendclient.c index 3fb4025e69..ec43041b1a 100644 --- a/src/or/rendclient.c +++ b/src/or/rendclient.c @@ -378,6 +378,9 @@ rend_client_introduction_acked(origin_circuit_t *circ, * it to specify when a circuit entered the * _C_REND_READY_INTRO_ACKED state. */ rendcirc->base_.timestamp_dirty = time(NULL); + + /* For path bias: This circuit was used successfully */ + circ->any_streams_succeeded = 1; } else { log_info(LD_REND,"...Found no rend circ. Dropping on the floor."); } diff --git a/src/or/rendservice.c b/src/or/rendservice.c index 09792bd1d7..775edd6046 100644 --- a/src/or/rendservice.c +++ b/src/or/rendservice.c @@ -1383,6 +1383,9 @@ rend_service_introduce(origin_circuit_t *circuit, const uint8_t *request, if (circuit_init_cpath_crypto(cpath,keys+DIGEST_LEN,1)<0) goto err; memcpy(cpath->handshake_digest, keys, DIGEST_LEN); + + /* For path bias: This circuit was used successfully */ + circuit->any_streams_succeeded = 1; goto done; -- cgit v1.2.3-54-g00ecf From 428fbfc1d5616b40b90a028813151a5d62f3493c Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Mon, 19 Nov 2012 10:45:47 -0800 Subject: Prop209: Rend circuits weren't ever marked dirty. --- src/or/rendservice.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/or/rendservice.c b/src/or/rendservice.c index 775edd6046..74e4bada92 100644 --- a/src/or/rendservice.c +++ b/src/or/rendservice.c @@ -2584,6 +2584,10 @@ 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 */ + 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); -- cgit v1.2.3-54-g00ecf From 7f8cbe389d095f673bfc03437e1f7521abae698b Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Mon, 19 Nov 2012 11:30:07 -0800 Subject: Fix a crash due to NULL circ->n_chan. Is this redundant? Can we always rely on circ->cpath->extend_info being present for origin circuits? --- src/or/circuitbuild.c | 55 ++++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 46 insertions(+), 9 deletions(-) diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c index 22f728972e..160ad3f1fe 100644 --- a/src/or/circuitbuild.c +++ b/src/or/circuitbuild.c @@ -1218,10 +1218,16 @@ pathbias_count_first_hop(origin_circuit_t *circ) /* Don't count cannibalized circs for path bias */ 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; @@ -1299,8 +1305,13 @@ pathbias_count_success(origin_circuit_t *circ) /* Don't count cannibalized/reused circs for path bias */ 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); + } else if (circ->base_.n_chan) { + guard = + entry_guard_get_by_id_digest(circ->base_.n_chan->identity_digest); + } if (guard) { if (circ->path_state == PATH_STATE_DID_FIRST_HOP) { @@ -1373,7 +1384,13 @@ pathbias_count_successful_close(origin_circuit_t *circ) return; } - 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); + } else if (circ->base_.n_chan) { + guard = + entry_guard_get_by_id_digest(circ->base_.n_chan->identity_digest); + } if (guard) { /* In the long run: circuit_success ~= successful_circuit_close + @@ -1408,7 +1425,13 @@ pathbias_count_collapse(origin_circuit_t *circ) return; } - 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); + } else if (circ->base_.n_chan) { + guard = + entry_guard_get_by_id_digest(circ->base_.n_chan->identity_digest); + } if (guard) { guard->collapsed_circuits++; @@ -1433,7 +1456,13 @@ pathbias_count_unusable(origin_circuit_t *circ) return; } - 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); + } else if (circ->base_.n_chan) { + guard = + entry_guard_get_by_id_digest(circ->base_.n_chan->identity_digest); + } if (guard) { guard->unusable_circuits++; @@ -1458,11 +1487,19 @@ pathbias_count_unusable(origin_circuit_t *circ) void pathbias_count_timeout(origin_circuit_t *circ) { + entry_guard_t *guard = NULL; + if (!pathbias_should_count(circ)) { return; } - entry_guard_t *guard = + + 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); + } if (guard) { guard->timeouts++; -- cgit v1.2.3-54-g00ecf From a630726884c434323d6af558b80f992fc018d255 Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Mon, 19 Nov 2012 11:31:35 -0800 Subject: Move a pathbias function that depends on entryguard_t. --- src/or/circuitbuild.h | 4 ---- src/or/entrynodes.h | 2 ++ 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/src/or/circuitbuild.h b/src/or/circuitbuild.h index 325a583edb..2d9eac1215 100644 --- a/src/or/circuitbuild.h +++ b/src/or/circuitbuild.h @@ -60,8 +60,4 @@ void pathbias_count_successful_close(origin_circuit_t *circ); void pathbias_count_collapse(origin_circuit_t *circ); void pathbias_count_unusable(origin_circuit_t *circ); -typedef struct entry_guard_t entry_guard_t; -int pathbias_get_closed_count(entry_guard_t *gaurd); - #endif - diff --git a/src/or/entrynodes.h b/src/or/entrynodes.h index b9c8052533..c3f7b14134 100644 --- a/src/or/entrynodes.h +++ b/src/or/entrynodes.h @@ -111,5 +111,7 @@ int find_transport_by_bridge_addrport(const tor_addr_t *addr, uint16_t port, int validate_pluggable_transports_config(void); +int pathbias_get_closed_count(entry_guard_t *gaurd); + #endif -- cgit v1.2.3-54-g00ecf From c3028edba6f8474175a59ad22dd0d3ca23c60f85 Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Tue, 20 Nov 2012 01:52:33 -0800 Subject: Remove n_chan codepaths for determinining guard. Cpath is apparently good enough. --- src/or/circuitbuild.c | 28 ++++++++-------------------- 1 file changed, 8 insertions(+), 20 deletions(-) diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c index 160ad3f1fe..aaa1959704 100644 --- a/src/or/circuitbuild.c +++ b/src/or/circuitbuild.c @@ -1308,9 +1308,6 @@ pathbias_count_success(origin_circuit_t *circ) 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); } if (guard) { @@ -1387,11 +1384,8 @@ pathbias_count_successful_close(origin_circuit_t *circ) 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); } - + if (guard) { /* In the long run: circuit_success ~= successful_circuit_close + * circ_failure + stream_failure */ @@ -1428,11 +1422,8 @@ pathbias_count_collapse(origin_circuit_t *circ) 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); } - + if (guard) { guard->collapsed_circuits++; entry_guards_changed(); @@ -1459,11 +1450,8 @@ pathbias_count_unusable(origin_circuit_t *circ) 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); } - + if (guard) { guard->unusable_circuits++; entry_guards_changed(); @@ -1496,9 +1484,6 @@ pathbias_count_timeout(origin_circuit_t *circ) 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); } if (guard) { @@ -1517,11 +1502,14 @@ pathbias_get_closed_count(entry_guard_t *guard) /* Count currently open circuits. Give them the benefit of the doubt */ for ( ; circ; circ = circ->next) { if (!CIRCUIT_IS_ORIGIN(circ) || /* didn't originate here */ - circ->marked_for_close) /* already counted */ + circ->marked_for_close || /* already counted */ + !circ->cpath || !circ->cpath->extend_info) continue; if (TO_ORIGIN_CIRCUIT(circ)->path_state == PATH_STATE_SUCCEEDED && - (memcmp(guard->identity, circ->n_chan->identity_digest, DIGEST_LEN) + (memcmp(guard->identity, + TO_ORIGIN_CIRCUIT(circ)->cpath->extend_info->identity_digest, + DIGEST_LEN) == 0)) { open_circuits++; } -- cgit v1.2.3-54-g00ecf From 9b4046607232c636c58f04672c1b762968a8b023 Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Wed, 21 Nov 2012 16:31:58 -0800 Subject: Document that care needs to be taken with any_streams_attached. --- src/or/or.h | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/or/or.h b/src/or/or.h index 8501235b7b..d165705e97 100644 --- a/src/or/or.h +++ b/src/or/or.h @@ -2875,6 +2875,10 @@ typedef struct origin_circuit_t { /** * 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). */ unsigned int any_streams_succeeded : 1; -- cgit v1.2.3-54-g00ecf From 721f7e375114abfcb1a41ade58ac59ec79b8a3af Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Wed, 21 Nov 2012 16:32:38 -0800 Subject: Fix a crash bug and pass down a remote reason code. Unexpected channel closures count as remote circ failures. --- src/or/circuitlist.c | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/src/or/circuitlist.c b/src/or/circuitlist.c index 66cdbe10c7..eb7fc75286 100644 --- a/src/or/circuitlist.c +++ b/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); @@ -1355,14 +1360,20 @@ circuit_mark_for_close_(circuit_t *circ, int reason, int line, int pathbias_is_normal_close = 1; /* FIXME: Is timestamp_dirty the right thing for these two checks? - * Should we use isolation_any_streams_attached instead? */ + * Should we use isolation_any_streams_attached instead? + * isolation_any_streams_attached is not used for hidservs.. */ if (!circ->timestamp_dirty) { - if (reason & END_CIRC_REASON_FLAG_REMOTE) { + if (orig_reason & END_CIRC_REASON_FLAG_REMOTE) { /* Unused remote circ close reasons all could be bias */ + // XXX: We hit this a lot for hidserv circs with purposes: + // CIRCUIT_PURPOSE_S_CONNECT_REND (reasons: 514,517,520) + // CIRCUIT_PURPOSE_S_REND_JOINED (reasons: 514,517,520) + // == reasons: 2,3,8. Client-side timeouts? pathbias_is_normal_close = 0; 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 */ -- cgit v1.2.3-54-g00ecf From 7a28862d56c15e4b83efc514621a330085781323 Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Wed, 21 Nov 2012 16:33:16 -0800 Subject: Fix another crash bug. --- src/or/circuitbuild.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c index aaa1959704..8304ad8b89 100644 --- a/src/or/circuitbuild.c +++ b/src/or/circuitbuild.c @@ -1501,14 +1501,19 @@ pathbias_get_closed_count(entry_guard_t *guard) /* 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 */ - !circ->cpath || !circ->cpath->extend_info) + circ->marked_for_close) /* already counted */ continue; - if (TO_ORIGIN_CIRCUIT(circ)->path_state == PATH_STATE_SUCCEEDED && + ocirc = TO_ORIGIN_CIRCUIT(circ); + + if(!ocirc->cpath || !ocirc->cpath->extend_info) + continue; + + if (ocirc->path_state == PATH_STATE_SUCCEEDED && (memcmp(guard->identity, - TO_ORIGIN_CIRCUIT(circ)->cpath->extend_info->identity_digest, + ocirc->cpath->extend_info->identity_digest, DIGEST_LEN) == 0)) { open_circuits++; -- cgit v1.2.3-54-g00ecf From ecaeb505fab12985d314aace49f1277b1b58dc1b Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Wed, 21 Nov 2012 16:40:25 -0800 Subject: Note a strange case for SOCKS streams. --- src/or/connection_edge.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/or/connection_edge.c b/src/or/connection_edge.c index a654b61711..31ff90093c 100644 --- a/src/or/connection_edge.c +++ b/src/or/connection_edge.c @@ -2181,6 +2181,7 @@ connection_ap_handshake_socks_reply(entry_connection_t *conn, char *reply, if (status == SOCKS5_SUCCEEDED) { if(!conn->edge_.on_circuit || !CIRCUIT_IS_ORIGIN(conn->edge_.on_circuit)) { + // XXX: Weird. We hit this a lot, and yet have no unusable_circs log_warn(LD_BUG, "No origin circuit for successful SOCKS stream"); } else { TO_ORIGIN_CIRCUIT(conn->edge_.on_circuit)->any_streams_succeeded = 1; -- cgit v1.2.3-54-g00ecf From dc86d7c35bd48d12d84feb6f63014904eabe0902 Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Sun, 25 Nov 2012 17:29:16 -0800 Subject: Note more potential issues. --- src/or/circuitlist.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/or/circuitlist.c b/src/or/circuitlist.c index eb7fc75286..7163c351c6 100644 --- a/src/or/circuitlist.c +++ b/src/or/circuitlist.c @@ -1383,6 +1383,12 @@ circuit_mark_for_close_(circuit_t *circ, int reason, int line, pathbias_count_collapse(ocirc); } } else if (circ->timestamp_dirty && !ocirc->any_streams_succeeded) { + // XXX: May open up attacks if the adversary can force connections + // on unresponsive hosts to use new circs. Vidalia displayes a "Retrying" + // state.. Can we use that? Does optimistic data change this? + // XXX: For the hidserv side, we could only care about INTRODUCING purposes + // for server+client, and REND purposes for the server... Can we + // somehow only count those? /* Any circuit where there were attempted streams but no successful * streams could be bias */ /* FIXME: This may be better handled by limiting the number of retries -- cgit v1.2.3-54-g00ecf From c3b71a3fc96c6f3eaaebd96ef8c15d4298d9639e Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Fri, 7 Dec 2012 15:50:31 -0800 Subject: Actually, both nacks and acks indicate a valid path --- src/or/rendclient.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/or/rendclient.c b/src/or/rendclient.c index ec43041b1a..1d473dec07 100644 --- a/src/or/rendclient.c +++ b/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->any_streams_succeeded = 1; + 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, @@ -378,9 +382,6 @@ rend_client_introduction_acked(origin_circuit_t *circ, * it to specify when a circuit entered the * _C_REND_READY_INTRO_ACKED state. */ rendcirc->base_.timestamp_dirty = time(NULL); - - /* For path bias: This circuit was used successfully */ - circ->any_streams_succeeded = 1; } else { log_info(LD_REND,"...Found no rend circ. Dropping on the floor."); } -- cgit v1.2.3-54-g00ecf From 26fa47226cab49b260ba764aa050880f71927ea0 Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Fri, 7 Dec 2012 17:47:23 -0800 Subject: Refactor path use bias code into own function. Also, improve and log some failure cases. --- src/or/circuitbuild.c | 98 +++++++++++++++++++++++++++++++++++++++++------- src/or/circuitbuild.h | 4 +- src/or/circuitlist.c | 48 +----------------------- src/or/connection_edge.c | 12 ++++-- src/or/or.h | 24 ++++++------ src/or/rendclient.c | 2 +- src/or/rendservice.c | 9 +++-- 7 files changed, 112 insertions(+), 85 deletions(-) diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c index 8304ad8b89..af36cb2c34 100644 --- a/src/or/circuitbuild.c +++ b/src/or/circuitbuild.c @@ -60,7 +60,10 @@ 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 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,7 +734,7 @@ 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 */ @@ -1129,8 +1132,10 @@ pathbias_state_to_string(path_state_t state) return "new"; case PATH_STATE_DID_FIRST_HOP: return "first hop"; - case PATH_STATE_SUCCEEDED: - return "succeeded"; + case PATH_STATE_BUILD_SUCCEEDED: + return "build succeeded"; + case PATH_STATE_USE_SUCCEEDED: + return "use succeeded"; } return "unknown"; @@ -1216,7 +1221,7 @@ 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 = NULL; @@ -1291,7 +1296,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 = @@ -1303,7 +1308,8 @@ pathbias_count_success(origin_circuit_t *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) { if (circ->cpath && circ->cpath->extend_info) { guard = entry_guard_get_by_id_digest( @@ -1312,7 +1318,7 @@ pathbias_count_success(origin_circuit_t *circ) if (guard) { if (circ->path_state == PATH_STATE_DID_FIRST_HOP) { - circ->path_state = PATH_STATE_SUCCEEDED; + circ->path_state = PATH_STATE_BUILD_SUCCEEDED; guard->circuit_successes++; log_info(LD_CIRC, "Got success count %u/%u for guard %s=%s", @@ -1354,7 +1360,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, @@ -1371,9 +1377,70 @@ pathbias_count_success(origin_circuit_t *circ) } /** - * Count a successfully closed circuit. + * Check if a circuit was used and/or closed successfully. */ void +pathbias_check_close(origin_circuit_t *ocirc, int reason) +{ + circuit_t *circ = ô->base_; + + if (ocirc->path_state == PATH_STATE_BUILD_SUCCEEDED) { + if (circ->timestamp_dirty) { + // XXX: May open up attacks if the adversary can force connections + // on unresponsive hosts to use new circs. Vidalia displayes a "Retrying" + // state.. Can we use that? Does optimistic data change this? + // XXX: For the hidserv side, we could only care about INTRODUCING purposes + // for server+client, and REND purposes for the server... Can we + // somehow only count those? + /* Any circuit where there were attempted streams but no successful + * streams could be bias */ + log_info(LD_CIRC, + "Circuit closed without successful use for reason %d. " + "Circuit is a %s currently %s.", + reason, circuit_purpose_to_string(circ->purpose), + circuit_state_to_string(circ->state)); + pathbias_count_unusable(ocirc); + } else { + if (reason & END_CIRC_REASON_FLAG_REMOTE) { + /* Unused remote circ close reasons all could be bias */ + // XXX: We hit this a lot for hidserv circs with purposes: + // CIRCUIT_PURPOSE_S_CONNECT_REND (reasons: 514,517,520) + // CIRCUIT_PURPOSE_S_REND_JOINED (reasons: 514,517,520) + // == reasons: 2,3,8. Client-side timeouts? + log_info(LD_CIRC, + "Circuit remote-closed without successful use for reason %d. " + "Circuit is a %s currently %s.", + reason, circuit_purpose_to_string(circ->purpose), + circuit_state_to_string(circ->state)); + 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 */ + /* FIXME: Only count bias if the network is live? + * What about clock jumps/suspends? */ + log_info(LD_CIRC, + "Circuit's channel closed without successful use for reason %d, " + "channel reason %d. Circuit is a %s currently %s.", + reason, circ->n_chan->reason_for_closing, + circuit_purpose_to_string(circ->purpose), + circuit_state_to_string(circ->state)); + 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; @@ -1411,7 +1478,7 @@ pathbias_count_successful_close(origin_circuit_t *circ) * circuit after it has successfully completed. Right now, this is * used for purely informational/debugging purposes. */ -void +static void pathbias_count_collapse(origin_circuit_t *circ) { entry_guard_t *guard = NULL; @@ -1439,7 +1506,7 @@ pathbias_count_collapse(origin_circuit_t *circ) } } -void +static void pathbias_count_unusable(origin_circuit_t *circ) { entry_guard_t *guard = NULL; @@ -1511,7 +1578,7 @@ pathbias_get_closed_count(entry_guard_t *guard) if(!ocirc->cpath || !ocirc->cpath->extend_info) continue; - if (ocirc->path_state == PATH_STATE_SUCCEEDED && + if (ocirc->path_state >= PATH_STATE_BUILD_SUCCEEDED && (memcmp(guard->identity, ocirc->cpath->extend_info->identity_digest, DIGEST_LEN) @@ -2371,6 +2438,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; } diff --git a/src/or/circuitbuild.h b/src/or/circuitbuild.h index 2d9eac1215..53c9fe5c0f 100644 --- a/src/or/circuitbuild.h +++ b/src/or/circuitbuild.h @@ -56,8 +56,6 @@ const node_t *choose_good_entry_server(uint8_t purpose, 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_count_successful_close(origin_circuit_t *circ); -void pathbias_count_collapse(origin_circuit_t *circ); -void pathbias_count_unusable(origin_circuit_t *circ); +void pathbias_check_close(origin_circuit_t *circ, int reason); #endif diff --git a/src/or/circuitlist.c b/src/or/circuitlist.c index 7163c351c6..6fab4920ae 100644 --- a/src/or/circuitlist.c +++ b/src/or/circuitlist.c @@ -1354,53 +1354,7 @@ circuit_mark_for_close_(circuit_t *circ, int reason, int line, } if (CIRCUIT_IS_ORIGIN(circ)) { - origin_circuit_t *ocirc = TO_ORIGIN_CIRCUIT(circ); - - if (ocirc->path_state == PATH_STATE_SUCCEEDED) { - int pathbias_is_normal_close = 1; - - /* FIXME: Is timestamp_dirty the right thing for these two checks? - * Should we use isolation_any_streams_attached instead? - * isolation_any_streams_attached is not used for hidservs.. */ - if (!circ->timestamp_dirty) { - if (orig_reason & END_CIRC_REASON_FLAG_REMOTE) { - /* Unused remote circ close reasons all could be bias */ - // XXX: We hit this a lot for hidserv circs with purposes: - // CIRCUIT_PURPOSE_S_CONNECT_REND (reasons: 514,517,520) - // CIRCUIT_PURPOSE_S_REND_JOINED (reasons: 514,517,520) - // == reasons: 2,3,8. Client-side timeouts? - pathbias_is_normal_close = 0; - 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 */ - /* FIXME: Only count bias if the network is live? - * What about clock jumps/suspends? */ - pathbias_is_normal_close = 0; - pathbias_count_collapse(ocirc); - } - } else if (circ->timestamp_dirty && !ocirc->any_streams_succeeded) { - // XXX: May open up attacks if the adversary can force connections - // on unresponsive hosts to use new circs. Vidalia displayes a "Retrying" - // state.. Can we use that? Does optimistic data change this? - // XXX: For the hidserv side, we could only care about INTRODUCING purposes - // for server+client, and REND purposes for the server... Can we - // somehow only count those? - /* Any circuit where there were attempted streams but no successful - * streams could be bias */ - /* FIXME: This may be better handled by limiting the number of retries - * per stream? */ - pathbias_is_normal_close = 0; - pathbias_count_unusable(ocirc); - } - - if (pathbias_is_normal_close) { - pathbias_count_successful_close(ocirc); - } - } + pathbias_check_close(TO_ORIGIN_CIRCUIT(circ), reason); /* We don't send reasons when closing circuits at the origin. */ reason = END_CIRC_REASON_NONE; diff --git a/src/or/connection_edge.c b/src/or/connection_edge.c index 31ff90093c..79bb54cbb7 100644 --- a/src/or/connection_edge.c +++ b/src/or/connection_edge.c @@ -2181,10 +2181,14 @@ connection_ap_handshake_socks_reply(entry_connection_t *conn, char *reply, if (status == SOCKS5_SUCCEEDED) { if(!conn->edge_.on_circuit || !CIRCUIT_IS_ORIGIN(conn->edge_.on_circuit)) { - // XXX: Weird. We hit this a lot, and yet have no unusable_circs - log_warn(LD_BUG, "No origin circuit for successful SOCKS stream"); + // XXX: Weird. We hit this a lot, and yet have no unusable_circs. + // Maybe during addrmaps/resolves? + log_warn(LD_BUG, + "(Harmless.) No origin circuit for successful SOCKS stream. " + "Reason: %d", endreason); } else { - TO_ORIGIN_CIRCUIT(conn->edge_.on_circuit)->any_streams_succeeded = 1; + TO_ORIGIN_CIRCUIT(conn->edge_.on_circuit)->path_state + = PATH_STATE_USE_SUCCEEDED; } } @@ -2457,7 +2461,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->any_streams_succeeded = 1; + origin_circ->path_state = PATH_STATE_USE_SUCCEEDED; tor_free(address); return 0; diff --git a/src/or/or.h b/src/or/or.h index d165705e97..c8ea12f2c8 100644 --- a/src/or/or.h +++ b/src/or/or.h @@ -2768,9 +2768,17 @@ typedef enum { /** This circuit has completed a first hop, 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, + /** 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, } path_state_t; /** An origin_circuit_t holds data necessary to build and use a circuit. @@ -2872,16 +2880,6 @@ typedef struct origin_circuit_t { */ unsigned int isolation_any_streams_attached : 1; - /** - * 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). - */ - unsigned int any_streams_succeeded : 1; - /** A bitfield of ISO_* flags for every isolation field such that this * circuit has had streams with more than one value for that field * attached to it. */ diff --git a/src/or/rendclient.c b/src/or/rendclient.c index 1d473dec07..3393e0f628 100644 --- a/src/or/rendclient.c +++ b/src/or/rendclient.c @@ -363,7 +363,7 @@ rend_client_introduction_acked(origin_circuit_t *circ, /* For path bias: This circuit was used successfully. Valid * nacks and acks count. */ - circ->any_streams_succeeded = 1; + circ->path_state = PATH_STATE_USE_SUCCEEDED; if (request_len == 0) { /* It's an ACK; the introduction point relayed our introduction request. */ diff --git a/src/or/rendservice.c b/src/or/rendservice.c index 74e4bada92..fbf14e9349 100644 --- a/src/or/rendservice.c +++ b/src/or/rendservice.c @@ -1383,9 +1383,9 @@ rend_service_introduce(origin_circuit_t *circuit, const uint8_t *request, if (circuit_init_cpath_crypto(cpath,keys+DIGEST_LEN,1)<0) goto err; memcpy(cpath->handshake_digest, keys, DIGEST_LEN); - - /* For path bias: This circuit was used successfully */ - circuit->any_streams_succeeded = 1; + + /* For path bias: This intro circuit was used successfully */ + circuit->path_state = PATH_STATE_USE_SUCCEEDED; goto done; @@ -2586,7 +2586,8 @@ rend_service_rendezvous_has_opened(origin_circuit_t *circuit) tor_assert(circuit->rend_data); /* Declare the circuit dirty to avoid reuse, and for path-bias */ - circuit->base_.timestamp_dirty = time(NULL); + if(!circuit->base_.timestamp_dirty) + circuit->base_.timestamp_dirty = time(NULL); hop = circuit->build_state->service_pending_final_cpath_ref->cpath; -- cgit v1.2.3-54-g00ecf From 5f733ccd7382e8bb8289e4f8adf07f8ac001c28a Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Sat, 8 Dec 2012 12:07:58 -0800 Subject: Fix some hidden service edge cases. --- src/or/circuitbuild.c | 60 +++++++++++++++++++++++++++++++++++---------------- src/or/circuituse.c | 10 +++++++++ src/or/rendclient.c | 7 ++++++ 3 files changed, 59 insertions(+), 18 deletions(-) diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c index af36cb2c34..7eae0e7a9a 100644 --- a/src/or/circuitbuild.c +++ b/src/or/circuitbuild.c @@ -1155,10 +1155,14 @@ pathbias_should_count(origin_circuit_t *circ) 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. */ 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) { return 0; } @@ -1384,22 +1388,37 @@ pathbias_check_close(origin_circuit_t *ocirc, int reason) { circuit_t *circ = ô->base_; + if (!pathbias_should_count(circ)) { + 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 */ // XXX: May open up attacks if the adversary can force connections // on unresponsive hosts to use new circs. Vidalia displayes a "Retrying" // state.. Can we use that? Does optimistic data change this? - // XXX: For the hidserv side, we could only care about INTRODUCING purposes - // for server+client, and REND purposes for the server... Can we - // somehow only count those? - /* Any circuit where there were attempted streams but no successful - * streams could be bias */ - log_info(LD_CIRC, + // XXX: Sub-attack: in collusion with an intro point, you can induce bias + // through the web. Need a Torbutton patch to prevent this. + + /* FIXME: This is not ideal, but it prevents the case where a + * CPU overloaded intro point is chosen. + * XXX: Is this reason code authenticated? */ + if (circ->purpose == CIRCUIT_PURPOSE_C_INTRODUCING && + reason == + END_CIRC_REASON_FLAG_REMOTE|END_CIRC_REASON_RESOURCELIMIT) { + log_info(LD_CIRC, + "Ignoring CPU overload intro circuit without successful use. " + "Circuit purpose %d currently %s.", + reason, circ->purpose, circuit_state_to_string(circ->state)); + } else { + log_info(LD_CIRC, "Circuit closed without successful use for reason %d. " - "Circuit is a %s currently %s.", - reason, circuit_purpose_to_string(circ->purpose), - circuit_state_to_string(circ->state)); - pathbias_count_unusable(ocirc); + "Circuit purpose %d currently %s.", + reason, circ->purpose, circuit_state_to_string(circ->state)); + pathbias_count_unusable(ocirc); + } } else { if (reason & END_CIRC_REASON_FLAG_REMOTE) { /* Unused remote circ close reasons all could be bias */ @@ -1409,9 +1428,8 @@ pathbias_check_close(origin_circuit_t *ocirc, int reason) // == reasons: 2,3,8. Client-side timeouts? log_info(LD_CIRC, "Circuit remote-closed without successful use for reason %d. " - "Circuit is a %s currently %s.", - reason, circuit_purpose_to_string(circ->purpose), - circuit_state_to_string(circ->state)); + "Circuit purpose %d currently %s.", + reason, circ->purpose, circuit_state_to_string(circ->state)); pathbias_count_collapse(ocirc); } else if ((reason & ~END_CIRC_REASON_FLAG_REMOTE) == END_CIRC_REASON_CHANNEL_CLOSED && @@ -1423,10 +1441,9 @@ pathbias_check_close(origin_circuit_t *ocirc, int reason) * What about clock jumps/suspends? */ log_info(LD_CIRC, "Circuit's channel closed without successful use for reason %d, " - "channel reason %d. Circuit is a %s currently %s.", + "channel reason %d. Circuit purpose %d currently %s.", reason, circ->n_chan->reason_for_closing, - circuit_purpose_to_string(circ->purpose), - circuit_state_to_string(circ->state)); + circ->purpose, circuit_state_to_string(circ->state)); pathbias_count_collapse(ocirc); } else { pathbias_count_successful_close(ocirc); @@ -1548,6 +1565,13 @@ pathbias_count_timeout(origin_circuit_t *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); diff --git a/src/or/circuituse.c b/src/or/circuituse.c index 77822a36ad..9362e2420f 100644 --- a/src/or/circuituse.c +++ b/src/or/circuituse.c @@ -1402,6 +1402,16 @@ 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 && + 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. */ + circ->path_state = PATH_STATE_USE_SUCCEEDED; + /* This must be called before the purpose change */ + pathbias_check_close(circ); + } + 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 diff --git a/src/or/rendclient.c b/src/or/rendclient.c index 3393e0f628..88241a4b2c 100644 --- a/src/or/rendclient.c +++ b/src/or/rendclient.c @@ -862,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. */ -- cgit v1.2.3-54-g00ecf From b599a6ed07fd588500a256ef815e87729449626a Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Sat, 8 Dec 2012 14:16:29 -0800 Subject: Sadly, we can't safely count client intro circ success --- src/or/circuitbuild.c | 42 +++++++++++++++++------------------------- src/or/circuituse.c | 9 +++++++-- 2 files changed, 24 insertions(+), 27 deletions(-) diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c index 7eae0e7a9a..9b1236f34d 100644 --- a/src/or/circuitbuild.c +++ b/src/or/circuitbuild.c @@ -739,8 +739,12 @@ circuit_send_next_onion_skin(origin_circuit_t *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; } @@ -1156,13 +1160,19 @@ pathbias_should_count(origin_circuit_t *circ) /* We can't do path bias accounting without entry guards. * Testing and controller circuits also have no guards. + * * We also don't count server-side rends, because their - * endpoint could be chosen maliciously. */ + * 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_S_CONNECT_REND || - circ->base_.purpose == CIRCUIT_PURPOSE_S_REND_JOINED) { + 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; } @@ -1388,7 +1398,7 @@ pathbias_check_close(origin_circuit_t *ocirc, int reason) { circuit_t *circ = ô->base_; - if (!pathbias_should_count(circ)) { + if (!pathbias_should_count(ocirc)) { return; } @@ -1399,33 +1409,15 @@ pathbias_check_close(origin_circuit_t *ocirc, int reason) // XXX: May open up attacks if the adversary can force connections // on unresponsive hosts to use new circs. Vidalia displayes a "Retrying" // state.. Can we use that? Does optimistic data change this? - // XXX: Sub-attack: in collusion with an intro point, you can induce bias - // through the web. Need a Torbutton patch to prevent this. - - /* FIXME: This is not ideal, but it prevents the case where a - * CPU overloaded intro point is chosen. - * XXX: Is this reason code authenticated? */ - if (circ->purpose == CIRCUIT_PURPOSE_C_INTRODUCING && - reason == - END_CIRC_REASON_FLAG_REMOTE|END_CIRC_REASON_RESOURCELIMIT) { - log_info(LD_CIRC, - "Ignoring CPU overload intro circuit without successful use. " - "Circuit purpose %d currently %s.", - reason, circ->purpose, circuit_state_to_string(circ->state)); - } else { - log_info(LD_CIRC, + + log_info(LD_CIRC, "Circuit closed without successful use for reason %d. " "Circuit purpose %d currently %s.", reason, circ->purpose, circuit_state_to_string(circ->state)); - pathbias_count_unusable(ocirc); - } + pathbias_count_unusable(ocirc); } else { if (reason & END_CIRC_REASON_FLAG_REMOTE) { /* Unused remote circ close reasons all could be bias */ - // XXX: We hit this a lot for hidserv circs with purposes: - // CIRCUIT_PURPOSE_S_CONNECT_REND (reasons: 514,517,520) - // CIRCUIT_PURPOSE_S_REND_JOINED (reasons: 514,517,520) - // == reasons: 2,3,8. Client-side timeouts? log_info(LD_CIRC, "Circuit remote-closed without successful use for reason %d. " "Circuit purpose %d currently %s.", diff --git a/src/or/circuituse.c b/src/or/circuituse.c index 9362e2420f..0b799b11ae 100644 --- a/src/or/circuituse.c +++ b/src/or/circuituse.c @@ -1402,11 +1402,16 @@ 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 && + 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. */ + * 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). */ circ->path_state = PATH_STATE_USE_SUCCEEDED; /* This must be called before the purpose change */ pathbias_check_close(circ); -- cgit v1.2.3-54-g00ecf From 686fc222593fd46ec82d62f0fa62ca02900c1014 Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Sat, 8 Dec 2012 16:37:22 -0800 Subject: Allow any valid 'end' cell to mean a circuit was used successfully. Also improve some log messages. --- src/or/circuitbuild.c | 9 ++++++--- src/or/connection_edge.c | 17 +++++++++++------ src/or/relay.c | 8 ++++++++ 3 files changed, 25 insertions(+), 9 deletions(-) diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c index 9b1236f34d..c3a5827589 100644 --- a/src/or/circuitbuild.c +++ b/src/or/circuitbuild.c @@ -1411,16 +1411,18 @@ pathbias_check_close(origin_circuit_t *ocirc, int reason) // state.. Can we use that? Does optimistic data change this? log_info(LD_CIRC, - "Circuit closed without successful use for reason %d. " + "Circuit %d closed without successful use for reason %d. " "Circuit purpose %d currently %s.", + ocirc->global_identifier, reason, circ->purpose, circuit_state_to_string(circ->state)); 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 remote-closed without successful use for reason %d. " + "Circuit %d remote-closed without successful use for reason %d. " "Circuit purpose %d currently %s.", + ocirc->global_identifier, reason, circ->purpose, circuit_state_to_string(circ->state)); pathbias_count_collapse(ocirc); } else if ((reason & ~END_CIRC_REASON_FLAG_REMOTE) @@ -1432,8 +1434,9 @@ pathbias_check_close(origin_circuit_t *ocirc, int reason) /* FIXME: Only count bias if the network is live? * What about clock jumps/suspends? */ log_info(LD_CIRC, - "Circuit's channel closed without successful use for reason %d, " + "Circuit %d's channel closed without successful use for reason %d, " "channel reason %d. Circuit purpose %d currently %s.", + ocirc->global_identifier, reason, circ->n_chan->reason_for_closing, circ->purpose, circuit_state_to_string(circ->state)); pathbias_count_collapse(ocirc); diff --git a/src/or/connection_edge.c b/src/or/connection_edge.c index 79bb54cbb7..ca6060ca57 100644 --- a/src/or/connection_edge.c +++ b/src/or/connection_edge.c @@ -2178,14 +2178,19 @@ connection_ap_handshake_socks_reply(entry_connection_t *conn, char *reply, /* Flag this stream's circuit as having completed a stream successfully * (for path bias) */ - if (status == SOCKS5_SUCCEEDED) { + 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)) { - // XXX: Weird. We hit this a lot, and yet have no unusable_circs. - // Maybe during addrmaps/resolves? - log_warn(LD_BUG, - "(Harmless.) No origin circuit for successful SOCKS stream. " - "Reason: %d", endreason); + // DNS remaps can trigger this. So can failed hidden service + // lookups. + log_info(LD_BUG, + "(Harmless.) No origin circuit for successful SOCKS stream %ld. " + "Reason: %d", ENTRY_TO_CONN(conn)->global_identifier, endreason); } else { TO_ORIGIN_CIRCUIT(conn->edge_.on_circuit)->path_state = PATH_STATE_USE_SUCCEEDED; diff --git a/src/or/relay.c b/src/or/relay.c index 7f49299e2b..1638dae4fd 100644 --- a/src/or/relay.c +++ b/src/or/relay.c @@ -693,6 +693,14 @@ connection_ap_process_end_not_open( edge_connection_t *edge_conn = ENTRY_TO_EDGE_CONN(conn); (void) layer_hint; /* unused */ + if (rh->length > 0) { + /* Path bias: If we get a valid reason code from the exit, + * it wasn't due to tagging */ + // XXX: This relies on recognized+digest being strong enough not + // to be spoofable.. Is that a valid assumption? + 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)) { -- cgit v1.2.3-54-g00ecf From 930fbb2fec2b0c4e56cc4f10f8faec9d0d135274 Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Sun, 9 Dec 2012 19:18:04 -0800 Subject: Flag cannibalized circs as used (non-ideal). Also add some comments. --- src/or/circuitbuild.c | 4 ++++ src/or/circuituse.c | 17 ++++++++++++++++- 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c index c3a5827589..7282d57c74 100644 --- a/src/or/circuitbuild.c +++ b/src/or/circuitbuild.c @@ -1219,6 +1219,10 @@ pathbias_count_first_hop(origin_circuit_t *circ) return 0; } + // XXX: Technically, we could make this only count from the *second* hop.. + // Until we get per-hop MACs or a lower circ failure rate, this might be + // better from a false positive POV. Should we s/first_hop/circ_attempt/g? + // Then we can control this check from the consensus. if (circ->cpath->state == CPATH_STATE_AWAITING_KEYS) { /* Help track down the real cause of bug #6475: */ if (circ->has_opened && circ->path_state != PATH_STATE_DID_FIRST_HOP) { diff --git a/src/or/circuituse.c b/src/or/circuituse.c index 0b799b11ae..781e984511 100644 --- a/src/or/circuituse.c +++ b/src/or/circuituse.c @@ -1160,6 +1160,17 @@ 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. + 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. */ @@ -1411,7 +1422,11 @@ circuit_launch_by_extend_info(uint8_t purpose, * * Same deal goes for client side introductions. Clients * can be manipulated to connect repeatedly to them - * (especially web clients). */ + * (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); -- cgit v1.2.3-54-g00ecf From fbbf894d4db1b1109fedca5f74891642f61370fe Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Sun, 9 Dec 2012 20:02:08 -0800 Subject: Add intro+rend cannibalize param.. --- src/or/circuituse.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/or/circuituse.c b/src/or/circuituse.c index 781e984511..381c2b01cb 100644 --- a/src/or/circuituse.c +++ b/src/or/circuituse.c @@ -1429,7 +1429,7 @@ circuit_launch_by_extend_info(uint8_t purpose, */ circ->path_state = PATH_STATE_USE_SUCCEEDED; /* This must be called before the purpose change */ - pathbias_check_close(circ); + pathbias_check_close(circ, END_CIRC_REASON_FINISHED); } circuit_change_purpose(TO_CIRCUIT(circ), purpose); -- cgit v1.2.3-54-g00ecf From 04866055e8dadc9eb5b09773b3bbdc81e3b4dbbf Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Sun, 9 Dec 2012 20:02:41 -0800 Subject: Change from first hop accounting to 2nd hop accounting This has several advantages, including more resilience to ambient failure. I still need to rename all the first_hop vars tho.. Saving that for a separate commit. --- src/or/circuitbuild.c | 34 +++++++++++++++++++++++++++++----- 1 file changed, 29 insertions(+), 5 deletions(-) diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c index 7282d57c74..ad52a6c44e 100644 --- a/src/or/circuitbuild.c +++ b/src/or/circuitbuild.c @@ -1145,6 +1145,34 @@ pathbias_state_to_string(path_state_t state) return "unknown"; } +/** + * 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. * @@ -1219,11 +1247,7 @@ pathbias_count_first_hop(origin_circuit_t *circ) return 0; } - // XXX: Technically, we could make this only count from the *second* hop.. - // Until we get per-hop MACs or a lower circ failure rate, this might be - // better from a false positive POV. Should we s/first_hop/circ_attempt/g? - // Then we can control this check from the consensus. - if (circ->cpath->state == CPATH_STATE_AWAITING_KEYS) { + 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, -- cgit v1.2.3-54-g00ecf From a90f165b83bc1603873308d7918e99057afdf72a Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Sun, 9 Dec 2012 20:18:31 -0800 Subject: Rename first_hop to circ_attempt. Since we've generalized what we can count from (first or second hop), we should generalize the variable and constant naming too. --- src/or/circuitbuild.c | 76 +++++++++++++++++++++++++-------------------------- src/or/entrynodes.c | 12 ++++---- src/or/entrynodes.h | 2 +- src/or/or.h | 4 +-- 4 files changed, 47 insertions(+), 47 deletions(-) diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c index ad52a6c44e..42964ebefe 100644 --- a/src/or/circuitbuild.c +++ b/src/or/circuitbuild.c @@ -59,7 +59,7 @@ 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 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); @@ -987,7 +987,7 @@ 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) @@ -1134,8 +1134,8 @@ 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_BUILD_ATTEMPTED: + return "build attempted"; case PATH_STATE_BUILD_SUCCEEDED: return "build succeeded"; case PATH_STATE_USE_SUCCEEDED: @@ -1230,17 +1230,17 @@ pathbias_should_count(origin_circuit_t *circ) } /** - * 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. + * 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_first_hop(origin_circuit_t *circ) +pathbias_count_circ_attempt(origin_circuit_t *circ) { -#define FIRST_HOP_NOTICE_INTERVAL (600) - static ratelim_t first_hop_notice_limit = - RATELIM_INIT(FIRST_HOP_NOTICE_INTERVAL); +#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)) { @@ -1249,8 +1249,8 @@ pathbias_count_first_hop(origin_circuit_t *circ) 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. " @@ -1277,14 +1277,14 @@ pathbias_count_first_hop(origin_circuit_t *circ) 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. " @@ -1297,7 +1297,7 @@ 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, "Unopened circuit has no known guard. " @@ -1312,7 +1312,7 @@ 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, + if ((rate_msg = rate_limit_log(&circ_attempt_notice_limit, approx_time()))) { log_info(LD_BUG, "A %s circuit is in cpath state %d (opened: %d). " @@ -1359,12 +1359,12 @@ pathbias_count_build_success(origin_circuit_t *circ) } if (guard) { - if (circ->path_state == PATH_STATE_DID_FIRST_HOP) { + if (circ->path_state == PATH_STATE_BUILD_ATTEMPTED) { circ->path_state = PATH_STATE_BUILD_SUCCEEDED; guard->circuit_successes++; log_info(LD_CIRC, "Got success count %u/%u for guard %s=%s", - guard->circuit_successes, guard->first_hops, + guard->circuit_successes, guard->circ_attempts, guard->nickname, hex_str(guard->identity, DIGEST_LEN)); } else { if ((rate_msg = rate_limit_log(&success_notice_limit, @@ -1380,10 +1380,10 @@ pathbias_count_build_success(origin_circuit_t *circ) } } - if (guard->first_hops < guard->circuit_successes) { + if (guard->circ_attempts < guard->circuit_successes) { log_notice(LD_BUG, "Unexpectedly high circuit_successes (%u/%u) " "for guard %s=%s", - guard->circuit_successes, guard->first_hops, + guard->circuit_successes, guard->circ_attempts, guard->nickname, hex_str(guard->identity, DIGEST_LEN)); } /* In rare cases, CIRCUIT_PURPOSE_TESTING can get converted to @@ -1657,17 +1657,17 @@ pathbias_get_success_count(entry_guard_t *guard) * 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 > (unsigned)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 (pathbias_get_success_count(guard)/((double)guard->first_hops) + if (pathbias_get_success_count(guard)/((double)guard->circ_attempts) < pathbias_get_extreme_rate(options)) { /* Dropping is currently disabled by default. */ if (pathbias_get_dropguards(options)) { @@ -1680,7 +1680,7 @@ entry_guard_inc_first_hop_count(entry_guard_t *guard) "were unusable, %d collapsed, and %d timed out. For " "reference, your timeout cutoff is %ld seconds.", guard->nickname, hex_str(guard->identity, DIGEST_LEN), - pathbias_get_closed_count(guard), guard->first_hops, + pathbias_get_closed_count(guard), guard->circ_attempts, guard->circuit_successes, guard->unusable_circuits, guard->collapsed_circuits, guard->timeouts, (long)circ_times.close_ms/1000); @@ -1698,12 +1698,12 @@ entry_guard_inc_first_hop_count(entry_guard_t *guard) "were unusable, %d collapsed, and %d timed out. For " "reference, your timeout cutoff is %ld seconds.", guard->nickname, hex_str(guard->identity, DIGEST_LEN), - pathbias_get_closed_count(guard), guard->first_hops, + pathbias_get_closed_count(guard), guard->circ_attempts, guard->circuit_successes, guard->unusable_circuits, guard->collapsed_circuits, guard->timeouts, (long)circ_times.close_ms/1000); } - } else if (pathbias_get_success_count(guard)/((double)guard->first_hops) + } 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; @@ -1716,12 +1716,12 @@ entry_guard_inc_first_hop_count(entry_guard_t *guard) "were unusable, %d collapsed, and %d timed out. For " "reference, your timeout cutoff is %ld seconds.", guard->nickname, hex_str(guard->identity, DIGEST_LEN), - pathbias_get_closed_count(guard), guard->first_hops, + pathbias_get_closed_count(guard), guard->circ_attempts, guard->circuit_successes, guard->unusable_circuits, guard->collapsed_circuits, guard->timeouts, (long)circ_times.close_ms/1000); } - } else if (pathbias_get_success_count(guard)/((double)guard->first_hops) + } 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; @@ -1732,7 +1732,7 @@ entry_guard_inc_first_hop_count(entry_guard_t *guard) "were unusable, %d collapsed, and %d timed out. For " "reference, your timeout cutoff is %ld seconds.", guard->nickname, hex_str(guard->identity, DIGEST_LEN), - pathbias_get_closed_count(guard), guard->first_hops, + pathbias_get_closed_count(guard), guard->circ_attempts, guard->circuit_successes, guard->unusable_circuits, guard->collapsed_circuits, guard->timeouts, (long)circ_times.close_ms/1000); @@ -1741,27 +1741,27 @@ entry_guard_inc_first_hop_count(entry_guard_t *guard) } /* 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 > (unsigned)pathbias_get_scale_threshold(options)) { const int scale_factor = pathbias_get_scale_factor(options); const int mult_factor = pathbias_get_mult_factor(options); /* Only scale if there will be no rounding error for our scaling * factors */ - if (((mult_factor*guard->first_hops) % scale_factor) == 0 && + if (((mult_factor*guard->circ_attempts) % scale_factor) == 0 && ((mult_factor*guard->circuit_successes) % scale_factor) == 0) { log_info(LD_CIRC, "Scaling pathbias counts to (%u/%u)*(%d/%d) for guard %s=%s", - guard->circuit_successes, guard->first_hops, mult_factor, + guard->circuit_successes, guard->circ_attempts, mult_factor, scale_factor, guard->nickname, hex_str(guard->identity, DIGEST_LEN)); - guard->first_hops *= mult_factor; + guard->circ_attempts *= mult_factor; guard->circuit_successes *= mult_factor; guard->timeouts *= mult_factor; guard->successful_circuits_closed *= mult_factor; guard->collapsed_circuits *= mult_factor; guard->unusable_circuits *= mult_factor; - guard->first_hops /= scale_factor; + guard->circ_attempts /= scale_factor; guard->circuit_successes /= scale_factor; guard->timeouts /= scale_factor; guard->successful_circuits_closed /= scale_factor; @@ -1769,9 +1769,9 @@ entry_guard_inc_first_hop_count(entry_guard_t *guard) guard->unusable_circuits /= scale_factor; } } - guard->first_hops++; + guard->circ_attempts++; log_info(LD_CIRC, "Got success count %u/%u for guard %s=%s", - guard->circuit_successes, guard->first_hops, guard->nickname, + guard->circuit_successes, guard->circ_attempts, guard->nickname, hex_str(guard->identity, DIGEST_LEN)); return 0; } @@ -1794,7 +1794,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) { diff --git a/src/or/entrynodes.c b/src/or/entrynodes.c index 1e64aaf985..14a1e3c7f9 100644 --- a/src/or/entrynodes.c +++ b/src/or/entrynodes.c @@ -1050,7 +1050,7 @@ entry_guards_parse_state(or_state_t *state, int set, char **msg) unusable = 0; } - node->first_hops = hop_cnt; + node->circ_attempts = hop_cnt; node->circuit_successes = success_cnt; node->successful_circuits_closed = successful_closed; @@ -1059,17 +1059,17 @@ entry_guards_parse_state(or_state_t *state, int set, char **msg) node->unusable_circuits = unusable; log_info(LD_GENERAL, "Read %u/%u path bias for node %s", - node->circuit_successes, node->first_hops, node->nickname); + node->circuit_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) + if ((node->circuit_successes/((double)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); + node->circuit_successes, node->circ_attempts, node->nickname); } } else { @@ -1192,14 +1192,14 @@ 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) { *next = line = tor_malloc_zero(sizeof(config_line_t)); line->key = tor_strdup("EntryGuardPathBias"); /* In the long run: circuit_success ~= successful_circuit_close + * collapsed_circuits + * unusable_circuits */ tor_asprintf(&line->value, "%u %u %u %u %u %u", - e->first_hops, e->circuit_successes, + e->circ_attempts, e->circuit_successes, pathbias_get_closed_count(e), e->collapsed_circuits, e->unusable_circuits, e->timeouts); next = &(line->next); diff --git a/src/or/entrynodes.h b/src/or/entrynodes.h index c3f7b14134..b737dad5a5 100644 --- a/src/or/entrynodes.h +++ b/src/or/entrynodes.h @@ -48,7 +48,7 @@ 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 circ_attempts; /**< Number of circuits this guard has "attempted" */ unsigned circuit_successes; /**< Number of successfully built circuits using * this guard as first hop. */ unsigned successful_circuits_closed; /**< Number of circuits that carried diff --git a/src/or/or.h b/src/or/or.h index c8ea12f2c8..aaf817d450 100644 --- a/src/or/or.h +++ b/src/or/or.h @@ -2765,9 +2765,9 @@ 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, + 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 -- cgit v1.2.3-54-g00ecf From ab1fce5c19b64b3f1ba15d6ffa1f0d11d6a959c3 Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Sun, 9 Dec 2012 20:20:44 -0800 Subject: Also shorten circuit_successes to circ_successes. For consistency and great justice. Ok, mostly consistency. --- src/or/circuitbuild.c | 32 ++++++++++++++++---------------- src/or/entrynodes.c | 10 +++++----- src/or/entrynodes.h | 2 +- 3 files changed, 22 insertions(+), 22 deletions(-) diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c index 42964ebefe..04b429b2b6 100644 --- a/src/or/circuitbuild.c +++ b/src/or/circuitbuild.c @@ -1361,10 +1361,10 @@ pathbias_count_build_success(origin_circuit_t *circ) if (guard) { if (circ->path_state == PATH_STATE_BUILD_ATTEMPTED) { circ->path_state = PATH_STATE_BUILD_SUCCEEDED; - guard->circuit_successes++; + guard->circ_successes++; log_info(LD_CIRC, "Got success count %u/%u for guard %s=%s", - guard->circuit_successes, guard->circ_attempts, + guard->circ_successes, guard->circ_attempts, guard->nickname, hex_str(guard->identity, DIGEST_LEN)); } else { if ((rate_msg = rate_limit_log(&success_notice_limit, @@ -1380,10 +1380,10 @@ pathbias_count_build_success(origin_circuit_t *circ) } } - if (guard->circ_attempts < 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 (%u/%u) " "for guard %s=%s", - guard->circuit_successes, guard->circ_attempts, + guard->circ_successes, guard->circ_attempts, guard->nickname, hex_str(guard->identity, DIGEST_LEN)); } /* In rare cases, CIRCUIT_PURPOSE_TESTING can get converted to @@ -1639,7 +1639,7 @@ pathbias_get_closed_count(entry_guard_t *guard) /** * This function checks the consensus parameters to decide - * if it should return guard->circuit_successes or + * if it should return guard->circ_successes or * guard->successful_circuits_closed. */ static int @@ -1648,7 +1648,7 @@ pathbias_get_success_count(entry_guard_t *guard) if (pathbias_use_close_counts(get_options())) { return pathbias_get_closed_count(guard); } else { - return guard->circuit_successes; + return guard->circ_successes; } } @@ -1681,7 +1681,7 @@ entry_guard_inc_circ_attempt_count(entry_guard_t *guard) "reference, your timeout cutoff is %ld seconds.", guard->nickname, hex_str(guard->identity, DIGEST_LEN), pathbias_get_closed_count(guard), guard->circ_attempts, - guard->circuit_successes, guard->unusable_circuits, + guard->circ_successes, guard->unusable_circuits, guard->collapsed_circuits, guard->timeouts, (long)circ_times.close_ms/1000); guard->path_bias_disabled = 1; @@ -1699,7 +1699,7 @@ entry_guard_inc_circ_attempt_count(entry_guard_t *guard) "reference, your timeout cutoff is %ld seconds.", guard->nickname, hex_str(guard->identity, DIGEST_LEN), pathbias_get_closed_count(guard), guard->circ_attempts, - guard->circuit_successes, guard->unusable_circuits, + guard->circ_successes, guard->unusable_circuits, guard->collapsed_circuits, guard->timeouts, (long)circ_times.close_ms/1000); } @@ -1717,7 +1717,7 @@ entry_guard_inc_circ_attempt_count(entry_guard_t *guard) "reference, your timeout cutoff is %ld seconds.", guard->nickname, hex_str(guard->identity, DIGEST_LEN), pathbias_get_closed_count(guard), guard->circ_attempts, - guard->circuit_successes, guard->unusable_circuits, + guard->circ_successes, guard->unusable_circuits, guard->collapsed_circuits, guard->timeouts, (long)circ_times.close_ms/1000); } @@ -1733,7 +1733,7 @@ entry_guard_inc_circ_attempt_count(entry_guard_t *guard) "reference, your timeout cutoff is %ld seconds.", guard->nickname, hex_str(guard->identity, DIGEST_LEN), pathbias_get_closed_count(guard), guard->circ_attempts, - guard->circuit_successes, guard->unusable_circuits, + guard->circ_successes, guard->unusable_circuits, guard->collapsed_circuits, guard->timeouts, (long)circ_times.close_ms/1000); } @@ -1747,22 +1747,22 @@ entry_guard_inc_circ_attempt_count(entry_guard_t *guard) /* Only scale if there will be no rounding error for our scaling * factors */ if (((mult_factor*guard->circ_attempts) % scale_factor) == 0 && - ((mult_factor*guard->circuit_successes) % scale_factor) == 0) { + ((mult_factor*guard->circ_successes) % scale_factor) == 0) { log_info(LD_CIRC, "Scaling pathbias counts to (%u/%u)*(%d/%d) for guard %s=%s", - guard->circuit_successes, guard->circ_attempts, mult_factor, + guard->circ_successes, guard->circ_attempts, mult_factor, scale_factor, guard->nickname, hex_str(guard->identity, DIGEST_LEN)); guard->circ_attempts *= mult_factor; - guard->circuit_successes *= 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->circuit_successes /= scale_factor; + guard->circ_successes /= scale_factor; guard->timeouts /= scale_factor; guard->successful_circuits_closed /= scale_factor; guard->collapsed_circuits /= scale_factor; @@ -1771,7 +1771,7 @@ entry_guard_inc_circ_attempt_count(entry_guard_t *guard) } guard->circ_attempts++; log_info(LD_CIRC, "Got success count %u/%u for guard %s=%s", - guard->circuit_successes, guard->circ_attempts, guard->nickname, + guard->circ_successes, guard->circ_attempts, guard->nickname, hex_str(guard->identity, DIGEST_LEN)); return 0; } diff --git a/src/or/entrynodes.c b/src/or/entrynodes.c index 14a1e3c7f9..84764d1869 100644 --- a/src/or/entrynodes.c +++ b/src/or/entrynodes.c @@ -1051,7 +1051,7 @@ entry_guards_parse_state(or_state_t *state, int set, char **msg) } node->circ_attempts = hop_cnt; - node->circuit_successes = success_cnt; + node->circ_successes = success_cnt; node->successful_circuits_closed = successful_closed; node->timeouts = timeouts; @@ -1059,17 +1059,17 @@ entry_guards_parse_state(or_state_t *state, int set, char **msg) node->unusable_circuits = unusable; log_info(LD_GENERAL, "Read %u/%u path bias for node %s", - node->circuit_successes, node->circ_attempts, node->nickname); + 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->circ_attempts) + if ((node->circ_successes/((double)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->circ_attempts, node->nickname); + node->circ_successes, node->circ_attempts, node->nickname); } } else { @@ -1199,7 +1199,7 @@ entry_guards_update_state(or_state_t *state) * collapsed_circuits + * unusable_circuits */ tor_asprintf(&line->value, "%u %u %u %u %u %u", - e->circ_attempts, e->circuit_successes, + e->circ_attempts, e->circ_successes, pathbias_get_closed_count(e), e->collapsed_circuits, e->unusable_circuits, e->timeouts); next = &(line->next); diff --git a/src/or/entrynodes.h b/src/or/entrynodes.h index b737dad5a5..0e58802cbe 100644 --- a/src/or/entrynodes.h +++ b/src/or/entrynodes.h @@ -49,7 +49,7 @@ typedef struct entry_guard_t { * at which we last failed to connect to it. */ unsigned circ_attempts; /**< Number of circuits this guard has "attempted" */ - unsigned circuit_successes; /**< Number of successfully built circuits using + unsigned circ_successes; /**< Number of successfully built circuits using * this guard as first hop. */ unsigned successful_circuits_closed; /**< Number of circuits that carried * streams successfully. */ -- cgit v1.2.3-54-g00ecf From 2dbb62f1b571ea57af111f1f660a5149d160c4fb Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Sun, 9 Dec 2012 20:53:22 -0800 Subject: Convert to doubles for all pathbias state. Let's hope this solves the rounding error issue.. --- src/or/circuitbuild.c | 83 ++++++++++++++++++++++++--------------------------- src/or/entrynodes.c | 18 ++++++----- src/or/entrynodes.h | 14 ++++----- 3 files changed, 56 insertions(+), 59 deletions(-) diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c index 04b429b2b6..cbc1af9618 100644 --- a/src/or/circuitbuild.c +++ b/src/or/circuitbuild.c @@ -1363,7 +1363,7 @@ pathbias_count_build_success(origin_circuit_t *circ) circ->path_state = PATH_STATE_BUILD_SUCCEEDED; guard->circ_successes++; - log_info(LD_CIRC, "Got success count %u/%u for guard %s=%s", + log_info(LD_CIRC, "Got success count %lf/%lf for guard %s=%s", guard->circ_successes, guard->circ_attempts, guard->nickname, hex_str(guard->identity, DIGEST_LEN)); } else { @@ -1381,7 +1381,7 @@ pathbias_count_build_success(origin_circuit_t *circ) } if (guard->circ_attempts < guard->circ_successes) { - log_notice(LD_BUG, "Unexpectedly high successes counts (%u/%u) " + log_notice(LD_BUG, "Unexpectedly high successes counts (%lf/%lf) " "for guard %s=%s", guard->circ_successes, guard->circ_attempts, guard->nickname, hex_str(guard->identity, DIGEST_LEN)); @@ -1607,7 +1607,7 @@ pathbias_count_timeout(origin_circuit_t *circ) } // XXX: DOCDOC -int +double pathbias_get_closed_count(entry_guard_t *guard) { circuit_t *circ = global_circuitlist; @@ -1642,7 +1642,7 @@ pathbias_get_closed_count(entry_guard_t *guard) * if it should return guard->circ_successes or * guard->successful_circuits_closed. */ -static int +static double pathbias_get_success_count(entry_guard_t *guard) { if (pathbias_use_close_counts(get_options())) { @@ -1663,11 +1663,11 @@ entry_guard_inc_circ_attempt_count(entry_guard_t *guard) entry_guards_changed(); - if (guard->circ_attempts > (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 (pathbias_get_success_count(guard)/((double)guard->circ_attempts) + 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)) { @@ -1680,9 +1680,9 @@ entry_guard_inc_circ_attempt_count(entry_guard_t *guard) "were unusable, %d collapsed, and %d timed out. For " "reference, your timeout cutoff is %ld seconds.", guard->nickname, hex_str(guard->identity, DIGEST_LEN), - pathbias_get_closed_count(guard), guard->circ_attempts, - guard->circ_successes, guard->unusable_circuits, - guard->collapsed_circuits, guard->timeouts, + (int)pathbias_get_closed_count(guard), (int)guard->circ_attempts, + (int)guard->circ_successes, (int)guard->unusable_circuits, + (int)guard->collapsed_circuits, (int)guard->timeouts, (long)circ_times.close_ms/1000); guard->path_bias_disabled = 1; guard->bad_since = approx_time(); @@ -1698,9 +1698,9 @@ entry_guard_inc_circ_attempt_count(entry_guard_t *guard) "were unusable, %d collapsed, and %d timed out. For " "reference, your timeout cutoff is %ld seconds.", guard->nickname, hex_str(guard->identity, DIGEST_LEN), - pathbias_get_closed_count(guard), guard->circ_attempts, - guard->circ_successes, guard->unusable_circuits, - guard->collapsed_circuits, guard->timeouts, + (int)pathbias_get_closed_count(guard), (int)guard->circ_attempts, + (int)guard->circ_successes, (int)guard->unusable_circuits, + (int)guard->collapsed_circuits, (int)guard->timeouts, (long)circ_times.close_ms/1000); } } else if (pathbias_get_success_count(guard)/((double)guard->circ_attempts) @@ -1716,9 +1716,9 @@ entry_guard_inc_circ_attempt_count(entry_guard_t *guard) "were unusable, %d collapsed, and %d timed out. For " "reference, your timeout cutoff is %ld seconds.", guard->nickname, hex_str(guard->identity, DIGEST_LEN), - pathbias_get_closed_count(guard), guard->circ_attempts, - guard->circ_successes, guard->unusable_circuits, - guard->collapsed_circuits, guard->timeouts, + (int)pathbias_get_closed_count(guard), (int)guard->circ_attempts, + (int)guard->circ_successes, (int)guard->unusable_circuits, + (int)guard->collapsed_circuits, (int)guard->timeouts, (long)circ_times.close_ms/1000); } } else if (pathbias_get_success_count(guard)/((double)guard->circ_attempts) @@ -1732,45 +1732,40 @@ entry_guard_inc_circ_attempt_count(entry_guard_t *guard) "were unusable, %d collapsed, and %d timed out. For " "reference, your timeout cutoff is %ld seconds.", guard->nickname, hex_str(guard->identity, DIGEST_LEN), - pathbias_get_closed_count(guard), guard->circ_attempts, - guard->circ_successes, guard->unusable_circuits, - guard->collapsed_circuits, guard->timeouts, + (int)pathbias_get_closed_count(guard), (int)guard->circ_attempts, + (int)guard->circ_successes, (int)guard->unusable_circuits, + (int)guard->collapsed_circuits, (int)guard->timeouts, (long)circ_times.close_ms/1000); } } } /* If we get a ton of circuits, just scale everything down */ - if (guard->circ_attempts > (unsigned)pathbias_get_scale_threshold(options)) { + if (guard->circ_attempts > pathbias_get_scale_threshold(options)) { const int scale_factor = pathbias_get_scale_factor(options); const int mult_factor = pathbias_get_mult_factor(options); - /* Only scale if there will be no rounding error for our scaling - * factors */ - if (((mult_factor*guard->circ_attempts) % scale_factor) == 0 && - ((mult_factor*guard->circ_successes) % scale_factor) == 0) { - log_info(LD_CIRC, - "Scaling pathbias counts to (%u/%u)*(%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; - } + log_info(LD_CIRC, + "Scaling pathbias counts to (%lf/%lf)*(%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->circ_attempts++; - log_info(LD_CIRC, "Got success count %u/%u for guard %s=%s", + log_info(LD_CIRC, "Got success count %lf/%lf for guard %s=%s", guard->circ_successes, guard->circ_attempts, guard->nickname, hex_str(guard->identity, DIGEST_LEN)); return 0; diff --git a/src/or/entrynodes.c b/src/or/entrynodes.c index 84764d1869..21c09f79c3 100644 --- a/src/or/entrynodes.c +++ b/src/or/entrynodes.c @@ -1021,7 +1021,7 @@ 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, timeouts, collapsed, successful_closed, + double hop_cnt, success_cnt, timeouts, collapsed, successful_closed, unusable; if (!node) { @@ -1031,20 +1031,22 @@ entry_guards_parse_state(or_state_t *state, int set, char **msg) } /* First try 3 params, then 2. */ - // XXX: We want to convert this to floating point before merge /* In the long run: circuit_success ~= successful_circuit_close + * collapsed_circuits + * unusable_circuits */ - if (tor_sscanf(line->value, "%u %u %u %u %u %u", + if (tor_sscanf(line->value, "%lf %lf %lf %lf %lf %lf", &hop_cnt, &success_cnt, &successful_closed, &collapsed, &unusable, &timeouts) != 6) { - if (tor_sscanf(line->value, "%u %u", &success_cnt, &hop_cnt) != 2) { + 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)); - successful_closed = success_cnt; + success_cnt = old_success; + successful_closed = old_success; + hop_cnt = old_hops; timeouts = 0; collapsed = 0; unusable = 0; @@ -1058,7 +1060,7 @@ entry_guards_parse_state(or_state_t *state, int set, char **msg) node->collapsed_circuits = collapsed; node->unusable_circuits = unusable; - log_info(LD_GENERAL, "Read %u/%u path bias for node %s", + log_info(LD_GENERAL, "Read %lf/%lf 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 @@ -1068,7 +1070,7 @@ entry_guards_parse_state(or_state_t *state, int set, char **msg) pathbias_get_dropguards(options)) { node->path_bias_disabled = 1; log_info(LD_GENERAL, - "Path bias is too high (%u/%u); disabling node %s", + "Path bias is too high (%lf/%lf); disabling node %s", node->circ_successes, node->circ_attempts, node->nickname); } @@ -1198,7 +1200,7 @@ entry_guards_update_state(or_state_t *state) /* In the long run: circuit_success ~= successful_circuit_close + * collapsed_circuits + * unusable_circuits */ - tor_asprintf(&line->value, "%u %u %u %u %u %u", + tor_asprintf(&line->value, "%lf %lf %lf %lf %lf %lf", e->circ_attempts, e->circ_successes, pathbias_get_closed_count(e), e->collapsed_circuits, e->unusable_circuits, e->timeouts); diff --git a/src/or/entrynodes.h b/src/or/entrynodes.h index 0e58802cbe..de8c60c33d 100644 --- a/src/or/entrynodes.h +++ b/src/or/entrynodes.h @@ -48,17 +48,17 @@ 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 circ_attempts; /**< Number of circuits this guard has "attempted" */ - unsigned circ_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. */ - unsigned successful_circuits_closed; /**< Number of circuits that carried + double successful_circuits_closed; /**< Number of circuits that carried * streams successfully. */ - unsigned collapsed_circuits; /**< Number of fully built circuits that were + double collapsed_circuits; /**< Number of fully built circuits that were * remotely closed before any streams were * attempted. */ - unsigned unusable_circuits; /**< Number of circuits for which streams were + double unusable_circuits; /**< Number of circuits for which streams were * attempted, but none succeeded. */ - unsigned timeouts; /**< Number of 'right-censored' circuit timeouts for this + double timeouts; /**< Number of 'right-censored' circuit timeouts for this * guard. */ } entry_guard_t; @@ -111,7 +111,7 @@ int find_transport_by_bridge_addrport(const tor_addr_t *addr, uint16_t port, int validate_pluggable_transports_config(void); -int pathbias_get_closed_count(entry_guard_t *gaurd); +double pathbias_get_closed_count(entry_guard_t *gaurd); #endif -- cgit v1.2.3-54-g00ecf From b75880d7b3d02f5c60bf2e215c6e84da4f3e1938 Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Sun, 9 Dec 2012 20:56:48 -0800 Subject: Fix a rather serious use-count state bug. We need to use the success count or the use count depending on the consensus parameter. --- src/or/circuitbuild.c | 2 +- src/or/entrynodes.c | 4 ++-- src/or/entrynodes.h | 1 + 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c index cbc1af9618..349063d325 100644 --- a/src/or/circuitbuild.c +++ b/src/or/circuitbuild.c @@ -1642,7 +1642,7 @@ pathbias_get_closed_count(entry_guard_t *guard) * if it should return guard->circ_successes or * guard->successful_circuits_closed. */ -static double +double pathbias_get_success_count(entry_guard_t *guard) { if (pathbias_use_close_counts(get_options())) { diff --git a/src/or/entrynodes.c b/src/or/entrynodes.c index 21c09f79c3..96b075a35c 100644 --- a/src/or/entrynodes.c +++ b/src/or/entrynodes.c @@ -1065,8 +1065,8 @@ 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 ((node->circ_successes/((double)node->circ_attempts) - < pathbias_get_extreme_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, diff --git a/src/or/entrynodes.h b/src/or/entrynodes.h index de8c60c33d..b9d0e555f1 100644 --- a/src/or/entrynodes.h +++ b/src/or/entrynodes.h @@ -112,6 +112,7 @@ 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 -- cgit v1.2.3-54-g00ecf From 4590993ff3d4393caaa1d9d68d04cf0af95c23c7 Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Sun, 9 Dec 2012 23:47:04 -0800 Subject: Space fixes. --- src/or/circuitbuild.c | 38 +++++++++++++++++++++----------------- src/or/circuitbuild.h | 1 + src/or/circuitlist.c | 6 +++--- src/or/circuituse.c | 4 ++-- src/or/connection_edge.c | 6 +++--- src/or/entrynodes.c | 4 ++-- src/or/entrynodes.h | 2 +- src/or/relay.c | 2 +- src/or/rendservice.c | 4 ++-- 9 files changed, 36 insertions(+), 31 deletions(-) diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c index 349063d325..a724006b28 100644 --- a/src/or/circuitbuild.c +++ b/src/or/circuitbuild.c @@ -1199,7 +1199,7 @@ pathbias_should_count(origin_circuit_t *circ) 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_INTRODUCING && circ->base_.purpose <= CIRCUIT_PURPOSE_C_INTRODUCE_ACKED)) { return 0; } @@ -1350,7 +1350,7 @@ pathbias_count_build_success(origin_circuit_t *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) { if (circ->cpath && circ->cpath->extend_info) { @@ -1456,14 +1456,14 @@ pathbias_check_close(origin_circuit_t *ocirc, int reason) } else if ((reason & ~END_CIRC_REASON_FLAG_REMOTE) == END_CIRC_REASON_CHANNEL_CLOSED && circ->n_chan && - circ->n_chan->reason_for_closing + circ->n_chan->reason_for_closing != CHANNEL_CLOSE_REQUESTED) { /* If we didn't close the channel ourselves, it could be bias */ /* FIXME: 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 %s.", + "Circuit %d's channel closed without successful use for reason " + "%d, channel reason %d. Circuit purpose %d currently %s.", ocirc->global_identifier, reason, circ->n_chan->reason_for_closing, circ->purpose, circuit_state_to_string(circ->state)); @@ -1494,7 +1494,7 @@ pathbias_count_successful_close(origin_circuit_t *circ) } if (guard) { - /* In the long run: circuit_success ~= successful_circuit_close + + /* In the long run: circuit_success ~= successful_circuit_close + * circ_failure + stream_failure */ guard->successful_circuits_closed++; entry_guards_changed(); @@ -1511,7 +1511,7 @@ pathbias_count_successful_close(origin_circuit_t *circ) } /** - * Count a circuit that fails after it is built, but before it can + * 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 @@ -1622,7 +1622,7 @@ pathbias_get_closed_count(entry_guard_t *guard) ocirc = TO_ORIGIN_CIRCUIT(circ); - if(!ocirc->cpath || !ocirc->cpath->extend_info) + if (!ocirc->cpath || !ocirc->cpath->extend_info) continue; if (ocirc->path_state >= PATH_STATE_BUILD_SUCCEEDED && @@ -1642,7 +1642,7 @@ pathbias_get_closed_count(entry_guard_t *guard) * if it should return guard->circ_successes or * guard->successful_circuits_closed. */ -double +double pathbias_get_success_count(entry_guard_t *guard) { if (pathbias_use_close_counts(get_options())) { @@ -1680,8 +1680,9 @@ entry_guard_inc_circ_attempt_count(entry_guard_t *guard) "were unusable, %d collapsed, and %d timed out. For " "reference, your timeout cutoff is %ld seconds.", guard->nickname, hex_str(guard->identity, DIGEST_LEN), - (int)pathbias_get_closed_count(guard), (int)guard->circ_attempts, - (int)guard->circ_successes, (int)guard->unusable_circuits, + (int)pathbias_get_closed_count(guard), + (int)guard->circ_attempts, (int)guard->circ_successes, + (int)guard->unusable_circuits, (int)guard->collapsed_circuits, (int)guard->timeouts, (long)circ_times.close_ms/1000); guard->path_bias_disabled = 1; @@ -1698,8 +1699,9 @@ entry_guard_inc_circ_attempt_count(entry_guard_t *guard) "were unusable, %d collapsed, and %d timed out. For " "reference, your timeout cutoff is %ld seconds.", guard->nickname, hex_str(guard->identity, DIGEST_LEN), - (int)pathbias_get_closed_count(guard), (int)guard->circ_attempts, - (int)guard->circ_successes, (int)guard->unusable_circuits, + (int)pathbias_get_closed_count(guard), + (int)guard->circ_attempts, (int)guard->circ_successes, + (int)guard->unusable_circuits, (int)guard->collapsed_circuits, (int)guard->timeouts, (long)circ_times.close_ms/1000); } @@ -1716,8 +1718,9 @@ entry_guard_inc_circ_attempt_count(entry_guard_t *guard) "were unusable, %d collapsed, and %d timed out. For " "reference, your timeout cutoff is %ld seconds.", guard->nickname, hex_str(guard->identity, DIGEST_LEN), - (int)pathbias_get_closed_count(guard), (int)guard->circ_attempts, - (int)guard->circ_successes, (int)guard->unusable_circuits, + (int)pathbias_get_closed_count(guard), + (int)guard->circ_attempts, (int)guard->circ_successes, + (int)guard->unusable_circuits, (int)guard->collapsed_circuits, (int)guard->timeouts, (long)circ_times.close_ms/1000); } @@ -1732,8 +1735,9 @@ entry_guard_inc_circ_attempt_count(entry_guard_t *guard) "were unusable, %d collapsed, and %d timed out. For " "reference, your timeout cutoff is %ld seconds.", guard->nickname, hex_str(guard->identity, DIGEST_LEN), - (int)pathbias_get_closed_count(guard), (int)guard->circ_attempts, - (int)guard->circ_successes, (int)guard->unusable_circuits, + (int)pathbias_get_closed_count(guard), + (int)guard->circ_attempts, (int)guard->circ_successes, + (int)guard->unusable_circuits, (int)guard->collapsed_circuits, (int)guard->timeouts, (long)circ_times.close_ms/1000); } diff --git a/src/or/circuitbuild.h b/src/or/circuitbuild.h index 53c9fe5c0f..8cd61fae2d 100644 --- a/src/or/circuitbuild.h +++ b/src/or/circuitbuild.h @@ -59,3 +59,4 @@ void pathbias_count_timeout(origin_circuit_t *circ); void pathbias_check_close(origin_circuit_t *circ, int reason); #endif + diff --git a/src/or/circuitlist.c b/src/or/circuitlist.c index 6fab4920ae..0ee29000ed 100644 --- a/src/or/circuitlist.c +++ b/src/or/circuitlist.c @@ -1040,9 +1040,9 @@ circuit_unlink_all_from_channel(channel_t *chan, int reason) if (circ->n_chan == chan) { 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. */ + * bit to mark_for_close. */ if (chan->reason_for_closing != CHANNEL_CLOSE_REQUESTED) reason |= END_CIRC_REASON_FLAG_REMOTE; } @@ -1352,7 +1352,7 @@ 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); diff --git a/src/or/circuituse.c b/src/or/circuituse.c index 381c2b01cb..d3b480139e 100644 --- a/src/or/circuituse.c +++ b/src/or/circuituse.c @@ -1161,7 +1161,7 @@ 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 + * (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 @@ -1418,7 +1418,7 @@ circuit_launch_by_extend_info(uint8_t purpose, 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. + * because the rend point could be malicious. * * Same deal goes for client side introductions. Clients * can be manipulated to connect repeatedly to them diff --git a/src/or/connection_edge.c b/src/or/connection_edge.c index ca6060ca57..570ffe4941 100644 --- a/src/or/connection_edge.c +++ b/src/or/connection_edge.c @@ -2184,13 +2184,13 @@ connection_ap_handshake_socks_reply(entry_connection_t *conn, char *reply, endreason == END_STREAM_REASON_CONNRESET || endreason == END_STREAM_REASON_NOROUTE || endreason == END_STREAM_REASON_RESOURCELIMIT) { - if(!conn->edge_.on_circuit || + 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, - "(Harmless.) No origin circuit for successful SOCKS stream %ld. " - "Reason: %d", ENTRY_TO_CONN(conn)->global_identifier, endreason); + "No origin circuit for successful SOCKS stream %ld. Reason: " + "%d", ENTRY_TO_CONN(conn)->global_identifier, endreason); } else { TO_ORIGIN_CIRCUIT(conn->edge_.on_circuit)->path_state = PATH_STATE_USE_SUCCEEDED; diff --git a/src/or/entrynodes.c b/src/or/entrynodes.c index 96b075a35c..066dbecc2a 100644 --- a/src/or/entrynodes.c +++ b/src/or/entrynodes.c @@ -1031,7 +1031,7 @@ entry_guards_parse_state(or_state_t *state, int set, char **msg) } /* First try 3 params, then 2. */ - /* In the long run: circuit_success ~= successful_circuit_close + + /* In the long run: circuit_success ~= successful_circuit_close + * collapsed_circuits + * unusable_circuits */ if (tor_sscanf(line->value, "%lf %lf %lf %lf %lf %lf", @@ -1197,7 +1197,7 @@ entry_guards_update_state(or_state_t *state) if (e->circ_attempts) { *next = line = tor_malloc_zero(sizeof(config_line_t)); line->key = tor_strdup("EntryGuardPathBias"); - /* In the long run: circuit_success ~= successful_circuit_close + + /* In the long run: circuit_success ~= successful_circuit_close + * collapsed_circuits + * unusable_circuits */ tor_asprintf(&line->value, "%lf %lf %lf %lf %lf %lf", diff --git a/src/or/entrynodes.h b/src/or/entrynodes.h index b9d0e555f1..2686a4f34d 100644 --- a/src/or/entrynodes.h +++ b/src/or/entrynodes.h @@ -56,7 +56,7 @@ typedef struct entry_guard_t { 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 + 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. */ diff --git a/src/or/relay.c b/src/or/relay.c index 1638dae4fd..3ee0c835b2 100644 --- a/src/or/relay.c +++ b/src/or/relay.c @@ -694,7 +694,7 @@ connection_ap_process_end_not_open( (void) layer_hint; /* unused */ if (rh->length > 0) { - /* Path bias: If we get a valid reason code from the exit, + /* Path bias: If we get a valid reason code from the exit, * it wasn't due to tagging */ // XXX: This relies on recognized+digest being strong enough not // to be spoofable.. Is that a valid assumption? diff --git a/src/or/rendservice.c b/src/or/rendservice.c index fbf14e9349..e70f969e8f 100644 --- a/src/or/rendservice.c +++ b/src/or/rendservice.c @@ -1383,7 +1383,7 @@ rend_service_introduce(origin_circuit_t *circuit, const uint8_t *request, if (circuit_init_cpath_crypto(cpath,keys+DIGEST_LEN,1)<0) 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; @@ -2586,7 +2586,7 @@ rend_service_rendezvous_has_opened(origin_circuit_t *circuit) tor_assert(circuit->rend_data); /* Declare the circuit dirty to avoid reuse, and for path-bias */ - if(!circuit->base_.timestamp_dirty) + if (!circuit->base_.timestamp_dirty) circuit->base_.timestamp_dirty = time(NULL); hop = circuit->build_state->service_pending_final_cpath_ref->cpath; -- cgit v1.2.3-54-g00ecf From aa16d59ee7abdf7c3309c267052ae265e141e1b3 Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Sun, 9 Dec 2012 23:50:05 -0800 Subject: Clean up some XXX comments. --- src/or/circuitbuild.c | 4 ---- src/or/circuituse.c | 1 + src/or/relay.c | 4 +++- 3 files changed, 4 insertions(+), 5 deletions(-) diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c index a724006b28..f903bbf095 100644 --- a/src/or/circuitbuild.c +++ b/src/or/circuitbuild.c @@ -1434,10 +1434,6 @@ pathbias_check_close(origin_circuit_t *ocirc, int reason) if (circ->timestamp_dirty) { /* Any circuit where there were attempted streams but no successful * streams could be bias */ - // XXX: May open up attacks if the adversary can force connections - // on unresponsive hosts to use new circs. Vidalia displayes a "Retrying" - // state.. Can we use that? Does optimistic data change this? - log_info(LD_CIRC, "Circuit %d closed without successful use for reason %d. " "Circuit purpose %d currently %s.", diff --git a/src/or/circuituse.c b/src/or/circuituse.c index d3b480139e..cb44bba328 100644 --- a/src/or/circuituse.c +++ b/src/or/circuituse.c @@ -1166,6 +1166,7 @@ circuit_has_opened(origin_circuit_t *circ) * 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; diff --git a/src/or/relay.c b/src/or/relay.c index 3ee0c835b2..6ed4b930ae 100644 --- a/src/or/relay.c +++ b/src/or/relay.c @@ -697,7 +697,9 @@ connection_ap_process_end_not_open( /* Path bias: If we get a valid reason code from the exit, * it wasn't due to tagging */ // XXX: This relies on recognized+digest being strong enough not - // to be spoofable.. Is that a valid assumption? + // to be spoofable.. Is that a valid assumption? + // Or more accurately: is it better than nothing? Can the attack + // be done offline? circ->path_state = PATH_STATE_USE_SUCCEEDED; } -- cgit v1.2.3-54-g00ecf From 08da247042a106fbf8a437b7cd25460b0f2ee045 Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Sun, 9 Dec 2012 23:58:01 -0800 Subject: Update changes file. --- changes/bug7157 | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/changes/bug7157 b/changes/bug7157 index fad3977bfa..4f0e3b3fcb 100644 --- a/changes/bug7157 +++ b/changes/bug7157 @@ -13,3 +13,14 @@ 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. -- cgit v1.2.3-54-g00ecf From 43a00877cf01888229d5d34085a2b75748848af1 Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Mon, 10 Dec 2012 00:13:55 -0800 Subject: Update manpage. --- doc/tor.1.txt | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/doc/tor.1.txt b/doc/tor.1.txt index 734bc64bf7..7ceefb6a4c 100644 --- a/doc/tor.1.txt +++ b/doc/tor.1.txt @@ -1183,12 +1183,18 @@ The following options are useful only for clients (that is, if **PathBiasMultFactor** __NUM__ + -**PathBiasScaleFactor** __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 + 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. -- cgit v1.2.3-54-g00ecf From d409c8a90d876c2f45a1c4ea14ddae44fa7c8f18 Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Mon, 10 Dec 2012 00:28:07 -0800 Subject: More log message and space fixups. --- src/or/circuitbuild.c | 22 ++++++++++++++-------- src/or/relay.c | 2 +- 2 files changed, 15 insertions(+), 9 deletions(-) diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c index f903bbf095..d39817c042 100644 --- a/src/or/circuitbuild.c +++ b/src/or/circuitbuild.c @@ -1436,18 +1436,22 @@ pathbias_check_close(origin_circuit_t *ocirc, int reason) * streams could be bias */ log_info(LD_CIRC, "Circuit %d closed without successful use for reason %d. " - "Circuit purpose %d currently %s.", + "Circuit purpose %d currently %d,%s. Len %d.", ocirc->global_identifier, - reason, circ->purpose, circuit_state_to_string(circ->state)); + 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 %s.", + "Circuit purpose %d currently %d,%s. Len %d.", ocirc->global_identifier, - reason, circ->purpose, circuit_state_to_string(circ->state)); + 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 && @@ -1455,14 +1459,16 @@ pathbias_check_close(origin_circuit_t *ocirc, int reason) circ->n_chan->reason_for_closing != CHANNEL_CLOSE_REQUESTED) { /* If we didn't close the channel ourselves, it could be bias */ - /* FIXME: Only count bias if the network is live? + /* 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 %s.", - ocirc->global_identifier, + "%d, channel reason %d. Circuit purpose %d currently %d,%s. Len " + "%d.", ocirc->global_identifier, reason, circ->n_chan->reason_for_closing, - circ->purpose, circuit_state_to_string(circ->state)); + 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); diff --git a/src/or/relay.c b/src/or/relay.c index 6ed4b930ae..fd8f8579a7 100644 --- a/src/or/relay.c +++ b/src/or/relay.c @@ -697,7 +697,7 @@ connection_ap_process_end_not_open( /* Path bias: If we get a valid reason code from the exit, * it wasn't due to tagging */ // XXX: This relies on recognized+digest being strong enough not - // to be spoofable.. Is that a valid assumption? + // to be spoofable.. Is that a valid assumption? // Or more accurately: is it better than nothing? Can the attack // be done offline? circ->path_state = PATH_STATE_USE_SUCCEEDED; -- cgit v1.2.3-54-g00ecf From c1bc6a112498ba53243cfc8ec259a9e20124d86a Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Mon, 10 Dec 2012 00:36:10 -0800 Subject: Add a missing comment. --- src/or/circuitbuild.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c index d39817c042..cc4379762d 100644 --- a/src/or/circuitbuild.c +++ b/src/or/circuitbuild.c @@ -1608,7 +1608,13 @@ pathbias_count_timeout(origin_circuit_t *circ) } } -// XXX: DOCDOC +/** + * 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) { -- cgit v1.2.3-54-g00ecf From af9011f82430a5fac0a6db368f1afb1aa4bbc9f6 Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Tue, 11 Dec 2012 17:19:39 -0800 Subject: Woops, this log message triggers with the 2-hop bias commit. --- src/or/circuitbuild.c | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c index cc4379762d..3e2568cb13 100644 --- a/src/or/circuitbuild.c +++ b/src/or/circuitbuild.c @@ -1309,22 +1309,6 @@ pathbias_count_circ_attempt(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(&circ_attempt_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; -- cgit v1.2.3-54-g00ecf From ccaeef22e168af34e9b6a63d65ce17e58dd702e2 Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Tue, 11 Dec 2012 17:49:12 -0800 Subject: Tags on relay cells can result in certain reason codes. Close the circuit (it's probably junk anyways), and make sure we don't probe it/count it as a success. --- src/or/circuitbuild.c | 2 ++ src/or/or.h | 8 +++++++- src/or/relay.c | 24 +++++++++++++++++------- 3 files changed, 26 insertions(+), 8 deletions(-) diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c index 3e2568cb13..f93b04f579 100644 --- a/src/or/circuitbuild.c +++ b/src/or/circuitbuild.c @@ -1140,6 +1140,8 @@ pathbias_state_to_string(path_state_t state) return "build succeeded"; case PATH_STATE_USE_SUCCEEDED: return "use succeeded"; + case PATH_STATE_USE_FAILED: + return "use failed"; } return "unknown"; diff --git a/src/or/or.h b/src/or/or.h index aaf817d450..ccc20b94d7 100644 --- a/src/or/or.h +++ b/src/or/or.h @@ -2779,6 +2779,12 @@ typedef enum { * 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. @@ -2816,7 +2822,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 diff --git a/src/or/relay.c b/src/or/relay.c index fd8f8579a7..b4b77007cd 100644 --- a/src/or/relay.c +++ b/src/or/relay.c @@ -694,13 +694,23 @@ connection_ap_process_end_not_open( (void) layer_hint; /* unused */ if (rh->length > 0) { - /* Path bias: If we get a valid reason code from the exit, - * it wasn't due to tagging */ - // XXX: This relies on recognized+digest being strong enough not - // to be spoofable.. Is that a valid assumption? - // Or more accurately: is it better than nothing? Can the attack - // be done offline? - circ->path_state = PATH_STATE_USE_SUCCEEDED; + 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 shat itself. 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 */ + // XXX: This relies on recognized+digest being strong enough not + // to be spoofable.. Is that a valid assumption? + // Or more accurately: is it better than nothing? Can the attack + // be done offline? + circ->path_state = PATH_STATE_USE_SUCCEEDED; + } } if (rh->length > 0 && edge_reason_is_retriable(reason) && -- cgit v1.2.3-54-g00ecf From b0fc18c37e0d30d9941109382df4b2b44f0f0ff8 Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Tue, 18 Dec 2012 12:39:03 -0800 Subject: Changes from Nick's code review 'part 1' I think this is actually his third code review of this branch so far. --- src/or/circuitbuild.c | 90 ++++++++++++++++++++++++++---------------------- src/or/config.c | 4 +-- src/or/connection_edge.c | 2 +- src/or/entrynodes.c | 2 +- src/or/relay.c | 12 +++---- 5 files changed, 59 insertions(+), 51 deletions(-) diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c index f93b04f579..58020f25ba 100644 --- a/src/or/circuitbuild.c +++ b/src/or/circuitbuild.c @@ -1015,7 +1015,7 @@ 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 */ -double +static double pathbias_get_warn_rate(const or_options_t *options) { #define DFLT_PATH_BIAS_WARN_PCT 50 @@ -1055,7 +1055,7 @@ pathbias_get_dropguards(const or_options_t *options) return options->PathBiasDropGuards; else return networkstatus_get_param(NULL, "pb_dropguards", - DFLT_PATH_BIAS_DROP_GUARDS, 0, 100)/100.0; + DFLT_PATH_BIAS_DROP_GUARDS, 0, 1); } /** @@ -1078,9 +1078,10 @@ pathbias_get_scale_threshold(const or_options_t *options) /** * The scale factor is the denominator for our scaling - * of circuit counts for our path bias window. Note that - * we must be careful of the values we use here, as the - * code only scales in the event of no integer truncation. + * 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) @@ -1301,7 +1302,7 @@ pathbias_count_circ_attempt(origin_circuit_t *circ) } else { 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), @@ -1378,7 +1379,7 @@ pathbias_count_build_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), @@ -1490,7 +1491,7 @@ pathbias_count_successful_close(origin_circuit_t *circ) /* 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_BUG, + log_info(LD_CIRC, "Successfully closed circuit has no known guard. " "Circuit is a %s currently %s", circuit_purpose_to_string(circ->base_.purpose), @@ -1526,7 +1527,7 @@ pathbias_count_collapse(origin_circuit_t *circ) /* 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_BUG, + log_info(LD_CIRC, "Destroyed circuit has no known guard. " "Circuit is a %s currently %s", circuit_purpose_to_string(circ->base_.purpose), @@ -1554,7 +1555,7 @@ pathbias_count_unusable(origin_circuit_t *circ) /* 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_BUG, + log_info(LD_CIRC, "Stream-failing circuit has no known guard. " "Circuit is a %s currently %s", circuit_purpose_to_string(circ->base_.purpose), @@ -1620,10 +1621,9 @@ pathbias_get_closed_count(entry_guard_t *guard) continue; if (ocirc->path_state >= PATH_STATE_BUILD_SUCCEEDED && - (memcmp(guard->identity, + fast_memeq(guard->identity, ocirc->cpath->extend_info->identity_digest, - DIGEST_LEN) - == 0)) { + DIGEST_LEN)) { open_circuits++; } } @@ -1670,15 +1670,17 @@ entry_guard_inc_circ_attempt_count(entry_guard_t *guard) "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 %d/%d. %d circuits completed, %d " - "were unusable, %d collapsed, and %d timed out. For " + "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), - (int)pathbias_get_closed_count(guard), - (int)guard->circ_attempts, (int)guard->circ_successes, - (int)guard->unusable_circuits, - (int)guard->collapsed_circuits, (int)guard->timeouts, - (long)circ_times.close_ms/1000); + 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; @@ -1689,15 +1691,17 @@ entry_guard_inc_circ_attempt_count(entry_guard_t *guard) "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 %d/%d. %d circuits completed, %d " - "were unusable, %d collapsed, and %d timed out. For " + "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), - (int)pathbias_get_closed_count(guard), - (int)guard->circ_attempts, (int)guard->circ_successes, - (int)guard->unusable_circuits, - (int)guard->collapsed_circuits, (int)guard->timeouts, - (long)circ_times.close_ms/1000); + 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)) { @@ -1708,15 +1712,17 @@ entry_guard_inc_circ_attempt_count(entry_guard_t *guard) "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 %d/%d. %d circuits completed, %d " - "were unusable, %d collapsed, and %d timed out. For " + "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), - (int)pathbias_get_closed_count(guard), - (int)guard->circ_attempts, (int)guard->circ_successes, - (int)guard->unusable_circuits, - (int)guard->collapsed_circuits, (int)guard->timeouts, - (long)circ_times.close_ms/1000); + 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)) { @@ -1725,15 +1731,17 @@ entry_guard_inc_circ_attempt_count(entry_guard_t *guard) 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 %d/%d. %d circuits completed, %d " - "were unusable, %d collapsed, and %d timed out. For " + "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), - (int)pathbias_get_closed_count(guard), - (int)guard->circ_attempts, (int)guard->circ_successes, - (int)guard->unusable_circuits, - (int)guard->collapsed_circuits, (int)guard->timeouts, - (long)circ_times.close_ms/1000); + 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)); } } } diff --git a/src/or/config.c b/src/or/config.c index 79725cbe03..1d81c540a8 100644 --- a/src/or/config.c +++ b/src/or/config.c @@ -322,8 +322,8 @@ static config_var_t option_vars_[] = { V(PathBiasScaleThreshold, INT, "-1"), V(PathBiasScaleFactor, INT, "-1"), V(PathBiasMultFactor, INT, "-1"), - V(PathBiasDropGuards, BOOL, "0"), - V(PathBiasUseCloseCounts, BOOL, "1"), + V(PathBiasDropGuards, AUTOBOOL, "0"), + V(PathBiasUseCloseCounts, AUTOBOOL, "1"), OBSOLETE("PathlenCoinWeight"), V(PerConnBWBurst, MEMUNIT, "0"), diff --git a/src/or/connection_edge.c b/src/or/connection_edge.c index 570ffe4941..fa91c3a1ba 100644 --- a/src/or/connection_edge.c +++ b/src/or/connection_edge.c @@ -2189,7 +2189,7 @@ connection_ap_handshake_socks_reply(entry_connection_t *conn, char *reply, // DNS remaps can trigger this. So can failed hidden service // lookups. log_info(LD_BUG, - "No origin circuit for successful SOCKS stream %ld. Reason: " + "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 diff --git a/src/or/entrynodes.c b/src/or/entrynodes.c index 066dbecc2a..fc7f6ed352 100644 --- a/src/or/entrynodes.c +++ b/src/or/entrynodes.c @@ -1194,7 +1194,7 @@ entry_guards_update_state(or_state_t *state) d, e->chosen_by_version, t); next = &(line->next); } - if (e->circ_attempts) { + if (e->circ_attempts > 0) { *next = line = tor_malloc_zero(sizeof(config_line_t)); line->key = tor_strdup("EntryGuardPathBias"); /* In the long run: circuit_success ~= successful_circuit_close + diff --git a/src/or/relay.c b/src/or/relay.c index b4b77007cd..8b3b27f036 100644 --- a/src/or/relay.c +++ b/src/or/relay.c @@ -698,17 +698,17 @@ connection_ap_process_end_not_open( 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 shat itself. Do not probe. + * 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 */ - // XXX: This relies on recognized+digest being strong enough not - // to be spoofable.. Is that a valid assumption? - // Or more accurately: is it better than nothing? Can the attack - // be done offline? + * 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; } } -- cgit v1.2.3-54-g00ecf From 406d59a9c93e46bcb5be0e0a5c087f4860522d56 Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Tue, 18 Dec 2012 14:16:01 -0800 Subject: Nick's Code review #3 part 2. --- src/or/circuitbuild.c | 15 +++++++++++---- src/or/entrynodes.c | 6 +++--- 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c index 58020f25ba..804782df33 100644 --- a/src/or/circuitbuild.c +++ b/src/or/circuitbuild.c @@ -1350,7 +1350,7 @@ pathbias_count_build_success(origin_circuit_t *circ) circ->path_state = PATH_STATE_BUILD_SUCCEEDED; guard->circ_successes++; - log_info(LD_CIRC, "Got success count %lf/%lf for guard %s=%s", + 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 { @@ -1368,7 +1368,7 @@ pathbias_count_build_success(origin_circuit_t *circ) } if (guard->circ_attempts < guard->circ_successes) { - log_notice(LD_BUG, "Unexpectedly high successes counts (%lf/%lf) " + log_notice(LD_BUG, "Unexpectedly high successes counts (%f/%f) " "for guard %s=%s", guard->circ_successes, guard->circ_attempts, guard->nickname, hex_str(guard->identity, DIGEST_LEN)); @@ -1407,6 +1407,13 @@ pathbias_count_build_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) @@ -1751,7 +1758,7 @@ entry_guard_inc_circ_attempt_count(entry_guard_t *guard) const int scale_factor = pathbias_get_scale_factor(options); const int mult_factor = pathbias_get_mult_factor(options); log_info(LD_CIRC, - "Scaling pathbias counts to (%lf/%lf)*(%d/%d) for guard %s=%s", + "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)); @@ -1771,7 +1778,7 @@ entry_guard_inc_circ_attempt_count(entry_guard_t *guard) guard->unusable_circuits /= scale_factor; } guard->circ_attempts++; - log_info(LD_CIRC, "Got success count %lf/%lf for guard %s=%s", + 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; diff --git a/src/or/entrynodes.c b/src/or/entrynodes.c index fc7f6ed352..7aa4d7a731 100644 --- a/src/or/entrynodes.c +++ b/src/or/entrynodes.c @@ -1060,7 +1060,7 @@ entry_guards_parse_state(or_state_t *state, int set, char **msg) node->collapsed_circuits = collapsed; node->unusable_circuits = unusable; - log_info(LD_GENERAL, "Read %lf/%lf path bias for node %s", + 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 @@ -1070,7 +1070,7 @@ entry_guards_parse_state(or_state_t *state, int set, char **msg) pathbias_get_dropguards(options)) { node->path_bias_disabled = 1; log_info(LD_GENERAL, - "Path bias is too high (%lf/%lf); disabling node %s", + "Path bias is too high (%f/%f); disabling node %s", node->circ_successes, node->circ_attempts, node->nickname); } @@ -1200,7 +1200,7 @@ entry_guards_update_state(or_state_t *state) /* In the long run: circuit_success ~= successful_circuit_close + * collapsed_circuits + * unusable_circuits */ - tor_asprintf(&line->value, "%lf %lf %lf %lf %lf %lf", + 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); -- cgit v1.2.3-54-g00ecf