Opened 3 years ago

Closed 2 years ago

#24050 closed defect (fixed)

We still do client-side caching. We just don't use the cache.

Reported by: arma Owned by: nickm
Priority: Medium Milestone: Tor: 0.3.2.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: 032-backport, ???-backport
Cc: Actual Points:
Parent ID: Points:
Reviewer: dgoulet Sponsor:


In Tor, we said Turn off the client-side DNS cache by default. (Proposal 205, #7570)

But actually, in port_cfg_new() we say

  cfg->entry_cfg.cache_ipv4_answers = 1;

And similarly in parse_port_config() we say

      cache_ipv4 = 1,

Now, fortunately, there is a bonus surprise field named use_cached_ipv4 and that is left to 0. You can read more in commit ac990aa44a which removes the line

-  cfg->use_cached_ipv4_answers = 1;

Now, that's all well and good -- we keep a client-side dns cache but we don't use it. I hacked up my Tor client to confirm, and it's really true.

But then there's commit bbad23b, which says No, client-side DNS cacheing should not be on by default. and its patch to parse_unix_socket_config() is

-        port->entry_cfg.cache_ipv4_answers = 1;
-        port->entry_cfg.cache_ipv6_answers = 1;
+        port->entry_cfg.cache_ipv4_answers = 0;
+        port->entry_cfg.cache_ipv6_answers = 0;

But then in commit 485fdcf82 we took out that whole parse_unix_socket_config() concept, unifying stuff back to the "yes, keep the client-side dns cache" default.

Do we want to set the cache to default to off? Since we don't use it? Does anything need it to be on? It would seem that anything that relies on it is a bug (and a surprise) at this point.

Child Tickets

Change History (15)

comment:1 Changed 3 years ago by nickm

Keywords: 032-backport ???-backport added
Milestone: Tor: 0.3.3.x-final

comment:2 Changed 3 years ago by arthuredelstein

Let's say we're using Tor Browser to visit a website with embedded content (images, scripts, etc.) from various third parties. Each domain needs a DNS resolve of course. Now, if at some point a stream times out for any reason, the circuit is closed and we start using a new circuit. Doesn't that mean we need to resolve all of our domains over again?

So from Tor Browser's point of view, it seems we would have a performance benefit (not sure how significant) from a client-side DNS cache keyed by first-party domain. Just as the content cache and other state in the browser is first-party isolated. In the tor proxy, the Tor Browser indicates the first party domain via SOCKS credentials. Then, for a given website, even when we switch circuits, we still have the domain->IP mappings.

Last edited 3 years ago by arma (previous) (diff)

comment:3 Changed 3 years ago by nickm

One big problem with client-side DNS caching in Tor is that if the exit on the first circuit lies about the IP address, the exit on the second circuit will be told about the false IP address too. In this way, one bad exit can set up a "sticky" MITM that will persist even on a new circuit if the user is using the same DNS cache. Similarly, IPv6 addresses can trivially be used to set up unique client identifiers that will last for as long as the DNS cache lasts.

How bad is this attack? Consider:

  1. The more we reuse DNS caches across multiple circuits, the worse this attack gets... but on the other hand, the DNS cache is only beneficial to the extent that we can reuse it.
  1. These attacks seem especially bad when performed against uncommon sites... but common sites are likely to be in the exit-side DNS cache, making client-side caching unnecessary.

So it seems to me that client-side DNS caching is risky to the extent that it is useful, and vice versa. :)

One more consideration: client-side DNS caching can also lower performance for big sites that use CDNs to match exits with nearby servers.

comment:4 Changed 3 years ago by nickm

Status: newneeds_review

For review in branch bug24050_029

comment:5 Changed 3 years ago by cypherpunks

DNS on the DNSPort is very slow without the cache. I just want it to work and I'm not bothered in the least about caching a bad reply - I can just NEWNYM if that happens. Here I am letting you know, as in the message about deprecation!

Last edited 3 years ago by cypherpunks (previous) (diff)

comment:6 Changed 3 years ago by nickm

I can just NEWNYM if that happens.

How would you know ifit happened?

comment:7 Changed 3 years ago by nickm

Owner: set to nickm
Status: needs_reviewaccepted

(setting owner)

comment:8 Changed 3 years ago by nickm

Status: acceptedneeds_review

comment:9 Changed 3 years ago by nickm

Keywords: review-group-26 added

Creating review-group-26.

comment:10 Changed 3 years ago by dgoulet

Reviewer: dgoulet

comment:11 Changed 3 years ago by dgoulet

Status: needs_reviewneeds_revision

Hmmm... commit be973b3f38386c84 has this but doesn't seem relevant to the fix at all?

+  int fd; /**< FD for the file, if it's extensible. */

Else lgtm;

comment:12 Changed 3 years ago by nickm

Milestone: Tor: 0.3.3.x-finalTor: 0.3.1.x-final
Status: needs_revisionmerge_ready

yikes; that was indeed unrelated. Sorry!

Added a fixup commit and squashed as bug24050_029_squashed, then added a unit test fix.

Merged to 032 and forward; marking for possible backport.

comment:13 Changed 3 years ago by arma

I would argue 'no backport', on the theory that everything was basically ok before, and this change is just cleaning things up so we don't hurt ourselves in the future.

comment:14 Changed 3 years ago by nickm

Keywords: review-group-26 removed

comment:15 Changed 2 years ago by nickm

Milestone: Tor: 0.3.1.x-finalTor: 0.3.2.x-final
Resolution: fixed
Status: merge_readyclosed

okay, no backport. The suspenders where there; the belt is superfluous.

Note: See TracTickets for help on using tickets.