Opened 19 months ago

Last modified 8 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: unspecified
Component: Core Tor/Tor Version: Tor: unspecified
Severity: Normal Keywords: tor-dirauth, ipv6, 033-triage-20180320, 033-removed-20180320, 035-triaged-in-20180711, 040-deferred-20190220
Cc: neel@… Actual Points: 1
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 (31)

comment:1 Changed 19 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 15 months ago by nickm

Keywords: 033-triage-20180320 added

Marking all tickets reached by current round of 033 triage.

comment:3 Changed 15 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 15 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 12 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 12 months 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 12 months 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 12 months ago by neel

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

comment:9 Changed 12 months ago by teor

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

comment:10 Changed 12 months ago by asn

Reviewer: ahf

comment:11 Changed 12 months 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 12 months 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 12 months ago by nickm

Keywords: 035-triaged-in-20180711 added

comment:14 Changed 9 months ago by nickm

Milestone: Tor: 0.3.5.x-finalTor: 0.3.6.x-final

Deferring various feature-y things to 0.3.6. If one of these is actually happening in 0.3.5, please let me know!

comment:15 Changed 8 months ago by nickm

Milestone: Tor: 0.3.6.x-finalTor: 0.4.0.x-final

Tor 0.3.6.x has been renamed to 0.4.0.x.

comment:16 Changed 4 months ago by nickm

Keywords: 040-deferred-20190220 added
Milestone: Tor: 0.4.0.x-finalTor: unspecified

Deferring 51 tickets from 0.4.0.x-final. Tagging them with 040-deferred-20190220 for visibility. These are the tickets that did not get 040-must, 040-can, or tor-ci.

comment:17 Changed 3 months ago by neel

Owner: set to neel
Status: needs_revisionassigned

I'll work on an updated patch for this.

comment:18 Changed 3 months ago by neel

Status: assignedneeds_review

I have an updated PR for this bug: https://github.com/torproject/tor/pull/826

The reason for a new PR is because the Tor codebase has changed significantly since my last PR.

comment:19 Changed 3 months ago by teor

Status: needs_reviewneeds_revision
Version: Tor: unspecified

The CI on this ticket did not pass:

src/core/or/policies.c: In function ‘exit_policy_remove_redundancies’:
src/core/or/policies.c:1709:19: error: variable ‘family’ set but not used [-Werror=unused-but-set-variable]
       sa_family_t family;
                   ^
cc1: all warnings being treated as errors
make: *** [src/core/or/src_core_libtor_app_testing_a-policies.o] Error 1
make: *** Waiting for unfinished jobs....

https://travis-ci.org/torproject/tor/jobs/509624393

Please make sure that the CI passes before putting it in needs_review again.

Also, the bugfix release in the changes file is the release where the bug was introduced, not the release where the bug was fixed. In this ticket, that's the release where tor_addr_is_v4() was introduced.

Here's how you can find the release that this bug was introduced in:

# find the first commit that talks about tor_addr_is_v4
$ git log --reverse -S tor_addr_is_v4 master
commit bbbf504281
Author: Nick Mathewson <nickm@torproject.org>
Date:   Thu Jul 19 18:46:09 2007 +0000

     r13827@catbus:  nickm | 2007-07-19 14:42:25 -0400
     Merge in some generic address manipulation code from croup.  Needs some work.
...
# find the release that contains that commit
$ git describe --contains bbbf504281
tor-0.2.0.3-alpha~82

comment:20 Changed 3 months ago by neel

Status: needs_revisionneeds_review

It passes Travis now.

comment:21 Changed 3 months ago by ahf

Status: needs_reviewneeds_revision

There seems to be an issue with AppVeyor here: https://github.com/torproject/tor/pull/826

The code looks fine to me other than that.

comment:22 Changed 3 months ago by neel

Status: needs_revisionneeds_review

I fixed the buggy line and it passes AppVeyor.

comment:23 Changed 3 months ago by teor

Reviewer: ahfahf, teor
Status: needs_reviewneeds_revision

Thanks for this pull request. I did a review on the code.

I am concerned about the size of the diff in commit 53ff26b. It changes over 100 lines. It contains at least one logic error that passed review. There could be more.

Here are our options for moving forward:

  1. Use an automated tool like coccinelle to do the changes in 53ff26b. Then we can verify the output using the tool. We should also turn tor_addr_is_*() into inline header functions, so they are more efficient.
  2. Delete the function tor_addr_is_v4(), and replace all its uses with tor_addr_family(addr) == AF_INET. There are only 13 uses of tor_addr_is_v4() in master, so the diff should be much smaller and easier to review.

I prefer option 2, because we will end up with fewer functions, and less code complexity. And the overall diff will be smaller.

What do you think?

comment:24 Changed 3 months ago by teor

Hi, you force-pushed the branch. Please don't do that, it makes reviews much harder. For details, see my comment at:
https://trac.torproject.org/projects/tor/ticket/22029#comment:35

comment:25 in reply to:  23 Changed 3 months ago by teor

Hi, I saw your comments on the pull request.

Replying to teor:

I am concerned about the size of the diff in commit 53ff26b. It changes over 100 lines. It contains at least one logic error that passed review. There could be more.

Here are our options for moving forward:

  1. Use an automated tool like coccinelle to do the changes in 53ff26b. Then we can verify the output using the tool. We should also turn tor_addr_is_*() into inline header functions, so they are more efficient.

I can understand that you don't want to use an automated tool. But we can't review a hundred lines of very similar changes effectively. We've already missed one mistake even though we did multiple reviews, and there could be more.

  1. Delete the function tor_addr_is_v4(), and replace all its uses with tor_addr_family(addr) == AF_INET. There are only 13 uses of tor_addr_is_v4() in master, so the diff should be much smaller and easier to review.

I prefer option 2, because we will end up with fewer functions, and less code complexity. And the overall diff will be smaller.

Would you like to do a new pull request that removes tor_addr_is_v4()?
You could use your existing first and last commits 56ec375359032eee15cbda9a6d5960cdd5a28135 and 1d5c703ec2e2d418180f9cdeba1140b9dbaf730d. Then add a commit that removes tor_addr_is_v4(), which should only be about 20-30 changed lines.

comment:26 Changed 3 months ago by neel

The new PR removing tor_addr_is_v4() is here: https://github.com/torproject/tor/pull/861

comment:27 Changed 3 months ago by neel

Status: needs_revisionneeds_review

comment:28 Changed 3 months ago by teor

Actual Points: 1
Status: needs_reviewneeds_revision

Thanks, that looks much better.

Now that we have removed special handling for v4-mapped IPv6 addresses, there's some more code that we can remove. And we need to move the IPV6_V6ONLY code so that it applies to all sockets. See my detailed comments on the pull request.

Please make any further changes in separate commits on the same branch, then push the branch.

comment:29 Changed 2 months ago by teor

Reviewer: ahf, teorahf

Clearing myself as a reviewer.

comment:30 Changed 8 weeks ago by neel

Owner: neel deleted
Status: needs_revisionassigned

I no longer want to work on this ticket.

comment:31 Changed 8 weeks ago by neel

Status: assignedneeds_revision
Note: See TracTickets for help on using tickets.