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/config.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) (limited to 'src/or/config.c') 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"), -- 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(-) (limited to 'src/or/config.c') 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 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(-) (limited to 'src/or/config.c') 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 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(-) (limited to 'src/or/config.c') 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