[tor-bugs] #7193 [Core Tor/Tor]: Tor's sybil protection doesn't consider IPv6

[Tor Bug Tracker & Wiki]

#7193: Tor's sybil protection doesn't consider IPv6
-------------------------------------------------+-------------------------
 Reporter:  asn                                  |          Owner:  (none)
     Type:  enhancement                          |         Status:
                                                 |  needs_revision
 Priority:  Medium                               |      Milestone:  Tor:
                                                 |  unspecified
Component:  Core Tor/Tor                         |        Version:
 Severity:  Normal                               |     Resolution:
 Keywords:  ipv6, intro, tor-dirauth, security,  |  Actual Points:
  sybil, network-health, outreachy-ipv6          |
Parent ID:  #24403                               |         Points:  1
 Reviewer:  nickm                                |        Sponsor:
                                                 |  Sponsor55-can
-------------------------------------------------+-------------------------
Changes (by nickm):

 * status:  needs_review => needs_revision


Comment:

 Some notes on the content of the patch:
   * This like is incorrect `node_t *node_running =
 tor_malloc(sizeof(node_t *));`.  That "sizeof" is taking the size of a
 pointer, not the size of a node_t.  Because of that, you'll only allocate
 a few bytes, not a whole node_t.
   * It looks like the `mock_node_get*` functions have a memory leak -- you
 malloc node_running, but there is nothing that frees it.  Please remember
 that in C, every allocated object needs to have a corresponding free.  If
 you run configure with `--enable-fragile-hardening` it will turn on extra
 run-time checking that will catch this kind of problem for you.
   * When using strlcpy or strlcat, it's good style to use the size of the
 target buffer.
   * For the address comparison, could we use tor_addr_compare() ?
   * I don't understand why we're looking at the family type for the ipv6
 address -- if it's an IPv6 address, then the family should always be
 AF_INET6.  (And if the family is AF_INET, then it shouldn't be stored in a
 variable called ipv6_addr).
   * Most fundamentally: this function seems to be for sorting relays into
 a list by address and then by bandwidth.  But now it is trying to sort by
 ipv6 address _and_ by ipv4 address at the same time.  I don't think that
 makes sense -- a relay can have both kinds of address, but it wouldn't
 appear at more than one point in the list necessarily.

 It probably makes sense for there to be two different variants of the
 function -- one that sorts by ipv6 and one that sorts by ipv4.  The parts
 of the two functions that would be shared in common should turn into a
 third function that they both would call.

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