[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]

Re: [tor-bugs] #12377 [Core Tor/Tor]: get_interface_address6() behaviour iff all interface addresses are internal

#12377: get_interface_address6() behaviour iff all interface addresses are internal
-------------------------------------------------+-------------------------
 Reporter:  rl1987                               |          Owner:  (none)
     Type:  defect                               |         Status:
                                                 |  needs_revision
 Priority:  Medium                               |      Milestone:  Tor:
                                                 |  0.3.4.x-final
Component:  Core Tor/Tor                         |        Version:
 Severity:  Normal                               |     Resolution:
 Keywords:  tor-relay, address-detection,        |  Actual Points:
  review-group-34                                |
Parent ID:                                       |         Points:  1
 Reviewer:  dgoulet                              |        Sponsor:
-------------------------------------------------+-------------------------
Changes (by teor):

 * status:  merge_ready => needs_revision
 * version:  Tor: unspecified =>


Comment:

 Functional review:

 This code breaks the calls to get_interface_address() and
 get_interface_address6() in resolve_my_address(), because they rely on its
 promise to "Prefer public IP addresses to internal IP addresses." But this
 code will return a private IP addres if it is the default route.

 I'm also not sure how this code fixes the actual bug we are trying to fix.
 We already keep a list of interface addresses in
 client_check_address_changed(), as suggested by dgoulet in comment 4.

 One simple way to make the existing address check more reliable is to make
 get_interface_address6() return the *first* internal address in the list,
 not the *last* internal address. We should make this change anyway,
 because it will benefit macOS, BSD, and Windows.

 Design review:

 I don't understand the overall design of this API.

 If netlink works, we should use it to produce an ordered list of IP
 addresses, which has the default gateway as the first entry. Then we
 should use the first entry when a single address is requested, and all
 entries when a list of addresses is requested. The existing code already
 implements this logic for the other address resolution methods. To use the
 existing code, we need to make the netlink code return a list, and use
 that list as the first alternative in get_interface_addresses_raw().

 Also, this code only deals with the Linux case. Please open tickets for
 BSD/macOS and Windows functionality if you want to close this ticket.

 Merging this code may break some relay address configs, because it changes
 address resolution order to use netlink rather than the first address
 returned by getifaddrs(). So we need to write a note to relay operators at
 the top of the 0.3.4 release notes.

 Code review:

 Why check the `__linux__` macro, when you can just check for the relevant
 headers?

 Some of the functions are missing comments.

 This code never changes the value of ret, the logic is probably inverted.
 Please check for other instances of this bug.
 {{{
 if (ret == 0 && result.found)
     ret = 0;
 }}}

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

_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs