Opened 8 months ago

Last modified 5 weeks ago

#24546 needs_revision defect

Use tor_addr_is_v4() rather than family, or reject all v6-mapped IPv4 addresses

Reported by: teor Owned by:
Priority: Medium Milestone: Tor: 0.3.5.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-dirauth, ipv6, 033-triage-20180320, 033-removed-20180320, 035-triaged-in-20180711
Cc: neel@… Actual Points:
Parent ID: Points: 1
Reviewer: ahf Sponsor: SponsorV-can

Description

In Tor, we try to support v6-mapped IPv4 addresses.
We should either:

  • reject them unconditionally, or
  • audit all uses of tor_addr_t.family to see if we should be calling tor_addr_is_v4() instead, and add a comment to the struct that says we should consider using tor_addr_is_v4() rather than comparing family.

If no relay in the consensus is currently using these addresses, then maybe we should just call them internal on authorities, relays, and clients, and remove all the code that tries to deal with them.

Discovered as part of #15518.

Child Tickets

TicketStatusOwnerSummaryComponent
#26436newCheck uses of CMP_SEMANTIC for IP addressesCore Tor/Tor

Change History (13)

comment:1 Changed 8 months ago by teor

We should also audit all uses of tor_addr_compare(a1, a2, CMP_EXACT) to see if they should be CMP_SEMANTIC instead.

The current uses of CMP_SEMANTIC seem reasonable, but not essential: they code would still work if we rejected mapped addresses and did exact comparisons instead.

comment:2 Changed 5 months ago by nickm

Keywords: 033-triage-20180320 added

Marking all tickets reached by current round of 033 triage.

comment:3 Changed 5 months ago by nickm

Keywords: 033-removed-20180320 added

Mark all not-already-included tickets as pending review for removal from 0.3.3 milestone.

comment:4 Changed 5 months ago by nickm

Milestone: Tor: 0.3.3.x-finalTor: unspecified

These tickets were marked as removed, and nobody has said that they can fix them. Let's remember to look at 033-removed-20180320 as we re-evaluate our triage process, to see whether we're triaging out unnecessarily, and to evaluate whether we're deferring anything unnecessarily. But for now, we can't do these: we need to fix the 033-must stuff now.

comment:5 Changed 2 months ago by neel

Cc: neel@… added

When I was grepping the instances of ->family == AF_INET, I got this:

neel@flex:~/code/tor/tor/src % grep -R tor_addr_is_v4 */*.h
common/address.h:int tor_addr_is_v4(const tor_addr_t *addr);
neel@flex:~/code/tor/tor/src % grep -R "[-][>]family == AF_INET" *
common/address.c:  if (addr->family == AF_INET) {
common/address.c:  } else if (addr->family == AF_INET6) {
common/address.c:  if (for_listening && addr->family == AF_INET
common/address.h:  return a->family == AF_INET6 ? &a->addr.in6_addr : NULL;
common/address.h:  tor_assert(a->family == AF_INET6);
common/address.h:  return a->family == AF_INET ? a->addr.in_addr.s_addr : 0;
common/address.h:  if (a->family == AF_INET6) {
common/address.h:  return a->family == AF_INET ? &a->addr.in_addr : NULL;
common/address.h:  return a->family == AF_INET ? (tor_addr_to_ipv4h(a) == u) : 0;
neel@flex:~/code/tor/tor/src %

My questions are that:

  1. Should I change the a->family == AF_INET in address.h to tor_addr_is_v4(a) (along with changing addr->family == AF_INET to tor_addr_is_v4(addr))?
  2. Is it okay if I implement a tor_addr_is_v6() which is like tor_addr_is_v4() but with IPv6/AF_INET, and replace addr->family == AF_INET6 and the like with tor_addr_is_v6(addr)?
  3. If I do #2 on this list, then should I reject IPv6 mapped IPv4 addresses in tor_addr_is_v6()?

comment:6 in reply to:  5 Changed 8 weeks ago by teor

Replying to neel:

When I was grepping the instances of ->family == AF_INET, I got this:

neel@flex:~/code/tor/tor/src % grep -R tor_addr_is_v4 */*.h
common/address.h:int tor_addr_is_v4(const tor_addr_t *addr);
neel@flex:~/code/tor/tor/src % grep -R "[-][>]family == AF_INET" *
common/address.c:  if (addr->family == AF_INET) {
common/address.c:  } else if (addr->family == AF_INET6) {
common/address.c:  if (for_listening && addr->family == AF_INET
common/address.h:  return a->family == AF_INET6 ? &a->addr.in6_addr : NULL;
common/address.h:  tor_assert(a->family == AF_INET6);
common/address.h:  return a->family == AF_INET ? a->addr.in_addr.s_addr : 0;
common/address.h:  if (a->family == AF_INET6) {
common/address.h:  return a->family == AF_INET ? &a->addr.in_addr : NULL;
common/address.h:  return a->family == AF_INET ? (tor_addr_to_ipv4h(a) == u) : 0;
neel@flex:~/code/tor/tor/src %

My questions are that:

  1. Should I change the a->family == AF_INET in address.h to tor_addr_is_v4(a) (along with changing addr->family == AF_INET to tor_addr_is_v4(addr))?

Yes, that's fine.

  1. Is it okay if I implement a tor_addr_is_v6() which is like tor_addr_is_v4() but with IPv6/AF_INET, and replace addr->family == AF_INET6 and the like with tor_addr_is_v6(addr)?

Yes, and please add unit tests for the new function.

  1. If I do #2 on this list, then should I reject IPv6 mapped IPv4 addresses in tor_addr_is_v6()?

You should reject IPv6-mapped IPv4 addresses in the v4 function. Tor doesn't use them, and we don't test for or support them.

comment:7 in reply to:  1 Changed 8 weeks ago by teor

Replying to teor:

We should also audit all uses of tor_addr_compare(a1, a2, CMP_EXACT) to see if they should be CMP_SEMANTIC instead.

The current uses of CMP_SEMANTIC seem reasonable, but not essential: they code would still work if we rejected mapped addresses and did exact comparisons instead.

I split this off into #26436.

comment:8 Changed 7 weeks ago by neel

My PR for this bug is here: https://github.com/torproject/tor/pull/177

comment:9 Changed 7 weeks ago by teor

Milestone: Tor: unspecifiedTor: 0.3.5.x-final
Status: newneeds_review

comment:10 Changed 7 weeks ago by asn

Reviewer: ahf

comment:11 Changed 5 weeks ago by ahf

Status: needs_reviewneeds_revision

The patches looks good to me, but there is a big merge conflict right now because of the recent refactoring work. This needs to be fixed before I can give it the final OK.

comment:12 Changed 5 weeks ago by ahf

Actually, ignore my last comment. I'll take a look at rebasing this on top of master. We don't want to add extra work because of the big refactoring that happened recently.

comment:13 Changed 5 weeks ago by nickm

Keywords: 035-triaged-in-20180711 added
Note: See TracTickets for help on using tickets.