Tor lets transports advertise private IP addresses in descriptor
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
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.
Activity
changed milestone to %Tor: 0.4.5.x-final
Trac:
Child Ticket(s): #31011 (moved)added 035-backport 040-backport 041-backport 042-deferred-20190918 043-should 044-deferred anticensorship-wants component::core tor/tor milestone::Tor: 0.4.5.x-final network-team-roadmap-2020Q1 points::0.5 priority::medium severity::normal sponsor::28-can status::new tor-bridge tor-pt type::enhancement labels
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?
-
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.
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.
-
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.
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 (moved) for the bridge authority side.
Gaba, this ticket should go in the PT sponsor with #31011 (moved).
Trac:
Keywords: N/A deleted, 041-backport, 040-backport, 029-backport, 035-backport, tor-bridge, tor-pt added
Cc: ahf to ahf, gaba
Status: new to needs_review
Milestone: N/A to Tor: 0.4.2.x-final-
For a related ticket, check out #7875 (moved): 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.
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.
Trac:
Status: needs_revision to needs_reviewThanks 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.
Trac:
Keywords: 029-backport deleted, N/A added
Status: needs_review to needs_revision
Milestone: Tor: unspecified to Tor: 0.4.3.x-final- if the address is an IPv6 address, it is replaced with an IPv4 address
I noticed that cjb asked in the anti-censorship team meeting notes:
took a stab at #31009 (moved), 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 (moved), 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 (moved), 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.
Trac:
Cc: ahf, gaba to ahf, gaba, cjb
Status: assigned to needs_revision- if the address is an IPv6 address, it is replaced with an IPv4 address
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 (moved), which talks about a similar IPv6 issue in pt_get_extra_info_descriptor_string(). And #29128 (moved), which talks about writing the bridge line to a file.
mentioned in issue #31011 (moved)
moved to tpo/core/tor#31009
mentioned in issue tpo/core/tor#31011 (closed)
