From ca6c8136128eed09a33aeeddc6d11b58b4eb361b Mon Sep 17 00:00:00 2001 From: Sebastian Hahn Date: Mon, 27 Dec 2010 18:44:42 +0100 Subject: [PATCH 1/6] Make get_net_param_from_list() static This prepares for making the accessor method for consensus parameters safer in the next commit. --- src/or/networkstatus.c | 2 +- src/or/networkstatus.h | 2 -- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/src/or/networkstatus.c b/src/or/networkstatus.c index dfc3a45f76..76e21592dc 100644 --- a/src/or/networkstatus.c +++ b/src/or/networkstatus.c @@ -2125,7 +2125,7 @@ networkstatus_dump_bridge_status_to_file(time_t now) tor_free(status); } -int32_t +static int32_t get_net_param_from_list(smartlist_t *net_params, const char *param_name, int default_val) { diff --git a/src/or/networkstatus.h b/src/or/networkstatus.h index 2f18dc9525..f95c1563b4 100644 --- a/src/or/networkstatus.h +++ b/src/or/networkstatus.h @@ -81,8 +81,6 @@ void signed_descs_update_status_from_consensus_networkstatus( char *networkstatus_getinfo_helper_single(routerstatus_t *rs); char *networkstatus_getinfo_by_purpose(const char *purpose_string, time_t now); void networkstatus_dump_bridge_status_to_file(time_t now); -int32_t get_net_param_from_list(smartlist_t *net_params, const char *name, - int default_val); int32_t networkstatus_get_param(networkstatus_t *ns, const char *param_name, int32_t default_val); int getinfo_helper_networkstatus(control_connection_t *conn, From 026e7987ad312a26efb926ae44adc158770de7cd Mon Sep 17 00:00:00 2001 From: Sebastian Hahn Date: Thu, 30 Dec 2010 19:54:13 +0100 Subject: [PATCH 2/6] Sanity-check consensus param values We need to make sure that the worst thing that a weird consensus param can do to us is to break our Tor (and only if the other Tors are reliably broken in the same way) so that the majority of directory authorities can't pull any attacks that are worse than the DoS that they can trigger by simply shutting down. One of these worse things was the cbtnummodes parameter, which could lead to heap corruption on some systems if the value was sufficiently large. This commit fixes this particular issue and also introduces sanity checking for all consensus parameters. --- changes/bug2317 | 9 ++++++ doc/spec/dir-spec.txt | 8 +++-- doc/spec/path-spec.txt | 26 ++++++++++++++-- src/or/circuitbuild.c | 67 +++++++++++++++++++++++++++++------------- src/or/circuitbuild.h | 2 ++ src/or/circuitlist.c | 4 ++- src/or/connection_or.c | 8 ++--- src/or/dirvote.h | 3 -- src/or/networkstatus.c | 43 ++++++++++++++++++++------- src/or/networkstatus.h | 3 +- src/or/or.h | 28 +++++++++++++++++- src/or/relay.c | 6 ++-- src/or/router.c | 2 +- src/or/routerlist.c | 3 +- src/or/routerparse.c | 3 +- src/test/test_dir.c | 7 +++-- 16 files changed, 168 insertions(+), 54 deletions(-) create mode 100644 changes/bug2317 diff --git a/changes/bug2317 b/changes/bug2317 new file mode 100644 index 0000000000..0b9366c36f --- /dev/null +++ b/changes/bug2317 @@ -0,0 +1,9 @@ + o Major features: + - Introduce minimum/maximum values that a client is going to believe + in a consensus. This helps to avoid crashes or worse when a param + has a weird value. + + o Major bugfixes: + - Prevent crash/heap corruption when cbtnumnodes consensus parameter is + set to 0 or large values. Fixes bug 2317. + diff --git a/doc/spec/dir-spec.txt b/doc/spec/dir-spec.txt index 6e35deb00e..4ba2ee38bf 100644 --- a/doc/spec/dir-spec.txt +++ b/doc/spec/dir-spec.txt @@ -1161,12 +1161,14 @@ research indicates that a lower value would mean fewer cells in transit in the network at any given time. Obeyed by Tor 0.2.1.20 and later. + Min: 1, Max: 100000 XXX are these sane "CircuitPriorityHalflifeMsec" -- the halflife parameter used when weighting which circuit will send the next cell. Obeyed by Tor 0.2.2.10-alpha and later. (Versions of Tor between 0.2.2.7-alpha and 0.2.2.10-alpha recognized a "CircPriorityHalflifeMsec" parameter, but mishandled it badly.) + Min: -1, Max: 2147483647 (INT32_MAX) XXX are these sane? "perconnbwrate" and "perconnbwburst" -- if set, each relay sets up a separate token bucket for every client OR connection, @@ -1176,12 +1178,14 @@ and later. (Note that relays running 0.2.2.7-alpha through 0.2.2.14-alpha looked for bwconnrate and bwconnburst, but then did the wrong thing with them; see bug 1830 for details.) + Min: 1, Max: 2147483647 (INT32_MAX) - "refuseunknownexits" -- if set and non-zero, exit relays look at + "refuseunknownexits" -- if set to one, exit relays look at the previous hop of circuits that ask to open an exit stream, and refuse to exit if they don't recognize it as a relay. The goal is to make it harder for people to use them as one-hop proxies. See trac entry 1751 for details. + Min: 0, Max: 1 See also "2.4.5. Consensus parameters governing behavior" in path-spec.txt for a series of circuit build time related @@ -1820,7 +1824,7 @@ To ensure consensus, all calculations are performed using integer math with a fixed precision determined by the bwweightscale consensus - parameter (defaults at 10000). + parameter (defaults at 10000, Min: 1, Max: INT32_MAX). For future balancing improvements, Tor clients support 11 additional weights for directory requests and middle weighting. These weights are currently diff --git a/doc/spec/path-spec.txt b/doc/spec/path-spec.txt index 2e4207bd56..4e1bdd08cb 100644 --- a/doc/spec/path-spec.txt +++ b/doc/spec/path-spec.txt @@ -421,12 +421,16 @@ of their choices. cbtdisabled Default: 0 - Effect: If non-zero, all CircuitBuildTime learning code should be + Min: 0 + Max: 1 + Effect: If 1, all CircuitBuildTime learning code should be disabled and history should be discarded. For use in emergency situations only. cbtnummodes Default: 3 + Min: 1 + Max: 20 Effect: This value governs how many modes to use in the weighted average calculation of Pareto paramter Xm. A value of 3 introduces some bias (2-5% of CDF) under ideal conditions, but allows for better @@ -435,43 +439,61 @@ of their choices. cbtrecentcount Default: 20 + Min: 3 + Max: 1000 Effect: This is the number of circuit build times to keep track of for the following option. cbtmaxtimeouts Default: 18 + Min: 3 + Max: 10000 Effect: When this many timeouts happen in the last 'cbtrecentcount' circuit attempts, the client should discard all of its history and begin learning a fresh timeout value. cbtmincircs Default: 100 + Min: 1 + Max: 10000 Effect: This is the minimum number of circuits to build before computing a timeout. cbtquantile Default: 80 + Min: 10 + Max: 99 Effect: This is the position on the quantile curve to use to set the - timeout value. It is a percent (0-99). + timeout value. It is a percent (10-99). cbtclosequantile Default: 95 + Min: Value of cbtquantile parameter + Max: 99 Effect: This is the position on the quantile curve to use to set the timeout value to use to actually close circuits. It is a percent (0-99). cbttestfreq Default: 60 + Min: 1 + Max: 2147483647 (INT32_MAX) Effect: Describes how often in seconds to build a test circuit to gather timeout values. Only applies if less than 'cbtmincircs' have been recorded. cbtmintimeout Default: 2000 + Min: 500 + Max: 2147483647 (INT32_MAX) Effect: This is the minimum allowed timeout value in milliseconds. + The minimum is to prevent rounding to 0 (we only check once + per second). cbtinitialtimeout Default: 60000 + Min: Value of cbtmintimeout + Max: 2147483647 (INT32_MAX) Effect: This is the timeout value to use before computing a timeout, in milliseconds. diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c index b1743847c8..a8e9778789 100644 --- a/src/or/circuitbuild.c +++ b/src/or/circuitbuild.c @@ -106,7 +106,7 @@ circuit_build_times_disabled(void) return 0; } else { int consensus_disabled = networkstatus_get_param(NULL, "cbtdisabled", - 0); + 0, 0, 1); int config_disabled = !get_options()->LearnCircuitBuildTimeout; int dirauth_disabled = get_options()->AuthoritativeDir; int state_disabled = (get_or_state()->LastWritten == -1); @@ -128,16 +128,19 @@ circuit_build_times_disabled(void) static int32_t circuit_build_times_max_timeouts(void) { - int32_t num = networkstatus_get_param(NULL, "cbtmaxtimeouts", - CBT_DEFAULT_MAX_RECENT_TIMEOUT_COUNT); - return num; + return networkstatus_get_param(NULL, "cbtmaxtimeouts", + CBT_DEFAULT_MAX_RECENT_TIMEOUT_COUNT, + CBT_MIN_MAX_RECENT_TIMEOUT_COUNT, + CBT_MAX_MAX_RECENT_TIMEOUT_COUNT); } static int32_t circuit_build_times_default_num_xm_modes(void) { int32_t num = networkstatus_get_param(NULL, "cbtnummodes", - CBT_DEFAULT_NUM_XM_MODES); + CBT_DEFAULT_NUM_XM_MODES, + CBT_MIN_NUM_XM_MODES, + CBT_MAX_NUM_XM_MODES); return num; } @@ -145,7 +148,9 @@ static int32_t circuit_build_times_min_circs_to_observe(void) { int32_t num = networkstatus_get_param(NULL, "cbtmincircs", - CBT_DEFAULT_MIN_CIRCUITS_TO_OBSERVE); + CBT_DEFAULT_MIN_CIRCUITS_TO_OBSERVE, + CBT_MIN_MIN_CIRCUITS_TO_OBSERVE, + CBT_MAX_MIN_CIRCUITS_TO_OBSERVE); return num; } @@ -161,24 +166,39 @@ double circuit_build_times_quantile_cutoff(void) { int32_t num = networkstatus_get_param(NULL, "cbtquantile", - CBT_DEFAULT_QUANTILE_CUTOFF); + CBT_DEFAULT_QUANTILE_CUTOFF, + CBT_MIN_QUANTILE_CUTOFF, + CBT_MAX_QUANTILE_CUTOFF); return num/100.0; } +int +circuit_build_times_get_bw_scale(networkstatus_t *ns) +{ + return networkstatus_get_param(ns, "bwweightscale", + BW_WEIGHT_SCALE, + BW_MIN_WEIGHT_SCALE, + BW_MAX_WEIGHT_SCALE); +} + static double circuit_build_times_close_quantile(void) { - int32_t num = networkstatus_get_param(NULL, "cbtclosequantile", - CBT_DEFAULT_CLOSE_QUANTILE); - - return num/100.0; + return networkstatus_get_param(NULL, "cbtclosequantile", + CBT_DEFAULT_CLOSE_QUANTILE, + /* Cast is safe, cbtquantile is capped at + * CBT_MAX_QUANTILE_CUTOFF. */ + (int)tor_lround(100*circuit_build_times_quantile_cutoff()), + CBT_MAX_CLOSE_QUANTILE) / 100.0; } static int32_t circuit_build_times_test_frequency(void) { int32_t num = networkstatus_get_param(NULL, "cbttestfreq", - CBT_DEFAULT_TEST_FREQUENCY); + CBT_DEFAULT_TEST_FREQUENCY, + CBT_MIN_TEST_FREQUENCY, + CBT_MAX_TEST_FREQUENCY); return num; } @@ -186,7 +206,9 @@ static int32_t circuit_build_times_min_timeout(void) { int32_t num = networkstatus_get_param(NULL, "cbtmintimeout", - CBT_DEFAULT_TIMEOUT_MIN_VALUE); + CBT_DEFAULT_TIMEOUT_MIN_VALUE, + CBT_MIN_TIMEOUT_MIN_VALUE, + CBT_MAX_TIMEOUT_MIN_VALUE); return num; } @@ -194,16 +216,19 @@ int32_t circuit_build_times_initial_timeout(void) { int32_t num = networkstatus_get_param(NULL, "cbtinitialtimeout", - CBT_DEFAULT_TIMEOUT_INITIAL_VALUE); + CBT_DEFAULT_TIMEOUT_INITIAL_VALUE, + circuit_build_times_min_timeout(), + CBT_MAX_TIMEOUT_INITIAL_VALUE); return num; } static int32_t -circuit_build_times_recent_circuit_count(void) +circuit_build_times_recent_circuit_count(networkstatus_t *ns) { - int32_t num = networkstatus_get_param(NULL, "cbtrecentcount", - CBT_DEFAULT_RECENT_CIRCUITS); - return num; + return networkstatus_get_param(ns, "cbtrecentcount", + CBT_DEFAULT_RECENT_CIRCUITS, + CBT_MIN_RECENT_CIRCUITS, + CBT_MAX_RECENT_CIRCUITS); } /** @@ -216,8 +241,7 @@ void circuit_build_times_new_consensus_params(circuit_build_times_t *cbt, networkstatus_t *ns) { - int32_t num = networkstatus_get_param(ns, "cbtrecentcount", - CBT_DEFAULT_RECENT_CIRCUITS); + int32_t num = circuit_build_times_recent_circuit_count(ns); if (num > 0 && num != cbt->liveness.num_recent_circs) { int8_t *recent_circs; @@ -307,7 +331,8 @@ void circuit_build_times_init(circuit_build_times_t *cbt) { memset(cbt, 0, sizeof(*cbt)); - cbt->liveness.num_recent_circs = circuit_build_times_recent_circuit_count(); + cbt->liveness.num_recent_circs = + circuit_build_times_recent_circuit_count(NULL); cbt->liveness.timeouts_after_firsthop = tor_malloc_zero(sizeof(int8_t)* cbt->liveness.num_recent_circs); cbt->close_ms = cbt->timeout_ms = circuit_build_times_get_initial_timeout(); diff --git a/src/or/circuitbuild.h b/src/or/circuitbuild.h index 74bbd4f22a..af24931878 100644 --- a/src/or/circuitbuild.h +++ b/src/or/circuitbuild.h @@ -121,5 +121,7 @@ void circuit_build_times_network_is_live(circuit_build_times_t *cbt); int circuit_build_times_network_check_live(circuit_build_times_t *cbt); void circuit_build_times_network_circ_success(circuit_build_times_t *cbt); +int circuit_build_times_get_bw_scale(networkstatus_t *ns); + #endif diff --git a/src/or/circuitlist.c b/src/or/circuitlist.c index 58ff27e5e1..b4f5f45615 100644 --- a/src/or/circuitlist.c +++ b/src/or/circuitlist.c @@ -384,7 +384,9 @@ circuit_purpose_to_controller_string(uint8_t purpose) int32_t circuit_initial_package_window(void) { - int32_t num = networkstatus_get_param(NULL, "circwindow", CIRCWINDOW_START); + int32_t num = networkstatus_get_param(NULL, "circwindow", CIRCWINDOW_START, + CIRCWINDOW_START_MIN, + CIRCWINDOW_START_MAX); /* If the consensus tells us a negative number, we'd assert. */ if (num < 0) num = CIRCWINDOW_START; diff --git a/src/or/connection_or.c b/src/or/connection_or.c index cf7c09a3cd..b93699ccc1 100644 --- a/src/or/connection_or.c +++ b/src/or/connection_or.c @@ -370,11 +370,11 @@ connection_or_update_token_buckets_helper(or_connection_t *conn, int reset, * bandwidth parameters in the consensus, but allow local config * options to override. */ rate = options->PerConnBWRate ? (int)options->PerConnBWRate : - (int)networkstatus_get_param(NULL, "perconnbwrate", - (int)options->BandwidthRate); + networkstatus_get_param(NULL, "perconnbwrate", + (int)options->BandwidthRate, 1, INT32_MAX); burst = options->PerConnBWBurst ? (int)options->PerConnBWBurst : - (int)networkstatus_get_param(NULL, "perconnbwburst", - (int)options->BandwidthBurst); + networkstatus_get_param(NULL, "perconnbwburst", + (int)options->BandwidthBurst, 1, INT32_MAX); } conn->bandwidthrate = rate; diff --git a/src/or/dirvote.h b/src/or/dirvote.h index cd5fe86bc1..67540a37fb 100644 --- a/src/or/dirvote.h +++ b/src/or/dirvote.h @@ -19,9 +19,6 @@ /** Smallest allowable voting interval. */ #define MIN_VOTE_INTERVAL 300 -/** Precision multiplier for the Bw weights */ -#define BW_WEIGHT_SCALE 10000 - void dirvote_free_all(void); /* vote manipulation */ diff --git a/src/or/networkstatus.c b/src/or/networkstatus.c index 76e21592dc..50bb88bb96 100644 --- a/src/or/networkstatus.c +++ b/src/or/networkstatus.c @@ -2127,30 +2127,50 @@ networkstatus_dump_bridge_status_to_file(time_t now) static int32_t get_net_param_from_list(smartlist_t *net_params, const char *param_name, - int default_val) + int32_t default_val, int32_t min_val, int32_t max_val) { + int32_t res = default_val; size_t name_len = strlen(param_name); + tor_assert(max_val > min_val); + tor_assert(min_val <= default_val); + tor_assert(max_val >= default_val); + SMARTLIST_FOREACH_BEGIN(net_params, const char *, p) { if (!strcmpstart(p, param_name) && p[name_len] == '=') { int ok=0; long v = tor_parse_long(p+name_len+1, 10, INT32_MIN, INT32_MAX, &ok, NULL); - if (ok) - return (int32_t) v; + if (ok) { + res = (int32_t) v; + break; + } } } SMARTLIST_FOREACH_END(p); - return default_val; + if (res < min_val) { + log_warn(LD_DIR, "Consensus parameter %s is too small. Got %d, raising to " + "%d.", param_name, res, min_val); + res = min_val; + } else if (res > max_val) { + log_warn(LD_DIR, "Consensus parameter %s is too large. Got %d, capping to " + "%d.", param_name, res, max_val); + res = max_val; + } + + return res; } /** Return the value of a integer parameter from the networkstatus ns * whose name is param_name. If ns is NULL, try loading the * latest consensus ourselves. Return default_val if no latest - * consensus, or if it has no parameter called param_name. */ + * consensus, or if it has no parameter called param_name. + * Make sure the value parsed from the consensus is at least + * min_val and at most max_val and raise/cap the parsed value + * if necessary. */ int32_t networkstatus_get_param(networkstatus_t *ns, const char *param_name, - int32_t default_val) + int32_t default_val, int32_t min_val, int32_t max_val) { if (!ns) /* if they pass in null, go find it ourselves */ ns = networkstatus_get_latest_consensus(); @@ -2158,16 +2178,17 @@ networkstatus_get_param(networkstatus_t *ns, const char *param_name, if (!ns || !ns->net_params) return default_val; - return get_net_param_from_list(ns->net_params, param_name, default_val); + return get_net_param_from_list(ns->net_params, param_name, + default_val, min_val, max_val); } /** Return the value of a integer bw weight parameter from the networkstatus * ns whose name is weight_name. If ns is NULL, try * loading the latest consensus ourselves. Return default_val if no - * latest consensus, or if it has no parameter called param_name. */ + * latest consensus, or if it has no parameter called weight_name. */ int32_t networkstatus_get_bw_weight(networkstatus_t *ns, const char *weight_name, - int32_t default_val) + int32_t default_val) { if (!ns) /* if they pass in null, go find it ourselves */ ns = networkstatus_get_latest_consensus(); @@ -2175,7 +2196,9 @@ networkstatus_get_bw_weight(networkstatus_t *ns, const char *weight_name, if (!ns || !ns->weight_params) return default_val; - return get_net_param_from_list(ns->weight_params, weight_name, default_val); + return get_net_param_from_list(ns->weight_params, weight_name, + default_val, -1, + circuit_build_times_get_bw_scale(ns)); } /** Return the name of the consensus flavor flav as used to identify diff --git a/src/or/networkstatus.h b/src/or/networkstatus.h index f95c1563b4..ec2e8f884d 100644 --- a/src/or/networkstatus.h +++ b/src/or/networkstatus.h @@ -82,7 +82,8 @@ char *networkstatus_getinfo_helper_single(routerstatus_t *rs); char *networkstatus_getinfo_by_purpose(const char *purpose_string, time_t now); void networkstatus_dump_bridge_status_to_file(time_t now); int32_t networkstatus_get_param(networkstatus_t *ns, const char *param_name, - int32_t default_val); + int32_t default_val, int32_t min_val, + int32_t max_val); int getinfo_helper_networkstatus(control_connection_t *conn, const char *question, char **answer, const char **errmsg); diff --git a/src/or/or.h b/src/or/or.h index cb36126d99..01ff5e89d5 100644 --- a/src/or/or.h +++ b/src/or/or.h @@ -765,6 +765,8 @@ typedef enum { /** Initial value for both sides of a circuit transmission window when the * circuit is initialized. Measured in cells. */ #define CIRCWINDOW_START 1000 +#define CIRCWINDOW_START_MIN 1 +#define CIRCWINDOW_START_MAX 100000 /** Amount to increment a circuit window when we get a circuit SENDME. */ #define CIRCWINDOW_INCREMENT 100 /** Initial value on both sides of a stream transmission window when the @@ -2942,6 +2944,11 @@ struct socks_request_t { /* Circuit Build Timeout "public" structures. */ +/** Precision multiplier for the Bw weights */ +#define BW_WEIGHT_SCALE 10000 +#define BW_MIN_WEIGHT_SCALE 1 +#define BW_MAX_WEIGHT_SCALE INT32_MAX + /** Total size of the circuit timeout history to accumulate. * 1000 is approx 2.5 days worth of continual-use circuits. */ #define CBT_NCIRCUITS_TO_OBSERVE 1000 @@ -2951,6 +2958,8 @@ struct socks_request_t { /** Number of modes to use in the weighted-avg computation of Xm */ #define CBT_DEFAULT_NUM_XM_MODES 3 +#define CBT_MIN_NUM_XM_MODES 1 +#define CBT_MAX_NUM_XM_MODES 20 /** A build_time_t is milliseconds */ typedef uint32_t build_time_t; @@ -2972,12 +2981,16 @@ typedef uint32_t build_time_t; * build in terms of CDF quantile. */ #define CBT_DEFAULT_CLOSE_QUANTILE 95 +/* Minimum value derived from cbtquantile parameter. */ +#define CBT_MAX_CLOSE_QUANTILE 99 /** * How many circuits count as recent when considering if the * connection has gone gimpy or changed. */ #define CBT_DEFAULT_RECENT_CIRCUITS 20 +#define CBT_MIN_RECENT_CIRCUITS 3 +#define CBT_MAX_RECENT_CIRCUITS 1000 /** * Maximum count of timeouts that finish the first hop in the past @@ -2988,25 +3001,38 @@ typedef uint32_t build_time_t; * gives us. */ #define CBT_DEFAULT_MAX_RECENT_TIMEOUT_COUNT (CBT_DEFAULT_RECENT_CIRCUITS*9/10) +#define CBT_MIN_MAX_RECENT_TIMEOUT_COUNT 3 +#define CBT_MAX_MAX_RECENT_TIMEOUT_COUNT 10000 /** Minimum circuits before estimating a timeout */ #define CBT_DEFAULT_MIN_CIRCUITS_TO_OBSERVE 100 +#define CBT_MIN_MIN_CIRCUITS_TO_OBSERVE 1 +#define CBT_MAX_MIN_CIRCUITS_TO_OBSERVE 10000 /** Cutoff percentile on the CDF for our timeout estimation. */ #define CBT_DEFAULT_QUANTILE_CUTOFF 80 +#define CBT_MIN_QUANTILE_CUTOFF 10 +#define CBT_MAX_QUANTILE_CUTOFF 99 double circuit_build_times_quantile_cutoff(void); /** How often in seconds should we build a test circuit */ #define CBT_DEFAULT_TEST_FREQUENCY 60 +#define CBT_MIN_TEST_FREQUENCY 1 +#define CBT_MAX_TEST_FREQUENCY INT32_MAX /** Lowest allowable value for CircuitBuildTimeout in milliseconds */ #define CBT_DEFAULT_TIMEOUT_MIN_VALUE (1500) +#define CBT_MIN_TIMEOUT_MIN_VALUE 500 +#define CBT_MAX_TIMEOUT_MIN_VALUE INT32_MAX /** Initial circuit build timeout in milliseconds */ #define CBT_DEFAULT_TIMEOUT_INITIAL_VALUE (60*1000) +#define CBT_MAX_TIMEOUT_INITIAL_VALUE INT32_MAX +/* CBT_MIN_TIMEOUT_INITIAL_VALUE dependent on + * circuit_build_times_min_timeout() */ int32_t circuit_build_times_initial_timeout(void); -#if CBT_DEFAULT_MAX_RECENT_TIMEOUT_COUNT < 1 +#if CBT_DEFAULT_MAX_RECENT_TIMEOUT_COUNT < CBT_MIN_MAX_RECENT_TIMEOUT_COUNT #error "RECENT_CIRCUITS is set too low." #endif diff --git a/src/or/relay.c b/src/or/relay.c index 32ac96edf4..a6c25062a3 100644 --- a/src/or/relay.c +++ b/src/or/relay.c @@ -1999,9 +1999,9 @@ cell_ewma_set_scale_factor(or_options_t *options, networkstatus_t *consensus) if (options && options->CircuitPriorityHalflife >= -EPSILON) { halflife = options->CircuitPriorityHalflife; source = "CircuitPriorityHalflife in configuration"; - } else if (consensus && - (halflife_ms = networkstatus_get_param( - consensus, "CircuitPriorityHalflifeMsec", -1)) >= 0) { + } else if (consensus && (halflife_ms = networkstatus_get_param( + consensus, "CircuitPriorityHalflifeMsec", + -1, -1, INT32_MAX)) >= 0) { halflife = ((double)halflife_ms)/1000.0; source = "CircuitPriorityHalflifeMsec in consensus"; } else { diff --git a/src/or/router.c b/src/or/router.c index 3bb37de8cf..26ac351fc4 100644 --- a/src/or/router.c +++ b/src/or/router.c @@ -1068,7 +1068,7 @@ should_refuse_unknown_exits(or_options_t *options) if (options->RefuseUnknownExits_ != -1) { return options->RefuseUnknownExits_; } else { - return networkstatus_get_param(NULL, "refuseunknownexits", 1); + return networkstatus_get_param(NULL, "refuseunknownexits", 1, 0, 1); } } diff --git a/src/or/routerlist.c b/src/or/routerlist.c index e29b4c49d8..47caebf8df 100644 --- a/src/or/routerlist.c +++ b/src/or/routerlist.c @@ -1650,8 +1650,7 @@ smartlist_choose_by_bandwidth_weights(smartlist_t *sl, return NULL; } - weight_scale = networkstatus_get_param(NULL, "bwweightscale", - BW_WEIGHT_SCALE); + weight_scale = circuit_build_times_get_bw_scale(NULL); if (rule == WEIGHT_FOR_GUARD) { Wg = networkstatus_get_bw_weight(NULL, "Wgg", -1); diff --git a/src/or/routerparse.c b/src/or/routerparse.c index a6eef2df6c..08f81d9f76 100644 --- a/src/or/routerparse.c +++ b/src/or/routerparse.c @@ -11,6 +11,7 @@ #include "or.h" #include "config.h" +#include "circuitbuild.h" #include "dirserv.h" #include "dirvote.h" #include "policies.h" @@ -2373,7 +2374,7 @@ networkstatus_verify_bw_weights(networkstatus_t *ns) const char *casename = NULL; int valid = 1; - weight_scale = networkstatus_get_param(ns, "bwweightscale", BW_WEIGHT_SCALE); + weight_scale = circuit_build_times_get_bw_scale(ns); Wgg = networkstatus_get_bw_weight(ns, "Wgg", -1); Wgm = networkstatus_get_bw_weight(ns, "Wgm", -1); Wgd = networkstatus_get_bw_weight(ns, "Wgd", -1); diff --git a/src/test/test_dir.c b/src/test/test_dir.c index e61815027c..1f3beb4baa 100644 --- a/src/test/test_dir.c +++ b/src/test/test_dir.c @@ -609,8 +609,11 @@ test_dir_param_voting(void) "abcd=20 c=60 cw=500 x-yz=-9 zzzzz=101", NULL, 0, 0); smartlist_split_string(vote4.net_params, "ab=900 abcd=200 c=1 cw=51 x-yz=100", NULL, 0, 0); - test_eq(100, networkstatus_get_param(&vote4, "x-yz", 50)); - test_eq(222, networkstatus_get_param(&vote4, "foobar", 222)); + test_eq(100, networkstatus_get_param(&vote4, "x-yz", 50, 0, 300)); + test_eq(222, networkstatus_get_param(&vote4, "foobar", 222, 0, 300)); + test_eq(80, networkstatus_get_param(&vote4, "ab", 12, 0, 80)); + test_eq(-8, networkstatus_get_param(&vote4, "ab", -12, -100, -8)); + test_eq(0, networkstatus_get_param(&vote4, "foobar", 0, -100, 8)); smartlist_add(votes, &vote1); smartlist_add(votes, &vote2); From 932e5c3cf0bd890313b035a4ab00003e81adb720 Mon Sep 17 00:00:00 2001 From: Sebastian Hahn Date: Sat, 15 Jan 2011 18:32:46 +0100 Subject: [PATCH 3/6] Fix a typo spotted by Roger --- doc/spec/path-spec.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/spec/path-spec.txt b/doc/spec/path-spec.txt index 4e1bdd08cb..7c313f8ab0 100644 --- a/doc/spec/path-spec.txt +++ b/doc/spec/path-spec.txt @@ -432,7 +432,7 @@ of their choices. Min: 1 Max: 20 Effect: This value governs how many modes to use in the weighted - average calculation of Pareto paramter Xm. A value of 3 introduces + average calculation of Pareto parameter Xm. A value of 3 introduces some bias (2-5% of CDF) under ideal conditions, but allows for better performance in the event that a client chooses guard nodes of radically different performance characteristics. From b06617c9481ff577e2f0fed4264c80a718f98c29 Mon Sep 17 00:00:00 2001 From: Sebastian Hahn Date: Sat, 15 Jan 2011 19:31:23 +0100 Subject: [PATCH 4/6] Provide constant limits for all consensus params This addresses Nick's concern about doing non-constant bounds checking inside networkstatus_get_param(). --- src/or/circuitbuild.c | 33 +++++++++++++++++++++++---------- src/or/networkstatus.c | 15 ++++++++++++--- src/or/or.h | 7 +++---- 3 files changed, 38 insertions(+), 17 deletions(-) diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c index a8e9778789..3788959556 100644 --- a/src/or/circuitbuild.c +++ b/src/or/circuitbuild.c @@ -184,12 +184,19 @@ circuit_build_times_get_bw_scale(networkstatus_t *ns) static double circuit_build_times_close_quantile(void) { - return networkstatus_get_param(NULL, "cbtclosequantile", + int32_t param; + /* Cast is safe - circuit_build_times_quantile_cutoff() is capped */ + int32_t min = (int)tor_lround(100*circuit_build_times_quantile_cutoff()); + param = networkstatus_get_param(NULL, "cbtclosequantile", CBT_DEFAULT_CLOSE_QUANTILE, - /* Cast is safe, cbtquantile is capped at - * CBT_MAX_QUANTILE_CUTOFF. */ - (int)tor_lround(100*circuit_build_times_quantile_cutoff()), - CBT_MAX_CLOSE_QUANTILE) / 100.0; + CBT_MIN_CLOSE_QUANTILE, + CBT_MAX_CLOSE_QUANTILE); + if (param < min) { + log_warn(LD_DIR, "Consensus parameter cbtclosequantile is " + "too small, raising to %d", min); + param = min; + } + return param / 100.0; } static int32_t @@ -215,11 +222,17 @@ circuit_build_times_min_timeout(void) int32_t circuit_build_times_initial_timeout(void) { - int32_t num = networkstatus_get_param(NULL, "cbtinitialtimeout", - CBT_DEFAULT_TIMEOUT_INITIAL_VALUE, - circuit_build_times_min_timeout(), - CBT_MAX_TIMEOUT_INITIAL_VALUE); - return num; + int32_t min = circuit_build_times_min_timeout(); + int32_t param = networkstatus_get_param(NULL, "cbtinitialtimeout", + CBT_DEFAULT_TIMEOUT_INITIAL_VALUE, + CBT_MIN_TIMEOUT_INITIAL_VALUE, + CBT_MAX_TIMEOUT_INITIAL_VALUE); + if (param < min) { + log_warn(LD_DIR, "Consensus parameter cbtinitialtimeout is too small, " + "raising to %d", min); + param = min; + } + return param; } static int32_t diff --git a/src/or/networkstatus.c b/src/or/networkstatus.c index 50bb88bb96..687ac03fa0 100644 --- a/src/or/networkstatus.c +++ b/src/or/networkstatus.c @@ -2190,15 +2190,24 @@ int32_t networkstatus_get_bw_weight(networkstatus_t *ns, const char *weight_name, int32_t default_val) { + int32_t param; + int max; if (!ns) /* if they pass in null, go find it ourselves */ ns = networkstatus_get_latest_consensus(); if (!ns || !ns->weight_params) return default_val; - return get_net_param_from_list(ns->weight_params, weight_name, - default_val, -1, - circuit_build_times_get_bw_scale(ns)); + max = circuit_build_times_get_bw_scale(ns); + param = get_net_param_from_list(ns->weight_params, weight_name, + default_val, -1, + BW_MAX_WEIGHT_SCALE); + if (param > max) { + log_warn(LD_DIR, "Value of consensus weight %s was too large, capping " + "to %d", weight_name, max); + param = max; + } + return param; } /** Return the name of the consensus flavor flav as used to identify diff --git a/src/or/or.h b/src/or/or.h index 01ff5e89d5..acca61f7d3 100644 --- a/src/or/or.h +++ b/src/or/or.h @@ -2981,8 +2981,8 @@ typedef uint32_t build_time_t; * build in terms of CDF quantile. */ #define CBT_DEFAULT_CLOSE_QUANTILE 95 -/* Minimum value derived from cbtquantile parameter. */ -#define CBT_MAX_CLOSE_QUANTILE 99 +#define CBT_MIN_CLOSE_QUANTILE CBT_MIN_QUANTILE_CUTOFF +#define CBT_MAX_CLOSE_QUANTILE CBT_MAX_QUANTILE_CUTOFF /** * How many circuits count as recent when considering if the @@ -3027,9 +3027,8 @@ double circuit_build_times_quantile_cutoff(void); /** Initial circuit build timeout in milliseconds */ #define CBT_DEFAULT_TIMEOUT_INITIAL_VALUE (60*1000) +#define CBT_MIN_TIMEOUT_INITIAL_VALUE CBT_MIN_TIMEOUT_MIN_VALUE #define CBT_MAX_TIMEOUT_INITIAL_VALUE INT32_MAX -/* CBT_MIN_TIMEOUT_INITIAL_VALUE dependent on - * circuit_build_times_min_timeout() */ int32_t circuit_build_times_initial_timeout(void); #if CBT_DEFAULT_MAX_RECENT_TIMEOUT_COUNT < CBT_MIN_MAX_RECENT_TIMEOUT_COUNT From 0df51a7f39599aa799fd91f12e13e404a490e603 Mon Sep 17 00:00:00 2001 From: Sebastian Hahn Date: Sat, 15 Jan 2011 19:39:27 +0100 Subject: [PATCH 5/6] Tighten accepted circwindow parameters Based on discussion in bug 2317, these values seem to be sane. --- src/or/or.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/or/or.h b/src/or/or.h index acca61f7d3..22c8498b66 100644 --- a/src/or/or.h +++ b/src/or/or.h @@ -765,8 +765,8 @@ typedef enum { /** Initial value for both sides of a circuit transmission window when the * circuit is initialized. Measured in cells. */ #define CIRCWINDOW_START 1000 -#define CIRCWINDOW_START_MIN 1 -#define CIRCWINDOW_START_MAX 100000 +#define CIRCWINDOW_START_MIN 100 +#define CIRCWINDOW_START_MAX 1000 /** Amount to increment a circuit window when we get a circuit SENDME. */ #define CIRCWINDOW_INCREMENT 100 /** Initial value on both sides of a stream transmission window when the From a1860cc3f1d6224f3be40319a075471cb491e1aa Mon Sep 17 00:00:00 2001 From: Sebastian Hahn Date: Sat, 15 Jan 2011 19:49:39 +0100 Subject: [PATCH 6/6] Update the spec with the new bounds --- doc/spec/dir-spec.txt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/doc/spec/dir-spec.txt b/doc/spec/dir-spec.txt index 4ba2ee38bf..eebceeafd6 100644 --- a/doc/spec/dir-spec.txt +++ b/doc/spec/dir-spec.txt @@ -1161,14 +1161,14 @@ research indicates that a lower value would mean fewer cells in transit in the network at any given time. Obeyed by Tor 0.2.1.20 and later. - Min: 1, Max: 100000 XXX are these sane + Min: 100, Max: 1000 "CircuitPriorityHalflifeMsec" -- the halflife parameter used when weighting which circuit will send the next cell. Obeyed by Tor 0.2.2.10-alpha and later. (Versions of Tor between 0.2.2.7-alpha and 0.2.2.10-alpha recognized a "CircPriorityHalflifeMsec" parameter, but mishandled it badly.) - Min: -1, Max: 2147483647 (INT32_MAX) XXX are these sane? + Min: -1, Max: 2147483647 (INT32_MAX) "perconnbwrate" and "perconnbwburst" -- if set, each relay sets up a separate token bucket for every client OR connection,