Use tor_addr_is_v4() rather than family, or reject all v6-mapped IPv4 addresses
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 (moved).
Activity
changed milestone to %Tor: unspecified
added 033-removed-20180320 033-triage-20180320 035-triaged-in-20180711 040-deferred-20190220 actualpoints::1 component::core tor/tor ipv6 milestone::Tor: unspecified points::1 priority::medium reviewer::ahf severity::normal sponsor::V-can status::needs-revision tor-dirauth type::defect version::tor unspecified labels
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.
Trac:
Milestone: Tor: 0.3.3.x-final to Tor: unspecifiedWhen 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:
- Should I change the
a->family == AF_INETinaddress.htotor_addr_is_v4(a)(along with changingaddr->family == AF_INETtotor_addr_is_v4(addr))? - Is it okay if I implement a
tor_addr_is_v6()which is liketor_addr_is_v4()but with IPv6/AF_INET, and replaceaddr->family == AF_INET6and the like withtor_addr_is_v6(addr)? - If I do #2 (closed) on this list, then should I reject IPv6 mapped IPv4 addresses in
tor_addr_is_v6()?
Trac:
Cc: N/A to neel@neelc.org- Should I change the
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:
- Should I change the
a->family == AF_INETinaddress.htotor_addr_is_v4(a)(along with changingaddr->family == AF_INETtotor_addr_is_v4(addr))?
Yes, that's fine.
- Is it okay if I implement a
tor_addr_is_v6()which is liketor_addr_is_v4()but with IPv6/AF_INET, and replaceaddr->family == AF_INET6and the like withtor_addr_is_v6(addr)?
Yes, and please add unit tests for the new function.
- If I do #2 (closed) 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.
- Should I change the
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 (moved).
My PR for this bug is here: https://github.com/torproject/tor/pull/177
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.
Trac:
Status: assigned to needs_reviewThe 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~82Trac:
Version: N/A to Tor: unspecified
Status: needs_review to needs_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.
Trac:
Status: needs_review to needs_revisionThanks 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:
- 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.
- 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?
Trac:
Reviewer: ahf to ahf, teor
Status: needs_review to needs_revisionHi, 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
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:
- 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.
- 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.
The new PR removing tor_addr_is_v4() is here: https://github.com/torproject/tor/pull/861
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.
Trac:
Status: needs_review to needs_revision
Actualpoints: N/A to 1I'll come back to this ticket.
Trac:
Owner: N/A to neel
Cc: neel@neelc.org to neel
Status: needs_revision to assignedmentioned in issue #26436 (moved)
moved to tpo/core/tor#24546 (closed)
mentioned in issue tpo/core/tor#26436 (closed)
