Opened 12 years ago

Last modified 6 years ago

#619 closed defect (Fixed)

exit-policy-reject-star relays should refuse dns?

Reported by: arma Owned by:
Priority: Low Milestone: 0.2.0.x-final
Component: Core Tor/Tor Version: 0.2.0.21-rc
Severity: Keywords:
Cc: arma, sjmurdoch, nickm, rovv Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

lodger points out that non-exit relays could reject dns and reverse dns
attempts. (Currently clients try not to ask them any questions, but the
relays don't enforce it. Non-exit relays might be surprised at the dns
requests they are forced to do. "also permit reverse resolve for private
addresses, which could lead to leaks of names, in normal circumstances,
only available locally."

Here's his patch:

--- dns.c Tue Feb 26 19:56:28 2008
+++ dns.c Sat Mar 8 12:11:34 2008
@@ -550,7 +550,12 @@

char *hostname = NULL;
is_resolve = exitconn->_base.purpose == EXIT_PURPOSE_RESOLVE;


  • r = dns_resolve_impl(exitconn, is_resolve, oncirc, &hostname);

+ routerinfo_t *me = router_get_my_routerinfo();
+ if (is_resolve && me &&
+ policy_is_reject_star(me->exit_policy)) /* non-exit */
+ r = -1;
+ else
+ r = dns_resolve_impl(exitconn, is_resolve, oncirc, &hostname);

switch (r) {

case 1:

/* We got an answer without a lookup -- either the answer was

@@ -659,9 +664,12 @@

  • .in-addr.arpa address but this isn't a resolve request, kill the
  • connection. */
  • if ((r = parse_inaddr_arpa_address(exitconn->_base.address, NULL)) != 0) {
  • if (r == 1)

+ if ((r = parse_inaddr_arpa_address(exitconn->_base.address, &in)) != 0) {
+ if (r == 1) {

is_reverse = 1;

+ if (is_internal_IP(ntohl(in.s_addr), 0)) /* internal address */
+ return -1;
+ }

if (!is_reverse
!is_resolve) {

if (!is_reverse)

[Automatically added by flyspray2trac: Operating System: All]

Child Tickets

Change History (10)

comment:1 Changed 12 years ago by arma

lodger: huh. this is really two patches, isn't it. and the second one is

more of a security patch, that even exit relays might want to apply.

lodger> arma: yes. second part for exit relay.

comment:2 Changed 12 years ago by sjmurdoch

First part looks fine to me. Assuming clients don't ask non-exit relays DNS queries, it shouldn't break anything*.

I can think of some cases where operators of middleman nodes might be surprised of DNS requests, for example in

places which do split-horizon DNS or if they have a premium subscription to a DNS blacklisting service (e.g. MAPS).

The second part is similar. I can think of places where reverse-resolving internal IP addresses would leak some
information (e.g. Cambridge) and I can't think of what this would break. However. it will not fix all the problems.
For example, in one site I am aware of they have internal-IP addresses which are not from RFC1918 (this was from the
time that IPv4 addresses were sufficiently abundant that internal networks were given globally-routable ones).

The patch will cause a problem for setups which send all reverse-DNS inquiries over Tor. Do the virtual machine
bundles of Tor do this? How will they fail if some reverse DNS fails. I would have thought it would be OK --
reverse DNS fails all the time.

The right solution is for Tor exit nodes to use a recursive resolver with no special access to information. Still,
that doesn't mean we shouldn't deal with the common case, which is what this patch addresses.

  • I did have an idea a while back of using this "feature" to identify which recursive resolver all Tor nodes are

using. If two nodes share the same one, then maybe they should be in the same family. I never implemented this,
and the patch will break this feature, but maybe that's OK.

comment:3 Changed 11 years ago by nickm

Applied in svn.

comment:4 Changed 11 years ago by nickm

Reopened by request from rovv.

comment:5 Changed 11 years ago by rovv

non-exit relays can still be induced to do arbitrary DNS requests
if 'begin' relay cells contains DNS hostnames.

Some optimal solution for task: non-exit relays should answer with
'resolve_failed' for 'begin' or 'resolve' relay cells if it contained
DNS hostnames, and check of cells contained ip addresses with usual
conditions.

thats could be normal behaviour for well-behaved clients also, if
client have 'obsolete' descriptor for some relay which newest
descripor include non-exit policy.
(if connection was requested to ip address and 'REASON_EXITPOLICY'
was received then marking relay as non-exit, else don't)

it's possible balanced fix:

--- dns.original.c Fri Aug 22 17:52:26 2008
+++ dns.c Sat Oct 18 06:54:52 2008
@@ -553,15 +553,9 @@

or_circuit_t *oncirc = TO_OR_CIRCUIT(exitconn->on_circuit);
int is_resolve, r;
char *hostname = NULL;

  • routerinfo_t *me;

is_resolve = exitconn->_base.purpose == EXIT_PURPOSE_RESOLVE;


  • if (is_resolve &&
(!(me = router_get_my_routerinfo())
  • policy_is_reject_star(me->exit_policy))) /* non-exit */
  • r = -1;
  • else
  • r = dns_resolve_impl(exitconn, is_resolve, oncirc, &hostname);

+ r = dns_resolve_impl(exitconn, is_resolve, oncirc, &hostname);

switch (r) {

case 1:

/* We got an answer without a lookup -- either the answer was

@@ -636,6 +630,7 @@

cached_resolve_t *resolve;
cached_resolve_t search;
pending_connection_t *pending_connection;

+ routerinfo_t *me;

struct in_addr in;
time_t now = time(NULL);
uint8_t is_reverse = 0;

@@ -652,6 +647,12 @@

exitconn->address_ttl = DEFAULT_DNS_TTL;
return 1;

}

+ /* non-exit relay not even attempts do resolve hostname,
+ * request will be rejected in any case. */

+ if (!(me = router_get_my_routerinfo())

+ policy_is_reject_star(me->exit_policy)) {
+ return -1;
+ }

if (address_is_invalid_destination(exitconn->_base.address, 0)) {

log(LOG_PROTOCOL_WARN, LD_EXIT,

"Rejecting invalid destination address %s",

comment:6 Changed 11 years ago by rovv

Sorry about suggestion above fix:
It's not smart to call policy_is_reject_star() for every
connection/resolve.

I prefer if 0.2.0.x possibly not be affected with above patch.
(currently only checks for much rare 'resolve' requests)

later 0.2.x could have more complex patch with more test period.

a first draft version of smart more patch:

--- or.original.h Tue Sep 30 09:32:54 2008
+++ or.h Mon Oct 20 19:00:18 2008
@@ -4100,6 +4100,7 @@

void router_new_address_suggestion(const char *suggestion,

const dir_connection_t *d_conn);

int router_compare_to_my_exit_policy(edge_connection_t *conn);

+int router_my_router_non_exit(void);

routerinfo_t *router_get_my_routerinfo(void);
extrainfo_t *router_get_my_extrainfo(void);
const char *router_get_my_descriptor(void);

--- router.original.c Sat Sep 27 02:32:40 2008
+++ router.c Mon Oct 20 19:12:10 2008
@@ -1066,6 +1066,8 @@

static time_t desc_clean_since = 0;
/ Boolean: do we need to regenerate the above? */
static int desc_needs_upload = 0;

+/ Boolean: are we non-exit relay? */
+static int desc_with_non_exit_policy = 0;

/ OR only: If <b>force</b> is true, or we haven't uploaded this

  • descriptor successfully yet, try to upload our signed descriptor to

@@ -1128,6 +1130,17 @@

desc_routerinfo->exit_policy) != ADDR_POLICY_ACCEPTED;

}


+/ OR only: Check whether my exit policy says about connection to any address
+ * Return 0 if we allow exit connections; non-0 if we disallow any.
+ */
+int
+router_my_router_non_exit(void)
+{
+ if (!router_get_my_routerinfo()) /* make sure desc_routerinfo exists */
+ return -1;
+ return desc_with_non_exit_policy;
+}
+

/ Return true iff I'm a server and <b>digest</b> is equal to

  • my identity digest. */

int

@@ -1414,6 +1427,10 @@

extrainfo_free(desc_extrainfo);

desc_extrainfo = ei;


+ if (policy_is_reject_star(ri->exit_policy)) /* we are non-exit */
+ desc_with_non_exit_policy = 1;
+ else
+ desc_with_non_exit_policy = 0;

desc_clean_since = time(NULL);
desc_needs_upload = 1;
control_event_my_descriptor_changed();

--- dns.original.c Fri Aug 22 17:52:26 2008
+++ dns.c Mon Oct 20 19:13:52 2008
@@ -553,15 +553,9 @@

or_circuit_t *oncirc = TO_OR_CIRCUIT(exitconn->on_circuit);
int is_resolve, r;
char *hostname = NULL;

  • routerinfo_t *me;

is_resolve = exitconn->_base.purpose == EXIT_PURPOSE_RESOLVE;


  • if (is_resolve &&
(!(me = router_get_my_routerinfo())
  • policy_is_reject_star(me->exit_policy))) /* non-exit */
  • r = -1;
  • else
  • r = dns_resolve_impl(exitconn, is_resolve, oncirc, &hostname);

+ r = dns_resolve_impl(exitconn, is_resolve, oncirc, &hostname);

switch (r) {

case 1:

/* We got an answer without a lookup -- either the answer was

@@ -652,6 +646,11 @@

exitconn->address_ttl = DEFAULT_DNS_TTL;
return 1;

}

+ /* non-exit relay not even attempts do resolve hostname,
+ * request will be rejected in any case. */
+ if (router_my_router_non_exit()) {
+ return -1;
+ }

if (address_is_invalid_destination(exitconn->_base.address, 0)) {

log(LOG_PROTOCOL_WARN, LD_EXIT,

"Rejecting invalid destination address %s",

comment:7 Changed 11 years ago by nickm

I think I'll go with the original fix; policy_is_reject_star() is pretty fast. If it shows up in profiles, we
can start caching it.

"Premature optimization is the root of all evil in programming." -- Don Knuth

comment:8 Changed 11 years ago by nickm

Applied as r17138.

comment:9 Changed 11 years ago by nickm

flyspray2trac: bug closed.

comment:10 Changed 7 years ago by nickm

Component: Tor RelayTor
Note: See TracTickets for help on using tickets.