[tor-bugs] #23502 [Core Tor/Tor]: prop224: Don't make IPv4 mandatory because one day we'll have IPv6 only relays

[Tor Bug Tracker & Wiki]

#23502: prop224: Don't make IPv4 mandatory because one day we'll have IPv6 only
relays
----------------------------------------+----------------------------------
 Reporter:  dgoulet                     |          Owner:  dgoulet
     Type:  defect                      |         Status:
                                        |  needs_information
 Priority:  Medium                      |      Milestone:  Tor:
                                        |  0.3.3.x-final
Component:  Core Tor/Tor                |        Version:
 Severity:  Normal                      |     Resolution:
 Keywords:  prop224, tor-relay, tor-hs  |  Actual Points:
Parent ID:  #23493                      |         Points:
 Reviewer:  asn                         |        Sponsor:
----------------------------------------+----------------------------------
Changes (by teor):

 * status:  needs_revision => needs_information
 * parent:  #23820 => #23493
 * milestone:  Tor: 0.3.2.x-final => Tor: 0.3.3.x-final


Comment:

 I just reviewed this code in detail, and I don't think we can include it
 as a bugfix in 0.3.2. And I don't think we can merge it to 0.3.3 as it is,
 because we need to work out some design issues first. See my detailed
 review below.

 Also, I think we should rebase this patch on top of #23820, which will rip
 out all of the IPv6 code as a bugfix on 0.3.2.

 But I think we should merge the spec branch.

 Code review:

 circuit_send_intermediate_onion_skin() and extend_cell_format() assume
 that all extend infos sent to them contain IPv4 addresses. So this code
 will trigger a bug when the rendezvous link specifiers from the untrusted
 remote client contain only an IPv6 address. In particular, you will end up
 packing 0.0.0.0 into the extend cell, rather than the intended IPv6
 address. I've split this bug in those functions off into #240000.
 (Winner!)

 We can't choose IPv6 extends until we support them, so this part of the
 code results in a bug, and the following comment is confusing:

 {{{
    /* By default, we pick IPv4 but this might change to v6 if certain
 -   * conditions are met. */
 -  addr = &addr_v4; port = port_v4;
 +   * conditions are met or we simply don't have v4. */
 +  if (have_v4) {
 +    addr = &addr_v4; port = port_v4;
 +  } else {
 +    tor_assert(have_v6);
 +    addr = &addr_v6; port = port_v6;
 +  }

    /* If we are NOT in a direct connection, we'll use our Guard and a
 3-hop
     * circuit so we can't extend in IPv6 by default because we don't know
 if
     * the middle hop will be able to. And at this point, we do have an
 IPv4 or
     * IPv6 address available so go to validation. */
 }}}

 This existing part of the code is also buggy, because it assumes that at
 least one of the link specifiers will be accepted by our reachable address
 settings:

 {{{
   /* From this point on, we have a request for a direct connection to the
    * rendezvous point so make sure we can actually connect through our
    * firewall. We'll prefer IPv6. */

   /* IPv6 test. */
   if (have_v6 &&
       fascist_firewall_allows_address_addr(&addr_v6, port_v6,
                                            FIREWALL_OR_CONNECTION, 1, 1))
 {
     /* Direct connection and we can reach it in IPv6 so go for it. */
     addr = &addr_v6; port = port_v6;
     goto validate;
   }
   /* IPv4 test and we are sure we have a v4 because of the check above. */
   if (fascist_firewall_allows_address_addr(&addr_v4, port_v4,
                                            FIREWALL_OR_CONNECTION, 0, 0))
 {
     /* Direct connection and we can reach it in IPv4 so go for it. */
     addr = &addr_v4; port = port_v4;
     goto validate;
   }
 }}}

 Also, service_intro_point_new() still suffers from a design issue: it
 needs to take a node, not an extend_info (see #23576).

 Spec review:

 Nitpick:

 Saying `"TLS-over-TCP IPv4 or IPv6"` is ambiguous, because there is no
 specifier with this name. I think what you mean is `"TLS-over-TCP IPv4" or
 "TLS-over-TCP IPv6"`.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/23502#comment:25>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online