Opened 3 months ago

Last modified 4 hours ago

#28525 needs_review enhancement

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

Reported by: neel Owned by: neel
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Normal Keywords: ipv6, 040-deferred-20190220
Cc: neel Actual Points:
Parent ID: Points:
Reviewer: nickm Sponsor:

Description (last modified by neel)

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.

Child Tickets

Change History (14)

comment:1 Changed 3 months ago by neel

Description: modified (diff)
Type: defectenhancement

comment:3 Changed 3 months ago by neel

Status: assignedneeds_revision

comment:4 Changed 3 months ago by neel

Status: needs_revisionneeds_review

Oops.

comment:5 Changed 3 months ago by dgoulet

Reviewer: mikeperry

comment:6 Changed 3 months ago by nickm

Milestone: Tor: 0.4.0.x-final

comment:7 Changed 3 months ago by teor

Status: needs_reviewneeds_revision

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.

comment:8 Changed 3 months ago by teor

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.

comment:9 Changed 3 months ago by neel

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

comment:10 Changed 3 months ago by neel

Status: needs_revisionneeds_review

comment:11 Changed 2 months ago by nickm

Reviewer: mikeperrynickm
Status: needs_reviewneeds_revision

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?

comment:12 in reply to:  11 Changed 2 months ago by teor

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.

comment:13 Changed 4 days ago by neel

Status: needs_revisionneeds_review

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.

comment:14 Changed 4 hours 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.

Note: See TracTickets for help on using tickets.