Opened 13 months ago

Closed 4 months ago

#19025 closed defect (fixed)

Exit relays always return DNS TTL 60 to tor clients

Reported by: phw Owned by:
Priority: Very High Milestone: Tor: 0.3.0.x-final
Component: Core Tor/Tor Version: Tor: 0.2.8.2-alpha
Severity: Normal Keywords: dns, TorCoreTeam201607, 029-proposed, review-group-15
Cc: tobias.pulls@…, pulls Actual Points:
Parent ID: Points: 0.5
Reviewer: Sponsor:

Description

When tor clients resolve a domain name, exit relays are supposed to return the DNS TTL as part of their response.

At the moment, it looks like exit relays always return TTL 0 for both A and AAAA records. Only PTR records seem to come with a TTL > 0. The relevant variables on the exit side are ttl_ipv4 and ttl_ipv6 in src/or/dns_structs.h. The variables should be initialised in the function cached_resolve_add_answer. The variable ttl_hostname for PTR records is assigned ttl:

resolve->ttl_hostname = ttl;

The variables ttl_ipv4 and ttl_ipv6, however, are not. Therefore, exit relays always send back TTL 60 to clients (60 instead of 0 because the function dns_clip_ttl turns it into MIN_DNS_TTL, i.e., 60).

Commit 2889bd264 added the code to tor. It added ttl_hostname, ttl_ipv4 and ttl_ipv6, but never initialised the latter two. I wonder if this is an oversight? Commit c660a0f6 talks about potential attacks, but I don't think that explains this issue.

Child Tickets

Change History (12)

comment:1 Changed 10 months ago by phw

This bug also affects an exit relay's DNS cache. Exits cache DNS responses for the duration of their TTL (see make_pending_resolve_cached), but since they are always set to 0, we end up with MIN_DNS_TTL (see dns_get_expiry_ttl), which is 60. So each domain, regardless of its TTL, is cached for only 60 seconds, resulting in more DNS requests than necessary.

I have a patch in the branch bug-19025 in the following repository:
https://github.com/NullHypothesis/tor

I briefly tested it on my exit relay, and it seems to work. The following log is the result of requesting the domain cartography.nymity.ch three times. The exit's cache was cold.

Jul 27 18:17:14.000 [notice] Added domain cartography.nymity.ch with expiry=1800, ttlv4=10800, ttlv6=0, ttlhost=0 to cache.
Jul 27 18:17:22.000 [notice] Address cartography.nymity.ch was already in cache, expire=1792.
Jul 27 18:23:59.000 [notice] Address cartography.nymity.ch was already in cache, expire=1395.

My Tor client also received the correct TTL from the exit.

comment:2 follow-up: Changed 10 months ago by teor

  • Keywords TorCoreTeam201607 029-proposed added
  • Milestone set to Tor: 0.2.???
  • Points set to 0.5
  • Status changed from new to needs_revision

Design Comments:

I would feel more comfortable if we rounded down each TTL received from a DNS server, to avoid tagging attacks.

But I think that's probably a separate ticket, split off into #19769.

I'd like to merge both tickets in the same release, so that we only ever send rounded values for IPv4 and IPv6 TTLs.

Code Review:

This is a 2-line patch, that we really should put in 0.2.9 with #19769 if we can.
(It seems that #19769 would also be a very small change.)

It needs a changes file tor/changes/bug19025, matching the format of the other bug* files in that directory. It could easily be created using ticket and the commit comment.

phw, do you want to do the changes file? Do you want to do #19769?
I'm happy to do either.

comment:3 in reply to: ↑ 2 Changed 10 months ago by phw

Replying to teor:

phw, do you want to do the changes file? Do you want to do #19769?
I'm happy to do either.

Please go ahead.

comment:4 Changed 10 months ago by pulls

  • Cc pulls added

comment:5 Changed 8 months ago by teor

I think we should just continue with the TTL 60 behaviour for all clients, until we're sure what we want to do about #19769

comment:6 Changed 7 months ago by nickm

  • Milestone changed from Tor: 0.2.??? to Tor: 0.3.0.x-final

comment:7 Changed 5 months ago by nickm

  • Priority changed from Medium to Very High

comment:8 Changed 5 months ago by nickm

  • Status changed from needs_revision to merge_ready

comment:9 Changed 5 months ago by nickm

We should merge this whenever we merge #19769.

comment:10 Changed 4 months ago by nickm

  • Keywords review-group-15 added

comment:11 Changed 4 months ago by nickm

I've rolled this into my branch for #19769.

comment:12 Changed 4 months ago by nickm

  • Resolution set to fixed
  • Status changed from merge_ready to closed
Note: See TracTickets for help on using tickets.