Opened 3 years ago

Last modified 5 weeks ago

#17949 needs_revision enhancement

Make loopback address search more accurate

Reported by: teor Owned by: rl1987
Priority: Medium Milestone: Tor: 0.3.6.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: easy, tor-client, tor-relay, loopback, weird-configuration, 035-triaged-in-20180711
Cc: rl1987@… Actual Points:
Parent ID: #17991 Points:
Reviewer: teor Sponsor:

Description

#17901 and #11360 search local interfaces for loopback addresses.

We could make this more efficient by using the flags returned with each address:

  • getifaddrs returns struct ifaddrs with ifa_flags which has IFF_LOOPBACK
  • ioctl(.,SIOCGIFCONF,.) returns struct ifreq with ifr_flags which has IFF_LOOPBACK
  • GetAdaptersAddresses (Win32) returns IP_ADAPTER_ADDRESSES with IfType which has IF_TYPE_SOFTWARE_LOOPBACK
    • Also pass GAA_FLAG_SKIP_FRIENDLY_NAME as we never use it
  • tor_getsockname/get_interface_address6_via_udp_socket_hack connects to a public address. When passed the localhost flag, it could connect to a loopback address, but this seems rather tautologous, and it's hard to know which loopback address to pick in 127/8. Alternately, we could lookup localhost using the resolver, and check the returned address is 127/8 or ::1 (otherwise, broken DNS configs could lead to security issues).

A design for this could be:

  • pass a flag to get_interface_address* indicating whether we want loopback addresses
  • pass that flag down to get_interface_addresses_raw
  • pass that flag to the API-specific functions
  • when converting the address to a smartlist, include/exclude addresses matching the specified types
    • always exclude tor_addr_is_null()
    • always exclude addresses matching tor_addr_is_multicast
  • for extra points, create a flags argument for:
    • loopback
      • this will require disabling the checks for tor_addr_is_loopback() in get_interface_address6_list() and get_interface_address6_via_udp_socket_hack(), see #17901
    • listen any (0.0.0.0 and [::], needs tor_addr_is_listen_any())
    • link-local (needs tor_addr_is_link_local())
    • other internal/private
    • public
    • with #defines:
      • _INTERNAL: everything except public
      • _INTERNAL_FOR_LISTENING: everything except public and listen any

After doing this, refactor the changes in #17901 and #11360, and all existing code, to use the new flags argument.

This will also resolve issues where systems have addresses other than 127.0.0.0/8 or ::1 as the loopback address (like some BSD jails and OpenVZ). But we're not too worried about this, they're non-standards-compliant, so the operator can configure the exact address correctly, as long as we fail with an informative message.

I need this to do #17835, I'll likely do it then if no-one gets to it first.

Child Tickets

Change History (53)

comment:1 Changed 3 years ago by rl1987

Cc: rl1987@… added

comment:2 Changed 3 years ago by teor

Parent ID: #17991

comment:3 Changed 3 years ago by teor

Milestone: Tor: 0.2.8.x-finalTor: 0.2.9.x-final

comment:4 Changed 3 years ago by rl1987

Owner: changed from teor to rl1987
Status: newaccepted

comment:5 Changed 3 years ago by teor

Keywords: TorCoreTeam201602 added
Milestone: Tor: 0.2.9.x-finalTor: 0.2.8.x-final

comment:6 Changed 3 years ago by bugzilla

In general, localhost is a TLD, and it must be resolved through DNS. In one of related tickets stated that 127.0.0.1 can be seamlessly redirected to public IP by the system. DNS can return "not found". So, there are enough reasons to stop rely on localhost as a security solution.
General practice is that services listen on 0 (0.0.0.0 and/or [::]). Address filtering is a task of firewall. To handle all tasks by tor instance is not a good practice.

comment:7 in reply to:  6 Changed 3 years ago by teor

Replying to bugzilla:

In general, localhost is a TLD, and it must be resolved through DNS. In one of related tickets stated that 127.0.0.1 can be seamlessly redirected to public IP by the system. DNS can return "not found". So, there are enough reasons to stop rely on localhost as a security solution.
General practice is that services listen on 0 (0.0.0.0 and/or [::]). Address filtering is a task of firewall. To handle all tasks by tor instance is not a good practice.

Tor aims to be secure by design and by default. Having secure defaults means that even if operators are unaware of or forget a particular security best practice, they will end up with an acceptably secure configuration.

comment:8 Changed 3 years ago by bugzilla

@teor:
Exactly. But relying on 127.0.0.1 led to #17901. You asked to research the situation with localhost, results were written. It's better stop trying to hack the technologies and warn users explicitly.

comment:9 in reply to:  8 Changed 3 years ago by teor

Replying to bugzilla:

@teor:
Exactly. But relying on 127.0.0.1 led to #17901. You asked to research the situation with localhost, results were written. It's better stop trying to hack the technologies and warn users explicitly.

While I recognise the accuracy of your research, I do not agree with your conclusions. If tor can make changes to be secure by default, we will do so. It's part of the design philosophy of the project. If you want to change that, the best way is via a mailing list discussion, proposal, or research (and not this ticket).

I'll respond to your question about #17901 on that ticket.

comment:10 Changed 3 years ago by nickm

Points: medium

comment:12 Changed 3 years ago by rl1987

Status: acceptedneeds_review

comment:13 Changed 3 years ago by teor

Looks great, let's get it merged!

comment:14 Changed 3 years ago by cypherpunks

make spaces detects some trailing spaces.

comment:15 Changed 3 years ago by bugzilla

int for bool, no tests for loopback != 0, ok?

comment:16 in reply to:  15 ; Changed 3 years ago by teor

Replying to bugzilla:

int for bool, no tests for loopback != 0, ok?

We don't use bool anywhere else in the codebase. This isn't the place to start.
And if (loopback) is equivalent to if (loopback != 0).

(If you really want a one-bit field, bool isn't sufficient, as it's 8 bits or more. You need a one-bit bitfield in a struct. Which is more trouble than it's worth, unless you already have a struct.)

comment:17 Changed 3 years ago by nickm

Hm. This makes a big pile of changes throughout the code, and adds an extra flag argument to get_interface_address6_list, which all callers must get right. It doesn't actually seem to make loopback detection more efficient, except by avoiding some allocations. Also, I don't know any reason to think that loopback lookup is actually performance-critical.

teor, rl1987 -- Are you sure this is a win?

comment:18 in reply to:  17 Changed 3 years ago by teor

Status: needs_reviewneeds_revision
Summary: Make loopback address search more efficientMake loopback address search more accurate

Replying to nickm:

Hm. This makes a big pile of changes throughout the code, and adds an extra flag argument to get_interface_address6_list, which all callers must get right. It doesn't actually seem to make loopback detection more efficient, except by avoiding some allocations. Also, I don't know any reason to think that loopback lookup is actually performance-critical.

teor, rl1987 -- Are you sure this is a win?

These changes fix bugs in the current loopback detection code. (Sorry, the ticket summary is badly worded.) The previous code avoided returning loopback addresses. But need to know local loopback addresses for #17901 and IPv6 localhost support (#11360 and #17835).

The patch checks that loopback addresses are 127/8 or ::1, which is halfway there.

But the patch doesn't actually check that the addresses returned are on a loopback interface:

getifaddrs returns struct ifaddrs with ifa_flags which has IFF_LOOPBACK
ioctl(.,SIOCGIFCONF,.) returns struct ifreq with ifr_flags which has IFF_LOOPBACK
GetAdaptersAddresses (Win32) returns IP_ADAPTER_ADDRESSES with IfType which has IF_TYPE_SOFTWARE_LOOPBACK

So once it's modified to do that, it's a definite win.

comment:19 in reply to:  17 Changed 3 years ago by teor

(This is my second comment in a row.)

Replying to nickm:

Hm. This makes a big pile of changes throughout the code, and adds an extra flag argument to get_interface_address6_list, which all callers must get right.

Why don't we call the new behaviour get_loopback_address6_list, and keep get_interface_address6_list for the old behaviour?

They can be implemented using get_interface_address6_list_impl, with the extra argument. Then we only have to get it right in two places.

And I can't see any reason that we'd want to get loopback and other addresses in the same list. We typically either want one or the other.

comment:20 Changed 3 years ago by nickm

Why don't we call the new behaviour get_loopback_address6_list, and keep get_interface_address6_list for the old behaviour?

That sounds like a pretty good idea to me.

comment:21 in reply to:  16 Changed 3 years ago by bugzilla

Replying to teor:

And if (loopback) is equivalent to if (loopback != 0).

loopback in function calls was meant.

We don't use bool anywhere else in the codebase. This isn't the place to start.

Developers of C language couldn't decide when to start too (until C99 became a standard).
See Mozilla's pain about it: https://blog.mozilla.org/mwu/2011/07/28/the-twelve-booleans-of-mozilla/
For this ticket proposing the advice of Brian Smith (:briansmith, :bsmith):

Please use an enum here. In the calling code, it would be unclear whether true means "strong" or "weak". By using an enum, you avoid that confusion.

(No, bitfield is for the other purposes.)

comment:22 Changed 3 years ago by nickm

Milestone: Tor: 0.2.8.x-finalTor: 0.2.9.x-final

These seem like features, or like other stuff unlikely to be possible this month. Bumping them to 0.2.9

comment:23 Changed 3 years ago by nickm

Keywords: TorCoreTeam201602 removed

comment:24 Changed 3 years ago by isabela

Milestone: Tor: 0.2.9.x-finalTor: 0.2.???

tickets market to be removed from milestone 029

comment:25 Changed 2 years ago by teor

Milestone: Tor: 0.2.???Tor: 0.3.???

Milestone renamed

comment:26 Changed 22 months ago by nickm

Keywords: tor-03-unspecified-201612 added
Milestone: Tor: 0.3.???Tor: unspecified

Finally admitting that 0.3.??? was a euphemism for Tor: unspecified all along.

comment:27 Changed 17 months ago by nickm

Keywords: tor-03-unspecified-201612 removed

Remove an old triaging keyword.

comment:28 Changed 16 months ago by nickm

Keywords: tor-client tor-relay loopback weird-configuration added

comment:29 Changed 6 months ago by rl1987

Status: needs_revisionneeds_review

Updated branch: https://github.com/rl1987/tor/commits/feature17949_cont

I am not sure if Windows part of this is right. Can anyone give it some attention? Does make test pass on Windows?

comment:30 Changed 6 months ago by teor

Milestone: Tor: unspecifiedTor: 0.3.4.x-final
Points: medium

I'm happy to look at this ticket eventually, but I'd like someone else to do the initial review here.

comment:31 in reply to:  29 Changed 6 months ago by dgoulet

Status: needs_reviewneeds_revision

Replying to rl1987:

Updated branch: https://github.com/rl1987/tor/commits/feature17949_cont

I am not sure if Windows part of this is right. Can anyone give it some attention? Does make test pass on Windows?

Hi rl1987! Thanks for this. You think I can ask you to submit a PR against torproject/tor.git on Github? Second thing, the current commit structure is a bit intense I would say. You think you can squash some commits in there together (like the fixes on previous commits) so the structure is just a bit more logical for us to understand. Thanks.

comment:32 Changed 6 months ago by rl1987

Status: needs_revisionneeds_review

I made a new branch, rebased and cleaned up my git history. It seems that I cannot create pull request against torproject repo on Github as I'm not "officially" a fork (I think my fork historically predates torproject mirror repo). However I made a pull request against my git master which I keep in-sync with upstream:

https://github.com/rl1987/tor/pull/2

This should make the patch easier to read.

comment:33 Changed 6 months ago by dgoulet

Reviewer: ahf

comment:34 Changed 6 months ago by teor

Reviewer: ahfahf, teor

ahf, if you do the first review, I'll do the second.

comment:35 Changed 6 months ago by rl1987

Fixed Windows tests in 79c82c933f44685b520d650485aa48fd53bdb75b.

comment:36 Changed 6 months ago by ahf

Status: needs_reviewneeds_revision

Commented on GH PR.

comment:37 Changed 6 months ago by rl1987

Status: needs_revisionneeds_review

Responded to your feedback on Github.

comment:38 Changed 6 months ago by rl1987

Status: needs_reviewneeds_revision

test_address_ifreq_to_smartlist fails on Travis.

comment:39 Changed 6 months ago by ahf

We need to make sure this passed on Trac :-) Thanks for the response!

Once this works correctly with Travis we should probably let Teor give it a round of review too.

comment:40 Changed 6 months ago by rl1987

Status: needs_revisionneeds_review

New pull request: https://github.com/torproject/tor/pull/92

In 0b121a10d6c91cd5cc4bb0e15918fe19dff55815 I made a workaround to get entire test suite working on Travis.

It passes successfully now: https://travis-ci.org/torproject/tor/builds/375907313

comment:41 Changed 6 months ago by isis

Reviewer: ahf, teorahf, teor, isis

I'm taking over secondary round of review for teor since he's away this week.

comment:42 Changed 5 months ago by isis

@rl1987 Where did ahf's original review go?

comment:43 Changed 5 months ago by isis

Status: needs_reviewmerge_ready

Let a review on the ticket, it mostly LGTM but there's one commit with some cross platform specific stuff (0b121a10) that I'm not sure I fully understand.

Tentatively setting as merge_ready if ahf and teor agree.

comment:44 in reply to:  42 Changed 5 months ago by rl1987

Replying to isis:

@rl1987 Where did ahf's original review go?

https://github.com/rl1987/tor_old/pull/2

comment:45 Changed 5 months ago by nickm

I'm going to defer this merge here till teor is back from holiday and can pronounce on this ticket. It can still maybe go in 0.3.4

comment:46 Changed 5 months ago by nickm

Milestone: Tor: 0.3.4.x-finalTor: 0.3.5.x-final

Pessimisticly assuming we don't get to merge these until 0.3.5, and hoping I'll be wrong.

comment:47 Changed 3 months ago by nickm

Reviewer: ahf, teor, isisteor
Status: merge_readyneeds_review

Since this ticket is in state "waiting for teor", let's actually have it reflect that state.

comment:48 Changed 3 months ago by teor

Status: needs_reviewneeds_revision

Hi, please see my comments on the pull request https://github.com/torproject/tor/pull/92

If you'd like to undo the function movement, that would make the pull request easier to review on GitHub.

Some of the GitHub comments are hidden due to the large 0.3.5 refactor.

Please don't rebases on master until we're ready to merge.

comment:49 Changed 3 months ago by nickm

Keywords: 035-triaged-in-20180711 added

comment:50 Changed 3 months ago by teor

rl1987, are your extra commits ready for review?

comment:51 Changed 3 months ago by rl1987

Not yet. Will try to make it ready today.

comment:52 Changed 3 months ago by rl1987

In get_interface_addresses_ioctl, we call ioctl(.,SIOCGIFCONF,.) to get ifconf struct, which has a pointer to struct ifreq:

struct ifreq {
#define IFHWADDRLEN	6
	union
	{
		char	ifrn_name[IFNAMSIZ];		/* if name, e.g. "en0" */
	} ifr_ifrn;
	
	union {
		struct	sockaddr ifru_addr;
		struct	sockaddr ifru_dstaddr;
		struct	sockaddr ifru_broadaddr;
		struct	sockaddr ifru_netmask;
		struct  sockaddr ifru_hwaddr;
		short	ifru_flags;
		int	ifru_ivalue;
		int	ifru_mtu;
		struct  ifmap ifru_map;
		char	ifru_slave[IFNAMSIZ];	/* Just fits the size */
		char	ifru_newname[IFNAMSIZ];
		void __user *	ifru_data;
		struct	if_settings ifru_settings;
	} ifr_ifru;
};

See:
https://github.com/freebsd/freebsd/blob/master/sys/net/if.h
https://github.com/aosm/xnu/blob/master/bsd/net/if.h
https://github.com/torvalds/linux/blob/master/include/uapi/linux/if.h

Note that both ifru_addr and ifru_flags are members of the same union, meaning they share the same memory and only one is to be set at a time. Yet ifreq_to_smartlist() reads both fields from the same instance of above struct.

To properly benefit from a ifru_flags here, we need to rework how we're using ioctl(). One ioctl() syscall is not enough!

comment:53 Changed 5 weeks ago by nickm

Milestone: Tor: 0.3.5.x-finalTor: 0.3.6.x-final

Deferring various feature-y things to 0.3.6. If one of these is actually happening in 0.3.5, please let me know!

Note: See TracTickets for help on using tickets.