November 2017 - tor-commits - lists.torproject.org

[tor/master] Fix DNS resolution on busy exit relays
by nickm＠torproject.org 05 Nov '17

05 Nov '17

commit 98275743081728b1538f70111c891ea573127e85 Author: Sebastian Hahn <sebastian(a)torproject.org> Date: Fri Nov 3 03:06:12 2017 +0100 Fix DNS resolution on busy exit relays Thanks to Dhalgren who analyzed this issue two years ago and found a solution! --- changes/bug21394 | 9 +++++++++ src/or/dns.c | 23 ++++++++++++++++++++--- 2 files changed, 29 insertions(+), 3 deletions(-) diff --git a/changes/bug21394 b/changes/bug21394 new file mode 100644 index 000000000..e5452e20b --- /dev/null +++ b/changes/bug21394 @@ -0,0 +1,9 @@ + o Major bugfixes (Exit nodes): + - Fix an issue causing high-bandwidth exit nodes to fail a majority + or all of their DNS requests, making them basically unsuitable for + regular usage in Tor circuits. The problem is related to + libevent's DNS handling, but we can work around it in Tor. Fixes + bugs 21394 and 18580; bugfix on 0.1.2.2-alpha which introduced + eventdns. Credit goes to Dhalgren for identifying and finding a + workaround to this bug and to gamambel, arthuredelstein and + arma in helping to track it down and analyze it. diff --git a/src/or/dns.c b/src/or/dns.c index 0ad4c0f50..c1e3c3256 100644 --- a/src/or/dns.c +++ b/src/or/dns.c @@ -1425,14 +1425,31 @@ configure_nameservers(int force) #define SET(k,v) evdns_base_set_option(the_evdns_base, (k), (v)) + // If we only have one nameserver, it does not make sense to back off + // from it for a timeout. Unfortunately, the value for max-timeouts is + // currently clamped by libevent to 255, but it does not hurt to set + // it higher in case libevent gets a patch for this. + // Reducing attempts in the case of just one name server too, because + // it is very likely to be a local one where a network connectivity + // issue should not cause an attempt to fail. if (evdns_base_count_nameservers(the_evdns_base) == 1) { - SET("max-timeouts:", "16"); - SET("timeout:", "10"); + SET("max-timeouts:", "1000000"); + SET("attempts:", "1"); } else { SET("max-timeouts:", "3"); - SET("timeout:", "5"); } + // Elongate the queue of maximum inflight dns requests, so if a bunch + // time out at the resolver (happens commonly with unbound) we won't + // stall every other DNS request. This potentially means some wasted + // CPU as there's a walk over a linear queue involved, but this is a + // much better tradeoff compared to just failing DNS requests because + // of a full queue. + SET("max-inflight:", "8192"); + + // Time out after 5 seconds if no reply. + SET("timeout:", "5"); + if (options->ServerDNSRandomizeCase) SET("randomize-case:", "1"); else

1 0

[tor/master] Merge branch 'bug21394_029' into maint-0.3.2
by nickm＠torproject.org 05 Nov '17

05 Nov '17

commit 9f3d9c68ab21e4856da11580c29f4895bf398a09 Merge: 7c1f33dd0 982757430 Author: Nick Mathewson <nickm(a)torproject.org> Date: Sun Nov 5 14:39:37 2017 -0500 Merge branch 'bug21394_029' into maint-0.3.2 changes/bug21394 | 9 +++++++++ src/or/dns.c | 23 ++++++++++++++++++++--- 2 files changed, 29 insertions(+), 3 deletions(-)

1 0

[tor/master] lintchanges: Allow 'fixes bugs a, b, and c'
by nickm＠torproject.org 05 Nov '17

05 Nov '17

commit f46cdb3ff85be4c4241928bfd3f558eb878cff28 Author: Nick Mathewson <nickm(a)torproject.org> Date: Sun Nov 5 14:41:39 2017 -0500 lintchanges: Allow 'fixes bugs a, b, and c' --- scripts/maint/lintChanges.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/scripts/maint/lintChanges.py b/scripts/maint/lintChanges.py index c2dda6dc8..d5b8fcae5 100755 --- a/scripts/maint/lintChanges.py +++ b/scripts/maint/lintChanges.py @@ -76,13 +76,13 @@ def lintfile(fname): if isBug and not re.search(r'(\d+)', contents): warn("Ticket marked as bugfix, but does not mention a number.") - elif isBug and not re.search(r'Fixes ([a-z ]*)bug (\d+)', contents): + elif isBug and not re.search(r'Fixes ([a-z ]*)bugs? (\d+)', contents): warn("Ticket marked as bugfix, but does not say 'Fixes bug XXX'") if re.search(r'[bB]ug (\d+)', contents): if not re.search(r'[Bb]ugfix on ', contents): warn("Bugfix does not say 'bugfix on X.Y.Z'") - elif not re.search('[fF]ixes ([a-z ]*)bug (\d+); bugfix on ', + elif not re.search('[fF]ixes ([a-z ]*)bugs? (\d+)((, \d+)* and \d+)?; bugfix on ', contents): warn("Bugfix does not say 'Fixes bug X; bugfix on Y'") elif re.search('tor-([0-9]+)', contents):

1 0

[tor/release-0.3.2] Fix DNS resolution on busy exit relays
by nickm＠torproject.org 05 Nov '17

05 Nov '17

commit 98275743081728b1538f70111c891ea573127e85 Author: Sebastian Hahn <sebastian(a)torproject.org> Date: Fri Nov 3 03:06:12 2017 +0100 Fix DNS resolution on busy exit relays Thanks to Dhalgren who analyzed this issue two years ago and found a solution! --- changes/bug21394 | 9 +++++++++ src/or/dns.c | 23 ++++++++++++++++++++--- 2 files changed, 29 insertions(+), 3 deletions(-) diff --git a/changes/bug21394 b/changes/bug21394 new file mode 100644 index 000000000..e5452e20b --- /dev/null +++ b/changes/bug21394 @@ -0,0 +1,9 @@ + o Major bugfixes (Exit nodes): + - Fix an issue causing high-bandwidth exit nodes to fail a majority + or all of their DNS requests, making them basically unsuitable for + regular usage in Tor circuits. The problem is related to + libevent's DNS handling, but we can work around it in Tor. Fixes + bugs 21394 and 18580; bugfix on 0.1.2.2-alpha which introduced + eventdns. Credit goes to Dhalgren for identifying and finding a + workaround to this bug and to gamambel, arthuredelstein and + arma in helping to track it down and analyze it. diff --git a/src/or/dns.c b/src/or/dns.c index 0ad4c0f50..c1e3c3256 100644 --- a/src/or/dns.c +++ b/src/or/dns.c @@ -1425,14 +1425,31 @@ configure_nameservers(int force) #define SET(k,v) evdns_base_set_option(the_evdns_base, (k), (v)) + // If we only have one nameserver, it does not make sense to back off + // from it for a timeout. Unfortunately, the value for max-timeouts is + // currently clamped by libevent to 255, but it does not hurt to set + // it higher in case libevent gets a patch for this. + // Reducing attempts in the case of just one name server too, because + // it is very likely to be a local one where a network connectivity + // issue should not cause an attempt to fail. if (evdns_base_count_nameservers(the_evdns_base) == 1) { - SET("max-timeouts:", "16"); - SET("timeout:", "10"); + SET("max-timeouts:", "1000000"); + SET("attempts:", "1"); } else { SET("max-timeouts:", "3"); - SET("timeout:", "5"); } + // Elongate the queue of maximum inflight dns requests, so if a bunch + // time out at the resolver (happens commonly with unbound) we won't + // stall every other DNS request. This potentially means some wasted + // CPU as there's a walk over a linear queue involved, but this is a + // much better tradeoff compared to just failing DNS requests because + // of a full queue. + SET("max-inflight:", "8192"); + + // Time out after 5 seconds if no reply. + SET("timeout:", "5"); + if (options->ServerDNSRandomizeCase) SET("randomize-case:", "1"); else

1 0

[tor/release-0.3.2] lintchanges: Allow 'fixes bugs a, b, and c'
by nickm＠torproject.org 05 Nov '17

05 Nov '17

commit f46cdb3ff85be4c4241928bfd3f558eb878cff28 Author: Nick Mathewson <nickm(a)torproject.org> Date: Sun Nov 5 14:41:39 2017 -0500 lintchanges: Allow 'fixes bugs a, b, and c' --- scripts/maint/lintChanges.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/scripts/maint/lintChanges.py b/scripts/maint/lintChanges.py index c2dda6dc8..d5b8fcae5 100755 --- a/scripts/maint/lintChanges.py +++ b/scripts/maint/lintChanges.py @@ -76,13 +76,13 @@ def lintfile(fname): if isBug and not re.search(r'(\d+)', contents): warn("Ticket marked as bugfix, but does not mention a number.") - elif isBug and not re.search(r'Fixes ([a-z ]*)bug (\d+)', contents): + elif isBug and not re.search(r'Fixes ([a-z ]*)bugs? (\d+)', contents): warn("Ticket marked as bugfix, but does not say 'Fixes bug XXX'") if re.search(r'[bB]ug (\d+)', contents): if not re.search(r'[Bb]ugfix on ', contents): warn("Bugfix does not say 'bugfix on X.Y.Z'") - elif not re.search('[fF]ixes ([a-z ]*)bug (\d+); bugfix on ', + elif not re.search('[fF]ixes ([a-z ]*)bugs? (\d+)((, \d+)* and \d+)?; bugfix on ', contents): warn("Bugfix does not say 'Fixes bug X; bugfix on Y'") elif re.search('tor-([0-9]+)', contents):

1 0

[tor/master] Merge branch 'maint-0.3.2'
by nickm＠torproject.org 05 Nov '17

05 Nov '17

commit f755f9b9e67232d9d39682cbcdf4433ac738e17a Merge: 9f650b24e f46cdb3ff Author: Nick Mathewson <nickm(a)torproject.org> Date: Sun Nov 5 14:41:53 2017 -0500 Merge branch 'maint-0.3.2' changes/bug21394 | 9 +++++++++ scripts/maint/lintChanges.py | 4 ++-- src/or/dns.c | 23 ++++++++++++++++++++--- 3 files changed, 31 insertions(+), 5 deletions(-)

1 0

[tor/release-0.3.2] Merge branch 'bug21394_029' into maint-0.3.2
by nickm＠torproject.org 05 Nov '17

05 Nov '17

commit 9f3d9c68ab21e4856da11580c29f4895bf398a09 Merge: 7c1f33dd0 982757430 Author: Nick Mathewson <nickm(a)torproject.org> Date: Sun Nov 5 14:39:37 2017 -0500 Merge branch 'bug21394_029' into maint-0.3.2 changes/bug21394 | 9 +++++++++ src/or/dns.c | 23 ++++++++++++++++++++--- 2 files changed, 29 insertions(+), 3 deletions(-)

1 0

[tor/release-0.3.2] Merge branch 'maint-0.3.2' into release-0.3.2
by nickm＠torproject.org 05 Nov '17

05 Nov '17

commit 6bc1632b724da0593b0d781998bece33a0b6cec6 Merge: ed10b34ef f46cdb3ff Author: Nick Mathewson <nickm(a)torproject.org> Date: Sun Nov 5 14:41:53 2017 -0500 Merge branch 'maint-0.3.2' into release-0.3.2 changes/bug21394 | 9 +++++++++ scripts/maint/lintChanges.py | 4 ++-- src/or/dns.c | 23 ++++++++++++++++++++--- 3 files changed, 31 insertions(+), 5 deletions(-)

1 0

[tor/release-0.3.2] Merge branch 'maint-0.3.2' into release-0.3.2
by nickm＠torproject.org 05 Nov '17

05 Nov '17

commit ed10b34efeb29e75e214e37e04fc01d61fb07612 Merge: 3d966f57a 7c1f33dd0 Author: Nick Mathewson <nickm(a)torproject.org> Date: Sun Nov 5 14:09:21 2017 -0500 Merge branch 'maint-0.3.2' into release-0.3.2 changes/ticket21031 | 7 +++++++ doc/tor.1.txt | 17 ++++++++--------- src/or/config.c | 18 ++++++++++++------ src/or/connection_edge.c | 2 +- src/or/or.h | 2 +- src/or/relay.c | 6 +++--- src/test/test_options.c | 5 +++-- src/test/test_relaycell.c | 4 ++-- 8 files changed, 37 insertions(+), 24 deletions(-)

1 0

[tor/release-0.3.2] Revert "Make ClientDNSRejectInternalAddresses testing-only."
by nickm＠torproject.org 05 Nov '17

05 Nov '17

commit 5a46074e550229e6c19eac9625b26af346b487fc Author: Sebastian Hahn <sebastian(a)torproject.org> Date: Fri Nov 3 23:00:27 2017 +0100 Revert "Make ClientDNSRejectInternalAddresses testing-only." This reverts commit 27fa4a98d23972213122fa99499efa4baebe49e3. --- doc/tor.1.txt | 15 +++++++-------- src/or/config.c | 16 ++++++++++------ src/or/connection_edge.c | 2 +- src/or/or.h | 2 +- src/or/relay.c | 6 +++--- src/test/test_relaycell.c | 4 ++-- 6 files changed, 24 insertions(+), 21 deletions(-) diff --git a/doc/tor.1.txt b/doc/tor.1.txt index 86999667a..d701288af 100644 --- a/doc/tor.1.txt +++ b/doc/tor.1.txt @@ -1440,6 +1440,12 @@ The following options are useful only for clients (that is, if addresses/ports. See SocksPort for an explanation of isolation flags. (Default: 0) +[[ClientDNSRejectInternalAddresses]] **ClientDNSRejectInternalAddresses** **0**|**1**:: + If true, Tor does not believe any anonymously retrieved DNS answer that + tells it that an address resolves to an internal address (like 127.0.0.1 or + 192.168.0.1). This option prevents certain browser-based attacks; don't + turn it off unless you know what you're doing. (Default: 1) + [[ClientRejectInternalAddresses]] **ClientRejectInternalAddresses** **0**|**1**:: If true, Tor does not try to fulfill requests to connect to an internal address (like 127.0.0.1 or 192.168.0.1) __unless a exit node is @@ -2507,7 +2513,7 @@ The following options are used for running a testing Tor network. 4 (for 40 seconds), 8, 16, 32, 60 ClientBootstrapConsensusMaxDownloadTries 80 ClientBootstrapConsensusAuthorityOnlyMaxDownloadTries 80 - TestingClientDNSRejectInternalAddresses 0 + ClientDNSRejectInternalAddresses 0 ClientRejectInternalAddresses 0 CountPrivateBandwidth 1 ExitPolicyRejectPrivate 0 @@ -2718,13 +2724,6 @@ The following options are used for running a testing Tor network. we replace it and issue a new key? (Default: 3 hours for link and auth; 1 day for signing.) -[[ClientDNSRejectInternalAddresses]] [[TestingClientDNSRejectInternalAddresses]] **TestingClientDNSRejectInternalAddresses** **0**|**1**:: - If true, Tor does not believe any anonymously retrieved DNS answer that - tells it that an address resolves to an internal address (like 127.0.0.1 or - 192.168.0.1). This option prevents certain browser-based attacks; don't - turn it off unless you know what you're doing. (Default: 1) - - NON-PERSISTENT OPTIONS ---------------------- diff --git a/src/or/config.c b/src/or/config.c index e7fe3a547..c87786cb9 100644 --- a/src/or/config.c +++ b/src/or/config.c @@ -169,8 +169,6 @@ static config_abbrev_t option_abbrevs_[] = { { "BridgeAuthoritativeDirectory", "BridgeAuthoritativeDir", 0, 0}, { "HashedControlPassword", "__HashedControlSessionPassword", 1, 0}, { "VirtualAddrNetwork", "VirtualAddrNetworkIPv4", 0, 0}, - { "ClientDNSRejectInternalAddresses", - "TestingClientDNSRejectInternalAddresses", 0, 1, }, { NULL, NULL, 0, 0}, }; @@ -262,7 +260,7 @@ static config_var_t option_vars_[] = { V(CircuitsAvailableTimeout, INTERVAL, "0"), V(CircuitStreamTimeout, INTERVAL, "0"), V(CircuitPriorityHalflife, DOUBLE, "-100.0"), /*negative:'Use default'*/ - V(TestingClientDNSRejectInternalAddresses, BOOL,"1"), + V(ClientDNSRejectInternalAddresses, BOOL,"1"), V(ClientOnly, BOOL, "0"), V(ClientPreferIPv6ORPort, AUTOBOOL, "auto"), V(ClientPreferIPv6DirPort, AUTOBOOL, "auto"), @@ -648,7 +646,7 @@ static const config_var_t testing_tor_network_defaults[] = { "0, 1, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 8, 16, 32, 60"), V(ClientBootstrapConsensusMaxDownloadTries, UINT, "80"), V(ClientBootstrapConsensusAuthorityOnlyMaxDownloadTries, UINT, "80"), - V(TestingClientDNSRejectInternalAddresses, BOOL,"0"), + V(ClientDNSRejectInternalAddresses, BOOL,"0"), // deprecated in 0.2.9.2-alpha V(ClientRejectInternalAddresses, BOOL, "0"), V(CountPrivateBandwidth, BOOL, "1"), V(ExitPolicyRejectPrivate, BOOL, "0"), @@ -693,7 +691,14 @@ static const config_var_t testing_tor_network_defaults[] = { #undef OBSOLETE static const config_deprecation_t option_deprecation_notes_[] = { - /* Deprecated since 0.3.2.1-alpha. */ + /* Deprecated since 0.2.9.2-alpha... */ + { "AllowDotExit", "Unrestricted use of the .exit notation can be used for " + "a wide variety of application-level attacks." }, + { "ClientDNSRejectInternalAddresses", "Turning this on makes your client " + "easier to fingerprint, and may open you to esoteric attacks." }, + /* End of options deprecated since 0.2.9.2-alpha. */ + + /* Deprecated since 0.3.2.0-alpha. */ { "HTTPProxy", "It only applies to direct unencrypted HTTP connections " "to your directory server, which your Tor probably wasn't using." }, { "HTTPProxyAuthenticator", "HTTPProxy is deprecated in favor of HTTPSProxy " @@ -4211,7 +4216,6 @@ options_validate(or_options_t *old_options, or_options_t *options, CHECK_DEFAULT(TestingSigningKeySlop); CHECK_DEFAULT(TestingAuthKeySlop); CHECK_DEFAULT(TestingLinkKeySlop); - CHECK_DEFAULT(TestingClientDNSRejectInternalAddresses); #undef CHECK_DEFAULT if (options->SigningKeyLifetime < options->TestingSigningKeySlop*2) diff --git a/src/or/connection_edge.c b/src/or/connection_edge.c index b9d8eeaff..f178917f0 100644 --- a/src/or/connection_edge.c +++ b/src/or/connection_edge.c @@ -1344,7 +1344,7 @@ connection_ap_handshake_rewrite(entry_connection_t *conn, /* Hang on, did we find an answer saying that this is a reverse lookup for * an internal address? If so, we should reject it if we're configured to * do so. */ - if (options->TestingClientDNSRejectInternalAddresses) { + if (options->ClientDNSRejectInternalAddresses) { /* Don't let clients try to do a reverse lookup on 10.0.0.1. */ tor_addr_t addr; int ok; diff --git a/src/or/or.h b/src/or/or.h index d9ab815cb..809714b5e 100644 --- a/src/or/or.h +++ b/src/or/or.h @@ -4204,7 +4204,7 @@ typedef struct { /** If true, do not believe anybody who tells us that a domain resolves * to an internal address, or that an internal address has a PTR mapping. * Helps avoid some cross-site attacks. */ - int TestingClientDNSRejectInternalAddresses; + int ClientDNSRejectInternalAddresses; /** If true, do not accept any requests to connect to internal addresses * over randomly chosen exits. */ diff --git a/src/or/relay.c b/src/or/relay.c index 48c1dd6f8..09f70793d 100644 --- a/src/or/relay.c +++ b/src/or/relay.c @@ -949,7 +949,7 @@ connection_ap_process_end_not_open( connection_mark_unattached_ap(conn, END_STREAM_REASON_TORPROTOCOL); return 0; } - if (get_options()->TestingClientDNSRejectInternalAddresses && + if (get_options()->ClientDNSRejectInternalAddresses && tor_addr_is_internal(&addr, 0)) { log_info(LD_APP,"Address '%s' resolved to internal. Closing,", safe_str(conn->socks_request->address)); @@ -1366,7 +1366,7 @@ connection_edge_process_resolved_cell(edge_connection_t *conn, goto done; } - if (get_options()->TestingClientDNSRejectInternalAddresses) { + if (get_options()->ClientDNSRejectInternalAddresses) { int orig_len = smartlist_len(resolved_addresses); SMARTLIST_FOREACH_BEGIN(resolved_addresses, address_ttl_t *, addr) { if (addr->hostname == NULL && tor_addr_is_internal(&addr->addr, 0)) { @@ -1459,7 +1459,7 @@ connection_edge_process_relay_cell_not_open( if (tor_addr_family(&addr) != AF_UNSPEC) { const sa_family_t family = tor_addr_family(&addr); if (tor_addr_is_null(&addr) || - (get_options()->TestingClientDNSRejectInternalAddresses && + (get_options()->ClientDNSRejectInternalAddresses && tor_addr_is_internal(&addr, 0))) { log_info(LD_APP, "...but it claims the IP address was %s. Closing.", fmt_addr(&addr)); diff --git a/src/test/test_relaycell.c b/src/test/test_relaycell.c index 9c010deec..eea1f5dc8 100644 --- a/src/test/test_relaycell.c +++ b/src/test/test_relaycell.c @@ -112,7 +112,7 @@ test_relaycell_resolved(void *arg) MOCK(connection_mark_unattached_ap_, mark_unattached_mock); MOCK(connection_ap_handshake_socks_resolved, socks_resolved_mock); - options->TestingClientDNSRejectInternalAddresses = 0; + options->ClientDNSRejectInternalAddresses = 0; SET_CELL(/* IPv4: 127.0.1.2, ttl 256 */ "\x04\x04\x7f\x00\x01\x02\x00\x00\x01\x00" @@ -151,7 +151,7 @@ test_relaycell_resolved(void *arg) /* But we may be discarding private answers. */ MOCK_RESET(); - options->TestingClientDNSRejectInternalAddresses = 1; + options->ClientDNSRejectInternalAddresses = 1; r = connection_edge_process_resolved_cell(edgeconn, &cell, &rh); tt_int_op(r, OP_EQ, 0); ASSERT_MARK_CALLED(END_STREAM_REASON_DONE|

1 0