Opened 4 years ago

Closed 3 years 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:
Severity: Normal Keywords: dns, TorCoreTeam201607, 029-proposed, review-group-15
Cc: tobias.pulls@…, pulls Actual Points:
Parent ID: Points: 0.5
Reviewer: Sponsor:


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 4 years 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:

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

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

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

comment:2 Changed 4 years ago by teor

Keywords: TorCoreTeam201607 029-proposed added
Milestone: Tor: 0.2.???
Points: 0.5
Status: newneeds_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 4 years 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 4 years ago by pulls

Cc: pulls added

comment:5 Changed 4 years 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 4 years ago by nickm

Milestone: Tor: 0.2.???Tor: 0.3.0.x-final

comment:7 Changed 3 years ago by nickm

Priority: MediumVery High

comment:8 Changed 3 years ago by nickm

Status: needs_revisionmerge_ready

comment:9 Changed 3 years ago by nickm

We should merge this whenever we merge #19769.

comment:10 Changed 3 years ago by nickm

Keywords: review-group-15 added

comment:11 Changed 3 years ago by nickm

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

comment:12 Changed 3 years ago by nickm

Resolution: fixed
Status: merge_readyclosed
Note: See TracTickets for help on using tickets.