Opened 3 years ago

Last modified 13 days ago

#15518 assigned defect

Tor considers routers in the same IPv6 /16 to be "in the same subnet"

Reported by: isis Owned by: Samdney
Priority: High Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Normal Keywords: ipv6, path, path-bias, tor-client easy
Cc: isis, nicoo, Sebastian, Samdney Actual Points:
Parent ID: #24393 Points: 1
Reviewer: Sponsor:

Description (last modified by isis)

When EnforceDistinctSubnets is enabled, tor uses:

/** Return true iff router1 and router2 have similar enough network addresses                                                                                                                                       
 * that we should treat them as being in the same family */
static INLINE int
addrs_in_same_network_family(const tor_addr_t *a1,
                             const tor_addr_t *a2)
{
  return 0 == tor_addr_compare_masked(a1, a2, 16, CMP_SEMANTIC);
}

to determine if an address is in the same family. For an example IPv6 address, 2001:1234::0:1, its /16 representation is 2001::/16, meaning that 2001:ffff:: would be in the same family. A \16 for IPv6 is huge, particularly considering that only one-eighth of all IPv6 space is currently allocated for use on the internet (2000::/3). For the path selection code, using /16 essentially means that no two IPv6 routers in the same country (or possibly even continent) will be in the same path, and might possibly provide extremely increased chance of selection to routers in weird/rare IPv6 subnets.

For a related ticket, see #15517 governing how BridgeDB's version of EnforceDistinctSubnets will work for IPv6. (In that ticket, I proposed using IPv6 /32s, since that is the minimum ARIN IPv6 subnet allocation for a LIR.

Child Tickets

Change History (31)

comment:1 Changed 3 years ago by isis

Description: modified (diff)

comment:2 Changed 3 years ago by isis

Description: modified (diff)

comment:3 Changed 3 years ago by nickm

Keywords: 026-backport added
Milestone: Tor: 0.2.7.x-final
Priority: normalmajor

comment:4 Changed 3 years ago by nickm

Milestone: Tor: 0.2.7.x-finalTor: 0.2.???
Priority: majornormal

Fortunately, that function is only called on the output of node_get_addr(), which calls node_get_prim_orport(), which can only return an IPv4 address. But we should come back to this when we do more with IPv6 addresses, and also when we fix #7193.

comment:5 Changed 2 years ago by teor

Keywords: 026-backport removed
Milestone: Tor: 0.2.???Tor: 0.2.9.x-final
Severity: Normal

This could be in 0.2.9, and there's no reason for an 0.2.6 backport from there.

comment:6 Changed 2 years ago by teor

We should probably do this in 0.2.9 if the IPv6 client bootstrap in #17840 gets merged in 0.2.8.

comment:7 Changed 2 years ago by nickm

Points: small

comment:8 Changed 21 months ago by isabela

Points: small1

comment:9 Changed 19 months ago by isabela

Keywords: isaremoved added
Milestone: Tor: 0.2.9.x-finalTor: 0.2.???

comment:10 Changed 15 months ago by teor

Milestone: Tor: 0.2.???Tor: 0.3.???

Milestone renamed

comment:11 Changed 14 months ago by nickm

Keywords: tor-03-unspecified-201612 added
Milestone: Tor: 0.3.???Tor: unspecified

Finally admitting that 0.3.??? was a euphemism for Tor: unspecified all along.

comment:12 Changed 9 months ago by nickm

Keywords: tor-03-unspecified-201612 removed

Remove an old triaging keyword.

comment:13 Changed 9 months ago by nickm

Keywords: isaremoved removed

comment:14 Changed 8 months ago by nickm

Keywords: tor-client easy added
Priority: MediumHigh

comment:15 Changed 3 months ago by teor

Parent ID: #7193

comment:16 Changed 3 months ago by Samdney

Cc: Samdney added

comment:17 Changed 3 months ago by teor

Parent ID: #7193#24393

comment:18 Changed 3 months ago by aruna1234

Hey!! I am a beginner! Can I take this up!

comment:19 Changed 3 months ago by teor

You can!
Try to stick with the same coding style we use,
And submit a patch as a diff attachment or a git branch.

comment:20 Changed 3 months ago by Samdney

*argh* I forgot to assign it to me. *argh*. I have already spent some time with this.

But it was my mistake, hence you can take it aruna1234. ;)

Edit: Please give me a short feedback, if you want to do it. If possible I would prefer to work on it.

Last edited 3 months ago by Samdney (previous) (diff)

comment:21 Changed 3 months ago by aruna1234

I am okay in working with you, as I am a beginner, I'll get to learn in the process. Can you give me a small feedback on how much you have worked. We can communicate via irc or e-mail.

comment:22 Changed 3 months ago by teor

Here's how to fix this bug:

  • tor_addr_t has a family member, it can be AF_INET or AF_INET6.
  • if the addresses have different families, they are not "in the same subnet".
  • if the addresses are both IPv4, use the existing code to compare /16s.
  • if the addresses are both IPv6, add new code that compares /32s.

comment:23 Changed 3 months ago by Samdney

Thanks for the guideline. I'm already beyond on this :P.

comment:24 Changed 3 months ago by aruna1234

I have two queries, namely-

  1. What is the use of AF_INET and AF_INET6?
  2. Which file should I search for the function tor_addr_t?

comment:25 Changed 3 months ago by irl

  1. AF_INET for IPv4 and AF_INET6 for IPv6.
  2. src/common/address.h

comment:26 Changed 3 months ago by aruna1234

I found tor_addr_compare and tor_addr_compare_masked in address.h which compares the address:

int tor_addr_compare(const tor_addr_t *addr1, const tor_addr_t *addr2,

tor_addr_comparison_t how);

int tor_addr_compare_masked(const tor_addr_t *addr1, const tor_addr_t *addr2,

maskbits_t mask, tor_addr_comparison_t how);

I also came across this snippet in address.c which is below. Is the the block of code which compares for /16 i.e IPv4?
case AF_INET: {

uint32_t a1 = tor_addr_to_ipv4h(addr1);
uint32_t a2 = tor_addr_to_ipv4h(addr2);
if (mbits <= 0)

return 0;

if (mbits > 32)

mbits = 32;

a1 >>= (32-mbits);
a2 >>= (32-mbits);
r = TRISTATE(a1, a2);
return r;

}

comment:27 Changed 2 months ago by teor

The current code in addrs_in_same_network_family() calls tor_addr_compare_masked() to compare all addresses by /16.

You need to add more calls to tor_addr_compare_masked() in addrs_in_same_network_family() so that:

  • if the family of a1 is AF_INET, it uses /16 for IPv4
  • if the family of a1 is AF_INET6 and tor_addr_is_v4(a1), it uses /112 for an IPv6-mapped IPv4 address (96 bits for the v4 map and 16 for IPv6)
  • otherwise, it is native IPv6, and it uses /32.

I realise this is a change from what I said above, I just read tor_addr_compare_masked() and it's more complicated than I thought.

comment:28 Changed 2 months ago by aruna1234

where is the function defination of addr_in_same_network_family()? I searched in both address.h and address.c but couldn't find it.

comment:29 Changed 2 months ago by Samdney

Hi,
btw, I already spent time with this. If you can wait, I can help you tomorrow with my solution ;).
I have only been very distracted by other stuff, the last days.
It's a longer thing, and I'm sleepy. >.<
See you.

comment:30 Changed 2 months ago by teor

Hi aruna1234,

Some development environments will index C code and let you search it.
Or you can use an editor or search application that looks for a string in all the files in the tor source directory.

Or you can use command line tools to do the search like this:

git clone https://git.torproject.org/tor.git
grep -r addrs_in_same_network_family tor/src

When I use those commands, I can see that the function is defined in nodelist.c.

comment:31 Changed 13 days ago by Samdney

Owner: set to Samdney
Status: newassigned
Note: See TracTickets for help on using tickets.