[tor-dev] Support for full DNS resolution and DNSSEC validation

Fri May 15 15:39:23 UTC 2020

On Thu, 2020-05-14 at 15:56 -0400, David Goulet wrote:
> On 26 Apr (19:37:56), Christian Hofer wrote:
> > Hi there,
> 
> Greetings Christian!
> 

Hi David!

> > I have a proposal regarding DNS name resolution.
> > 
> > Ticket: https://trac.torproject.org/projects/tor/ticket/34004
> > Proposal: 
> > https://trac.torproject.org/projects/tor/attachment/ticket/34004/317-secure-dns-name-resolution.txt
> > Implementation: https://github.com/torproject/tor/pull/1869
> 
> First, this is quite impressive piece of work. I was _NOT_ expecting
> a 27k
> line diff ;).
> 

Yeah this might look scary at first, but if you take a closer look it
is not that bad.

More than half of the lines account for tests: 15,437

3,584 test_crypto_dnssec.c
1,456 test_crypto_dnssec_openssl.c
  726 test_dns_message.c
5,105 test_dns_resolver.c
1,389 test_dns_string.c
3,177 test_dns_wireformat.c

Another big part accounts for containers and translators: 6,531

They don't contain much logic and their main purpose is to translate
DNS message <-> wireformat and DNS message <-> string. So this area
shouldn't change very often, except you want to add support for new DNS
types or the specification for an existing DNS type changes, which
shouldn't actually happen.

1,150 dns_message.c
  173 dns_message.h
  457 dns_message_st.h
  199 dns_types.h 
1,564 dns_string.c
   80 dns_string.h
2,586 dns_wireformat.c
  322 dns_wireformat.h

The most interesting part accounts (only) for 4,970 lines (2,066
relevant lines). It contains all the DNS resolution and DNSSEC
validation logic. 

* The dns_lookup_st contains state related types that lives in AP
connections (entry_connection_t).
* The dns_resolver is responsible for sending and receiving DNS
messages, validating DNS messages using crypto_dnssec, and caching DNS
messages.
* The crypto_dnssec contains all required DNSSEC algorithms.

lines / relevant / coverage / filename 

   57 /        0 /    100.0 / dns_lookup_st.h
2,564 /    1,197 /    98.75 / dns_resolver.c
  179 /        0 /    100.0 / dns_resolver.h 
1,478 /      684 /    98.83 / crypto_dnssec.c
  495 /      185 /    99.46 / crypto_dnssec_openssl.c
  197 /        0 /    100.0 / crypto_dnssec.h

These numbers were taken from the coveralls report: 
https://coveralls.io/builds/30352518

> So the proposal looks very good. I like the idea very much. I
> honestly thought
> that you were about to propose a way for Tor to talk to an *external*
> DNS
> resolver client application (third part) but I see that client DNSSEC
> is
> basically implemented in tor with your patch which is... interesting?
> 
> Before we go further, can you walk me through the reasons (if you had
> thought
> of it of course) why you didn't use something like libunbound?
> 

I might be wrong, please correct me if so. The scenario I was aiming
for is when the target address that is received within the socks
handshake in connection_ap_handshake_process_socks is a hostname. As
far as I understand it, in this case the name resolution is done at the
exit relay that is also responsible for retrieving the contents from
the target. This means that the user has to trust the exit relay in
terms of name resolution? However, I am not sure if this is a valid use
case or if you can cover this using libunbound. So please let me know.

Using the DNS resolver would resolve the hostname upfront in a separate
connection that is not related to the connection that is responsible
for retrieving the contents from the target. Additionally, the user has
control over the nameserver that should be used for name resolution and
is also able to verify the response using DNSSEC. So this should
reduces the number of entities that must be trusted.

> There are side effects of adding DNSSEC client support (with our own
> implementation) that we, people maintaining tor, have to become
> DNSSEC expert
> in some ways just to be able to understand what is happening in that
> code, fix
> issues but also possibly implement new features if any. That is where
> a third
> part library like unbound becomes very nice because they are the
> experts at
> providing such features.
> 

I can understand this.

> Of course, everytime we have to link to an external library we do it
> carefully
> and with considerations because of the "yet another attack" vector
> problem.
> But adding that much code to support a well known feature like DNSSEC
> also has
> huge implications for tor maintainability and security.
> 
> Finally, something I noticed and made me itch a bit. You hardcoded a
> lot of
> .onions where one appears to be Cloudflare (?) resolver. What are the
> other
> addresses? I worry here because default options are always the one
> used the
> most so I'm concerned here about shipping hardcoded addresses
> _within_ our C
> code.
> 

Regarding the the default configuration. There is a configuration
called DNSResolver which defaults to off. As long as this flag is
turned off nothing changes related to DNS resolution.
Yes, the first onion is the Cloudflare resolver. The other onion
addresses are DNS resolvers (PowerDNS Recursor) behind hidden services
which I used and am still using for testing. They can be easily changed
/ removed (?) since it is only a configuration and as mentioned if you
want to activate this feature you have to enable the DNSResolver flag
first.

Final remarks. When I started, I didn't expect it to get this big, and
frankly, if I had known before, I might not have even started. However,
I learned a lot about DNS, DNSSEC, SOCKS, and Tor. So even if you
decide not to merge it, it was not a waste :-)

In the end you have to decide if you want to add this and I can
understand if you don't want to maintain DNS related code. At least it
would be interesting how the different solutions compare regarding
performance, security, and maintainability. What do you think?

> Thanks!
> David

BR
Christian

> 
> _______________________________________________
> tor-dev mailing list
> tor-dev at lists.torproject.org
> https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-dev