Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#20151 closed defect (fixed)

Fix parse_virtual_addr_network minimum network size

Reported by: teor Owned by:
Priority: Medium Milestone: Tor: 0.2.9.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: easy intro
Cc: Actual Points:
Parent ID: Points: 0.5
Reviewer: Sponsor:

Description (last modified by teor)

parse_virtual_addr_network does:

  const int max_bits = ipv6 ? 40 : 16;

then:

  if (bits > max_bits) {
    if (msg)
      tor_asprintf(msg, "VirtualAddressNetwork%s expects a /%d "
                   "network or larger",ipv6?"IPv6":"", max_bits);
    return -1;
  }

Firstly, the log message refers to a minimum ("n or larger" makes n a minimum, not a maximum), but the variable is named "max_bits". So we should rename it to min_bits.

Secondly, an IPv6 /40 is terribly restrictive.

For people to use their local IPv6 allocations, we should allow at least a /64.

If the goal is to have a /16 available, we could allow up to 128 - 16 = /112. But IPv6 has more addresses than IPv4, so I suggest that a /104 is a sensible minimum. (If someone wants to map more than 2^24 addresses at once, they can choose a larger network. We could make the minimum /96, but some providers split up /64s into /96s and give them out to end users.)

These limitations should also be documented in the tor man page.

Child Tickets

Change History (15)

comment:1 Changed 3 years ago by teor

Description: modified (diff)

comment:2 Changed 3 years ago by eternaleye

I disagree with your assessment of the variable names - in particular, the old names refer to the minimum/maximum length of the network prefix, which matches convention. The error message refers to a larger network (i.e., one containing more addresses). Both of these are entirely sensible, IMO, even together. The former is the normal way to discuss allocations, while the latter is a less technical framing of the problem.

It would likely be clearer, however, if the variable name was "max_prefix_bits".

In addition, the thinko was most likely around the IPv6 guideline that each host should get a /64, and so the author of that code was intending to ensure the ability to allocate 216 /64s (and actually mandated the ability to allocate 224, possibly due to "IPv6 should let people have more room than IPv4").

However, the addresses allocated by VirtualAddressNetworkIPv6 are not actually hosts (they're services), and as a result, should have fully-specified addresses, not prefixes. This is, in fact, part of the reason IPv6 instructs that each host should get a /64.

As a result, using /104 (or even /112) as the maximum prefix length seems entirely sensible to me.

Of course, if the code did switch from "max prefix length" to "min network length", we could just use "16" across the board ("we need at least 216 addresses to allocate from") and drop the ternary (or at least move it to formatting the error message, as ipv6 ? 128 - 16 : 32 - 16)

Last edited 3 years ago by eternaleye (previous) (diff)

comment:3 Changed 3 years ago by nickm

Milestone: Tor: 0.2.???Tor: 0.2.9.x-final

This is a one-line fix. Anybody want to submit a patch?

+1 if the patch fixes the manpage too, and if the patch explains that wider networks are better, because they lower the odds of an attacker successfully guessing an in-use IP.

comment:4 Changed 3 years ago by pingl

Changed max_bits to max_prefix_bits and set the maximum prefix length to /104

https://github.com/aigna/tor/tree/defect20151

Still need to fix the manpage :)

comment:5 Changed 3 years ago by teor

Status: newneeds_revision

Thanks for this fix!

Here's what I would change before merging it:

  • rebase on the latest master
    • that is, cherry pick afbda70 onto master, removing dbbd36e and b5b8d62 - these are merge and fix commits that aren't relevant to the patch
  • keep the newline that's removed from the end of addressmap.c
  • add the smallest network size to the manpage, with a note that larger sizes are better :-)

comment:6 Changed 3 years ago by pingl

I think now it's fixed. I've also added something in the tor man page.

comment:7 Changed 3 years ago by nickm

Status: needs_revisionneeds_review

comment:8 Changed 3 years ago by teor

Status: needs_reviewneeds_revision

One more minor fix to the man page:

  • the maximum prefix is 104 for IPv6 and 16 for IPv4.

Then we're good to go, and I'll flip it into ready for merge.

comment:9 Changed 3 years ago by pingl

Fixed :)

comment:10 Changed 3 years ago by teor

Thanks for the quick response - I forgot one thing - a changes file.
Would you like to have a go at doing a changes file for this?

See "How we log changes" in:
https://gitweb.torproject.org/tor.git/tree/doc/HACKING/CodingStandards.md

The relevant commit (using git blame) is de4cc12.

git describe --contains de4cc12 says tor-0.2.4.7-alpha (and then some exact commit and branch info that we don't need).

comment:11 Changed 3 years ago by pingl

Ok! I added the changes file changes/ticket20151.

comment:12 Changed 3 years ago by nickm

Status: needs_revisionneeds_review

comment:13 Changed 3 years ago by nickm

Looks correct; merged.

comment:14 Changed 3 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

(Also, thanks!)

comment:15 Changed 3 years ago by pingl

Thanks for guiding me during the process :)

Note: See TracTickets for help on using tickets.