Opened 16 months ago

Last modified 5 months ago

#31009 new enhancement

Tor lets transports advertise private IP addresses in descriptor

Reported by: phw Owned by:
Priority: Medium Milestone: Tor: 0.4.5.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-pt, tor-bridge, 035-backport, 040-backport, 041-backport, 042-deferred-20190918, network-team-roadmap-2020Q1, 043-should, anticensorship-wants, 044-deferred
Cc: ahf, gaba, cjb, catalyst Actual Points:
Parent ID: Points: 0.5
Reviewer: Sponsor: Sponsor28-can

Description

While dealing with broken obfs4 bridges, I realised that our bridge authority has several obfs4 bridges in its cached-extrainfo document that have private IP addresses, e.g.:

transport obfs4 10.0.254.17:[redacted]

The PT spec explicitly allows private addresses in TOR_PT_SERVER_BINDADDR:

The <address> MAY be a locally scoped address as long as port forwarding is done externally.


BridgeDB however ignores bridges with private IP addresses, so these obfs4 bridges are effectively useless. We could address this issue in BridgeDB by replacing an obfs4 bridge's private IP address with the address in its ORPort but I think that tor shouldn't be writing private addresses to a descriptor in the first place.

Child Tickets

TicketTypeStatusOwnerSummary
#31011defectneeds_revisioncjbMake the bridge authority reject private PT addresses when DirAllowPrivateAddresses is 0

Change History (31)

comment:1 Changed 16 months ago by teor

We should try to imitate or re-use Tor's implementation of OR addresses as much as possible:

  • ExtendAllowPrivateAddresses
    • should Tor connect to bridges on private addresses?
    • currently Tor connects to private bridges, we probably don't want to break those configs, so we should make the default auto, which is relays 0 bridges 1
  • DirAllowPrivateAddresses
    • this setting should make the bridge authority reject pluggable transport lines with private addresses
  • ORPort NoListen / NoAdvertise
    • these are ORPort flags, I'm not sure if we want to add similar flags to ServerTransportListenAddr
    • alternately, we could add a ServerTransportAdvertiseAddr in ServerTransportOptions, so the pluggable transport can find out about it

Is there a TOR_PT_SERVER_ADVERTISEADDR in the PT spec?
If not, we should add one?

comment:2 in reply to:  1 Changed 16 months ago by dcf

Replying to teor:

Is there a TOR_PT_SERVER_ADVERTISEADDR in the PT spec?
If not, we should add one?

That's not necessary--the PT doesn't need to know the advertised external address because it is not the thing that constructs the descriptor. All that a PT could do with that information is reflect it back to tor.

A PT process isn't even required to honor the address (port number) in TOR_PT_SERVER_BINDADDR. tor may suggest that the PT use a particular address, but it is ultimately the PT that chooses and informs tor of the actual address in an SMETHOD line.

comment:3 Changed 16 months ago by arma

My first thought, in terms of a low-impact hack, would be: when Tor is building its extrainfo descriptor, it should notice that it's about to advertise an internal address, and if so, put in the main ipv4 address of that descriptor instead.

(For internal and testing Tor networks, where the main ipv4 address is itself an internal address, no problem, we should use that.)

I think that simple change would resolve the vast majority of the cases that we're seeing right now. Then we could imagine also adding a config option to be able to say "I want to write a different line about my obfs4 PT into my extrainfo descriptor" -- but maybe that isn't an additional complexity that our operators actually need.

comment:4 in reply to:  3 Changed 16 months ago by arma

Replying to arma:

when Tor is building its extrainfo descriptor, it should notice that it's about to advertise an internal address

Here is my one-liner that implements this idea:

diff --git a/src/feature/client/transports.c b/src/feature/client/transports.c
index 97bfc8a..bd7d955 100644
--- a/src/feature/client/transports.c
+++ b/src/feature/client/transports.c
@@ -1641,7 +1641,7 @@ pt_get_extra_info_descriptor_string(void)
        * returned address. */
       const char *addrport = NULL;
       uint32_t external_ip_address = 0;
-      if (tor_addr_is_null(&t->addr) &&
+      if (tor_addr_is_internal(&t->addr, 0) &&
           router_pick_published_address(get_options(),
                                         &external_ip_address, 0) >= 0) {
         tor_addr_t addr;

It turns out the code already did the idea, but only in the case where it was about to advertise 0.0.0.0. So now it would do the same thing for a wider variety of internal addresses.

comment:5 Changed 16 months ago by teor

Cc: gaba added
Keywords: tor-pt tor-bridge 029-backport 035-backport 040-backport 041-backport added
Milestone: Tor: 0.4.2.x-final
Status: newneeds_review

Looks sensible to me.

I'll leave it to the reviewer to write the changes files, and make the backport branches.
Unless you want to do it, arma?

I also opened #31011 for the bridge authority side.

Gaba, this ticket should go in the PT sponsor with #31011.

comment:6 Changed 16 months ago by gaba

Sponsor: Sponsor28-can

comment:7 Changed 16 months ago by arma

I'd be pleased for somebody else to write up a changes file and make some git branches here.

comment:8 Changed 16 months ago by arma

For a related ticket, check out #7875: the issue there is that if you want to listen on a low-numbered port (<1024), e.g. by doing port forwarding in iptables, there's no way to tell your Tor bridge to advertise that low-numbered port in your extrainfo descriptor. So you can listen there, but clients won't know to connect there.

comment:9 Changed 16 months ago by asn

Reviewer: ahf

comment:10 Changed 16 months ago by ahf

Status: needs_reviewneeds_revision

Requested Github PR on IRC. The patch looks fine.

comment:11 Changed 15 months ago by gaba

Keywords: anti-censorship-roadmap-july added

comment:12 Changed 13 months ago by nickm

Keywords: 042-deferred-20190918 added
Milestone: Tor: 0.4.2.x-finalTor: unspecified

Deferring various tickets from 0.4.2 to Unspecified.

comment:13 Changed 10 months ago by phw

Status: needs_revisionneeds_review

I turned Roger's patch into a pull request on GitHub:
https://github.com/torproject/tor/pull/1622

The patch worked for me. When using a 192.168.0.0/16 address in ServerTransportListenAddr, tor rewrote it to my machine's external IP address. I tested it by taking a look at my bridge's extrainfo descriptor on BridgeDB.

My branch is based on master and I created a changes file as the documents in doc/HACKING/ told me. Let me know if there's anything wrong with the patch and I'll be happy to fix it.

comment:14 Changed 10 months ago by teor

Keywords: 029-backport removed
Milestone: Tor: unspecifiedTor: 0.4.3.x-final
Status: needs_reviewneeds_revision

Thanks for this patch!

This patch has two issues:

  • if the address is an IPv6 address, it is replaced with an IPv4 address
    • we should use the advertised IPv6 ORPort address to replace internal IPv6 addresses
  • the replacement happens in test and internal networks, as well as the public Tor network
    • there's no way that the bridge can know if internal addresses are acceptable to the bridge authority or BridgeDB. But I think it's still ok to replace the address, because the published address should be the right kind of address for these networks, anyway. But we should add comments explaining why it's ok.

I think we should also base this patch on maint-0.3.5, so we can backport it if needed.

comment:15 Changed 10 months ago by gaba

Keywords: network-team-roadmap-2020Q1 added

comment:16 Changed 9 months ago by nickm

(Is this needed urgently? Should we try to get it revised for 0.4.3, which is now in feature-freeze?)

comment:17 Changed 9 months ago by phw

It's not urgent from the anti-censorship team's point of view but certainly a "nice to have". For what it's worth, I'm unlikely to be able to revise my patch any time soon, so I would appreciate somebody else implementing this.

comment:18 Changed 9 months ago by phw

Keywords: anti-censorship-roadmap-july removed

comment:19 Changed 9 months ago by ahf

Owner: set to ahf
Reviewer: ahf
Status: needs_revisionassigned

I'm assigning myself to help with the IPv6 issue.

comment:20 Changed 9 months ago by ahf

Keywords: 043-should added

comment:21 in reply to:  14 Changed 9 months ago by teor

Cc: cjb added
Status: assignedneeds_revision

I noticed that cjb asked in the anti-censorship team meeting notes:

took a stab at #31009, but couldn't find an IPv6 replacement
for router_pick_published_address(). ahf's going to take it.

https://lists.torproject.org/pipermail/tor-project/2020-January/002672.html

There isn't an IPv6 version of router_pick_published_address(), but there will be in a few months time.
See #5940, and my upcoming proposal (312?) to tor-dev,

Here's what relays currently do, and what we should do for the moment:

Replying to teor:

  • if the address is an IPv6 address, it is replaced with an IPv4 address
    • we should use the advertised IPv6 ORPort address to replace internal IPv6 addresses

I'm going to make this ticket a child of #5940, so we don't forget to replace the IPv6 ORPort address with the new address function.

Alternatively, you could use the IPv4 and IPv6 address fields in the relay descriptor. That's probably a better design, because then the relay descriptor and extra-info descriptor addresses will always be in sync.

comment:22 Changed 7 months ago by catalyst

Cc: catalyst added

comment:23 Changed 7 months ago by catalyst

Owner: changed from ahf to catalyst
Status: needs_revisionassigned

comment:24 Changed 6 months ago by teor

For IPv6, you can use the router_get_advertised_ipv6_or_ap() function. But I'm not sure which tor version we added the function in.

See also #7961, which talks about a similar IPv6 issue in pt_get_extra_info_descriptor_string(). And #29128, which talks about writing the bridge line to a file.

comment:25 Changed 5 months ago by nickm

Milestone: Tor: 0.4.3.x-finalTor: 0.4.4.x-final

0.4.3 was released: Move non merge-ready 0.4.3 tickets to 044.

comment:26 Changed 5 months ago by nickm

Keywords: anticensorship-wants? added

comment:27 Changed 5 months ago by nickm

Keywords: 044-should added

Add 044-should to all foo-wants tickets in 044

comment:28 Changed 5 months ago by nickm

Keywords: anticensorship-wants added; anticensorship-wants? removed

comment:29 Changed 5 months ago by nickm

Owner: catalyst deleted

Un-assign tickets.

comment:30 Changed 5 months ago by nickm

Status: assignednew

comment:31 Changed 5 months ago by nickm

Keywords: 044-deferred added; 044-should removed
Milestone: Tor: 0.4.4.x-finalTor: 0.4.5.x-final
Type: defectenhancement
Note: See TracTickets for help on using tickets.