Opened 13 months ago

Closed 4 months ago

#28525 closed defect (fixed)

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: 0.2.9.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: consider-backport-after-040-stable, ipv6, 040-deferred-20190220, 040-backport, 035-backport, 029-backport, security-low
Cc: neel Actual Points:
Parent ID: #7971 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 (40)

comment:1 Changed 13 months ago by neel

Description: modified (diff)
Type: defectenhancement

comment:3 Changed 13 months ago by neel

Status: assignedneeds_revision

comment:4 Changed 13 months ago by neel

Status: needs_revisionneeds_review

Oops.

comment:5 Changed 13 months ago by dgoulet

Reviewer: mikeperry

comment:6 Changed 12 months ago by nickm

Milestone: Tor: 0.4.0.x-final

comment:7 Changed 12 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 12 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 12 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 12 months ago by neel

Status: needs_revisionneeds_review

comment:11 Changed 12 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 12 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 10 months 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 10 months 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.

comment:15 Changed 9 months ago by nickm

Status: needs_reviewneeds_revision

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.

comment:16 Changed 9 months ago by neel

Status: needs_revisionneeds_review

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

comment:17 Changed 9 months ago by nickm

Cc: teor added

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

comment:18 in reply to:  17 Changed 9 months ago by teor

Status: needs_reviewneeds_revision

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.

comment:19 in reply to:  11 Changed 9 months ago by teor

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.

comment:20 Changed 9 months ago by neel

Status: needs_revisionneeds_review

I made the changes and pushed them. Same PR.

comment:21 Changed 9 months ago by nickm

teor, you okay with it now?

comment:22 Changed 9 months ago by teor

Cc: teor removed
Keywords: 040-backport 035-backport 034-backport 029-backport security-low added
Milestone: Tor: unspecifiedTor: 0.4.1.x-final
Reviewer: nickmnickm, teor
Status: needs_reviewneeds_revision
Type: enhancementdefect

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.

comment:23 in reply to:  11 Changed 9 months ago by cypherpunks

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?

comment:24 Changed 9 months ago by neel

Status: needs_revisionneeds_review

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.

comment:25 Changed 9 months ago by teor

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

comment:26 Changed 9 months ago by teor

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

comment:27 Changed 9 months ago by teor

Reviewer: nickm, teornickm

comment:28 Changed 9 months ago by teor

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

comment:29 Changed 9 months ago by nickm

Keywords: asn-merge added
Milestone: Tor: 0.4.1.x-finalTor: 0.4.0.x-final
Status: needs_reviewmerge_ready

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.

comment:30 Changed 9 months ago by asn

Resolution: fixed
Status: merge_readyclosed

merged to 040 and forward!

comment:31 Changed 9 months ago by asn

comment:32 in reply to:  31 Changed 9 months ago by asn

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

comment:33 Changed 9 months ago by asn

Resolution: fixed
Status: closedreopened

comment:34 Changed 9 months ago by asn

Status: reopenedmerge_ready

comment:35 Changed 9 months ago by nickm

merged asn's branch, to fix CI.

comment:36 Changed 9 months ago by teor

Keywords: asn-merge removed
Milestone: Tor: 0.4.0.x-finalTor: 0.3.5.x-final

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.

comment:37 Changed 9 months ago by teor

Keywords: consider-backport-after-040-stable added

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.

comment:38 Changed 8 months ago by teor

Parent ID: #7971

comment:39 Changed 6 months ago by nickm

Keywords: 034-backport removed

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

comment:40 Changed 4 months ago by teor

Milestone: Tor: 0.3.5.x-finalTor: 0.2.9.x-final
Resolution: fixed
Status: merge_readyclosed

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

Note: See TracTickets for help on using tickets.