From c1bd10411116c549326f74b09658a50bbae812b4 Mon Sep 17 00:00:00 2001 From: Roger Dingledine Date: Thu, 28 Jun 2012 14:00:01 -0400 Subject: [PATCH 01/29] Detect bug 6252 (unexpected sendme cell) I only check on circuits, not streams, since bloating your stream window past the initial circuit window can't help you much. Also, I compare to CIRCWINDOW_START_MAX so we don't have surprising races if we lower CIRCWINDOW_START for an experiment. --- changes/bug6252 | 8 ++++++++ src/or/relay.c | 14 ++++++++++++++ 2 files changed, 22 insertions(+) create mode 100644 changes/bug6252 diff --git a/changes/bug6252 b/changes/bug6252 new file mode 100644 index 0000000000..0d29203fab --- /dev/null +++ b/changes/bug6252 @@ -0,0 +1,8 @@ + o Security fixes: + - Tear down the circuit if we get an unexpected SENDME cell. Clients + could use this trick to make their circuits receive cells faster + than our flow control would have allowed, or to gum up the network, + or possibly to do targeted memory denial-of-service attacks on + entry nodes. Fixes bug 6252. Bugfix on the 54th commit on Tor -- + from July 2002, before the release of Tor 0.0.0. + diff --git a/src/or/relay.c b/src/or/relay.c index 3e418ea13f..3d261c2650 100644 --- a/src/or/relay.c +++ b/src/or/relay.c @@ -1265,11 +1265,25 @@ connection_edge_process_relay_cell(cell_t *cell, circuit_t *circ, case RELAY_COMMAND_SENDME: if (!rh.stream_id) { if (layer_hint) { + if (layer_hint->package_window + CIRCWINDOW_INCREMENT > + CIRCWINDOW_START_MAX) { + log_fn(LOG_PROTOCOL_WARN, LD_PROTOCOL, + "Bug/attack: unexpected sendme cell from exit relay. " + "Closing circ."); + return -END_CIRC_REASON_TORPROTOCOL; + } layer_hint->package_window += CIRCWINDOW_INCREMENT; log_debug(LD_APP,"circ-level sendme at origin, packagewindow %d.", layer_hint->package_window); circuit_resume_edge_reading(circ, layer_hint); } else { + if (circ->package_window + CIRCWINDOW_INCREMENT > + CIRCWINDOW_START_MAX) { + log_fn(LOG_PROTOCOL_WARN, LD_PROTOCOL, + "Bug/attack: unexpected sendme cell from client. " + "Closing circ."); + return -END_CIRC_REASON_TORPROTOCOL; + } circ->package_window += CIRCWINDOW_INCREMENT; log_debug(LD_APP, "circ-level sendme at non-origin, packagewindow %d.", From 393944dfb67fb27991e152ec67f74cebbf3a23ba Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 18 Jul 2012 10:24:17 -0400 Subject: [PATCH 02/29] Have the bug6252 commit message reflect its on-again/off-again status --- changes/{bug6252 => bug6252_again} | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) rename changes/{bug6252 => bug6252_again} (65%) diff --git a/changes/bug6252 b/changes/bug6252_again similarity index 65% rename from changes/bug6252 rename to changes/bug6252_again index 0d29203fab..f7fd00cb38 100644 --- a/changes/bug6252 +++ b/changes/bug6252_again @@ -4,5 +4,8 @@ than our flow control would have allowed, or to gum up the network, or possibly to do targeted memory denial-of-service attacks on entry nodes. Fixes bug 6252. Bugfix on the 54th commit on Tor -- - from July 2002, before the release of Tor 0.0.0. + from July 2002, before the release of Tor 0.0.0. We had committed + this patch previously, but we had to revert it because of bug 6271. + Now that 6271 is fixed, this appears to work. + From 2503cfad240bfd59b033b4380b0ebd4a8a148802 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 31 Jul 2012 10:48:35 -0400 Subject: [PATCH 03/29] Allow microdescs to be up to 2k. Partial fix for 6404. --- changes/bug6404 | 7 +++++++ src/or/dirvote.c | 2 +- 2 files changed, 8 insertions(+), 1 deletion(-) create mode 100644 changes/bug6404 diff --git a/changes/bug6404 b/changes/bug6404 new file mode 100644 index 0000000000..d01964d1e5 --- /dev/null +++ b/changes/bug6404 @@ -0,0 +1,7 @@ + o Minor bugfixes: + + - Increase the maximum length of microdescriptor we are willing to + generate from 1K to 2K. Occasionally this is needed for routers + with complex policies or family declarations. Partial fix for + bug 6404; fix on 0.2.2.6-alpha. + diff --git a/src/or/dirvote.c b/src/or/dirvote.c index 79958739a5..85ea85ce73 100644 --- a/src/or/dirvote.c +++ b/src/or/dirvote.c @@ -3502,7 +3502,7 @@ dirvote_create_microdescriptor(const routerinfo_t *ri) { microdesc_t *result = NULL; char *key = NULL, *summary = NULL, *family = NULL; - char buf[1024]; + char buf[2048]; size_t keylen; char *out = buf, *end = buf+sizeof(buf); From 7143d112a69806bde4a29bf8da94704cfb435fe3 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 31 Jul 2012 10:54:14 -0400 Subject: [PATCH 04/29] Don't include a router in an md consensus if we can't find a md for it. The spec requires that every router in a microdesc consensus have an m line; we weren't obeying that spec. This creates a new consensus method (13) to allow voting to continue to work right. Partial fix for bug 6404; fix on 0.2.2.6-alpha. --- changes/bug6404 | 5 +++++ src/or/dirvote.c | 13 ++++++++++++- 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/changes/bug6404 b/changes/bug6404 index d01964d1e5..64d459418f 100644 --- a/changes/bug6404 +++ b/changes/bug6404 @@ -5,3 +5,8 @@ with complex policies or family declarations. Partial fix for bug 6404; fix on 0.2.2.6-alpha. + - Authorities no longer include any router in their + microdescriptor consensuses for which they couldn't generate or + agree on a microdescriptor. Partial fix for bug 6404; fix on + 0.2.2.6-alpha. + diff --git a/src/or/dirvote.c b/src/or/dirvote.c index 85ea85ce73..87af5becf7 100644 --- a/src/or/dirvote.c +++ b/src/or/dirvote.c @@ -54,7 +54,7 @@ static int dirvote_publish_consensus(void); static char *make_consensus_method_list(int low, int high, const char *sep); /** The highest consensus method that we currently support. */ -#define MAX_SUPPORTED_CONSENSUS_METHOD 12 +#define MAX_SUPPORTED_CONSENSUS_METHOD 13 /** Lowest consensus method that contains a 'directory-footer' marker */ #define MIN_METHOD_FOR_FOOTER 9 @@ -72,6 +72,10 @@ static char *make_consensus_method_list(int low, int high, const char *sep); * for a param. */ #define MIN_METHOD_FOR_MAJORITY_PARAMS 12 +/** Lowest consensus method where microdesc consensuses omit any entry + * with no microdesc. */ +#define MIN_METHOD_FOR_MANDATORY_MICRODESC 13 + /* ===== * Voting * =====*/ @@ -1935,6 +1939,13 @@ networkstatus_compute_consensus(smartlist_t *votes, } } + if (flavor == FLAV_MICRODESC && + consensus_method >= MIN_METHOD_FOR_MANDATORY_MICRODESC && + tor_digest256_is_zero(microdesc_digest)) { + /* With no microdescriptor digest, we omit the entry entirely. */ + continue; + } + { char buf[4096]; /* Okay!! Now we can write the descriptor... */ From 063138e001d3608b1e8b6619661d906cfea74ae0 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 31 Jul 2012 11:00:18 -0400 Subject: [PATCH 05/29] Warn at parse time for routerstatus entry missing a microdesc consensus In 0.2.3.18-rc, we started warning on this case while building a list of missing microdescriptor digests. That turned out to spam the logs; instead let's warn at parse time. Partial fix for bug 6404. --- changes/bug6404 | 4 ++++ src/or/microdesc.c | 7 +------ src/or/routerparse.c | 5 +++++ 3 files changed, 10 insertions(+), 6 deletions(-) diff --git a/changes/bug6404 b/changes/bug6404 index 64d459418f..77081481ce 100644 --- a/changes/bug6404 +++ b/changes/bug6404 @@ -10,3 +10,7 @@ agree on a microdescriptor. Partial fix for bug 6404; fix on 0.2.2.6-alpha. + - Move log message when unable to find a microdesc in a + routerstatus entry to parse time. Previously we'd spam this + warning every time we tried to figure out which microdescriptors + to download. Partial fix for bug 6404; fix on 0.2.3.18-rc. diff --git a/src/or/microdesc.c b/src/or/microdesc.c index 9395a9a051..c1ac1c3758 100644 --- a/src/or/microdesc.c +++ b/src/or/microdesc.c @@ -643,13 +643,8 @@ microdesc_list_missing_digest256(networkstatus_t *ns, microdesc_cache_t *cache, continue; if (skip && digestmap_get(skip, rs->descriptor_digest)) continue; - if (tor_mem_is_zero(rs->descriptor_digest, DIGEST256_LEN)) { - log_info(LD_BUG, "Found an entry in networkstatus with no " - "microdescriptor digest. (Router %s=%s at %s:%d.)", - rs->nickname, hex_str(rs->identity_digest, DIGEST_LEN), - fmt_addr32(rs->addr), rs->or_port); + if (tor_mem_is_zero(rs->descriptor_digest, DIGEST256_LEN)) continue; - } /* XXXX Also skip if we're a noncache and wouldn't use this router. * XXXX NM Microdesc */ diff --git a/src/or/routerparse.c b/src/or/routerparse.c index 4231a17c67..8b69ad1d13 100644 --- a/src/or/routerparse.c +++ b/src/or/routerparse.c @@ -2201,6 +2201,11 @@ routerstatus_parse_entry_from_string(memarea_t *area, escaped(tok->args[0])); goto err; } + } else { + log_info(LD_BUG, "Found an entry in networkstatus with no " + "microdescriptor digest. (Router %s=%s at %s:%d.)", + rs->nickname, hex_str(rs->identity_digest, DIGEST_LEN), + fmt_addr32(rs->addr), rs->or_port); } } From d3e1e458e11715a9fea2fea2cb228ff925c72851 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 31 Jul 2012 13:12:07 -0400 Subject: [PATCH 06/29] Remove the upper limit on the size of MD we can generate. --- src/or/dirvote.c | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/src/or/dirvote.c b/src/or/dirvote.c index 87af5becf7..dc1ba7e8c4 100644 --- a/src/or/dirvote.c +++ b/src/or/dirvote.c @@ -3513,9 +3513,9 @@ dirvote_create_microdescriptor(const routerinfo_t *ri) { microdesc_t *result = NULL; char *key = NULL, *summary = NULL, *family = NULL; - char buf[2048]; size_t keylen; - char *out = buf, *end = buf+sizeof(buf); + smartlist_t *chunks = smartlist_new(); + char *output; if (crypto_pk_write_public_key_to_string(ri->onion_pkey, &key, &keylen)<0) goto done; @@ -3523,23 +3523,19 @@ dirvote_create_microdescriptor(const routerinfo_t *ri) if (ri->declared_family) family = smartlist_join_strings(ri->declared_family, " ", 0, NULL); - if (tor_snprintf(out, end-out, "onion-key\n%s", key)<0) - goto done; - out += strlen(out); - if (family) { - if (tor_snprintf(out, end-out, "family %s\n", family)<0) - goto done; - out += strlen(out); - } - if (summary && strcmp(summary, "reject 1-65535")) { - if (tor_snprintf(out, end-out, "p %s\n", summary)<0) - goto done; - out += strlen(out); - } - *out = '\0'; /* Make sure it's nul-terminated. This should be a no-op */ + smartlist_add_asprintf(chunks, "onion-key\n%s", key); + + if (family) + smartlist_add_asprintf(chunks, "family %s\n", family); + + if (summary && strcmp(summary, "reject 1-65535")) + smartlist_add_asprintf(chunks, "p %s\n", summary); + + output = smartlist_join_strings(chunks, "", 0, NULL); { - smartlist_t *lst = microdescs_parse_from_string(buf, out, 0, 1); + smartlist_t *lst = microdescs_parse_from_string(output, + output+strlen(output), 0, 1); if (smartlist_len(lst) != 1) { log_warn(LD_DIR, "We generated a microdescriptor we couldn't parse."); SMARTLIST_FOREACH(lst, microdesc_t *, md, microdesc_free(md)); @@ -3554,6 +3550,10 @@ dirvote_create_microdescriptor(const routerinfo_t *ri) tor_free(key); tor_free(summary); tor_free(family); + if (chunks) { + SMARTLIST_FOREACH(chunks, char *, cp, tor_free(cp)); + smartlist_free(chunks); + } return result; } From 2d6d5db2fe4f8a4538c5bed903f7e1576e9eea40 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 1 Aug 2012 17:25:34 -0400 Subject: [PATCH 07/29] Defensive programming: clear rs_out between iterations. I can't currently find a bug here, but there are a couple of near-misses. Addresses ticket 6514; reported pseudonymously on IRC. --- changes/bug6514 | 5 +++++ src/or/dirvote.c | 1 + 2 files changed, 6 insertions(+) create mode 100644 changes/bug6514 diff --git a/changes/bug6514 b/changes/bug6514 new file mode 100644 index 0000000000..84633bd279 --- /dev/null +++ b/changes/bug6514 @@ -0,0 +1,5 @@ + o Minor bugfixes: + - Add a (probably redundant) memory clear between iterations of + the router status voting loop, to prevent future coding errors + where data might leak between iterations of the loop. Resolves + ticket 6514. diff --git a/src/or/dirvote.c b/src/or/dirvote.c index 79958739a5..bc7797355c 100644 --- a/src/or/dirvote.c +++ b/src/or/dirvote.c @@ -1752,6 +1752,7 @@ networkstatus_compute_consensus(smartlist_t *votes, rs = compute_routerstatus_consensus(matching_descs, consensus_method, microdesc_digest); /* Copy bits of that into rs_out. */ + memset(&rs_out, 0, sizeof(rs_out)); tor_assert(fast_memeq(lowest_id, rs->status.identity_digest,DIGEST_LEN)); memcpy(rs_out.identity_digest, lowest_id, DIGEST_LEN); memcpy(rs_out.descriptor_digest, rs->status.descriptor_digest, From dfe03d36c8749eb07e9bb2ea47e88ff05e9e3127 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Thu, 9 Aug 2012 15:48:43 -0400 Subject: [PATCH 08/29] Don't infer we have a FooPort from the presence of a FooPort line Thanks to the changes we started making with SocksPort and friends in 0.2.3.3-alpha, any of our code that did "if (options->Sockport)" became wrong, since "SocksPort 0" would make that test true whereas using the default SocksPort value would make it false. (We didn't actually do "if (options->SockPort)" but we did have tests for TransPort. When we moved DirPort, ORPort, and ControlPort over to the same system in 0.2.3.9-alpha, the problem got worse, since our code is littered with checks for DirPort and ORPort as booleans. This code renames the current linelist-based FooPort options to FooPort_lines, and adds new FooPort_set options which get set at parse-and-validate time on the or_options_t. FooPort_set is true iff we will actually try to open a listener of the given type. (I renamed the FooPort options rather than leave them alone so that every previous user of a FooPort would need to get inspected, and so that any new code that forgetfully uses FooPort will need fail to compile.) Fix for bug 6507. --- changes/bug6507 | 7 +++ src/or/config.c | 127 ++++++++++++++++++++++++++++--------------- src/or/directory.c | 2 +- src/or/directory.h | 2 +- src/or/dirserv.c | 12 ++-- src/or/or.h | 33 ++++++++--- src/or/router.c | 5 +- src/or/routerparse.c | 2 +- 8 files changed, 129 insertions(+), 61 deletions(-) create mode 100644 changes/bug6507 diff --git a/changes/bug6507 b/changes/bug6507 new file mode 100644 index 0000000000..5ca02984bd --- /dev/null +++ b/changes/bug6507 @@ -0,0 +1,7 @@ + o Major bugfixes: + - Detect 'ORPort 0' as meaning, uniformly, that we're not running + as a server. Previously, some of our code would treat the + presence of any ORPort line as meaning that we should act like a + server, even though our new listener code would correctly not + open any ORPorts for ORPort 0. Similar bugs in other Port + options are also fixed. Fixes bug 6507; bugfix on 0.2.3.3-alpha. diff --git a/src/or/config.c b/src/or/config.c index a369297518..7e67895af5 100644 --- a/src/or/config.c +++ b/src/or/config.c @@ -168,6 +168,9 @@ typedef struct config_var_t { /** An entry for config_vars: "The option name is obsolete." */ #define OBSOLETE(name) { name, CONFIG_TYPE_OBSOLETE, 0, NULL } +#define VPORT(member,conftype,initvalue) \ + VAR(#member, conftype, member ## _lines, initvalue) + /** Array of configuration options. Until we disallow nonstandard * abbreviations, order is significant, since the first matching option will * be chosen first. @@ -229,7 +232,7 @@ static config_var_t _option_vars[] = { V(ConstrainedSockSize, MEMUNIT, "8192"), V(ContactInfo, STRING, NULL), V(ControlListenAddress, LINELIST, NULL), - V(ControlPort, LINELIST, NULL), + VPORT(ControlPort, LINELIST, NULL), V(ControlPortFileGroupReadable,BOOL, "0"), V(ControlPortWriteToFile, FILENAME, NULL), V(ControlSocket, LINELIST, NULL), @@ -246,7 +249,7 @@ static config_var_t _option_vars[] = { V(DirListenAddress, LINELIST, NULL), OBSOLETE("DirFetchPeriod"), V(DirPolicy, LINELIST, NULL), - V(DirPort, LINELIST, NULL), + VPORT(DirPort, LINELIST, NULL), V(DirPortFrontPage, FILENAME, NULL), OBSOLETE("DirPostPeriod"), OBSOLETE("DirRecordUsageByCountry"), @@ -259,7 +262,7 @@ static config_var_t _option_vars[] = { V(DisableDebuggerAttachment, BOOL, "1"), V(DisableIOCP, BOOL, "1"), V(DynamicDHGroups, BOOL, "0"), - V(DNSPort, LINELIST, NULL), + VPORT(DNSPort, LINELIST, NULL), V(DNSListenAddress, LINELIST, NULL), V(DownloadExtraInfo, BOOL, "0"), V(EnforceDistinctSubnets, BOOL, "1"), @@ -345,7 +348,7 @@ static config_var_t _option_vars[] = { V(NewCircuitPeriod, INTERVAL, "30 seconds"), VAR("NamingAuthoritativeDirectory",BOOL, NamingAuthoritativeDir, "0"), V(NATDListenAddress, LINELIST, NULL), - V(NATDPort, LINELIST, NULL), + VPORT(NATDPort, LINELIST, NULL), V(Nickname, STRING, NULL), V(WarnUnsafeSocks, BOOL, "1"), OBSOLETE("NoPublish"), @@ -353,7 +356,7 @@ static config_var_t _option_vars[] = { V(NumCPUs, UINT, "0"), V(NumEntryGuards, UINT, "3"), V(ORListenAddress, LINELIST, NULL), - V(ORPort, LINELIST, NULL), + VPORT(ORPort, LINELIST, NULL), V(OutboundBindAddress, STRING, NULL), V(PathBiasCircThreshold, INT, "-1"), @@ -406,7 +409,7 @@ static config_var_t _option_vars[] = { V(ShutdownWaitLength, INTERVAL, "30 seconds"), V(SocksListenAddress, LINELIST, NULL), V(SocksPolicy, LINELIST, NULL), - V(SocksPort, LINELIST, NULL), + VPORT(SocksPort, LINELIST, NULL), V(SocksTimeout, INTERVAL, "2 minutes"), OBSOLETE("StatusFetchPeriod"), V(StrictNodes, BOOL, "0"), @@ -419,7 +422,7 @@ static config_var_t _option_vars[] = { V(TrackHostExitsExpire, INTERVAL, "30 minutes"), OBSOLETE("TrafficShaping"), V(TransListenAddress, LINELIST, NULL), - V(TransPort, LINELIST, NULL), + VPORT(TransPort, LINELIST, NULL), V(TunnelDirConns, BOOL, "1"), V(UpdateBridgesFromAuthority, BOOL, "0"), V(UseBridges, BOOL, "0"), @@ -622,7 +625,7 @@ static int parse_dir_server_line(const char *line, dirinfo_type_t required_type, int validate_only); static void port_cfg_free(port_cfg_t *port); -static int parse_ports(const or_options_t *options, int validate_only, +static int parse_ports(or_options_t *options, int validate_only, char **msg_out, int *n_ports_out); static int check_server_ports(const smartlist_t *ports, const or_options_t *options); @@ -1167,7 +1170,7 @@ options_act_reversible(const or_options_t *old_options, char **msg) #if defined(HAVE_NET_IF_H) && defined(HAVE_NET_PFVAR_H) /* Open /dev/pf before dropping privileges. */ - if (options->TransPort) { + if (options->TransPort_set) { if (get_pf_socket() < 0) { *msg = tor_strdup("Unable to open /dev/pf for transparent proxy."); goto rollback; @@ -1650,7 +1653,7 @@ options_act(const or_options_t *old_options) int was_relay = 0; if (options->BridgeRelay) { time_t int_start = time(NULL); - if (config_lines_eq(old_options->ORPort, options->ORPort)) { + if (config_lines_eq(old_options->ORPort_lines, options->ORPort_lines)) { int_start += RELAY_BRIDGE_STATS_DELAY; was_relay = 1; } @@ -1734,7 +1737,7 @@ options_act(const or_options_t *old_options) } else { options->DirReqStatistics = 0; /* Don't warn Tor clients, they don't use statistics */ - if (options->ORPort) + if (options->ORPort_set) log_notice(LD_CONFIG, "Configured to measure directory request " "statistics, but no GeoIP database found. " "Please specify a GeoIP database using the " @@ -3448,7 +3451,8 @@ options_validate(or_options_t *old_options, or_options_t *options, "Tor will still run, but probably won't do anything."); #ifndef USE_TRANSPARENT - if (options->TransPort || options->TransListenAddress) + /* XXXX024 I think we can remove this TransListenAddress */ + if (options->TransPort_set || options->TransListenAddress) REJECT("TransPort and TransListenAddress are disabled in this build."); #endif @@ -3518,10 +3522,10 @@ options_validate(or_options_t *old_options, or_options_t *options, } } - if (options->AuthoritativeDir && !options->DirPort) + if (options->AuthoritativeDir && !options->DirPort_set) REJECT("Running as authoritative directory, but no DirPort set."); - if (options->AuthoritativeDir && !options->ORPort) + if (options->AuthoritativeDir && !options->ORPort_set) REJECT("Running as authoritative directory, but no ORPort set."); if (options->AuthoritativeDir && options->ClientOnly) @@ -3708,11 +3712,12 @@ options_validate(or_options_t *old_options, or_options_t *options, "PublishServerDescriptor line."); } - if (options->BridgeRelay && options->DirPort) { + if (options->BridgeRelay && options->DirPort_set) { log_warn(LD_CONFIG, "Can't set a DirPort on a bridge relay; disabling " "DirPort"); - config_free_lines(options->DirPort); - options->DirPort = NULL; + config_free_lines(options->DirPort_lines); + options->DirPort_lines = NULL; + options->DirPort_set = 0; } if (options->MinUptimeHidServDirectoryV2 < 0) { @@ -3987,7 +3992,7 @@ options_validate(or_options_t *old_options, or_options_t *options, } } - if (options->ControlPort && !options->HashedControlPassword && + if (options->ControlPort_set && !options->HashedControlPassword && !options->HashedControlSessionPassword && !options->CookieAuthentication) { log_warn(LD_CONFIG, "ControlPort is open, but no authentication method " @@ -4067,7 +4072,7 @@ options_validate(or_options_t *old_options, or_options_t *options, MIN_CONSTRAINED_TCP_BUFFER, MAX_CONSTRAINED_TCP_BUFFER); return -1; } - if (options->DirPort) { + if (options->DirPort_set) { /* Providing cached directory entries while system TCP buffers are scarce * will exacerbate the socket errors. Suggest that this be disabled. */ COMPLAIN("You have requested constrained socket buffers while also " @@ -4226,7 +4231,7 @@ options_validate(or_options_t *old_options, or_options_t *options, } }); - if (options->BridgeRelay == 1 && ! options->ORPort) + if (options->BridgeRelay == 1 && ! options->ORPort_set) REJECT("BridgeRelay is 1, ORPort is not set. This is an invalid " "combination."); @@ -4326,7 +4331,7 @@ options_transition_affects_workers(const or_options_t *old_options, { if (!opt_streq(old_options->DataDirectory, new_options->DataDirectory) || old_options->NumCPUs != new_options->NumCPUs || - !config_lines_eq(old_options->ORPort, new_options->ORPort) || + !config_lines_eq(old_options->ORPort_lines, new_options->ORPort_lines) || old_options->ServerDNSSearchDomains != new_options->ServerDNSSearchDomains || old_options->_SafeLogging != new_options->_SafeLogging || @@ -4356,8 +4361,8 @@ options_transition_affects_descriptor(const or_options_t *old_options, !config_lines_eq(old_options->ExitPolicy,new_options->ExitPolicy) || old_options->ExitPolicyRejectPrivate != new_options->ExitPolicyRejectPrivate || - !config_lines_eq(old_options->ORPort, new_options->ORPort) || - !config_lines_eq(old_options->DirPort, new_options->DirPort) || + !config_lines_eq(old_options->ORPort_lines, new_options->ORPort_lines) || + !config_lines_eq(old_options->DirPort_lines, new_options->DirPort_lines) || old_options->ClientOnly != new_options->ClientOnly || old_options->DisableNetwork != new_options->DisableNetwork || old_options->_PublishServerDescriptor != @@ -5643,13 +5648,13 @@ warn_nonlocal_controller_ports(smartlist_t *ports, unsigned forbid) */ static int parse_port_config(smartlist_t *out, - const config_line_t *ports, - const config_line_t *listenaddrs, - const char *portname, - int listener_type, - const char *defaultaddr, - int defaultport, - unsigned flags) + const config_line_t *ports, + const config_line_t *listenaddrs, + const char *portname, + int listener_type, + const char *defaultaddr, + int defaultport, + unsigned flags) { smartlist_t *elts; int retval = -1; @@ -5687,7 +5692,7 @@ parse_port_config(smartlist_t *out, if (mainport == 0) { if (allow_spurious_listenaddr) - return 1; + return 1; /*DOCDOC*/ log_warn(LD_CONFIG, "%sPort must be defined if %sListenAddress is used", portname, portname); return -1; @@ -5968,16 +5973,34 @@ parse_unix_socket_config(smartlist_t *out, const config_line_t *cfg, return 0; } +/** Return the number of ports which are actually going to listen with type + * listenertype. Do not count no_listen ports. Do not count unix + * sockets. */ +static int +count_real_listeners(const smartlist_t *ports, int listenertype) +{ + int n = 0; + SMARTLIST_FOREACH_BEGIN(ports, port_cfg_t *, port) { + if (port->no_listen || port->is_unix_addr) + continue; + if (port->type != listenertype) + continue; + ++n; + } SMARTLIST_FOREACH_END(port); + return n; +} + /** Parse all client port types (Socks, DNS, Trans, NATD) from - * options. On success, set *n_ports_out to the number of - * ports that are listed and return 0. On failure, set *msg to a + * options. On success, set *n_ports_out to the number + * of ports that are listed, update the *Port_set values in + * options, and return 0. On failure, set *msg to a * description of the problem and return -1. * * If validate_only is false, set configured_client_ports to the * new list of ports parsed from options. **/ static int -parse_ports(const or_options_t *options, int validate_only, +parse_ports(or_options_t *options, int validate_only, char **msg, int *n_ports_out) { smartlist_t *ports; @@ -5988,7 +6011,7 @@ parse_ports(const or_options_t *options, int validate_only, *n_ports_out = 0; if (parse_port_config(ports, - options->SocksPort, options->SocksListenAddress, + options->SocksPort_lines, options->SocksListenAddress, "Socks", CONN_TYPE_AP_LISTENER, "127.0.0.1", 9050, CL_PORT_WARN_NONLOCAL|CL_PORT_ALLOW_EXTRA_LISTENADDR) < 0) { @@ -5996,7 +6019,7 @@ parse_ports(const or_options_t *options, int validate_only, goto err; } if (parse_port_config(ports, - options->DNSPort, options->DNSListenAddress, + options->DNSPort_lines, options->DNSListenAddress, "DNS", CONN_TYPE_AP_DNS_LISTENER, "127.0.0.1", 0, CL_PORT_WARN_NONLOCAL) < 0) { @@ -6004,7 +6027,7 @@ parse_ports(const or_options_t *options, int validate_only, goto err; } if (parse_port_config(ports, - options->TransPort, options->TransListenAddress, + options->TransPort_lines, options->TransListenAddress, "Trans", CONN_TYPE_AP_TRANS_LISTENER, "127.0.0.1", 0, CL_PORT_WARN_NONLOCAL) < 0) { @@ -6012,7 +6035,7 @@ parse_ports(const or_options_t *options, int validate_only, goto err; } if (parse_port_config(ports, - options->NATDPort, options->NATDListenAddress, + options->NATDPort_lines, options->NATDListenAddress, "NATD", CONN_TYPE_AP_NATD_LISTENER, "127.0.0.1", 0, CL_PORT_WARN_NONLOCAL) < 0) { @@ -6028,7 +6051,7 @@ parse_ports(const or_options_t *options, int validate_only, control_port_flags |= CL_PORT_FORBID_NONLOCAL; if (parse_port_config(ports, - options->ControlPort, options->ControlListenAddress, + options->ControlPort_lines, options->ControlListenAddress, "Control", CONN_TYPE_CONTROL_LISTENER, "127.0.0.1", 0, control_port_flags) < 0) { @@ -6045,7 +6068,7 @@ parse_ports(const or_options_t *options, int validate_only, } if (! options->ClientOnly) { if (parse_port_config(ports, - options->ORPort, options->ORListenAddress, + options->ORPort_lines, options->ORListenAddress, "OR", CONN_TYPE_OR_LISTENER, "0.0.0.0", 0, CL_PORT_SERVER_OPTIONS) < 0) { @@ -6053,7 +6076,7 @@ parse_ports(const or_options_t *options, int validate_only, goto err; } if (parse_port_config(ports, - options->DirPort, options->DirListenAddress, + options->DirPort_lines, options->DirListenAddress, "Dir", CONN_TYPE_DIR_LISTENER, "0.0.0.0", 0, CL_PORT_SERVER_OPTIONS) < 0) { @@ -6069,6 +6092,25 @@ parse_ports(const or_options_t *options, int validate_only, *n_ports_out = smartlist_len(ports); + retval = 0; + + /* Update the *Port_set options. The !! here is to force a boolean out of + an integer. */ + options->ORPort_set = + !! count_real_listeners(ports, CONN_TYPE_OR_LISTENER); + options->SocksPort_set = + !! count_real_listeners(ports, CONN_TYPE_AP_LISTENER); + options->TransPort_set = + !! count_real_listeners(ports, CONN_TYPE_AP_TRANS_LISTENER); + options->NATDPort_set = + !! count_real_listeners(ports, CONN_TYPE_AP_NATD_LISTENER); + options->ControlPort_set = + !! count_real_listeners(ports, CONN_TYPE_CONTROL_LISTENER); + options->DirPort_set = + !! count_real_listeners(ports, CONN_TYPE_DIR_LISTENER); + options->DNSPort_set = + !! count_real_listeners(ports, CONN_TYPE_AP_DNS_LISTENER); + if (!validate_only) { if (configured_ports) { SMARTLIST_FOREACH(configured_ports, @@ -6079,7 +6121,6 @@ parse_ports(const or_options_t *options, int validate_only, ports = NULL; /* prevent free below. */ } - retval = 0; err: if (ports) { SMARTLIST_FOREACH(ports, port_cfg_t *, p, port_cfg_free(p)); @@ -6620,7 +6661,7 @@ init_libevent(const or_options_t *options) suppress_libevent_log_msg(NULL); tor_check_libevent_version(tor_libevent_get_method(), - get_options()->ORPort != NULL, + server_mode(get_options()), &badness); if (badness) { const char *v = tor_libevent_get_version_str(); diff --git a/src/or/directory.c b/src/or/directory.c index f58aab256b..40344e6b05 100644 --- a/src/or/directory.c +++ b/src/or/directory.c @@ -3732,7 +3732,7 @@ download_status_reset(download_status_t *dls) const int *schedule; size_t schedule_len; - find_dl_schedule_and_len(dls, get_options()->DirPort != NULL, + find_dl_schedule_and_len(dls, get_options()->DirPort_set, &schedule, &schedule_len); dls->n_download_failures = 0; diff --git a/src/or/directory.h b/src/or/directory.h index 584fe84311..1ca1c5a6e0 100644 --- a/src/or/directory.h +++ b/src/or/directory.h @@ -80,7 +80,7 @@ time_t download_status_increment_failure(download_status_t *dls, * the optional status code sc. */ #define download_status_failed(dls, sc) \ download_status_increment_failure((dls), (sc), NULL, \ - get_options()->DirPort!=NULL, time(NULL)) + get_options()->DirPort_set, time(NULL)) void download_status_reset(download_status_t *dls); static int download_status_is_ready(download_status_t *dls, time_t now, diff --git a/src/or/dirserv.c b/src/or/dirserv.c index e21f5113f2..f1c9c6232d 100644 --- a/src/or/dirserv.c +++ b/src/or/dirserv.c @@ -1214,7 +1214,7 @@ directory_fetches_from_authorities(const or_options_t *options) return 1; /* we don't know our IP address; ask an authority. */ refuseunknown = ! router_my_exit_policy_is_reject_star() && should_refuse_unknown_exits(options); - if (options->DirPort == NULL && !refuseunknown) + if (!options->DirPort_set && !refuseunknown) return 0; if (!server_mode(options) || !advertised_server_mode()) return 0; @@ -1250,7 +1250,7 @@ directory_fetches_dir_info_later(const or_options_t *options) int directory_caches_v2_dir_info(const or_options_t *options) { - return options->DirPort != NULL; + return options->DirPort_set; } /** Return true iff we want to fetch and keep certificates for authorities @@ -1259,7 +1259,7 @@ directory_caches_v2_dir_info(const or_options_t *options) int directory_caches_unknown_auth_certs(const or_options_t *options) { - return options->DirPort || options->BridgeRelay; + return options->DirPort_set || options->BridgeRelay; } /** Return 1 if we want to keep descriptors, networkstatuses, etc around @@ -1268,7 +1268,7 @@ directory_caches_unknown_auth_certs(const or_options_t *options) int directory_caches_dir_info(const or_options_t *options) { - if (options->BridgeRelay || options->DirPort) + if (options->BridgeRelay || options->DirPort_set) return 1; if (!server_mode(options) || !advertised_server_mode()) return 0; @@ -1284,7 +1284,7 @@ directory_caches_dir_info(const or_options_t *options) int directory_permits_begindir_requests(const or_options_t *options) { - return options->BridgeRelay != 0 || options->DirPort != NULL; + return options->BridgeRelay != 0 || options->DirPort_set; } /** Return 1 if we want to allow controllers to ask us directory @@ -1293,7 +1293,7 @@ directory_permits_begindir_requests(const or_options_t *options) int directory_permits_controller_requests(const or_options_t *options) { - return options->DirPort != NULL; + return options->DirPort_set; } /** Return 1 if we have no need to fetch new descriptors. This generally diff --git a/src/or/or.h b/src/or/or.h index 3a53e5ed86..bb9d34035c 100644 --- a/src/or/or.h +++ b/src/or/or.h @@ -3012,19 +3012,38 @@ typedef struct { int DirAllowPrivateAddresses; char *User; /**< Name of user to run Tor as. */ char *Group; /**< Name of group to run Tor as. */ - config_line_t *ORPort; /**< Ports to listen on for OR connections. */ - config_line_t *SocksPort; /**< Ports to listen on for SOCKS connections. */ + config_line_t *ORPort_lines; /**< Ports to listen on for OR connections. */ + config_line_t *SocksPort_lines; /**< Ports to listen on for SOCKS connections. */ /** Ports to listen on for transparent pf/netfilter connections. */ - config_line_t *TransPort; - config_line_t *NATDPort; /**< Ports to listen on for transparent natd + config_line_t *TransPort_lines; + config_line_t *NATDPort_lines; /**< Ports to listen on for transparent natd * connections. */ - config_line_t *ControlPort; /**< Port to listen on for control + config_line_t *ControlPort_lines; /**< Port to listen on for control * connections. */ config_line_t *ControlSocket; /**< List of Unix Domain Sockets to listen on * for control connections. */ + int ControlSocketsGroupWritable; /**< Boolean: Are control sockets g+rw? */ - config_line_t *DirPort; /**< Port to listen on for directory connections. */ - config_line_t *DNSPort; /**< Port to listen on for DNS requests. */ + config_line_t *DirPort_lines; /**< Port to listen on for directory connections. */ + config_line_t *DNSPort_lines; /**< Port to listen on for DNS requests. */ + + /** @name port booleans + * + * Derived booleans: True iff there is a non-listener port on an AF_INET or + * AF_INET6 address of the given type configured in one of the _lines + * options above. + * + * @{ + */ + unsigned int ORPort_set : 1; + unsigned int SocksPort_set : 1; + unsigned int TransPort_set : 1; + unsigned int NATDPort_set : 1; + unsigned int ControlPort_set : 1; + unsigned int DirPort_set : 1; + unsigned int DNSPort_set : 1; + /**@}*/ + int AssumeReachable; /**< Whether to publish our descriptor regardless. */ int AuthoritativeDir; /**< Boolean: is this an authoritative directory? */ int V1AuthoritativeDir; /**< Boolean: is this an authoritative directory diff --git a/src/or/router.c b/src/or/router.c index 352c456f1f..38f1cdd495 100644 --- a/src/or/router.c +++ b/src/or/router.c @@ -797,7 +797,7 @@ int check_whether_dirport_reachable(void) { const or_options_t *options = get_options(); - return !options->DirPort || + return !options->DirPort_set || options->AssumeReachable || net_is_disabled() || can_reach_dir_port; @@ -1115,7 +1115,8 @@ int server_mode(const or_options_t *options) { if (options->ClientOnly) return 0; - return (options->ORPort || options->ORListenAddress); + /* XXXX024 I believe we can kill off ORListenAddress here.*/ + return (options->ORPort_set || options->ORListenAddress); } /** Return true iff we are trying to be a non-bridge server. diff --git a/src/or/routerparse.c b/src/or/routerparse.c index 970353a43c..3875198a2c 100644 --- a/src/or/routerparse.c +++ b/src/or/routerparse.c @@ -962,7 +962,7 @@ router_parse_runningrouters(const char *str) /* Now that we know the signature is okay, and we have a * publication time, cache the list. */ - if (get_options()->DirPort && !authdir_mode_v1(get_options())) + if (get_options()->DirPort_set && !authdir_mode_v1(get_options())) dirserv_set_cached_directory(str, published_on, 1); r = 0; From e1fb3b8d65080c9583451085b43c04e2f7adefcb Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Thu, 9 Aug 2012 16:02:57 -0400 Subject: [PATCH 09/29] Fix spaces from last patch --- src/or/config.c | 35 +++++++++++++++++++---------------- src/or/or.h | 10 ++++++---- 2 files changed, 25 insertions(+), 20 deletions(-) diff --git a/src/or/config.c b/src/or/config.c index 7e67895af5..6163599358 100644 --- a/src/or/config.c +++ b/src/or/config.c @@ -1653,7 +1653,7 @@ options_act(const or_options_t *old_options) int was_relay = 0; if (options->BridgeRelay) { time_t int_start = time(NULL); - if (config_lines_eq(old_options->ORPort_lines, options->ORPort_lines)) { + if (config_lines_eq(old_options->ORPort_lines,options->ORPort_lines)) { int_start += RELAY_BRIDGE_STATS_DELAY; was_relay = 1; } @@ -4361,8 +4361,10 @@ options_transition_affects_descriptor(const or_options_t *old_options, !config_lines_eq(old_options->ExitPolicy,new_options->ExitPolicy) || old_options->ExitPolicyRejectPrivate != new_options->ExitPolicyRejectPrivate || - !config_lines_eq(old_options->ORPort_lines, new_options->ORPort_lines) || - !config_lines_eq(old_options->DirPort_lines, new_options->DirPort_lines) || + !config_lines_eq(old_options->ORPort_lines, + new_options->ORPort_lines) || + !config_lines_eq(old_options->DirPort_lines, + new_options->DirPort_lines) || old_options->ClientOnly != new_options->ClientOnly || old_options->DisableNetwork != new_options->DisableNetwork || old_options->_PublishServerDescriptor != @@ -6019,26 +6021,26 @@ parse_ports(or_options_t *options, int validate_only, goto err; } if (parse_port_config(ports, - options->DNSPort_lines, options->DNSListenAddress, - "DNS", CONN_TYPE_AP_DNS_LISTENER, - "127.0.0.1", 0, - CL_PORT_WARN_NONLOCAL) < 0) { + options->DNSPort_lines, options->DNSListenAddress, + "DNS", CONN_TYPE_AP_DNS_LISTENER, + "127.0.0.1", 0, + CL_PORT_WARN_NONLOCAL) < 0) { *msg = tor_strdup("Invalid DNSPort/DNSListenAddress configuration"); goto err; } if (parse_port_config(ports, - options->TransPort_lines, options->TransListenAddress, - "Trans", CONN_TYPE_AP_TRANS_LISTENER, - "127.0.0.1", 0, - CL_PORT_WARN_NONLOCAL) < 0) { + options->TransPort_lines, options->TransListenAddress, + "Trans", CONN_TYPE_AP_TRANS_LISTENER, + "127.0.0.1", 0, + CL_PORT_WARN_NONLOCAL) < 0) { *msg = tor_strdup("Invalid TransPort/TransListenAddress configuration"); goto err; } if (parse_port_config(ports, - options->NATDPort_lines, options->NATDListenAddress, - "NATD", CONN_TYPE_AP_NATD_LISTENER, - "127.0.0.1", 0, - CL_PORT_WARN_NONLOCAL) < 0) { + options->NATDPort_lines, options->NATDListenAddress, + "NATD", CONN_TYPE_AP_NATD_LISTENER, + "127.0.0.1", 0, + CL_PORT_WARN_NONLOCAL) < 0) { *msg = tor_strdup("Invalid NatdPort/NatdListenAddress configuration"); goto err; } @@ -6051,7 +6053,8 @@ parse_ports(or_options_t *options, int validate_only, control_port_flags |= CL_PORT_FORBID_NONLOCAL; if (parse_port_config(ports, - options->ControlPort_lines, options->ControlListenAddress, + options->ControlPort_lines, + options->ControlListenAddress, "Control", CONN_TYPE_CONTROL_LISTENER, "127.0.0.1", 0, control_port_flags) < 0) { diff --git a/src/or/or.h b/src/or/or.h index bb9d34035c..55ad58fadc 100644 --- a/src/or/or.h +++ b/src/or/or.h @@ -3013,19 +3013,21 @@ typedef struct { char *User; /**< Name of user to run Tor as. */ char *Group; /**< Name of group to run Tor as. */ config_line_t *ORPort_lines; /**< Ports to listen on for OR connections. */ - config_line_t *SocksPort_lines; /**< Ports to listen on for SOCKS connections. */ + /** Ports to listen on for SOCKS connections. */ + config_line_t *SocksPort_lines; /** Ports to listen on for transparent pf/netfilter connections. */ config_line_t *TransPort_lines; config_line_t *NATDPort_lines; /**< Ports to listen on for transparent natd * connections. */ - config_line_t *ControlPort_lines; /**< Port to listen on for control + config_line_t *ControlPort_lines; /**< Ports to listen on for control * connections. */ config_line_t *ControlSocket; /**< List of Unix Domain Sockets to listen on * for control connections. */ int ControlSocketsGroupWritable; /**< Boolean: Are control sockets g+rw? */ - config_line_t *DirPort_lines; /**< Port to listen on for directory connections. */ - config_line_t *DNSPort_lines; /**< Port to listen on for DNS requests. */ + /** Ports to listen on for directory connections. */ + config_line_t *DirPort_lines; + config_line_t *DNSPort_lines; /**< Ports to listen on for DNS requests. */ /** @name port booleans * From d993b0448508d910be0c26096fe710b13e8af4e7 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Thu, 9 Aug 2012 16:13:03 -0400 Subject: [PATCH 10/29] Reject attempts to say FooPort and FooPort 0 in the same cfg domain --- changes/bug6507 | 8 ++++++++ src/or/config.c | 13 +++++++++++++ 2 files changed, 21 insertions(+) diff --git a/changes/bug6507 b/changes/bug6507 index 5ca02984bd..89940cbf7b 100644 --- a/changes/bug6507 +++ b/changes/bug6507 @@ -5,3 +5,11 @@ server, even though our new listener code would correctly not open any ORPorts for ORPort 0. Similar bugs in other Port options are also fixed. Fixes bug 6507; bugfix on 0.2.3.3-alpha. + + o Minor features: + + - Detect and reject attempts to specify both 'FooPort' and + 'FooPort 0' in the same configuration domain. (It's still okay + to have a FooPort in your configuration file,and use 'FooPort 0' + on the command line to disable it.) Fixes another case of + bug6507; bugfix on 0.2.3.3-alpha. diff --git a/src/or/config.c b/src/or/config.c index 6163599358..639abcdb47 100644 --- a/src/or/config.c +++ b/src/or/config.c @@ -5667,6 +5667,7 @@ parse_port_config(smartlist_t *out, const unsigned forbid_nonlocal = flags & CL_PORT_FORBID_NONLOCAL; const unsigned allow_spurious_listenaddr = flags & CL_PORT_ALLOW_EXTRA_LISTENADDR; + int got_zero_port=0, got_nonzero_port=0; /* FooListenAddress is deprecated; let's make it work like it used to work, * though. */ @@ -5919,6 +5920,11 @@ parse_port_config(smartlist_t *out, } SMARTLIST_FOREACH_END(elt); } + if (port) + got_nonzero_port = 1; + else + got_zero_port = 1; + if (out && port) { port_cfg_t *cfg = tor_malloc_zero(sizeof(port_cfg_t)); tor_addr_copy(&cfg->addr, &addr); @@ -5945,6 +5951,13 @@ parse_port_config(smartlist_t *out, warn_nonlocal_client_ports(out, portname); } + if (got_zero_port && got_nonzero_port) { + log_warn(LD_CONFIG, "You specified a nonzero %sPort along with '%sPort 0' " + "in the same configuration. Did you mean to disable %sPort or " + "not?", portname, portname, portname); + goto err; + } + retval = 0; err: SMARTLIST_FOREACH(elts, char *, cp, tor_free(cp)); From a9eed33111173d9f7c7fbff74ab1573c4c12e634 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 14 Aug 2012 03:06:47 -0400 Subject: [PATCH 11/29] Fix memory leak in dirvote_create_microdescriptor Found by George, who gets a cookie. --- src/or/dirvote.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/or/dirvote.c b/src/or/dirvote.c index dc1ba7e8c4..a7fea0a75e 100644 --- a/src/or/dirvote.c +++ b/src/or/dirvote.c @@ -3515,7 +3515,7 @@ dirvote_create_microdescriptor(const routerinfo_t *ri) char *key = NULL, *summary = NULL, *family = NULL; size_t keylen; smartlist_t *chunks = smartlist_new(); - char *output; + char *output = NULL; if (crypto_pk_write_public_key_to_string(ri->onion_pkey, &key, &keylen)<0) goto done; @@ -3547,6 +3547,7 @@ dirvote_create_microdescriptor(const routerinfo_t *ri) } done: + tor_free(output); tor_free(key); tor_free(summary); tor_free(family); From a4fbfa81b3fb732687efcefce851e2cd089ea6c1 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 14 Aug 2012 03:10:14 -0400 Subject: [PATCH 12/29] Update description of what we did to upper limit on md size Spotted by asn --- changes/bug6404 | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/changes/bug6404 b/changes/bug6404 index 77081481ce..948f00b92e 100644 --- a/changes/bug6404 +++ b/changes/bug6404 @@ -1,7 +1,7 @@ o Minor bugfixes: - - Increase the maximum length of microdescriptor we are willing to - generate from 1K to 2K. Occasionally this is needed for routers + - Remove the maximum length of microdescriptor we are willing to + generate. Occasionally this is needed for routers with complex policies or family declarations. Partial fix for bug 6404; fix on 0.2.2.6-alpha. From 959f850056f5f6a7ebbe7ec0e56da239e4f7de3e Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 15 Aug 2012 17:49:18 -0400 Subject: [PATCH 13/29] Raise the part of torrc mapaddress handling that knows wildcards This patch extracts the inner part of config_register_addressmaps -- the part that knows about detecting wildcard addresses addresses -- and makes it into a new function. The new function is deliberately not moved or reindented, so that the diff is smaller. I need this to fix bug 6244. --- src/or/config.c | 61 +++++++++++++++++++++++++++++++------------------ src/or/config.h | 6 +++++ 2 files changed, 45 insertions(+), 22 deletions(-) diff --git a/src/or/config.c b/src/or/config.c index a369297518..3498f6030e 100644 --- a/src/or/config.c +++ b/src/or/config.c @@ -4864,12 +4864,11 @@ config_register_addressmaps(const or_options_t *options) { smartlist_t *elts; config_line_t *opt; - char *from, *to; + const char *from, *to, *msg; addressmap_clear_configured(); elts = smartlist_new(); for (opt = options->AddressMap; opt; opt = opt->next) { - int from_wildcard = 0, to_wildcard = 0; smartlist_split_string(elts, opt->value, NULL, SPLIT_SKIP_SPACE|SPLIT_IGNORE_BLANK, 2); if (smartlist_len(elts) < 2) { @@ -4887,11 +4886,39 @@ config_register_addressmaps(const or_options_t *options) goto cleanup; } - if (!strcmp(to, "*") || !strcmp(from, "*")) { - log_warn(LD_CONFIG,"MapAddress '%s' is unsupported - can't remap from " - "or to *. Ignoring.",opt->value); + if (addressmap_register_auto(from, to, 0, ADDRMAPSRC_TORRC, &msg) < 0) { + log_warn(LD_CONFIG,"MapAddress '%s' failed: %s. Ignoring.", opt->value, + msg); goto cleanup; } + + if (smartlist_len(elts) > 2) + log_warn(LD_CONFIG,"Ignoring extra arguments to MapAddress."); + + cleanup: + SMARTLIST_FOREACH(elts, char*, cp, tor_free(cp)); + smartlist_clear(elts); + } + smartlist_free(elts); +} + +/** As addressmap_register(), but detect the wildcarded status of "from" and + * "to", and do not steal a reference to to. */ +/* XXXX024 move to connection_edge.c */ +int +addressmap_register_auto(const char *from, const char *to, + time_t expires, + addressmap_entry_source_t addrmap_source, + const char **msg) +{ + int from_wildcard = 0, to_wildcard = 0; + + *msg = "whoops, forgot the error message"; + if (1) { + if (!strcmp(to, "*") || !strcmp(from, "*")) { + *msg = "can't remap from or to *"; + return -1; + } /* Detect asterisks in expressions of type: '*.example.com' */ if (!strncmp(from,"*.",2)) { from += 2; @@ -4903,30 +4930,20 @@ config_register_addressmaps(const or_options_t *options) } if (to_wildcard && !from_wildcard) { - log_warn(LD_CONFIG, - "Skipping invalid argument '%s' to MapAddress: " - "can only use wildcard (i.e. '*.') if 'from' address " - "uses wildcard also", opt->value); - goto cleanup; + *msg = "can only use wildcard (i.e. '*.') if 'from' address " + "uses wildcard also"; + return -1; } if (address_is_invalid_destination(to, 1)) { - log_warn(LD_CONFIG, - "Skipping invalid argument '%s' to MapAddress", opt->value); - goto cleanup; + *msg = "destination is invalid"; + return -1; } - addressmap_register(from, tor_strdup(to), 0, ADDRMAPSRC_TORRC, + addressmap_register(from, tor_strdup(to), expires, addrmap_source, from_wildcard, to_wildcard); - - if (smartlist_len(elts) > 2) - log_warn(LD_CONFIG,"Ignoring extra arguments to MapAddress."); - - cleanup: - SMARTLIST_FOREACH(elts, char*, cp, tor_free(cp)); - smartlist_clear(elts); } - smartlist_free(elts); + return 0; } /** diff --git a/src/or/config.h b/src/or/config.h index ce7e319746..48f4e8630c 100644 --- a/src/or/config.h +++ b/src/or/config.h @@ -96,6 +96,12 @@ or_options_t *options_new(void); #endif void config_register_addressmaps(const or_options_t *options); +/* XXXX024 move to connection_edge.h */ +int addressmap_register_auto(const char *from, const char *to, + time_t expires, + addressmap_entry_source_t addrmap_source, + const char **msg); + #endif From 2ba52f409546961a0a950978a875b22bcb6bfaff Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 15 Aug 2012 17:59:30 -0400 Subject: [PATCH 14/29] Fix wildcarded address mappings from the control port Apparently, we weren't actually detecting wildcardedness when parsing them: whoops! bug 6244. Bugfix on 0.2.3.9-alpha --- changes/bug6244_part_c | 6 ++++++ src/or/control.c | 15 ++++++++++++--- 2 files changed, 18 insertions(+), 3 deletions(-) create mode 100644 changes/bug6244_part_c diff --git a/changes/bug6244_part_c b/changes/bug6244_part_c new file mode 100644 index 0000000000..dea6e7b69e --- /dev/null +++ b/changes/bug6244_part_c @@ -0,0 +1,6 @@ + o Major bugfixes (controller): + - Make wildcarded addresses (that is, ones beginning with *.) work when + provided via the controller's MapAddress command. Previously, they + were accepted, but we never actually noticed that they were wildcards. + Fix for bug 6244; bugfix on 0.2.3.9-alpha. + diff --git a/src/or/control.c b/src/or/control.c index ce571f99f3..913d18a7fc 100644 --- a/src/or/control.c +++ b/src/or/control.c @@ -1366,9 +1366,18 @@ handle_control_mapaddress(control_connection_t *conn, uint32_t len, smartlist_add_asprintf(reply, "250-%s=%s", address, to); } } else { - addressmap_register(from, tor_strdup(to), 1, - ADDRMAPSRC_CONTROLLER, 0, 0); - smartlist_add_asprintf(reply, "250-%s", line); + const char *msg; + if (addressmap_register_auto(from, to, 1, + ADDRMAPSRC_CONTROLLER, &msg) < 0) { + smartlist_add_asprintf(reply, + "512-syntax error: invalid address mapping " + " '%s': %s", line, msg); + log_warn(LD_CONTROL, + "Skipping invalid argument '%s' in MapAddress msg: %s", + line, msg); + } else { + smartlist_add_asprintf(reply, "250-%s", line); + } } } else { smartlist_add_asprintf(reply, "512-syntax error: mapping '%s' is " From 6a33c33a12f3af414e97b91d5718ea16f2d26673 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 15 Aug 2012 19:26:53 -0400 Subject: [PATCH 15/29] Fix warnings and 64-bit problems in openbsd-malloc code The warning fixes are: - Only define issetugid if it's missing. - Explicitly ignore the return value of writev. - Explicitly cast the retval of readlink() to int. The 64-bit problems are related to just storing a size_t in an int. Not cool! Use a size_t instead. Fix for bug 6379. Bugfix on 0.2.0.20-rc, which introduced openbsd-malloc. --- changes/bug6379 | 6 ++++++ configure.in | 1 + src/common/OpenBSD_malloc_Linux.c | 31 ++++++++++++++++++++++--------- 3 files changed, 29 insertions(+), 9 deletions(-) create mode 100644 changes/bug6379 diff --git a/changes/bug6379 b/changes/bug6379 new file mode 100644 index 0000000000..1f2b6941cd --- /dev/null +++ b/changes/bug6379 @@ -0,0 +1,6 @@ + o Minor bugfixes: + - Fix build warnings from --enable-openbsd-malloc with gcc warnings + enabled. Fixes bug 6379. + - Fix 64-bit warnings from --enable-openbsd-malloc. Fixes bug 6379. + Bugfix on 0.2.0.20-rc. + diff --git a/configure.in b/configure.in index 1db7d08e20..8aa90f6414 100644 --- a/configure.in +++ b/configure.in @@ -300,6 +300,7 @@ AC_CHECK_FUNCS( gmtime_r \ inet_aton \ ioctl \ + issetugid \ localtime_r \ lround \ memmem \ diff --git a/src/common/OpenBSD_malloc_Linux.c b/src/common/OpenBSD_malloc_Linux.c index 92ca9c0066..39c8f8b762 100644 --- a/src/common/OpenBSD_malloc_Linux.c +++ b/src/common/OpenBSD_malloc_Linux.c @@ -14,6 +14,10 @@ * ---------------------------------------------------------------------------- */ +/* We use this macro to remove some code that we don't actually want, + * rather than to fix its warnings. */ +#define BUILDING_FOR_TOR + /* * Defining MALLOC_EXTRA_SANITY will enable extra checks which are * related to internal conditions and consistency in malloc.c. This has @@ -79,6 +83,7 @@ static size_t g_alignment = 0; extern int __libc_enable_secure; +#ifndef HAVE_ISSETUGID static int issetugid(void) { if (__libc_enable_secure) return 1; @@ -86,8 +91,10 @@ static int issetugid(void) if (getgid() != getegid()) return 1; return 0; } +#endif #define PGSHIFT 12 +#undef MADV_FREE #define MADV_FREE MADV_DONTNEED #include static pthread_mutex_t gen_mutex = PTHREAD_MUTEX_INITIALIZER; @@ -456,7 +463,7 @@ wrterror(const char *p) iov[3].iov_len = strlen(p); iov[4].iov_base = (char*)"\n"; iov[4].iov_len = 1; - writev(STDERR_FILENO, iov, 5); + (void) writev(STDERR_FILENO, iov, 5); suicide = 1; #ifdef MALLOC_STATS @@ -489,8 +496,8 @@ wrtwarning(const char *p) iov[3].iov_len = strlen(p); iov[4].iov_base = (char*)"\n"; iov[4].iov_len = 1; - - writev(STDERR_FILENO, iov, 5); + + (void) writev(STDERR_FILENO, iov, 5); } #ifdef MALLOC_STATS @@ -665,7 +672,7 @@ malloc_init(void) for (i = 0; i < 3; i++) { switch (i) { case 0: - j = readlink("/etc/malloc.conf", b, sizeof b - 1); + j = (int) readlink("/etc/malloc.conf", b, sizeof b - 1); if (j <= 0) continue; b[j] = '\0'; @@ -1145,9 +1152,10 @@ malloc_bytes(size_t size) if (size == 0) j = 0; else { + size_t ii; j = 1; - i = size - 1; - while (i >>= 1) + ii = size - 1; + while (ii >>= 1) j++; } @@ -1971,6 +1979,7 @@ calloc(size_t num, size_t size) return(p); } +#ifndef BUILDING_FOR_TOR static int ispowerof2 (size_t a) { size_t b; for (b = 1ULL << (sizeof(size_t)*NBBY - 1); b > 1; b >>= 1) @@ -1978,7 +1987,9 @@ static int ispowerof2 (size_t a) { return 1; return 0; } +#endif +#ifndef BUILDING_FOR_TOR int posix_memalign(void **memptr, size_t alignment, size_t size) { void *r; @@ -2015,18 +2026,20 @@ void *valloc(size_t size) posix_memalign(&r, malloc_pagesize, size); return r; } +#endif size_t malloc_good_size(size_t size) { if (size == 0) { return 1; } else if (size <= malloc_maxsize) { - int i, j; + int j; + size_t ii; /* round up to the nearest power of 2, with same approach * as malloc_bytes() uses. */ j = 1; - i = size - 1; - while (i >>= 1) + ii = size - 1; + while (ii >>= 1) j++; return ((size_t)1) << j; } else { From ec6a7effb80af21484d83fd45d4e5eeb7c993dd4 Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Wed, 15 Aug 2012 19:59:55 -0700 Subject: [PATCH 16/29] Bug 6475: Explicitly track our path bias state. This is done to avoid spurious warns. Additional log lines are also added to try to track down the codepaths where we are somehow overcounting success counts. --- src/or/circuitbuild.c | 76 ++++++++++++++++++++++++++++++++++++------- src/or/or.h | 10 ++++++ 2 files changed, 75 insertions(+), 11 deletions(-) diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c index b82cce9881..7ac2b97824 100644 --- a/src/or/circuitbuild.c +++ b/src/or/circuitbuild.c @@ -2293,18 +2293,34 @@ circuit_send_next_onion_skin(origin_circuit_t *circ) entry_guard_get_by_id_digest(circ->_base.n_conn->identity_digest); if (guard) { - guard->circuit_successes++; + if (circ->path_state == PATH_STATE_DID_FIRST_HOP) { + circ->path_state = PATH_STATE_SUCCEEDED; + guard->circuit_successes++; - log_info(LD_PROTOCOL, "Got success count %u/%u for guard %s=%s", - guard->circuit_successes, guard->first_hops, - guard->nickname, hex_str(guard->identity, DIGEST_LEN)); + log_info(LD_PROTOCOL, "Got success count %u/%u for guard %s=%s", + guard->circuit_successes, guard->first_hops, + guard->nickname, hex_str(guard->identity, DIGEST_LEN)); + } else { + log_info(LD_BUG, + "Succeeded circuit has strange path state %d. " + "Circuit is a %s currently %s.", + circ->path_state, + circuit_purpose_to_string(circ->_base.purpose), + circuit_state_to_string(circ->_base.state)); + } if (guard->first_hops < guard->circuit_successes) { log_warn(LD_BUG, "Unexpectedly high circuit_successes (%u/%u) " - "for guard %s", + "for guard %s=%s", guard->circuit_successes, guard->first_hops, - guard->nickname); + guard->nickname, hex_str(guard->identity, DIGEST_LEN)); } + } else { + log_info(LD_BUG, + "Completed 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)); } } if (!can_complete_circuit && !circ->build_state->onehop_tunnel) { @@ -2666,8 +2682,9 @@ entry_guard_inc_first_hop_count(entry_guard_t *guard) guard->circuit_successes /= scale_factor; } guard->first_hops++; - log_info(LD_PROTOCOL, "Got success count %u/%u for guard %s", - guard->circuit_successes, guard->first_hops, guard->nickname); + log_info(LD_PROTOCOL, "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; } @@ -2690,6 +2707,16 @@ circuit_finish_handshake(origin_circuit_t *circ, uint8_t reply_type, if (circ->cpath->state == CPATH_STATE_AWAITING_KEYS) { hop = circ->cpath; + + /* Help track down the real cause of bug #6475: */ + if (circ->has_opened && circ->path_state == PATH_STATE_NEW_CIRC) { + log_info(LD_BUG, + "Opened circuit seems new. " + "Circuit is a %s currently %s.", + circuit_purpose_to_string(circ->_base.purpose), + circuit_state_to_string(circ->_base.state)); + } + /* Don't count cannibalized or onehop circs for path bias */ if (!circ->has_opened && !circ->build_state->onehop_tunnel) { entry_guard_t *guard; @@ -2697,13 +2724,40 @@ circuit_finish_handshake(origin_circuit_t *circ, uint8_t reply_type, guard = entry_guard_get_by_id_digest( circ->_base.n_conn->identity_digest); if (guard) { - if (entry_guard_inc_first_hop_count(guard) < 0) { - /* Bogus guard; we already warned. */ - return -END_CIRC_REASON_TORPROTOCOL; + if (circ->path_state == PATH_STATE_NEW_CIRC) { + circ->path_state = PATH_STATE_DID_FIRST_HOP; + + if (entry_guard_inc_first_hop_count(guard) < 0) { + /* Bogus guard; we already warned. */ + return -END_CIRC_REASON_TORPROTOCOL; + } + } else { + log_info(LD_BUG, + "Unopened circuit has strange path state %d. " + "Circuit is a %s currently %s.", + circ->path_state, + circuit_purpose_to_string(circ->_base.purpose), + circuit_state_to_string(circ->_base.state)); } + } else { + log_info(LD_BUG, + "Opened 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)); } } } else { + /* Help track down the real cause of bug #6475: */ + if (circ->path_state == PATH_STATE_NEW_CIRC) { + log_info(LD_BUG, + "New circuit is in cpath state %d (opened: %d). " + "Circuit is a %s currently %s.", + circ->cpath->state, circ->has_opened, + circuit_purpose_to_string(circ->_base.purpose), + circuit_state_to_string(circ->_base.state)); + } + hop = onion_next_hop_in_cpath(circ->cpath); if (!hop) { /* got an extended when we're all done? */ log_warn(LD_PROTOCOL,"got extended when circ already built? Closing."); diff --git a/src/or/or.h b/src/or/or.h index 3a53e5ed86..0db6137e97 100644 --- a/src/or/or.h +++ b/src/or/or.h @@ -2596,6 +2596,12 @@ typedef struct circuit_t { * circuit. */ #define MAX_RELAY_EARLY_CELLS_PER_CIRCUIT 8 +typedef enum { + PATH_STATE_NEW_CIRC = 0, + PATH_STATE_DID_FIRST_HOP = 1, + PATH_STATE_SUCCEEDED = 2, +} path_state_t; + /** An origin_circuit_t holds data necessary to build and use a circuit. */ typedef struct origin_circuit_t { @@ -2629,6 +2635,10 @@ typedef struct origin_circuit_t { * cannibalized circuits. */ unsigned int has_opened : 1; + /** 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; + /** Set iff this is a hidden-service circuit which has timed out * according to our current circuit-build timeout, but which has * been kept around because it might still succeed in connecting to From 4e42a8a2f20e501f9236452b41939964e6d942cf Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Thu, 16 Aug 2012 16:29:19 -0700 Subject: [PATCH 17/29] Address Nick's comments from code review. Also promote log messages to notice and rate-limit them. --- changes/bug6475 | 6 + src/or/circuitbuild.c | 284 +++++++++++++++++++++++++++++------------- src/or/or.h | 13 +- 3 files changed, 217 insertions(+), 86 deletions(-) create mode 100644 changes/bug6475 diff --git a/changes/bug6475 b/changes/bug6475 new file mode 100644 index 0000000000..67bab99622 --- /dev/null +++ b/changes/bug6475 @@ -0,0 +1,6 @@ + o Minor bugfixes: + - Add internal circuit construction state to protect against + the noisy warn message "Unexpectedly high circuit_successes". + Also add some additional rate-limited notice messages to help + determine the root cause of the warn. Fixes bug 6475. + Bugfix against 0.2.3.17-beta. diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c index 7ac2b97824..2fa3dd5949 100644 --- a/src/or/circuitbuild.c +++ b/src/or/circuitbuild.c @@ -135,6 +135,9 @@ static entry_guard_t *entry_guard_get_by_id_digest(const char *digest); static void bridge_free(bridge_info_t *bridge); +static int entry_guard_inc_first_hop_count(entry_guard_t *guard); +static void pathbias_count_success(origin_circuit_t *circ); + /** * This function decides if CBT learning should be disabled. It returns * true if one or more of the following four conditions are met: @@ -2285,44 +2288,11 @@ circuit_send_next_onion_skin(origin_circuit_t *circ) } log_info(LD_CIRC,"circuit built!"); circuit_reset_failure_count(0); - /* Don't count cannibalized or onehop circs for path bias */ + if (circ->build_state->onehop_tunnel || circ->has_opened) { control_event_bootstrap(BOOTSTRAP_STATUS_REQUESTING_STATUS, 0); - } else { - entry_guard_t *guard = - entry_guard_get_by_id_digest(circ->_base.n_conn->identity_digest); - - if (guard) { - if (circ->path_state == PATH_STATE_DID_FIRST_HOP) { - circ->path_state = PATH_STATE_SUCCEEDED; - guard->circuit_successes++; - - log_info(LD_PROTOCOL, "Got success count %u/%u for guard %s=%s", - guard->circuit_successes, guard->first_hops, - guard->nickname, hex_str(guard->identity, DIGEST_LEN)); - } else { - log_info(LD_BUG, - "Succeeded circuit has strange path state %d. " - "Circuit is a %s currently %s.", - circ->path_state, - circuit_purpose_to_string(circ->_base.purpose), - circuit_state_to_string(circ->_base.state)); - } - - if (guard->first_hops < guard->circuit_successes) { - log_warn(LD_BUG, "Unexpectedly high circuit_successes (%u/%u) " - "for guard %s=%s", - guard->circuit_successes, guard->first_hops, - guard->nickname, hex_str(guard->identity, DIGEST_LEN)); - } - } else { - log_info(LD_BUG, - "Completed 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)); - } } + if (!can_complete_circuit && !circ->build_state->onehop_tunnel) { const or_options_t *options = get_options(); can_complete_circuit=1; @@ -2338,6 +2308,8 @@ circuit_send_next_onion_skin(origin_circuit_t *circ) consider_testing_reachability(1, 1); } } + + pathbias_count_success(circ); circuit_rep_hist_note_result(circ); circuit_has_opened(circ); /* do other actions as necessary */ @@ -2637,6 +2609,194 @@ pathbias_get_scale_factor(const or_options_t *options) DFLT_PATH_BIAS_SCALE_THRESHOLD, 1, INT32_MAX); } +const char * +pathbias_state_to_string(path_state_t state) +{ + switch(state) { + case PATH_STATE_NEW_CIRC: + return "new"; + case PATH_STATE_DID_FIRST_HOP: + return "first hop"; + case PATH_STATE_SUCCEEDED: + return "succeeded"; + } + + return "unknown"; +} + + +/** + * Check our circuit state to see if this is a successful first hop. + * If so, record it in the current guard's path bias first_hop count. + * + * Also check for several potential error cases for bug #6475. + */ +static int +pathbias_count_first_hop(origin_circuit_t *circ) +{ +#define FIRST_HOP_NOTICE_INTERVAL (600) + static ratelim_t first_hop_notice_limit = RATELIM_INIT(FIRST_HOP_NOTICE_INTERVAL); + char *rate_msg = NULL; + + /* Completely ignore one hop circuits */ + if (circ->build_state->onehop_tunnel) { + tor_assert(circ->build_state->desired_path_len == 1); + return 0; + } + + 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) { + if ((rate_msg = rate_limit_log(&first_hop_notice_limit, + approx_time()))) { + log_notice(LD_BUG, + "Opened circuit is in strange path state %s. " + "Circuit is a %s currently %s. %s", + pathbias_state_to_string(circ->path_state), + circuit_purpose_to_string(circ->_base.purpose), + circuit_state_to_string(circ->_base.state), + rate_msg); + } + } + + /* Don't count cannibalized circs for path bias */ + if (!circ->has_opened) { + entry_guard_t *guard; + + guard = entry_guard_get_by_id_digest( + circ->_base.n_conn->identity_digest); + if (guard) { + if (circ->path_state == PATH_STATE_NEW_CIRC) { + circ->path_state = PATH_STATE_DID_FIRST_HOP; + + if (entry_guard_inc_first_hop_count(guard) < 0) { + /* Bogus guard; we already warned. */ + return -END_CIRC_REASON_TORPROTOCOL; + } + } else { + if ((rate_msg = rate_limit_log(&first_hop_notice_limit, + approx_time()))) { + log_notice(LD_BUG, + "Unopened circuit has strange path state %s. " + "Circuit is a %s currently %s. %s", + pathbias_state_to_string(circ->path_state), + circuit_purpose_to_string(circ->_base.purpose), + circuit_state_to_string(circ->_base.state), + rate_msg); + } + } + } else { + if ((rate_msg = rate_limit_log(&first_hop_notice_limit, + approx_time()))) { + log_notice(LD_BUG, + "Unopened circuit has no known guard. " + "Circuit is a %s currently %s. %s", + circuit_purpose_to_string(circ->_base.purpose), + circuit_state_to_string(circ->_base.state), + rate_msg); + } + } + } + } else { + /* Help track down the real cause of bug #6475: */ + if (circ->path_state == PATH_STATE_NEW_CIRC) { + if ((rate_msg = rate_limit_log(&first_hop_notice_limit, + approx_time()))) { + log_notice(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); + } + } + } + + return 0; +} + +/** + * Check our circuit state to see if this is a successful circuit + * completion. If so, record it in the current guard's path bias + * success count. + * + * Also check for several potential error cases for bug #6475. + */ +static void +pathbias_count_success(origin_circuit_t *circ) +{ +#define SUCCESS_NOTICE_INTERVAL (600) + static ratelim_t success_notice_limit = RATELIM_INIT(SUCCESS_NOTICE_INTERVAL); + char *rate_msg = NULL; + + /* Ignore one hop circuits */ + if (circ->build_state->onehop_tunnel) { + tor_assert(circ->build_state->desired_path_len == 1); + return; + } + + /* Don't count cannibalized/reused circs for path bias */ + if (!circ->has_opened) { + entry_guard_t *guard = + entry_guard_get_by_id_digest(circ->_base.n_conn->identity_digest); + + if (guard) { + if (circ->path_state == PATH_STATE_DID_FIRST_HOP) { + circ->path_state = PATH_STATE_SUCCEEDED; + guard->circuit_successes++; + + log_info(LD_PROTOCOL, "Got success count %u/%u for guard %s=%s", + guard->circuit_successes, guard->first_hops, + guard->nickname, hex_str(guard->identity, DIGEST_LEN)); + } else { + if ((rate_msg = rate_limit_log(&success_notice_limit, + approx_time()))) { + log_notice(LD_BUG, + "Succeeded circuit is in strange path state %s. " + "Circuit is a %s currently %s. %s", + pathbias_state_to_string(circ->path_state), + circuit_purpose_to_string(circ->_base.purpose), + circuit_state_to_string(circ->_base.state), + rate_msg); + } + } + + if (guard->first_hops < guard->circuit_successes) { + log_warn(LD_BUG, "Unexpectedly high circuit_successes (%u/%u) " + "for guard %s=%s", + guard->circuit_successes, guard->first_hops, + guard->nickname, hex_str(guard->identity, DIGEST_LEN)); + } + } else { + if ((rate_msg = rate_limit_log(&success_notice_limit, + approx_time()))) { + log_notice(LD_BUG, + "Completed circuit has no known guard. " + "Circuit is a %s currently %s. %s", + circuit_purpose_to_string(circ->_base.purpose), + circuit_state_to_string(circ->_base.state), + rate_msg); + } + } + } else { + if (circ->path_state != PATH_STATE_SUCCEEDED) { + if ((rate_msg = rate_limit_log(&success_notice_limit, + approx_time()))) { + log_notice(LD_BUG, + "Opened circuit is in strange path state %s. " + "Circuit is a %s currently %s. %s", + pathbias_state_to_string(circ->path_state), + circuit_purpose_to_string(circ->_base.purpose), + circuit_state_to_string(circ->_base.state), + rate_msg); + } + } + } +} + + /** 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 @@ -2704,60 +2864,14 @@ circuit_finish_handshake(origin_circuit_t *circ, uint8_t reply_type, { char keys[CPATH_KEY_MATERIAL_LEN]; crypt_path_t *hop; + int rv; + + if((rv = pathbias_count_first_hop(circ)) < 0) + return rv; if (circ->cpath->state == CPATH_STATE_AWAITING_KEYS) { hop = circ->cpath; - - /* Help track down the real cause of bug #6475: */ - if (circ->has_opened && circ->path_state == PATH_STATE_NEW_CIRC) { - log_info(LD_BUG, - "Opened circuit seems new. " - "Circuit is a %s currently %s.", - circuit_purpose_to_string(circ->_base.purpose), - circuit_state_to_string(circ->_base.state)); - } - - /* Don't count cannibalized or onehop circs for path bias */ - if (!circ->has_opened && !circ->build_state->onehop_tunnel) { - entry_guard_t *guard; - - guard = entry_guard_get_by_id_digest( - circ->_base.n_conn->identity_digest); - if (guard) { - if (circ->path_state == PATH_STATE_NEW_CIRC) { - circ->path_state = PATH_STATE_DID_FIRST_HOP; - - if (entry_guard_inc_first_hop_count(guard) < 0) { - /* Bogus guard; we already warned. */ - return -END_CIRC_REASON_TORPROTOCOL; - } - } else { - log_info(LD_BUG, - "Unopened circuit has strange path state %d. " - "Circuit is a %s currently %s.", - circ->path_state, - circuit_purpose_to_string(circ->_base.purpose), - circuit_state_to_string(circ->_base.state)); - } - } else { - log_info(LD_BUG, - "Opened 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)); - } - } } else { - /* Help track down the real cause of bug #6475: */ - if (circ->path_state == PATH_STATE_NEW_CIRC) { - log_info(LD_BUG, - "New circuit is in cpath state %d (opened: %d). " - "Circuit is a %s currently %s.", - circ->cpath->state, circ->has_opened, - circuit_purpose_to_string(circ->_base.purpose), - circuit_state_to_string(circ->_base.state)); - } - hop = onion_next_hop_in_cpath(circ->cpath); if (!hop) { /* got an extended when we're all done? */ log_warn(LD_PROTOCOL,"got extended when circ already built? Closing."); diff --git a/src/or/or.h b/src/or/or.h index 0db6137e97..d2cafb545a 100644 --- a/src/or/or.h +++ b/src/or/or.h @@ -2596,9 +2596,20 @@ typedef struct circuit_t { * circuit. */ #define MAX_RELAY_EARLY_CELLS_PER_CIRCUIT 8 +/** + * Describes the circuit building process in simplified terms based + * on the path bias accounting state for a circuit. Created to prevent + * overcounting due to unknown cases of circuit reuse. See Bug #6475. + */ 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 + * the path bias logic. */ PATH_STATE_DID_FIRST_HOP = 1, + /** This circuit has been completely built, and has been counted as + * successful by the path bias logic. */ PATH_STATE_SUCCEEDED = 2, } path_state_t; @@ -2637,7 +2648,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; + path_state_t path_state : 2; /** Set iff this is a hidden-service circuit which has timed out * according to our current circuit-build timeout, but which has From a74d4182f106b1b89367cf1a640b1140d3d35d2c Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 17 Aug 2012 12:10:31 -0400 Subject: [PATCH 18/29] Whitespace and build fixes on 6475 patch --- src/or/circuitbuild.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c index 2fa3dd5949..39a223b2f4 100644 --- a/src/or/circuitbuild.c +++ b/src/or/circuitbuild.c @@ -2609,14 +2609,14 @@ pathbias_get_scale_factor(const or_options_t *options) DFLT_PATH_BIAS_SCALE_THRESHOLD, 1, INT32_MAX); } -const char * +static const char * pathbias_state_to_string(path_state_t state) { - switch(state) { + switch (state) { case PATH_STATE_NEW_CIRC: - return "new"; + return "new"; case PATH_STATE_DID_FIRST_HOP: - return "first hop"; + return "first hop"; case PATH_STATE_SUCCEEDED: return "succeeded"; } @@ -2624,7 +2624,6 @@ pathbias_state_to_string(path_state_t state) return "unknown"; } - /** * Check our circuit state to see if this is a successful first hop. * If so, record it in the current guard's path bias first_hop count. @@ -2635,7 +2634,8 @@ static int pathbias_count_first_hop(origin_circuit_t *circ) { #define FIRST_HOP_NOTICE_INTERVAL (600) - static ratelim_t first_hop_notice_limit = RATELIM_INIT(FIRST_HOP_NOTICE_INTERVAL); + static ratelim_t first_hop_notice_limit = + RATELIM_INIT(FIRST_HOP_NOTICE_INTERVAL); char *rate_msg = NULL; /* Completely ignore one hop circuits */ @@ -2706,7 +2706,7 @@ pathbias_count_first_hop(origin_circuit_t *circ) "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, + circ->cpath->state, circ->has_opened, circuit_purpose_to_string(circ->_base.purpose), circuit_state_to_string(circ->_base.state), rate_msg); @@ -2728,7 +2728,8 @@ static void pathbias_count_success(origin_circuit_t *circ) { #define SUCCESS_NOTICE_INTERVAL (600) - static ratelim_t success_notice_limit = RATELIM_INIT(SUCCESS_NOTICE_INTERVAL); + static ratelim_t success_notice_limit = + RATELIM_INIT(SUCCESS_NOTICE_INTERVAL); char *rate_msg = NULL; /* Ignore one hop circuits */ @@ -2796,7 +2797,6 @@ pathbias_count_success(origin_circuit_t *circ) } } - /** 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 @@ -2866,7 +2866,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_first_hop(circ)) < 0) return rv; if (circ->cpath->state == CPATH_STATE_AWAITING_KEYS) { From 4c8fcba86c35bd8e2cfde4c65f394c7837638ff0 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 17 Aug 2012 13:49:52 -0400 Subject: [PATCH 19/29] Fix more warnings from openbsd_malloc Apparently, (void)writev is not enough to suppress the "you are ignoring the return value!" warnings on Linux. Instead, remove the whole warning/error logic when compiling openbsd_malloc for Tor: we can't use it. --- src/common/OpenBSD_malloc_Linux.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/src/common/OpenBSD_malloc_Linux.c b/src/common/OpenBSD_malloc_Linux.c index 39c8f8b762..da82729811 100644 --- a/src/common/OpenBSD_malloc_Linux.c +++ b/src/common/OpenBSD_malloc_Linux.c @@ -450,6 +450,7 @@ extern char *__progname; static void wrterror(const char *p) { +#ifndef BUILDING_FOR_TOR const char *q = " error: "; struct iovec iov[5]; @@ -463,8 +464,10 @@ wrterror(const char *p) iov[3].iov_len = strlen(p); iov[4].iov_base = (char*)"\n"; iov[4].iov_len = 1; - (void) writev(STDERR_FILENO, iov, 5); - + writev(STDERR_FILENO, iov, 5); +#else + (void)p; +#endif suicide = 1; #ifdef MALLOC_STATS if (malloc_stats) @@ -478,14 +481,17 @@ wrterror(const char *p) static void wrtwarning(const char *p) { +#ifndef BUILDING_FOR_TOR const char *q = " warning: "; struct iovec iov[5]; +#endif if (malloc_abort) wrterror(p); else if (malloc_silent) return; +#ifndef BUILDING_FOR_TOR iov[0].iov_base = __progname; iov[0].iov_len = strlen(__progname); iov[1].iov_base = (char*)malloc_func; @@ -498,6 +504,9 @@ wrtwarning(const char *p) iov[4].iov_len = 1; (void) writev(STDERR_FILENO, iov, 5); +#else + (void)p; +#endif } #ifdef MALLOC_STATS From 9ed87b37d04705983b11cd995140e8864454bf5e Mon Sep 17 00:00:00 2001 From: Linus Nordberg Date: Thu, 19 Jul 2012 20:41:23 +0200 Subject: [PATCH 20/29] Consider IPv6 OR ports when deciding whether a routerinfo change is cosmetic. Closes #6423. --- changes/bug6423 | 3 +++ src/or/routerlist.c | 2 ++ 2 files changed, 5 insertions(+) create mode 100644 changes/bug6423 diff --git a/changes/bug6423 b/changes/bug6423 new file mode 100644 index 0000000000..2ea4f1410d --- /dev/null +++ b/changes/bug6423 @@ -0,0 +1,3 @@ + o Minor features: + - Consider new, removed or changed IPv6 OR ports a non cosmetic + change. diff --git a/src/or/routerlist.c b/src/or/routerlist.c index 4979b933ad..3c39e362df 100644 --- a/src/or/routerlist.c +++ b/src/or/routerlist.c @@ -5160,6 +5160,8 @@ router_differences_are_cosmetic(const routerinfo_t *r1, const routerinfo_t *r2) if (strcasecmp(r1->address, r2->address) || strcasecmp(r1->nickname, r2->nickname) || r1->or_port != r2->or_port || + !tor_addr_eq(&r1->ipv6_addr, &r2->ipv6_addr) || + r1->ipv6_orport != r2->ipv6_orport || r1->dir_port != r2->dir_port || r1->purpose != r2->purpose || crypto_pk_cmp_keys(r1->onion_pkey, r2->onion_pkey) || From 223e7cfabec5a75a7acd919210f838511e39cb16 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 17 Aug 2012 16:46:11 -0400 Subject: [PATCH 21/29] When iterating over connections pending DNS, skip marked ones Failure to do this would lead to double-free cases and similar, especially when the exit's DNS was broken. See bug 6472 for full details; this is a fix for 6472. Anonymous patch from "cypherpunks" on trac. --- changes/bug6472 | 4 ++++ src/or/dns.c | 16 ++++++++++++---- 2 files changed, 16 insertions(+), 4 deletions(-) create mode 100644 changes/bug6472 diff --git a/changes/bug6472 b/changes/bug6472 new file mode 100644 index 0000000000..dcd42ebe68 --- /dev/null +++ b/changes/bug6472 @@ -0,0 +1,4 @@ + o Minor bugfixes: + - Avoid a pair of double-free and use-after-mark bugs that can + occur with certain timings in canceled and re-received DNS + requests. Fix for bug 6472; bugfix on 0.0.7rc1. diff --git a/src/or/dns.c b/src/or/dns.c index 3e88fad68c..78893bfbed 100644 --- a/src/or/dns.c +++ b/src/or/dns.c @@ -450,16 +450,17 @@ purge_expired_resolves(time_t now) if (resolve->pending_connections) { log_debug(LD_EXIT, "Closing pending connections on timed-out DNS resolve!"); - tor_fragile_assert(); while (resolve->pending_connections) { pend = resolve->pending_connections; resolve->pending_connections = pend->next; /* Connections should only be pending if they have no socket. */ tor_assert(!SOCKET_OK(pend->conn->_base.s)); pendconn = pend->conn; - connection_edge_end(pendconn, END_STREAM_REASON_TIMEOUT); - circuit_detach_stream(circuit_get_by_edge_conn(pendconn), pendconn); - connection_free(TO_CONN(pendconn)); + if (!pendconn->_base.marked_for_close) { + connection_edge_end(pendconn, END_STREAM_REASON_TIMEOUT); + circuit_detach_stream(circuit_get_by_edge_conn(pendconn), pendconn); + connection_free(TO_CONN(pendconn)); + } tor_free(pend); } } @@ -1091,6 +1092,13 @@ dns_found_answer(const char *address, uint8_t is_reverse, uint32_t addr, pendconn = pend->conn; /* don't pass complex things to the connection_mark_for_close macro */ assert_connection_ok(TO_CONN(pendconn),time(NULL)); + if (pendconn->_base.marked_for_close) { + /* prevent double-remove. */ + pendconn->_base.state = EXIT_CONN_STATE_RESOLVEFAILED; + resolve->pending_connections = pend->next; + tor_free(pend); + continue; + } tor_addr_from_ipv4h(&pendconn->_base.addr, addr); pendconn->address_ttl = ttl; From 88859b2ff1fdd4e5f4fab8ad818131ea3c2dd1c8 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 17 Aug 2012 17:10:03 -0400 Subject: [PATCH 22/29] whitespace fix --- src/or/config.h | 1 - 1 file changed, 1 deletion(-) diff --git a/src/or/config.h b/src/or/config.h index 48f4e8630c..dd76edcf1d 100644 --- a/src/or/config.h +++ b/src/or/config.h @@ -102,6 +102,5 @@ int addressmap_register_auto(const char *from, const char *to, addressmap_entry_source_t addrmap_source, const char **msg); - #endif From b7c172c9ec762363562220a354feefc521970d7c Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 27 Aug 2012 11:16:44 -0400 Subject: [PATCH 23/29] Disable extending to private/internal addresses by default This is important, since otherwise an attacker can use timing info to probe the internal network. Also, add an option (ExtendAllowPrivateAddresses) so that TestingTorNetwork won't break. Fix for bug 6710; bugfix on all released versions of Tor. --- changes/bug6710 | 7 +++++++ doc/tor.1.txt | 6 ++++++ src/or/circuitbuild.c | 7 +++++++ src/or/config.c | 2 ++ src/or/or.h | 4 +++- 5 files changed, 25 insertions(+), 1 deletion(-) create mode 100644 changes/bug6710 diff --git a/changes/bug6710 b/changes/bug6710 new file mode 100644 index 0000000000..69a3c7ef40 --- /dev/null +++ b/changes/bug6710 @@ -0,0 +1,7 @@ + o Major bugfixes (security): + - Reject any attempt to extend to an internal address. Without + this fix, a router could be used to probe addresses on an + internal network to see whether they were accepting + connections. Fix for bug 6710; bugfix on all released versions + of Tor. + diff --git a/doc/tor.1.txt b/doc/tor.1.txt index e7ba8485c0..7e0751b2b2 100644 --- a/doc/tor.1.txt +++ b/doc/tor.1.txt @@ -1470,6 +1470,11 @@ is non-zero): its extra-info documents that it uploads to the directory authorities. (Default: 1) +**ExtendAllowPrivateAddresses** **0**|**1**:: + When this option is enabled, Tor routers allow EXTEND request to + localhost, RFC1918 addresses, and so on. This can create security issues; + you should probably leave it off. (Default: 0) + DIRECTORY SERVER OPTIONS ------------------------ @@ -1795,6 +1800,7 @@ The following options are used for running a testing Tor network. ClientRejectInternalAddresses 0 CountPrivateBandwidth 1 ExitPolicyRejectPrivate 0 + ExtendAllowPrivateAddresses 1 V3AuthVotingInterval 5 minutes V3AuthVoteDelay 20 seconds V3AuthDistDelay 20 seconds diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c index 39a223b2f4..f11bebf7c9 100644 --- a/src/or/circuitbuild.c +++ b/src/or/circuitbuild.c @@ -2432,6 +2432,13 @@ circuit_extend(cell_t *cell, circuit_t *circ) return -1; } + if (tor_addr_is_internal(&n_addr, 0) && + !get_options()->ExtendAllowPrivateAddresses) { + log_fn(LOG_PROTOCOL_WARN, LD_PROTOCOL, + "Client asked me to extend to a private address"); + return -1; + } + /* Check if they asked us for 0000..0000. We support using * an empty fingerprint for the first hop (e.g. for a bridge relay), * but we don't want to let people send us extend cells for empty diff --git a/src/or/config.c b/src/or/config.c index 3970808fb3..d5e80cc832 100644 --- a/src/or/config.c +++ b/src/or/config.c @@ -276,6 +276,7 @@ static config_var_t _option_vars[] = { V(ExitPolicy, LINELIST, NULL), V(ExitPolicyRejectPrivate, BOOL, "1"), V(ExitPortStatistics, BOOL, "0"), + V(ExtendAllowPrivateAddresses, BOOL, "0"), V(ExtraInfoStatistics, BOOL, "1"), #if defined (WINCE) @@ -473,6 +474,7 @@ static const config_var_t testing_tor_network_defaults[] = { V(ClientRejectInternalAddresses, BOOL, "0"), V(CountPrivateBandwidth, BOOL, "1"), V(ExitPolicyRejectPrivate, BOOL, "0"), + V(ExtendAllowPrivateAddresses, BOOL, "1"), V(V3AuthVotingInterval, INTERVAL, "5 minutes"), V(V3AuthVoteDelay, INTERVAL, "20 seconds"), V(V3AuthDistDelay, INTERVAL, "20 seconds"), diff --git a/src/or/or.h b/src/or/or.h index 908daa61c0..9074083a04 100644 --- a/src/or/or.h +++ b/src/or/or.h @@ -3029,8 +3029,10 @@ typedef struct { config_line_t *RecommendedVersions; config_line_t *RecommendedClientVersions; config_line_t *RecommendedServerVersions; - /** Whether dirservers refuse router descriptors with private IPs. */ + /** Whether dirservers allow router descriptors with private IPs. */ int DirAllowPrivateAddresses; + /** Whether routers accept EXTEND cells to routers with private IPs. */ + int ExtendAllowPrivateAddresses; char *User; /**< Name of user to run Tor as. */ char *Group; /**< Name of group to run Tor as. */ config_line_t *ORPort_lines; /**< Ports to listen on for OR connections. */ From 62d96284f7e0f81c40d5df7e53dd7b4dfe7e56a5 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 27 Aug 2012 11:52:51 -0400 Subject: [PATCH 24/29] Do not assert when comparing a null address/port against a policy This can create a remote crash opportunity for/against directory authorities. --- changes/bug6690 | 7 +++++++ src/or/policies.c | 6 +++++- 2 files changed, 12 insertions(+), 1 deletion(-) create mode 100644 changes/bug6690 diff --git a/changes/bug6690 b/changes/bug6690 new file mode 100644 index 0000000000..99d42976ed --- /dev/null +++ b/changes/bug6690 @@ -0,0 +1,7 @@ + o Major bugfixes (security): + - Do not crash when comparing an address with port value 0 to an + address policy. This bug could have been used to cause a remote + assertion failure by or against directory authorities, or to + allow some applications to crash clients. Fixes bug 6690; bugfix + on 0.2.1.10-alpha. + diff --git a/src/or/policies.c b/src/or/policies.c index c87036013d..55d08afc81 100644 --- a/src/or/policies.c +++ b/src/or/policies.c @@ -685,7 +685,11 @@ compare_tor_addr_to_addr_policy(const tor_addr_t *addr, uint16_t port, /* no policy? accept all. */ return ADDR_POLICY_ACCEPTED; } else if (tor_addr_is_null(addr)) { - tor_assert(port != 0); + if (port == 0) { + log_info(LD_BUG, "Rejecting null address with 0 port (family %d)", + addr ? tor_addr_family(addr) : -1); + return ADDR_POLICY_REJECTED; + } return compare_unknown_tor_addr_to_addr_policy(port, policy); } else if (port == 0) { return compare_known_tor_addr_to_addr_policy_noport(addr, policy); From 45b520b6a4872a5c52b934df9c345e04536568b1 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 27 Aug 2012 12:29:04 -0400 Subject: [PATCH 25/29] Fix changes file for 6710: before 0.0.8pre1, you couldn't extend to a router that another router wasn't already connected to. --- changes/bug6710 | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/changes/bug6710 b/changes/bug6710 index 69a3c7ef40..2c89346114 100644 --- a/changes/bug6710 +++ b/changes/bug6710 @@ -2,6 +2,5 @@ - Reject any attempt to extend to an internal address. Without this fix, a router could be used to probe addresses on an internal network to see whether they were accepting - connections. Fix for bug 6710; bugfix on all released versions - of Tor. + connections. Fix for bug 6710; bugfix on 0.0.8pre1. From b252ffa7cb641398ee19886f71380cd6190bcebc Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 27 Aug 2012 10:46:17 -0400 Subject: [PATCH 26/29] Downgrade path-bias warning messages to INFO for now. We've had over two months to fix them, and didn't. Now we need 0.2.3.x stable. Yes, it would be cool to get this working in 0.2.3.x, but not at the expense of delaying every other feature that _does_ work in 0.2.3.x. We can do a real fix in 0.2.4. --- changes/disable_pathbias_messages | 3 +++ src/or/circuitbuild.c | 20 ++++++++++---------- 2 files changed, 13 insertions(+), 10 deletions(-) create mode 100644 changes/disable_pathbias_messages diff --git a/changes/disable_pathbias_messages b/changes/disable_pathbias_messages new file mode 100644 index 0000000000..3bc996347b --- /dev/null +++ b/changes/disable_pathbias_messages @@ -0,0 +1,3 @@ + o Disabeled features + - Downgrade path-bias warning messages to INFO. We'll try to get them + working better in 0.2.4. Fixes bug 6475; bugfix on 0.2.3.17-beta. diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c index f11bebf7c9..fcd13dc1d3 100644 --- a/src/or/circuitbuild.c +++ b/src/or/circuitbuild.c @@ -2656,7 +2656,7 @@ pathbias_count_first_hop(origin_circuit_t *circ) if (circ->has_opened && circ->path_state != PATH_STATE_DID_FIRST_HOP) { if ((rate_msg = rate_limit_log(&first_hop_notice_limit, approx_time()))) { - log_notice(LD_BUG, + log_info(LD_BUG, "Opened circuit is in strange path state %s. " "Circuit is a %s currently %s. %s", pathbias_state_to_string(circ->path_state), @@ -2683,7 +2683,7 @@ pathbias_count_first_hop(origin_circuit_t *circ) } else { if ((rate_msg = rate_limit_log(&first_hop_notice_limit, approx_time()))) { - log_notice(LD_BUG, + log_info(LD_BUG, "Unopened circuit has strange path state %s. " "Circuit is a %s currently %s. %s", pathbias_state_to_string(circ->path_state), @@ -2695,7 +2695,7 @@ pathbias_count_first_hop(origin_circuit_t *circ) } else { if ((rate_msg = rate_limit_log(&first_hop_notice_limit, approx_time()))) { - log_notice(LD_BUG, + log_info(LD_BUG, "Unopened circuit has no known guard. " "Circuit is a %s currently %s. %s", circuit_purpose_to_string(circ->_base.purpose), @@ -2709,7 +2709,7 @@ pathbias_count_first_hop(origin_circuit_t *circ) if (circ->path_state == PATH_STATE_NEW_CIRC) { if ((rate_msg = rate_limit_log(&first_hop_notice_limit, approx_time()))) { - log_notice(LD_BUG, + 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), @@ -2761,7 +2761,7 @@ pathbias_count_success(origin_circuit_t *circ) } else { if ((rate_msg = rate_limit_log(&success_notice_limit, approx_time()))) { - log_notice(LD_BUG, + log_info(LD_BUG, "Succeeded circuit is in strange path state %s. " "Circuit is a %s currently %s. %s", pathbias_state_to_string(circ->path_state), @@ -2772,7 +2772,7 @@ pathbias_count_success(origin_circuit_t *circ) } if (guard->first_hops < guard->circuit_successes) { - log_warn(LD_BUG, "Unexpectedly high circuit_successes (%u/%u) " + log_info(LD_BUG, "Unexpectedly high circuit_successes (%u/%u) " "for guard %s=%s", guard->circuit_successes, guard->first_hops, guard->nickname, hex_str(guard->identity, DIGEST_LEN)); @@ -2780,7 +2780,7 @@ pathbias_count_success(origin_circuit_t *circ) } else { if ((rate_msg = rate_limit_log(&success_notice_limit, approx_time()))) { - log_notice(LD_BUG, + log_info(LD_BUG, "Completed circuit has no known guard. " "Circuit is a %s currently %s. %s", circuit_purpose_to_string(circ->_base.purpose), @@ -2792,7 +2792,7 @@ pathbias_count_success(origin_circuit_t *circ) if (circ->path_state != PATH_STATE_SUCCEEDED) { if ((rate_msg = rate_limit_log(&success_notice_limit, approx_time()))) { - log_notice(LD_BUG, + log_info(LD_BUG, "Opened circuit is in strange path state %s. " "Circuit is a %s currently %s. %s", pathbias_state_to_string(circ->path_state), @@ -2822,7 +2822,7 @@ entry_guard_inc_first_hop_count(entry_guard_t *guard) if (guard->circuit_successes/((double)guard->first_hops) < pathbias_get_disable_rate(options)) { - log_warn(LD_PROTOCOL, + log_info(LD_PROTOCOL, "Extremely low circuit success rate %u/%u for guard %s=%s. " "This might indicate an attack, or a bug.", guard->circuit_successes, guard->first_hops, guard->nickname, @@ -2835,7 +2835,7 @@ entry_guard_inc_first_hop_count(entry_guard_t *guard) < pathbias_get_notice_rate(options) && !guard->path_bias_notice) { guard->path_bias_notice = 1; - log_notice(LD_PROTOCOL, + log_info(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)); From a7a4bbff47c92aa4df0a932492867d1601834118 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 27 Aug 2012 16:37:09 -0400 Subject: [PATCH 27/29] Quiet "Set buildtimeout to low val" warnings: make them info Fix for #6251 --- changes/bug6251 | 6 ++++++ src/or/circuitbuild.c | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-) create mode 100644 changes/bug6251 diff --git a/changes/bug6251 b/changes/bug6251 new file mode 100644 index 0000000000..c782a93e49 --- /dev/null +++ b/changes/bug6251 @@ -0,0 +1,6 @@ + o Minor bugfixes: + - Downgrade "set buildtimeout to low value" messages to INFO + severity; they were never an actual problem, there was never + anything reasonable to do about them, and they tended to spam + logs from time to time. Fix for bug 6251; bugfix on + 0.2.2.2-alpha. \ No newline at end of file diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c index fcd13dc1d3..e5576018a6 100644 --- a/src/or/circuitbuild.c +++ b/src/or/circuitbuild.c @@ -1627,7 +1627,7 @@ circuit_build_times_set_timeout(circuit_build_times_t *cbt) return; if (cbt->timeout_ms < circuit_build_times_min_timeout()) { - log_warn(LD_CIRC, "Set buildtimeout to low value %fms. Setting to %dms", + log_info(LD_CIRC, "Set buildtimeout to low value %fms. Setting to %dms", cbt->timeout_ms, circuit_build_times_min_timeout()); cbt->timeout_ms = circuit_build_times_min_timeout(); if (cbt->close_ms < cbt->timeout_ms) { From 774979ca4584036d17dd96071ec565955d9f04a7 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 31 Aug 2012 11:35:47 -0400 Subject: [PATCH 28/29] Document consensus and microdesc files Bugfix for #6732. --- changes/bug6732 | 3 +++ doc/tor.1.txt | 11 ++++++++++- 2 files changed, 13 insertions(+), 1 deletion(-) create mode 100644 changes/bug6732 diff --git a/changes/bug6732 b/changes/bug6732 new file mode 100644 index 0000000000..7a744e014a --- /dev/null +++ b/changes/bug6732 @@ -0,0 +1,3 @@ + o Documentation: + - Add missing documentation for consensus and microdesc files. Fix for + bug 6732. diff --git a/doc/tor.1.txt b/doc/tor.1.txt index e7ba8485c0..971a0efe4b 100644 --- a/doc/tor.1.txt +++ b/doc/tor.1.txt @@ -1876,7 +1876,10 @@ FILES __DataDirectory__**/cached-status/**:: The most recently downloaded network status document for each authority. Each file holds one such document; the filenames are the hexadecimal - identity key fingerprints of the directory authorities. + identity key fingerprints of the directory authorities. Mostly obsolete. + +__DataDirectory__**/cached-consensus** and/or **cached-microdesc-consensus**:: + The most recent consensus network status document we've downloaded. __DataDirectory__**/cached-descriptors** and **cached-descriptors.new**:: These files hold downloaded router statuses. Some routers may appear more @@ -1885,6 +1888,12 @@ __DataDirectory__**/cached-descriptors** and **cached-descriptors.new**:: a given router. The ".new" file is an append-only journal; when it gets too large, all entries are merged into a new cached-descriptors file. +__DataDirectory__**/cached-microdescs** and **cached-microdescs.new**:: + These files hold downloaded microdescriptors. Lines beginning with + @-signs are annotations that contain more information about a given + router. The ".new" file is an append-only journal; when it gets too + large, all entries are merged into a new cached-microdescs file. + __DataDirectory__**/cached-routers** and **cached-routers.new**:: Obsolete versions of cached-descriptors and cached-descriptors.new. When Tor can't find the newer files, it looks here instead. From 4bd90e20b92fbe2b93fc53e24254f00e663d057e Mon Sep 17 00:00:00 2001 From: Roger Dingledine Date: Mon, 3 Sep 2012 02:09:39 -0400 Subject: [PATCH 29/29] fix whitespace and trivial typo --- src/or/config.c | 2 +- src/or/microdesc.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/or/config.c b/src/or/config.c index d5e80cc832..b9170f4200 100644 --- a/src/or/config.c +++ b/src/or/config.c @@ -464,7 +464,7 @@ static config_var_t _option_vars[] = { /** Override default values with these if the user sets the TestingTorNetwork * option. */ static const config_var_t testing_tor_network_defaults[] = { - V(ServerDNSAllowBrokenConfig, BOOL, "1"), + V(ServerDNSAllowBrokenConfig, BOOL, "1"), V(DirAllowPrivateAddresses, BOOL, "1"), V(EnforceDistinctSubnets, BOOL, "0"), V(AssumeReachable, BOOL, "1"), diff --git a/src/or/microdesc.c b/src/or/microdesc.c index c1ac1c3758..b4d22c1c62 100644 --- a/src/or/microdesc.c +++ b/src/or/microdesc.c @@ -653,7 +653,7 @@ microdesc_list_missing_digest256(networkstatus_t *ns, microdesc_cache_t *cache, return result; } -/** Launch download requests for mircodescriptors as appropriate. +/** Launch download requests for microdescriptors as appropriate. * * Specifically, we should launch download requests if we are configured to * download mirodescriptors, and there are some microdescriptors listed the