Opened 19 months ago

Closed 6 months ago

#17975 closed enhancement (implemented)

Introduce OutboundExitAddress to enable exit-only traffic to go via a different IP address

Reported by: naif Owned by:
Priority: Low Milestone: Tor: 0.3.0.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: lorax, yawning, isaremoved
Cc: michaelsonntag Actual Points:
Parent ID: Points: 1
Reviewer: nickm Sponsor:

Description

There are use cases of Tor where the inbound IP address of the Tor Relay is not the Tor Relay traffic, that's managed trough the use of OutBoundBindAddress directive.

However in multi-homed environments, it could provide much more flexibility to be able to specify a specific IP address to be used only for Tor Exit Traffic, with a directive that could be "OutboundExitAddress" .

With such an approach a family of Tor Nodes, handling inbound/outbound OR traffic, could implement custom routing architecture only for Tor Exit traffic (that's the one generating abuses), by tunnelling it across more "abuse resilient servers" .

If the assumption that 66.66% of traffic is non-exit OR traffic (i'm dividing by 3), then only 33.33% would go trough a more resilient "Exit Gateway" that could also not be at all a Tor Relay.

If such information would stay in the Consensus, it would be possible to map which Tor Relay is explicitly doing that kind "asymmetric routing" vs. who's doing it "implicitly" (and thus, it maybe a pattern of suspicious activity) .

Child Tickets

Attachments (3)

patch.txt (16.6 KB) - added by michaelsonntag 8 months ago.
Patch suggestion (v2)
patch_2016_12_20.txt (14.5 KB) - added by michaelsonntag 7 months ago.
Patch suggestion (v3)
patch_2017_01_20.txt (16.2 KB) - added by michaelsonntag 6 months ago.
Patch suggestion (v4)

Download all attachments as: .zip

Change History (27)

comment:1 Changed 19 months ago by nickm

  • Component changed from - Select a component to Tor
  • Keywords lorax added
  • Milestone set to Tor: 0.2.8.x-final
  • Priority changed from Medium to Low

comment:2 Changed 18 months ago by nickm

  • Milestone changed from Tor: 0.2.8.x-final to Tor: 0.2.9.x-final

It is impossible that we will fix all 226 currently open 028 tickets before 028 releases. Time to move some out. This is my second pass through the "new" and tickets, looking for things to move to 0.2.9.

comment:3 Changed 18 months ago by naif

/cc @yawning

comment:4 Changed 18 months ago by naif

  • Keywords yawning added

comment:5 Changed 16 months ago by nickm

  • Points set to small

comment:6 Changed 14 months ago by isabela

  • Points changed from small to 1

comment:7 Changed 12 months ago by isabela

  • Keywords isaremoved added
  • Milestone changed from Tor: 0.2.9.x-final to Tor: 0.2.???

comment:8 Changed 9 months ago by teor

  • Milestone changed from Tor: 0.2.??? to Tor: 0.3.0.x-final
  • Status changed from new to needs_review

This has been implemented in:
https://lists.torproject.org/pipermail/tor-dev/2016-October/011585.html

But needs review.

comment:9 Changed 9 months ago by nickm

  • Keywords review-group-11 added

comment:10 Changed 9 months ago by dgoulet

I put the patch in my personal repo to make it easier to review and comment: ticket17975_030_01.

Please feel free to take it somewhere or gitlab merge request or whatever. The commit message and author should be changed!

comment:11 follow-up: Changed 9 months ago by nickm

  • Status changed from needs_review to needs_revision

Notes:

  • Changes file should mention the ticket number.
  • Commit message should mention the author.
  • parse_outbound_addresses has gotten really complicated, and full of duplicate code. Maybe it's possible to refactor it to be cleaner? This "adr_found[i]" stuff is fairly inelegant.
  • Would it make more sense to have the syntax be "OutboundBindAddress Exit 1.2.3.4" ?
  • conn_get_outbound_address falls back to the Exit address when the OR address is null. That doesn't make sense to me. Why did the user say OutboundBindAddressExit if they meant OutboundBindAddress?
  • Other outbound connection types are possible. For example, directory connections, DNS lookups, and local connections to hidden services. Should those get handled too? Should they get treated as if they were exit?

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

Replying to nickm:

Notes:
...

  • conn_get_outbound_address falls back to the Exit address when the OR address is null. That doesn't make sense to me. Why did the user say OutboundBindAddressExit if they meant OutboundBindAddress?

+1

  • Other outbound connection types are possible. For example, directory connections, DNS lookups, and local connections to hidden services. Should those get handled too? Should they get treated as if they were exit?

I would say:
Directory -> OR
DNS -> Exit
HS (non-localhost) -> Exit????

comment:13 Changed 9 months ago by michaelsonntag

  • parse_outbound_address is complicated, because it is complex. All cases are different. There is the (old) general option affecting both OR and exit traffic, as well as the separation between IPv4 and IPv6 and the random sequence any of these options may appear. Therefore I don't think factoring out is really going to be useful (e.g. checking for no duplicate settings and providing helpful error messages while checking for one/two conflicting options). The "adr_found" array could be changed to a two-dimensional array with constants as index (OR, EXIT; IPv4, IPv6) - this would render it extensions easier.
  • conn_get_outbound_address falls back to the exit address if OR is null, because the user explicitly specified something, so this should be honored (better something than nothing). But it can also be left to the default address.
  • Other outbound connection types: the following seem to exist:

#define CONN_TYPE_OR_LISTENER 3
#define CONN_TYPE_OR 4
These are relay connections, so should not be sent over the "exit" interface

#define CONN_TYPE_EXIT 5
Actual exit connection, should use the "exit" interface

#define CONN_TYPE_AP_LISTENER 6
#define CONN_TYPE_AP 7
Socks proxy connections. This is "input" and should therefore not use the "exit" interface.

#define CONN_TYPE_DIR_LISTENER 8
#define CONN_TYPE_DIR 9
Directory server, i.e. internal communication going into this or another node. It should not use the "exit" interface.

#define CONN_TYPE_CONTROL_LISTENER 11
#define CONN_TYPE_CONTROL 12
Connection to a user interface - should be locally only and is definitely not "exit" traffic.

#define CONN_TYPE_AP_TRANS_LISTENER 13
#define CONN_TYPE_AP_NATD_LISTENER 14
Traffic redirected into tor, so incoming connections and should not use the "exit" interface.

#define CONN_TYPE_AP_DNS_LISTENER 15
Listen for DNS requests from clients; like SOCKS so no exit traffic.

#define CONN_TYPE_EXT_OR 16
#define CONN_TYPE_EXT_OR_LISTENER 17
Relay connections, should not be sent over the "exit" interface

  • DNS request nameservers seem to be configured in or/dns.c:1355 (configure_nameservers). However, in my configuration I could not find any position where HAVE_EVDNS_SET_DEFAULT_OUTGOING_BIND_ADDRESS is set/defined (remnant of old library?). Generally: DNS requests are problematic. The local host or some internal server might be used for DNS resolving. Then another option for configuring DNS would be necessary, as these may not be reachable from the "exit" interface.

comment:14 Changed 8 months ago by nickm

  • Keywords review-group-11 removed

Changed 8 months ago by michaelsonntag

Patch suggestion (v2)

comment:15 Changed 8 months ago by nickm

  • Keywords review-group-13 added
  • Status changed from needs_revision to needs_review

Thanks for the patch; moving this ticket to needs-review!

comment:16 Changed 8 months ago by dgoulet

  • Status changed from needs_review to needs_revision

I can't apply this patch on 0.2.8.8 (which is specifically made for that version). Can you resubmit it to either upstream master or _not_ depending on a specific directory name (tor-0.2.8.8/) ?

patch: **** malformed patch at line 209: diff --git a/tor-0.2.8.8/src/or/connection.c b/tor-0.2.8.8_new/src/or/connection.c

comment:17 Changed 8 months ago by teor

Code review (reading only, not patching or running):

Thanks, this is getting better, I think we are almost there!

Design:

parsing addresses:

  • the way this code turns OutboundBindAddress into OutboundBindAddressOR and OutboundBindAddressExit means that when the options are written out (for example, by a SETCONF), the torrc will change. We avoid changing the user's torrc.

verify_and_store_address:

  • please turn verify_and_store_address into a setter function which take a family and an enum for Exit/OR/Both, and then get/set options->OutboundBindAddress{OR,Exit,}IPv{4,6}_. It would really simplify parse_outbound_addresses.
    • this would also fix the use of hard-coded constants when passing adrCount to verify_and_store_address, and avoid the bug-prone pointer twiddling
  • once you have this abstraction, you can parse each group of lines using the same function: it takes lines, a family, and an enum for Exit/OR/Both.

conn_get_outbound_address:

  • please don't fall back from OutboundBindAddressExit to OutboundBindAddressOR, it's confusing and undocumented. Multihomed relays might have good reasons to use a particular IP for their OR connections, but allow Exit traffic to use the best route for the destination IP.
  • Update the man page: s/This option cannot be used together with OutboundBindAddress, unless they specify a different protocol./This option overrides OutboundBindAddress for the same IP version./
  • configure_nameservers should use conn_get_outbound_address

policies_copy_outbound_addresses_to_smartlist:

  • this list of addresses is used to block all addresses on the local machine. It must include both the OR and Exit addresses.

Standardisation:

  • address families are typically sa_family_t family in tor

Nitpicks:

  • please put a newline at the end of the changes file
  • const int is somewhat redundant, we typically only use const on pointers, and on variables in functions

Changed 7 months ago by michaelsonntag

Patch suggestion (v3)

comment:18 Changed 7 months ago by michaelsonntag

  • Cc michaelsonntag added
  • Status changed from needs_revision to needs_review

New patch version against current master:
Now the new options override the general option as requested - this allows configuring something not actually used without any error, e.g. OutboundBindAddress 10.1.1.1 and both OutboundBindAddressOR and -Exit with different values.

comment:19 Changed 7 months ago by nickm

  • Status changed from needs_review to needs_revision

Good progress. I like the array usage. Some more comments:

  • Reserving the symbols "EXIT, OR, EXIT_AND_OR, OUTBOUND_MAX" globally seems problematic. They should probably get a prefix.
  • AF_MAX can be pretty big on some systems, but we're only using two address families here. That seems kind of wasteful.
  • The memset of " sizeof(tor_addr_t)*OUTBOUND_MAX*AF_MAX" seems fragile. Probably better to use "sizeof(options->OutboundBindAddresses)" instead so that it can't get out-of-sync with the actual array bounds.
  • The function names "verify_and_store_address" and "parse_address_lines" should probably make it clear that they are for outbound*address lines. Otherwise, it gets confusing.

Teor, any other notes here?

comment:20 Changed 7 months ago by nickm

  • Keywords review-group-14 added; review-group-13 removed

And that's all for review-group-13.

comment:21 Changed 6 months ago by nickm

  • Keywords review-group-14 removed

Changed 6 months ago by michaelsonntag

Patch suggestion (v4)

comment:22 Changed 6 months ago by michaelsonntag

  • Status changed from needs_revision to needs_review

New patch version against current master, taking into account the comments.
I did not add new constants for IPv4/IPv6, but just used [0] and [1], as only these two are goint to be used in the near future.

comment:23 Changed 6 months ago by dgoulet

  • Reviewer set to nickm

comment:24 Changed 6 months ago by nickm

  • Resolution set to implemented
  • Status changed from needs_review to closed

Okay, merged that. I fixed a warning, and made the logic of conn_get_outbound_address a little more resilient to non-AF_INET{,6} addresses.

Thanks!

Note: See TracTickets for help on using tickets.