From dcbc45e6b2f80e8bd3664972d6317331d6b3bc85 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 18 Feb 2020 12:00:24 -0500 Subject: [PATCH 1/6] Replace identifiers related to clipping DNS ttls. This is an automated commit, generated by this command: ./scripts/maint/rename_c_identifier.py \ MIN_DNS_TTL_AT_EXIT MIN_DNS_TTL \ MAX_DNS_TTL_AT_EXIT MAX_DNS_TTL \ dns_clip_ttl clip_dns_ttl --- src/core/or/connection_edge.c | 4 ++-- src/feature/client/addressmap.c | 2 +- src/feature/relay/dns.c | 16 ++++++++-------- src/feature/relay/dns.h | 6 +++--- src/test/test_dns.c | 8 ++++---- 5 files changed, 18 insertions(+), 18 deletions(-) diff --git a/src/core/or/connection_edge.c b/src/core/or/connection_edge.c index aeb9ec6460..eae07141c7 100644 --- a/src/core/or/connection_edge.c +++ b/src/core/or/connection_edge.c @@ -480,7 +480,7 @@ connection_edge_end(edge_connection_t *conn, uint8_t reason) memcpy(payload+1, tor_addr_to_in6_addr8(&conn->base_.addr), 16); addrlen = 16; } - set_uint32(payload+1+addrlen, htonl(dns_clip_ttl(conn->address_ttl))); + set_uint32(payload+1+addrlen, htonl(clip_dns_ttl(conn->address_ttl))); payload_len += 4+addrlen; } @@ -845,7 +845,7 @@ connected_cell_format_payload(uint8_t *payload_out, return -1; } - set_uint32(payload_out + connected_payload_len, htonl(dns_clip_ttl(ttl))); + set_uint32(payload_out + connected_payload_len, htonl(clip_dns_ttl(ttl))); connected_payload_len += 4; tor_assert(connected_payload_len <= MAX_CONNECTED_CELL_PAYLOAD_LEN); diff --git a/src/feature/client/addressmap.c b/src/feature/client/addressmap.c index 1a6958d38c..af76253e41 100644 --- a/src/feature/client/addressmap.c +++ b/src/feature/client/addressmap.c @@ -689,7 +689,7 @@ client_dns_set_addressmap_impl(entry_connection_t *for_conn, if (ttl<0) ttl = DEFAULT_DNS_TTL; else - ttl = dns_clip_ttl(ttl); + ttl = clip_dns_ttl(ttl); if (exitname) { /* XXXX fails to ever get attempts to get an exit address of diff --git a/src/feature/relay/dns.c b/src/feature/relay/dns.c index da0cbb1df4..08fe4d39cf 100644 --- a/src/feature/relay/dns.c +++ b/src/feature/relay/dns.c @@ -272,16 +272,16 @@ has_dns_init_failed(void) * OP that asked us to resolve it, and how long to cache that record * ourselves. */ uint32_t -dns_clip_ttl(uint32_t ttl) +clip_dns_ttl(uint32_t ttl) { /* This logic is a defense against "DefectTor" DNS-based traffic * confirmation attacks, as in https://nymity.ch/tor-dns/tor-dns.pdf . * We only give two values: a "low" value and a "high" value. */ - if (ttl < MIN_DNS_TTL_AT_EXIT) - return MIN_DNS_TTL_AT_EXIT; + if (ttl < MIN_DNS_TTL) + return MIN_DNS_TTL; else - return MAX_DNS_TTL_AT_EXIT; + return MAX_DNS_TTL; } /** Helper: free storage held by an entry in the DNS cache. */ @@ -521,7 +521,7 @@ send_resolved_cell,(edge_connection_t *conn, uint8_t answer_type, uint32_t ttl; buf[0] = answer_type; - ttl = dns_clip_ttl(conn->address_ttl); + ttl = clip_dns_ttl(conn->address_ttl); switch (answer_type) { @@ -593,7 +593,7 @@ send_resolved_hostname_cell,(edge_connection_t *conn, size_t namelen = strlen(hostname); tor_assert(namelen < 256); - ttl = dns_clip_ttl(conn->address_ttl); + ttl = clip_dns_ttl(conn->address_ttl); buf[0] = RESOLVED_TYPE_HOSTNAME; buf[1] = (uint8_t)namelen; @@ -1338,7 +1338,7 @@ make_pending_resolve_cached(cached_resolve_t *resolve) resolve->ttl_hostname < ttl) ttl = resolve->ttl_hostname; - set_expiry(new_resolve, time(NULL) + dns_clip_ttl(ttl)); + set_expiry(new_resolve, time(NULL) + clip_dns_ttl(ttl)); } assert_cache_ok(); @@ -2188,7 +2188,7 @@ dns_cache_handle_oom(time_t now, size_t min_remove_bytes) total_bytes_removed += bytes_removed; /* Increase time_inc by a reasonable fraction. */ - time_inc += (MAX_DNS_TTL_AT_EXIT / 4); + time_inc += (MAX_DNS_TTL / 4); } while (total_bytes_removed < min_remove_bytes); return total_bytes_removed; diff --git a/src/feature/relay/dns.h b/src/feature/relay/dns.h index 2b1da8d126..e445b23336 100644 --- a/src/feature/relay/dns.h +++ b/src/feature/relay/dns.h @@ -13,9 +13,9 @@ #define TOR_DNS_H /** Lowest value for DNS ttl that a server will give. */ -#define MIN_DNS_TTL_AT_EXIT (5*60) +#define MIN_DNS_TTL (5*60) /** Highest value for DNS ttl that a server will give. */ -#define MAX_DNS_TTL_AT_EXIT (60*60) +#define MAX_DNS_TTL (60*60) /** How long do we keep DNS cache entries before purging them (regardless of * their TTL)? */ @@ -27,7 +27,7 @@ int dns_init(void); int has_dns_init_failed(void); void dns_free_all(void); -uint32_t dns_clip_ttl(uint32_t ttl); +uint32_t clip_dns_ttl(uint32_t ttl); int dns_reset(void); void connection_dns_remove(edge_connection_t *conn); void assert_connection_edge_not_dns_pending(edge_connection_t *conn); diff --git a/src/test/test_dns.c b/src/test/test_dns.c index ec17e9e91e..299321ab64 100644 --- a/src/test/test_dns.c +++ b/src/test/test_dns.c @@ -80,11 +80,11 @@ test_dns_clip_ttl(void *arg) { (void)arg; - uint32_t ttl_mid = MIN_DNS_TTL_AT_EXIT / 2 + MAX_DNS_TTL_AT_EXIT / 2; + uint32_t ttl_mid = MIN_DNS_TTL / 2 + MAX_DNS_TTL / 2; - tt_int_op(dns_clip_ttl(MIN_DNS_TTL_AT_EXIT - 1),OP_EQ,MIN_DNS_TTL_AT_EXIT); - tt_int_op(dns_clip_ttl(ttl_mid),OP_EQ,MAX_DNS_TTL_AT_EXIT); - tt_int_op(dns_clip_ttl(MAX_DNS_TTL_AT_EXIT + 1),OP_EQ,MAX_DNS_TTL_AT_EXIT); + tt_int_op(clip_dns_ttl(MIN_DNS_TTL - 1),OP_EQ,MIN_DNS_TTL); + tt_int_op(clip_dns_ttl(ttl_mid),OP_EQ,MAX_DNS_TTL); + tt_int_op(clip_dns_ttl(MAX_DNS_TTL + 1),OP_EQ,MAX_DNS_TTL); done: return; From 1f06f494c8ce45946e565237a8a52785a8ece447 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 18 Feb 2020 12:08:29 -0500 Subject: [PATCH 2/6] Move DNS TTL manipulation code to src/core/or This removes a dependency from the client code on feature/relay. --- src/core/or/connection_edge.c | 15 +++++++++++++++ src/core/or/connection_edge.h | 15 +++++++++++++++ src/feature/client/addressmap.c | 1 - src/feature/relay/dns.c | 16 ---------------- src/feature/relay/dns.h | 14 -------------- 5 files changed, 30 insertions(+), 31 deletions(-) diff --git a/src/core/or/connection_edge.c b/src/core/or/connection_edge.c index eae07141c7..23c6e230cb 100644 --- a/src/core/or/connection_edge.c +++ b/src/core/or/connection_edge.c @@ -432,6 +432,21 @@ warn_if_hs_unreachable(const edge_connection_t *conn, uint8_t reason) } } +/** Given a TTL (in seconds) from a DNS response or from a relay, determine + * what TTL clients and relays should actually use for caching it. */ +uint32_t +clip_dns_ttl(uint32_t ttl) +{ + /* This logic is a defense against "DefectTor" DNS-based traffic + * confirmation attacks, as in https://nymity.ch/tor-dns/tor-dns.pdf . + * We only give two values: a "low" value and a "high" value. + */ + if (ttl < MIN_DNS_TTL) + return MIN_DNS_TTL; + else + return MAX_DNS_TTL; +} + /** Send a relay end cell from stream conn down conn's circuit, and * remember that we've done so. If this is not a client connection, set the * relay end cell's reason for closing as reason. diff --git a/src/core/or/connection_edge.h b/src/core/or/connection_edge.h index 11cb252935..8c06af5664 100644 --- a/src/core/or/connection_edge.h +++ b/src/core/or/connection_edge.h @@ -182,6 +182,21 @@ void connection_ap_warn_and_unmark_if_pending_circ( entry_connection_t *entry_conn, const char *where); +/** Lowest value for DNS ttl that a server should give or a client should + * believe. */ +#define MIN_DNS_TTL (5*60) +/** Highest value for DNS ttl that a server should give or a client should + * believe. */ +#define MAX_DNS_TTL (60*60) +/** How long do we keep DNS cache entries before purging them (regardless of + * their TTL)? */ +#define MAX_DNS_ENTRY_AGE (3*60*60) +/** How long do we cache/tell clients to cache DNS records when no TTL is + * known? */ +#define DEFAULT_DNS_TTL (30*60) + +uint32_t clip_dns_ttl(uint32_t ttl); + int connection_half_edge_is_valid_data(const smartlist_t *half_conns, streamid_t stream_id); int connection_half_edge_is_valid_sendme(const smartlist_t *half_conns, diff --git a/src/feature/client/addressmap.c b/src/feature/client/addressmap.c index af76253e41..cc97166f36 100644 --- a/src/feature/client/addressmap.c +++ b/src/feature/client/addressmap.c @@ -23,7 +23,6 @@ #include "app/config/config.h" #include "core/or/connection_edge.h" #include "feature/control/control_events.h" -#include "feature/relay/dns.h" #include "feature/nodelist/nodelist.h" #include "feature/nodelist/routerset.h" diff --git a/src/feature/relay/dns.c b/src/feature/relay/dns.c index 08fe4d39cf..5f4bddab9d 100644 --- a/src/feature/relay/dns.c +++ b/src/feature/relay/dns.c @@ -268,22 +268,6 @@ has_dns_init_failed(void) return nameserver_config_failed; } -/** Helper: Given a TTL from a DNS response, determine what TTL to give the - * OP that asked us to resolve it, and how long to cache that record - * ourselves. */ -uint32_t -clip_dns_ttl(uint32_t ttl) -{ - /* This logic is a defense against "DefectTor" DNS-based traffic - * confirmation attacks, as in https://nymity.ch/tor-dns/tor-dns.pdf . - * We only give two values: a "low" value and a "high" value. - */ - if (ttl < MIN_DNS_TTL) - return MIN_DNS_TTL; - else - return MAX_DNS_TTL; -} - /** Helper: free storage held by an entry in the DNS cache. */ static void free_cached_resolve_(cached_resolve_t *r) diff --git a/src/feature/relay/dns.h b/src/feature/relay/dns.h index e445b23336..a2275c724a 100644 --- a/src/feature/relay/dns.h +++ b/src/feature/relay/dns.h @@ -12,22 +12,9 @@ #ifndef TOR_DNS_H #define TOR_DNS_H -/** Lowest value for DNS ttl that a server will give. */ -#define MIN_DNS_TTL (5*60) -/** Highest value for DNS ttl that a server will give. */ -#define MAX_DNS_TTL (60*60) - -/** How long do we keep DNS cache entries before purging them (regardless of - * their TTL)? */ -#define MAX_DNS_ENTRY_AGE (3*60*60) -/** How long do we cache/tell clients to cache DNS records when no TTL is - * known? */ -#define DEFAULT_DNS_TTL (30*60) - int dns_init(void); int has_dns_init_failed(void); void dns_free_all(void); -uint32_t clip_dns_ttl(uint32_t ttl); int dns_reset(void); void connection_dns_remove(edge_connection_t *conn); void assert_connection_edge_not_dns_pending(edge_connection_t *conn); @@ -74,4 +61,3 @@ launch_resolve,(cached_resolve_t *resolve)); #endif /* defined(DNS_PRIVATE) */ #endif /* !defined(TOR_DNS_H) */ - From c43a2452921ac86e65eb70797c8a82b82c2b4076 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 18 Feb 2020 12:32:00 -0500 Subject: [PATCH 3/6] Disable dns.c when relay mode is disabled. This saves about 1% for me on a binary compiled without relay mode. Closes ticket 33366. --- changes/ticket33366 | 3 +++ src/feature/relay/dns.h | 39 ++++++++++++++++++++++++++++++++++++ src/feature/relay/include.am | 2 +- 3 files changed, 43 insertions(+), 1 deletion(-) create mode 100644 changes/ticket33366 diff --git a/changes/ticket33366 b/changes/ticket33366 new file mode 100644 index 0000000000..1310c493c2 --- /dev/null +++ b/changes/ticket33366 @@ -0,0 +1,3 @@ + o Minor features (compilation size): + - Most Server-side DNS code is now disabled when building without + support for relay mode. Closes ticket 33366. diff --git a/src/feature/relay/dns.h b/src/feature/relay/dns.h index a2275c724a..4e5766af0c 100644 --- a/src/feature/relay/dns.h +++ b/src/feature/relay/dns.h @@ -12,6 +12,8 @@ #ifndef TOR_DNS_H #define TOR_DNS_H +#ifdef HAVE_MODULE_RELAY + int dns_init(void); int has_dns_init_failed(void); void dns_free_all(void); @@ -29,6 +31,43 @@ size_t dns_cache_total_allocation(void); void dump_dns_mem_usage(int severity); size_t dns_cache_handle_oom(time_t now, size_t min_remove_bytes); +#else /* !defined(HAVE_MODULE_RELAY) */ + +#define dns_init() (0) +#define dns_seems_to_be_broken() (0) +#define has_dns_init_failed() (0) +#define dns_cache_total_allocation() (0) + +#define dns_reset_correctness_checks() STMT_NIL + +#define assert_connection_edge_not_dns_pending(conn) \ + ((void)(conn)) +#define dump_dns_mem_usage(severity)\ + ((void)(severity)) +#define dns_cache_handle_oom(now, bytes) \ + ((void)(now), (void)(bytes), 0) + +#define connection_dns_remove(conn) \ + STMT_BEGIN \ + (void)(conn); \ + tor_assert_nonfatal_unreached(); \ + STMT_END + +static inline int +dns_reset(void) +{ + return 0; +} +static inline int +dns_resolve(edge_connection_t *exitconn) +{ + (void)exitconn; + tor_assert_nonfatal_unreached(); + return -1; +} + +#endif /* defined(HAVE_MODULE_RELAY) */ + #ifdef DNS_PRIVATE #include "feature/relay/dns_structs.h" diff --git a/src/feature/relay/include.am b/src/feature/relay/include.am index a4c025ae12..ad81e60d75 100644 --- a/src/feature/relay/include.am +++ b/src/feature/relay/include.am @@ -1,7 +1,6 @@ # Legacy shared relay code: migrate to the relay module over time LIBTOR_APP_A_SOURCES += \ - src/feature/relay/dns.c \ src/feature/relay/ext_orport.c \ src/feature/relay/onion_queue.c \ src/feature/relay/router.c \ @@ -12,6 +11,7 @@ LIBTOR_APP_A_SOURCES += \ # ADD_C_FILE: INSERT SOURCES HERE. MODULE_RELAY_SOURCES = \ + src/feature/relay/dns.c \ src/feature/relay/routermode.c \ src/feature/relay/relay_config.c \ src/feature/relay/relay_periodic.c \ From 51b470dbc84786f656c8d1f26eb4ff84cc588d00 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 19 Feb 2020 12:17:13 -0500 Subject: [PATCH 4/6] dns.h: label functions that are only used inside feature/relay --- src/feature/relay/dns.h | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/feature/relay/dns.h b/src/feature/relay/dns.h index 4e5766af0c..70abaf138b 100644 --- a/src/feature/relay/dns.h +++ b/src/feature/relay/dns.h @@ -16,14 +16,12 @@ int dns_init(void); int has_dns_init_failed(void); -void dns_free_all(void); int dns_reset(void); void connection_dns_remove(edge_connection_t *conn); void assert_connection_edge_not_dns_pending(edge_connection_t *conn); void assert_all_pending_dns_resolves_ok(void); MOCK_DECL(void,dns_cancel_pending_resolve,(const char *question)); int dns_resolve(edge_connection_t *exitconn); -void dns_launch_correctness_checks(void); int dns_seems_to_be_broken(void); int dns_seems_to_be_broken_for_ipv6(void); void dns_reset_correctness_checks(void); @@ -31,6 +29,11 @@ size_t dns_cache_total_allocation(void); void dump_dns_mem_usage(int severity); size_t dns_cache_handle_oom(time_t now, size_t min_remove_bytes); +/* These functions are only used within the feature/relay module, and don't + * need stubs. */ +void dns_free_all(void); +void dns_launch_correctness_checks(void); + #else /* !defined(HAVE_MODULE_RELAY) */ #define dns_init() (0) From f739aa79620ad47051c4fd5f7e576899da7e80b5 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 19 Feb 2020 12:27:49 -0500 Subject: [PATCH 5/6] Remove assert_all_pending_dns_resolves_ok(). It hasn't been used since 2009. --- src/core/mainloop/mainloop.c | 1 - src/feature/relay/dns.c | 19 ------------------- src/feature/relay/dns.h | 1 - 3 files changed, 21 deletions(-) diff --git a/src/core/mainloop/mainloop.c b/src/core/mainloop/mainloop.c index 260de181e5..e4e17f6b76 100644 --- a/src/core/mainloop/mainloop.c +++ b/src/core/mainloop/mainloop.c @@ -966,7 +966,6 @@ conn_close_if_marked(int i) return 0; /* nothing to see here, move along */ now = time(NULL); assert_connection_ok(conn, now); - /* assert_all_pending_dns_resolves_ok(); */ log_debug(LD_NET,"Cleaning up connection (fd "TOR_SOCKET_T_FORMAT").", conn->s); diff --git a/src/feature/relay/dns.c b/src/feature/relay/dns.c index 5f4bddab9d..3ff1378df5 100644 --- a/src/feature/relay/dns.c +++ b/src/feature/relay/dns.c @@ -971,25 +971,6 @@ assert_connection_edge_not_dns_pending(edge_connection_t *conn) #endif /* 1 */ } -/** Log an error and abort if any connection waiting for a DNS resolve is - * corrupted. */ -void -assert_all_pending_dns_resolves_ok(void) -{ - pending_connection_t *pend; - cached_resolve_t **resolve; - - HT_FOREACH(resolve, cache_map, &cache_root) { - for (pend = (*resolve)->pending_connections; - pend; - pend = pend->next) { - assert_connection_ok(TO_CONN(pend->conn), 0); - tor_assert(!SOCKET_OK(pend->conn->base_.s)); - tor_assert(!connection_in_array(TO_CONN(pend->conn))); - } - } -} - /** Remove conn from the list of connections waiting for conn-\>address. */ void diff --git a/src/feature/relay/dns.h b/src/feature/relay/dns.h index 70abaf138b..462cc6361d 100644 --- a/src/feature/relay/dns.h +++ b/src/feature/relay/dns.h @@ -19,7 +19,6 @@ int has_dns_init_failed(void); int dns_reset(void); void connection_dns_remove(edge_connection_t *conn); void assert_connection_edge_not_dns_pending(edge_connection_t *conn); -void assert_all_pending_dns_resolves_ok(void); MOCK_DECL(void,dns_cancel_pending_resolve,(const char *question)); int dns_resolve(edge_connection_t *exitconn); int dns_seems_to_be_broken(void); From defd941fe7f28264effffbbffb487ee4383ca2ba Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 19 Feb 2020 12:30:09 -0500 Subject: [PATCH 6/6] Make dns_cancel_pending_resolve() STATIC. It is not called by anything outside of the tests and dns.c. --- src/feature/relay/dns.c | 2 +- src/feature/relay/dns.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/feature/relay/dns.c b/src/feature/relay/dns.c index 3ff1378df5..99f48ab2c2 100644 --- a/src/feature/relay/dns.c +++ b/src/feature/relay/dns.c @@ -1028,7 +1028,7 @@ connection_dns_remove(edge_connection_t *conn) * the resolve for address itself, and remove any cached results for * address from the cache. */ -MOCK_IMPL(void, +MOCK_IMPL(STATIC void, dns_cancel_pending_resolve,(const char *address)) { pending_connection_t *pend; diff --git a/src/feature/relay/dns.h b/src/feature/relay/dns.h index 462cc6361d..120b75bf8d 100644 --- a/src/feature/relay/dns.h +++ b/src/feature/relay/dns.h @@ -19,7 +19,6 @@ int has_dns_init_failed(void); int dns_reset(void); void connection_dns_remove(edge_connection_t *conn); void assert_connection_edge_not_dns_pending(edge_connection_t *conn); -MOCK_DECL(void,dns_cancel_pending_resolve,(const char *question)); int dns_resolve(edge_connection_t *exitconn); int dns_seems_to_be_broken(void); int dns_seems_to_be_broken_for_ipv6(void); @@ -78,6 +77,7 @@ size_t number_of_configured_nameservers(void); tor_addr_t *configured_nameserver_address(const size_t idx); #endif +MOCK_DECL(STATIC void,dns_cancel_pending_resolve,(const char *question)); MOCK_DECL(STATIC int,dns_resolve_impl,(edge_connection_t *exitconn, int is_resolve,or_circuit_t *oncirc, char **hostname_out, int *made_connection_pending_out, cached_resolve_t **resolve_out));