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

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

Here is what would happen in each of these cases, if we do what I describe in my last comment:

Replying to nickm:

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 would continue to warn when client ports are on RFC6598 addresses.

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

We would continue to warn when extorports are on RFC6598 addresses.

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

We would not rate-limit connections to RFC6598 addresses (addr is the remote address). That's a rare case, and probably ok for clients with private bridges on the same local network. It might be slightly worse for (multiple) clients, with rate limiting, on the same mobile network as a private bridge, but that's a rare case.

If intra-RFC6598 network connections become a more common case, we could add a FOR_RATE_LIMITING flag, and mark RFC6598 addresses as external when FOR_RATE_LIMITING is passed. Let's do that if needed, in a separate ticket.

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.

channel_is_local() is only called by onionskin_answer(), before calling router_orport_found_reachable(). (There are other calls, but they're only used for logging.)

We would stop calling router_orport_found_reachable() for remote connections from RFC6598 addresses, which is good.

I made the changes and pushed them. Same PR.

Trac:
Status: needs_revision to needs_review

teor, you okay with it now?

It seems ok to me.

It's a bit weird that we don't list 0.0.0.0 in the list of internal addresses. But no sensible OS will try to connect to it anyway, so that doesn't really matter. (And if we want to fix 0.0.0.0, we should do it in another ticket.)

This patch mitigates some security issues created by RFC 6598 by:

blocking control ports on RFC 6598 addresses
warning when client ports and ExtORPorts are on RFC 6598 addresses (Despite our earlier comments, we don't currently block or warn on RFC 6598 addresses.)

So I'm marking it for possible backport.

Here's what we should do before we merge:

update the changes file to describe these major, user-visible security changes
squash and cherry-pick to maint-0.2.9

neel, are you ok making these changes? Just let us know if you can't, and someone will do it eventually.

Trac:
Keywords: N/A deleted, security-low, 029-backport, 040-backport, 034-backport, 035-backport added
Milestone: Tor: unspecified to Tor: 0.4.1.x-final
Type: enhancement to defect
Status: needs_review to needs_revision
Reviewer: nickm to nickm, teor
Cc: neel, teor to neel

Replying to nickm:

In other words, these addresses are not useful enough to call them public, but not safe enough to call them private. Could you forget the times when private was related to safe?

I have updated the changes file and have squashed the commits. I am not planning to cherry-pick the commits to maint-0.2.9.

Trac:
Status: needs_revision to needs_review

Seems good to me.

The cherry-pick to 0.2.9 is here: https://github.com/torproject/tor/pull/762

There is a non-trivial merge to 0.3.5 here: https://github.com/torproject/tor/pull/763 (The function before tor_addr_is_internal_() in 0.2.9 was moved to another file in 0.3.5.)

I need nickm to review the cherry-pick and merge, because I'll be backporting them to 0.2.9 and 0.3.5. (After someone else merges to 0.4.0 and master.)

One of the Appveyor tests failed on 0.3.5 due to #29645 (moved). It is probably unrelated to this change.

Trac:
Reviewer: nickm, teor to nickm

One of the Travis tests failed on 0.3.5 due to #29437 (moved).

Okay, it looks good. asn, please merge PR 763 above to 0.4.0 and forward? Then I suggest we let it cook for a while before backporting: I think that if this patch causes any problems, they will be subtle and take some time to notice.

Trac:
Milestone: Tor: 0.4.1.x-final to Tor: 0.4.0.x-final
Keywords: N/A deleted, asn-merge added
Status: needs_review to merge_ready

merged to 040 and forward!

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

Hm this broke travis because of a broken changes file: https://travis-ci.org/torproject/tor/builds/505305153?utm_medium=notification&utm_source=email

Replying to asn:

Hm this broke travis because of a broken changes file: https://travis-ci.org/torproject/tor/builds/505305153?utm_medium=notification&utm_source=email

Fix for the broken changes file for 040 and master: https://github.com/torproject/tor/pull/784

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

Trac:
Status: reopened to merge_ready

merged asn's branch, to fix CI.

This ticket is eligible for backport to 0.2.9 and later. Please place tickets in 0.3.5 after merging to mainline. Please don't close tickets that can be backported.

Trac:
Keywords: asn-merge deleted, N/A added
Milestone: Tor: 0.4.0.x-final to Tor: 0.3.5.x-final

This ticket is higher-risk: mistakes in our address handling code have historically only been found after stable releases. We'll triage it again after 0.4.0 goes stable.

Trac:
Keywords: N/A deleted, consider-backport-after-040-stable added

Trac:
Parent: N/A to #7971 (moved)

Removing 034-backport from all open tickets: 034 has reached EOL.

Trac:
Keywords: 034-backport deleted, N/A added

Merged to 0.2.9 and 0.3.5, and cherry-picked the changes file fix from PR 784.

Trac:
Resolution: N/A to fixed
Status: merge_ready to closed
Milestone: Tor: 0.3.5.x-final to Tor: 0.2.9.x-final

closed

mentioned in issue #28673 (moved)

mentioned in issue #29437 (moved)

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

mentioned in issue tpo/core/tor#28673 (closed)

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

Child items 0

Activity