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@lists.torproject.org > https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-dev _______________________________________________ tor-dev mailing list tor-dev@lists.torproject.org https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-dev