Opened 6 years ago

Closed 5 years ago

#8380 closed defect (fixed)

DNS A records returned when asking for AAAA

Reported by: dhill Owned by:
Priority: Medium Milestone: Tor: 0.2.5.x-final
Component: Core Tor/Tor Version: Tor: unspecified
Severity: Keywords: DNSPort dns ipv6 tor-client 025-deferrable 025-triaged
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

I have Tor v0.2.5.0-alpha-dev running with DNSPort 53

Tor incorrectly returns an A record when asking for an AAAA

$ host -t aaaa google.com 127.0.0.1
Using domain server:
Name: 127.0.0.1
Address: 127.0.0.1#53
Aliases: 

google.com has address 74.125.132.138

Additionally, if there is no AAAA record, it still returns an A

Using Google's public DNS:

$ host -t aaaa chase.com 8.8.8.8   
Using domain server:
Name: 8.8.8.8
Address: 8.8.8.8#53
Aliases: 

chase.com has no AAAA record

Using Tor's DNSPort

$ host -t aaaa chase.com 127.0.0.1  
Using domain server:
Name: 127.0.0.1
Address: 127.0.0.1#53
Aliases: 

chase.com has address 159.53.84.126

Child Tickets

Change History (17)

comment:1 Changed 6 years ago by nickm

Keywords: tor-client added
Milestone: Tor: 0.2.4.x-final

We should see how far we are from making this work. If we're just a short bugfix away, it's for 0.2.4, but if not, we need to do it early in 0.2.5.

comment:2 Changed 6 years ago by nickm

So looking at the code in send_resolved_cell, I think we're cool there: we send back an IPv4 addr if we have that, and an IPv6 addr if we have that, and both if we have both.

Looking at the code in connection_ap_process_not_open() that handles RESOLVED cells, it appears that we're calling connection_ap_handshake_socks_resolved() with the first answer only, and ignoring subsequent answers.

It appears that dnsserv_resolved() can handle all answer types.

The right fix here will involve refactoring connection_ap_process_not_open() tand connection_ap_handshake_socks_resolved() a fair bit.

I'm inclined to call this an unimplemented feature and therefore put it into 0.2.5, not 0.2.4.

comment:3 Changed 6 years ago by nickm

Oh, also: there's no way AFAICT to ask for an IPv6 address over the SOCKS RESOLVE protocol extension.

And there doesn't seem to be any code to ensure that AAAA requests get sent to nodes that support them.

comment:4 Changed 6 years ago by nickm

Keywords: 024-deferrable added

comment:5 Changed 6 years ago by ln5

While testing #8475 (by trying to connect to ipv6.google.com using a
chutney network running master) I found out that
set_exitconn_info_from_resolve() won't return an IPv6 address unless
either exitconn->base_.purpose == EXIT_PURPOSE_RESOLVE or
exitconn->begincell_flags & BEGIN_FLAG_IPV6_OK != 0.

The former doesn't seem to happen when trying to connect and the
latter doesn't, from grepping code, seem to ever happen.

Commits 2889bd26 and 0f899518 seem to be the relevant ones
implementing this. I don't really understand the reason for the
is_resolve logic (and more). If Nick can spot an error here, great! if
not, I'll try ot wrap my head around this at a later time.

comment:6 in reply to:  5 Changed 6 years ago by nickm

Replying to ln5:

While testing #8475 (by trying to connect to ipv6.google.com using a
chutney network running master) I found out that
set_exitconn_info_from_resolve() won't return an IPv6 address unless
either exitconn->base_.purpose == EXIT_PURPOSE_RESOLVE or
exitconn->begincell_flags & BEGIN_FLAG_IPV6_OK != 0.

The former doesn't seem to happen when trying to connect and the
latter doesn't, from grepping code, seem to ever happen.

exitconn->begincell_flags is set in connection_exit_begin_conn().

EXIT_PURPOSE_RESOLVE is supposed to be set in connection_exit_begin_resolve().


comment:7 Changed 6 years ago by nickm

Milestone: Tor: 0.2.4.x-finalTor: 0.2.5.x-final

Calling this 0.2.5.x -- if it's not working yet, and it's not a regression, it's not in 0.2.4.

comment:8 Changed 5 years ago by nickm

Keywords: 025-deferrable added; 024-deferrable removed

marking as deferrable if nobody shows up with a patch here.

comment:9 Changed 5 years ago by nickm

Investigated more. At first glance, it appears that the

  if (conn->base_.type == CONN_TYPE_AP &&
      rh->command == RELAY_COMMAND_RESOLVED) {

case of connection_edge_process_relay_cell_not_open() is totally fubar. More investigation needed though; it's possible that the code that generates RESOLVED cells is fubar too or instead.

comment:10 Changed 5 years ago by nickm

Ah.

The code in connection_edge_process_relay_cell_not_open() that handled RELAY_COMMAND_RESOLVED never looked at any answer after the first answer, no matter what you asked for. I've started trying to fix it. There's an incomplete branch in "bug8380" in my public repository. It's incomplete because:

  • It requires the fix to #10468 to be merged as well
  • It needs unit tests
  • There's an XXXX I need to resolve there.

comment:11 Changed 5 years ago by nickm

Keywords: 025-triaged added

comment:12 Changed 5 years ago by nickm

Now that #10468 is merged to 0.2.5, I've rebased this onto a more recent master. The branch is now called "bug8380_re". Still needs:

  • Testing
  • The XXXX to get resolved

comment:13 Changed 5 years ago by andrea

This mostly looks okay to me; some comments:

  • Yeah, that XXXX in connection_ap_handshake_socks_got_resolve_cell()
  • Iterating over the list in that function, too - we always end up using the *last* address of the preferred address type, where most things have round-robin or random behavior when they get multiple answers for a DNS query. Are we sure that's what we want?

comment:14 Changed 5 years ago by nickm

It doesn't take the last address; it takes the first one.

    } else if (tor_addr_family(&addr->addr) == AF_INET6) {
      if (!addr_ipv6 && conn->ipv6_traffic_ok) {
        addr_ipv6 = addr;
      }
    }

Note how it doesn't set addr_ipv6 unless addr_ipv6 was previously NULL.

Will revisit other questions once the tests are done.

comment:15 Changed 5 years ago by nickm

I think I'm happy with the branch "bug8380_re" now. The new code is 100% covered, and it seems to work okay. The XXXX is fixed.

Iterating over the list in that function, too - we always end up using the *last*first address of the preferred address type, where most things have round-robin or random behavior when they get multiple answers for a DNS query. Are we sure that's what we want?

I'm not so sure, but it seems like a different ticket.

(The real fix for that would be to implement proposal 219-expanded-dns.txt IMO)

comment:16 Changed 5 years ago by nickm

Status: newneeds_review

comment:17 Changed 5 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed
15:32 < nickm> ok, good to merge 8380?
15:34 < athena> yeah

Squashed as bug8380_re_squashed and merged. Thanks!

Note: See TracTickets for help on using tickets.