Fix parse_virtual_addr_network minimum network size

changed milestone to %Tor: 0.2.9.x-final

added component::core tor/tor easy intro milestone::Tor: 0.2.9.x-final points::0.5 priority::medium resolution::fixed severity::normal status::closed type::defect labels

Trac:
Description: 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.

to

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.

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 2^16^ /64s (and actually mandated the ability to allocate 2^24^, 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 2^16^ addresses to allocate from") and drop the ternary (or at least move it to formatting the error message, as ipv6 ? 128 - 16 : 32 - 16)

Trac:
Username: eternaleye

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.

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

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 :)

Trac:
Username: pingl

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 :-)

Trac:
Status: new to needs_revision

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

Trac:
Username: pingl

Trac:
Status: needs_revision to needs_review

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.

Trac:
Status: needs_review to needs_revision

Fixed :)

Trac:
Username: pingl

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).

Ok! I added the changes file changes/ticket20151.

Trac:
Username: pingl

Trac:
Status: needs_revision to needs_review

Looks correct; merged.

(Also, thanks!)

Trac:
Resolution: N/A to fixed
Status: needs_review to closed

Thanks for guiding me during the process :)

Trac:
Username: pingl

closed

changed time estimate to 4h

moved to tpo/core/tor#20151 (closed)

Fix parse_virtual_addr_network minimum network size

Child items 0

Activity