Make tor_addr_is_internal_() aware of RFC 6598 (Carrier Grade NAT/Large Scale NAT) IPv4 Ranges

With the IPv4 depletion, many ISPs, cell carriers and cloud providers are deploying Carrier Grade NAT with the subnet defined in RFC 6598 (100.64.0.0/10). We should make Tor aware of this range as it is internal as well.

I will write a patch and give a link to a GitHub PR.

changed milestone to %Tor: 0.2.9.x-final

added 029-backport 035-backport 040-backport 040-deferred-20190220 component::core tor/tor consider-backport-after-040-stable ipv6 milestone::Tor: 0.2.9.x-final owner::neel parent::7971 priority::medium resolution::fixed reviewer::nickm security-low severity::normal status::closed type::defect labels

Trac:
Type: defect to enhancement
Description: With the IPv4 depletion, many ISPs, cell carriers and some cloud providers are deploying Carrier Grade NAT with the subnet defined in RFC 6598 (100.64.0.0/10). We should make tor_addr_is_internal_() aware of this range as it is internal as well.

I will write a patch and upload a PR shortly.

to

With the IPv4 depletion, many ISPs, cell carriers and cloud providers are deploying Carrier Grade NAT with the subnet defined in RFC 6598 (100.64.0.0/10). We should make Tor aware of this range as it is internal as well.

I will write a patch and give a link to a GitHub PR.

PR is here: https://github.com/torproject/tor/pull/529

Trac:
Status: assigned to needs_revision

Oops.

Trac:
Status: needs_revision to needs_review

Trac:
Reviewer: N/A to mikeperry

Trac:
Milestone: N/A to Tor: 0.4.0.x-final

This change to private_nets needs a new consensus method:

It is important
 *  that all authorities agree on that list when creating summaries, so don't
 *  just change this without a proper migration plan and a proposal and stuff.

Trac:
Status: needs_review to needs_revision

I suggest that you split this change into two separate tickets.

We can merge the tor_addr_is_internal_ change, then work on the new consensus method for private_nets.

I removed the private_nets change and only did tor_addr_is_internal_. It is pushed to the same PR.

Trac:
Status: needs_revision to needs_review

This patch does what it is supposed to do. It would be good to have a test.

One problem here is that I'm not sure that this changed behavior is correct. If you have an address inside a carrier NAT, you have the worst of both worlds: it is an address that the public internet cannot reach, but it is an address that other random people on your internet provider can still connect to. In other words, these addresses are not useful enough to call them public, but not safe enough to call them private. So we need to treat these addresses as internal for the purpose of "can this address go onto the public tor network", but we need to treat them as non-internal for the purpose of "is it safe to have a socksport/extorport/etc here."

The main purpose of the rest of my review here is to see what else we would need to change to make sure this change is safe. I'm going to do this by looking at all the users of tor_addr_is_internal in the codebase.

• In warn_nonlocal_client_ports(), we will stop warning about binding a socksport to one of these addresses. Is this a problem? I need more guidance from others.
• In warn_nonlocal_ext_orports(), we will stop warning about binding an extorport to one of these addresses. (same note as above)
• In connection_is_rate_limited(), we no longer count connections to or from one of these addresses as having any rate limits.
• In channeltls.c [which calls tor_addr_is_internal via is_local_addr()], we count any OR connections to these addresses as "local", which seems unwise.

But all the other cases that I could find seemed like an improvement.

Maybe what we need here is to replace the for_listening argument with a more general set of bitflags?

Trac:
Status: needs_review to needs_revision
Reviewer: mikeperry to nickm

Replying to nickm:

This patch does what it is supposed to do. It would be good to have a test.

One problem here is that I'm not sure that this changed behavior is correct. If you have an address inside a carrier NAT, you have the worst of both worlds: it is an address that the public internet cannot reach, but it is an address that other random people on your internet provider can still connect to. In other words, these addresses are not useful enough to call them public, but not safe enough to call them private. So we need to treat these addresses as internal for the purpose of "can this address go onto the public tor network", but we need to treat them as non-internal for the purpose of "is it safe to have a socksport/extorport/etc here."

The main purpose of the rest of my review here is to see what else we would need to change to make sure this change is safe. I'm going to do this by looking at all the users of tor_addr_is_internal in the codebase.

• In warn_nonlocal_client_ports(), we will stop warning about binding a socksport to one of these addresses. Is this a problem? I need more guidance from others.

We should not let random people at your ISP connect to your SOCKSPorts.

• In warn_nonlocal_ext_orports(), we will stop warning about binding an extorport to one of these addresses. (same note as above)

We should not let random people at your ISP connect to your ExtORPorts.

• In connection_is_rate_limited(), we no longer count connections to or from one of these addresses as having any rate limits.

If these addresses aren't allowed to be ORPorts on the public network, then rate limits probably aren't needed. There might be the rare case of a private bridge that we need to think about.

• In channeltls.c [which calls tor_addr_is_internal via is_local_addr()], we count any OR connections to these addresses as "local", which seems unwise.

I don't know what local OR connections mean. I'd need to look at the code and check.

But all the other cases that I could find seemed like an improvement.

Maybe what we need here is to replace the for_listening argument with a more general set of bitflags?

Seems sensible.

New PR here: https://github.com/torproject/tor/pull/708

I switched to using flags for tor_addr_is_internal_() and added tests.

I did a new PR because reusing the old one added many unnecessary commits. Sorry about that.

Trac:
Status: needs_revision to needs_review

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.

Trac:
Milestone: Tor: 0.4.0.x-final to Tor: unspecified
Keywords: N/A deleted, 040-deferred-20190220 added

This looks plausible to me. I'd like to see one more commit here, though, documenting the new behavior of for_listening in the documentation comments. I'd like it to explain when you should use IP_LISTEN_EXTERNAL and when (if ever?) you should use IP_LISTEN_INTERNAL.

Trac:
Status: needs_review to needs_revision

I added that comment and pushed it to the PR (the one in Comment 15).

Trac:
Status: needs_revision to needs_review

looks okay to me. I'd like Teor to take one last look too, if they're free. Then let's merge!

(you free, teor?)

Trac:
Cc: neel to neel, teor

Replying to nickm:

looks okay to me. I'd like Teor to take one last look too, if they're free. Then let's merge!

I don't think this patch changes Tor's behaviour at all:

• Tor previously returned 0 for RFC6598 addresses.
• This patch adds a new check for RFC6598 addresses, and then changes the calling code to pass IP_LISTEN_EXTERNAL, so that RFC6598 addresses always return 0 anyway.

Here's what I think the patch should do:

• When connecting, RFC6598 addresses are like internal addresses, because they are not publicly routable, so tor can not connect to relay ports on these addresses
• When listening, RFC6598 addresses are like external addresses, because other people might be able to access them, so tor should not listen to client ports on these addresses In short, RFC6598 addresses should be treated just like 0.0.0.0.

After we make that code change, here's how we can make tor_addr_is_internal_() easier to understand:

• document the return value of the function for localhost or local networks in RFC1918 or RFC4193 or RFC4291
• document the return value of the function for 0.0.0.0 and RFC6598 addresses:
  • when for_listening is set
  • when for_listening is not set
• explain why 0.0.0.0 and RFC6598 addresses are treated differently when for_listening is set (see my explanation above)

After we make these changes, I don't think IP_LISTEN_INTERNAL will ever be used in Tor. So we should remove IP_LISTEN_INTERNAL and IP_LISTEN_EXTERNAL, and just go back to passing 0 or 1.

Trac:
Status: needs_review to needs_revision