Opened 6 months ago

Last modified 3 months ago

#31009 needs_revision defect

Tor lets transports advertise private IP addresses in descriptor

Reported by: phw Owned by:
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-pt, tor-bridge, 029-backport, 035-backport, 040-backport, 041-backport, anti-censorship-roadmap-july, 042-deferred-20190918
Cc: ahf, gaba Actual Points:
Parent ID: Points: 0.5
Reviewer: ahf Sponsor: Sponsor28-can


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[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

#31011newMake the bridge authority reject private PT addresses when DirAllowPrivateAddresses is 0Core Tor/Tor

Change History (12)

comment:1 Changed 6 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 6 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 6 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 6 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) &&
                                         &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 So now it would do the same thing for a wider variety of internal addresses.

comment:5 Changed 6 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 6 months ago by gaba

Sponsor: Sponsor28-can

comment:7 Changed 6 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 5 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 5 months ago by asn

Reviewer: ahf

comment:10 Changed 5 months ago by ahf

Status: needs_reviewneeds_revision

Requested Github PR on IRC. The patch looks fine.

comment:11 Changed 5 months ago by gaba

Keywords: anti-censorship-roadmap-july added

comment:12 Changed 3 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.

Note: See TracTickets for help on using tickets.