Handle empty DNS reply without error as NOERROR

When after DNS resolution we don't receive an error from a relay, the response cell contains IPv4 and/or IPv6 addresses. However, when we only want to retrieve one of these (e.g. A lookup), and the response cell contains only the other type, this is currently handled as an error, resulting in an NXDOMAIN response. This prevents further queries for the other type (AAAA lookup), making IPv6/IPv4 only services unreachable in programs that use DNSPort for resolution. This merge request changes the current behavior to handle such cases gracefully and send NOERROR response instead, so that programs will initiate a second lookup for the other type.

Fixes #40248 (closed)

requested review from @dgoulet

changed milestone to %Tor: 0.4.9.x-freeze

This looks good. However, we'll need a spec change here to add the 0xF2 value in https://gitlab.torproject.org/tpo/core/torspec/-/blob/main/spec/tor-spec/remote-hostname-lookup.md?ref_type=heads

Once we have that, we can merge this. Thanks!

mentioned in merge request torspec!290 (merged)

approved this merge request

merged

mentioned in commit 7383d462

mentioned in issue arti#1687

Hi. I've come late to this party, but I find this confusing. I don't understand the problem description.

torspec!290 (merged) introduces a new error code for "Empty response, no error". I think this is supposed to correspond to NODATA (the DNS pseudo-RCODE).

But why wouldn't we represent NODATA simply by sending an empty set of addresses in the RELAY_RESOLVED message?

I also don't understand who is turning NODATA into NXDOMAIN. C Tor? But couldn't it just turn the empty message into a NODATA response (NOERROR, no answers)?

Hi @Diziet ,

when a tor relay returns a set of addresses associated with a host name, the tor client filters the result based on the queried type A/AAAA. If a service has no matching IP to our query, an error is returned !830 (diffs) , which is handled as NXDOMAIN: !830 (diffs) Since returning NXDOMAIN prevents any further queries, the response code here is changed to NOERROR with an empty result.

For example a client first looks for an A record of an IPv6 only service. This previously resulted in NXDOMAIN and the client aborting. New behavior is to send an empty response which will result in a client then attempting to fetch an AAAA record and finding the desired IP.

Hi. I think I understand what you've said. I think I understand the bug you are describing.

But, I still don't understand why a protocol change was needed, to fix it.

Why couldn't the Tor client simply handle the empty resulting address list as a NODATA response? Ie, I don't understand why this couldn't have been fixed simply by changing the client code, to not turn an empty address list into NXDoMAIN (which I think is wrong, regardless).

This is exactly what I have done here. There is no change in any of the relay packages, it is only client side within the tor client. Maybe the confusion comes from adding the new constant for the NODATA reply? This was previously not existing in tor and within the function that handles parsing and comparing the received DNS response connection_ap_handshake_socks_got_resolved_cell(), it will either set an error code, or send the found IP to go further down the function chain through connection_ap_handshake_socks_resolved() until in dnsserv_resolved() it gets sent to the DNS client. With currently only two error types being available, which are handled as NXDOMAIN or SERVFAIL, it made sense to me to add a new error code that will be used for the NODATA response. Anything else will likely require more complex changes.

To sum it up, this change introduced a new error code to differentiate it from the existing error codes and instead of sending NXDOMAIN for when no match between wanted address family and received address family is found, it sends NOERROR with an empty result (NODATA).

So, if I may summarise:

The function you needed to edit, controls its behaviour with error codes, and the most convenient way to implement the new behaviour was to have it represent this situation with a new error code.

But if these error codes never appear in messages they ought not to be in the spec. The spec as changed in torspec!290 (merged) says that the server ought to send this error code at some point. But, from what you say, that's neither necessary nor intended.

So this ought to have been done without a spec change.

That does leave the question of what to do in C Tor in this area. Perhaps C Tor could use a wider type than the single byte error code, and then it could invent a pseudo-error-code locally.

I don't really have an opinion about how the C Tor code should be structured. But we ought not to be allocating a code specially for internal use in C Tor.

Yes, correct. This is only used internally in C Tor. It is never sent as a message, because the exit node will send us available IP addresses, but the C Tor client then filters them based on the DNS query and return the error code if no match was found.

mentioned in merge request arti!2528 (closed)

mentioned in issue torspec#278 (closed)

mentioned in issue #40984 (closed)

mentioned in commit torspec@9402f99f

mentioned in issue #40248 (closed)