Opened 4 years ago

Last modified 17 months ago

#17901 new defect

Tor would bind ControlPort to public ip address if it has no localhost interface

Reported by: s7r Owned by:
Priority: High Milestone: Tor: unspecified
Component: Core Tor/Tor Version: Tor: 0.2.6.10
Severity: Major Keywords: tor-control misconfiguration security easy
Cc: teor Actual Points:
Parent ID: Points: 3
Reviewer: Sponsor:

Description

FreeBSD jails or some OpenVZ virtual machines do not have a lo interace (no 127.0.0.1 or localhost address), they only have an interface with either a public IP address either a NAT address routed upstream on the host.

In this case, if ControlPort 9051 is set, it will open the control port on the public IP address without even a warning which is not funny. This applies also to SocksPort, but that's less scary than ControlPort.

Child Tickets

Attachments (2)

test_loopback_sockname.c (3.7 KB) - added by teor 4 years ago.
Test for FreeBSD jail bind to 127.0.0.1 gives non-loopback address
test_loopback_interface.c (2.2 KB) - added by teor 4 years ago.
Test for OpenVZ 127.0.0.1 on non-loopback interface

Download all attachments as: .zip

Change History (54)

comment:1 Changed 4 years ago by teor

One way of resolving this issue is to check that we're actually binding to 127.0.0.1 or ::1 for the (default/no IP address) ControlPort and SOCKSPort, and complain loudly and fail to launch if we're not.

We can require the user to configure an explicit IP address (or access rules? does the ControlPort have those?) to silence the warning and start tor.

comment:2 in reply to:  1 Changed 4 years ago by yawning

Replying to teor:

One way of resolving this issue is to check that we're actually binding to 127.0.0.1 or ::1 for the (default/no IP address) ControlPort and SOCKSPort, and complain loudly and fail to launch if we're not.

I'm ok with this. We already have code for enumerating interfaces, so we could warn earlier as well.

We can require the user to configure an explicit IP address (or access rules? does the ControlPort have those?) to silence the warning and start tor.

There's flags for all the Ports, so adding another is easy-ish (to allow unsafe behavior). Even if they explicitly configure something I'd vote that we warn anyway, because it's still a horrific idea, just actually start up instead of terminating on the warning.

For future reference, if something that will never work correctly when jailed comes up in the future, there's a sysctl MIB (security.jail.jailed which will be set to 1) that can be queried via sysctl(3).

comment:3 Changed 4 years ago by s7r

While bind to whatever/default is good and wanted for DirPort and ORPort it's very not wanted for SocksPort, ControlPort, ExtORPort and other ports opened by Tor which are not meant to be open publicly.

teor I think if ControlPort <public IP>:<port> is manually and explicitly set we should assume that the user knows what he is doing and proceed, or be very protective and decide he'd rather not?

comment:4 in reply to:  3 Changed 4 years ago by yawning

Replying to s7r:

While bind to whatever/default is good and wanted for DirPort and ORPort it's very not wanted for SocksPort, ControlPort, ExtORPort and other ports opened by Tor which are not meant to be open publicly.

ExtOR being open to the world is... odd but won't be game breaking since it doesn't allow anything apart from serving as a sink for PT traffic (and it's authenticated similar to ControlPort with cookie auth).

teor I think if ControlPort <public IP>:<port> is manually and explicitly set we should assume that the user knows what he is doing and proceed, or be very protective and decide he'd rather not?

The former, but warn loudly that it's a bad idea, probably?

In an ideal world, we'd deprecate binding the ControlPort to non-AF_UNIX sockets where AF_UNIX is available (because it is that big of a foot + gun hazzard), but I expect that to be a non-starter just for legacy reasons, unfortunately.

comment:5 Changed 4 years ago by nickm

Keywords: 027-backport 026-backport added
Milestone: Tor: 0.2.???Tor: 0.2.8.x-final

comment:6 Changed 4 years ago by s7r

While we are looking at this, isn't #13953 somehow related? It appears to be related to the same IP address binding / selection part. Hopefully with not too much extra work we can fix both.

comment:7 Changed 4 years ago by teor

In #13953, the address that the ORPort binds to is correct, but the reachability test uses the wrong address. Therefore, #13953 would be best resolved with #17782, which also deals with reachability self-testing.

comment:8 Changed 4 years ago by nickm

Priority: MediumHigh
Severity: NormalMajor

comment:9 Changed 4 years ago by s7r

I think we should automatically disable ControlPort, ExtORPort, TransPort and DNSPort if we have no lo interface (127.0.0.1 localhost address) and they are set with just the port number or auto. If the setting for them is <pulic IP / NAT IP>:<port> we assume it's wanted and expected to be open there and proceed, but with loud warnings that it's a terrible idea. Maybe we should require authentication for ControlPort if opened on public / nat IP or quit otherwise? Not entirely sure if it's worth it.

For ORPort and DirPort binding to whatever IP address it sees is fine, we shouldn't change the behavior for these two, so whatever fixes we apply should be related to ControlPort TransPort DNSPort ExtORPort and SocksPort. Hope I didn't miss anything.

comment:10 Changed 4 years ago by nickm

I think we should have Tor exit if we have no loopback interface.

comment:11 in reply to:  10 Changed 4 years ago by yawning

Replying to nickm:

I think we should have Tor exit if we have no loopback interface.

Doesn't that totally break OpenVZ and FreeBSD jails? I think that's a bit extreme. We certainly should fail closed if the user automatically configures certain ports that have no business of being public, but Tor should continue to function jammed into a jail assuming it uses an AF_UNIX ControlPort for example...

comment:12 Changed 4 years ago by nickm

Ah; I hadn't understood how mandatory the "no localhost" thing was.

comment:13 Changed 4 years ago by teor

Owner: set to teor
Status: newassigned

To summarise, I think we need to implement the following changes:

  • For every *Port that currently listens on 127.0.0.1 by default:
    • ControlPort TransPort/NATDPort DNSPort ExtORPort SocksPort
  • If there is no 127.0.0.0/8 on the server, reject the *Port with a warning that tells the user to supply an explicit IP address if they really want their *Port listening on a non-local address.
  • Bind all *Ports to:
    • The first IPv4 address that "localhost" resolves to, as long as it is in 127.0.0.0/8, or 127.0.0.1 by default
      • This ensures that configurations that have localhost on an alternate address in 127.0.0.0/8 continue to work (this is another common BSD jail config)

This issue may also affect HiddenServicePort, which defaults to connecting to 127.0.0.1. We should check that it fails if there is no 127.0.0.1, and the warning is helpful, if so, the current behaviour is fine.

I can make these changes along with #11360.

comment:14 in reply to:  13 ; Changed 4 years ago by yawning

Replying to teor:

To summarise, I think we need to implement the following changes:

  • For every *Port that currently listens on 127.0.0.1 by default:
    • ControlPort TransPort/NATDPort DNSPort ExtORPort SocksPort
  • If there is no 127.0.0.0/8 on the server, reject the *Port with a warning that tells the user to supply an explicit IP address if they really want their *Port listening on a non-local address.
  • Bind all *Ports to:
    • The first IPv4 address that "localhost" resolves to, as long as it is in 127.0.0.0/8, or 127.0.0.1 by default
      • This ensures that configurations that have localhost on an alternate address in 127.0.0.0/8 continue to work (this is another common BSD jail config)

This issue may also affect HiddenServicePort, which defaults to connecting to 127.0.0.1. We should check that it fails if there is no 127.0.0.1, and the warning is helpful, if so, the current behaviour is fine.

I can make these changes along with #11360.

These seem ok. I'd suggest allowing localhost to also be [::1] for the far future. I'm vaguely inclined to also add an extra config option which needs to be enabled to allow non-localhost/AF_UNIX ControlPort, because it really is that bad of an idea, but that may be overly hand-holding.

comment:15 in reply to:  14 Changed 4 years ago by teor

Replying to yawning:

These seem ok. I'd suggest allowing localhost to also be [::1] for the far future.

See #11360.

(I've also split off #17948 for the hidden service IPv4/IPv6 localhost changes, as it's in a different area of the code, and not security related.)

I'm vaguely inclined to also add an extra config option which needs to be enabled to allow non-localhost/AF_UNIX ControlPort, because it really is that bad of an idea, but that may be overly hand-holding.

Another ticket?

comment:16 Changed 4 years ago by teor

Instead of resolving localhost, we could look through all the interfaces and find the first address present on the system that:

  • is 127.0.0.1, or
  • is in 127/8.

For stability, it would make sense to choose the numerically lowest valid 127/8 address.
(That is, exclude 127.0.0.0, and choose the address that is closest to 127.0.0.1.)

If a system doesn't allow tor to use those APIs, we could try to resolve localhost, but we have no guarantee that would work on a locked-down system either. For simplicity, I prefer to only use a single method to choose addresses, and leave the operator to sort it out otherwise.

comment:17 in reply to:  13 Changed 4 years ago by teor

Replying to teor:

  • If there is no 127.0.0.0/8 on the server, reject the *Port with a warning that tells the user to supply an explicit IP address if they really want their *Port listening on a non-local address.

If there is no 127.0.0.0/8 on the server, reject the *Port with a warning that tells the user to use AF_UNIX (if their system supports it), or supply an explicit IP address if they really want their *Port listening on a non-local address.

Edit: offer AF_UNIX first.

Last edited 4 years ago by teor (previous) (diff)

comment:18 in reply to:  16 Changed 4 years ago by yawning

Replying to teor:

Instead of resolving localhost, we could look through all the interfaces and find the first address present on the system that:

  • is 127.0.0.1, or
  • is in 127/8.

For stability, it would make sense to choose the numerically lowest valid 127/8 address.
(That is, exclude 127.0.0.0, and choose the address that is closest to 127.0.0.1.)

On Linux, when you query the interfaces, you can check the per-interface flags for IFF_LOOPBACK. On systems where this information is readily available (I think we even already have code that uses the relevant ioctl), we should use that to restrict the set of candidate addresses.

Edit: Yeah, get_interface_addresses_ioctl() issues the query we want, we just need to save the flags we care about somewhere.

Last edited 4 years ago by yawning (previous) (diff)

comment:19 Changed 4 years ago by teor

For systems that don't tell us which addresses are loopback addresses, we should use tor_addr_is_loopback.

comment:20 Changed 4 years ago by teor

For backporting to 0.2.7 and 0.2.6:

  • I've learned from previous experience not to make (internal) API changes when backporting, so I'll use tor_addr_is_loopback() on the list in the commits for this change
    • this will require disabling the check for tor_addr_is_loopback() in get_interface_address6_list() and get_interface_address6_via_udp_socket_hack(), so perhaps the backport will need a #define for backwards compatibility
      • in any case, the interface search routines changed between 0.2.6 and 0.2.7, so we'll need to be careful to make the minimal necessary changes in 0.2.6 to make sure they merge properly
  • I'd like to fall back to resolving localhost on systems that don't allow their interfaces to be enumerated, as long as we check that the returned values are standard 127/8 or [::1]. This should avoid any security issues, yet still give us an address on locked-down platforms. See #17953.

Because I don't think we can backport some of the changes suggested in this ticket, I've split them off for later:

  • #17949 for making loopback search more efficient
  • #17950 for making address family search more efficient
    • #17951 for returning both IPv4 and IPv6 when falling back to the socket hack with AF_UNSPEC
    • #17952 for returning both IPv4 and IPv6 from the ioctl on obscure platforms

comment:21 in reply to:  9 ; Changed 4 years ago by teor

Status: assignedneeds_information
Version: Tor: 0.2.7.6Tor: 0.2.6.10

Replying to s7r:

I think we should automatically disable ControlPort, ExtORPort, TransPort and DNSPort if we have no lo interface (127.0.0.1 localhost address) and they are set with just the port number or auto.

Has anyone checked that this is actually an issue?
I can't see how tor's code could cause this to happen, because it explicitly binds to 127.0.0.1.

I don't have access to a FreeBSD jail or OpenVZ vm with IPv4, but without a lo interface or 127.0.0.1

But on a FreeBSD IPv6-only instance (no IPv4 or 127.0.0.1), when I run tor --SOCKSPort 54321 --ControlPort 12345, I get:

[notice] Opening Socks listener on 127.0.0.1:54321
[warn] Socket creation failed: Protocol not supported
[notice] Opening Control listener on 127.0.0.1:12345
[warn] Socket creation failed: Protocol not supported

Looking at the code in parse_port_config, tor converts the default address string "127.0.0.1" using tor_addr_parse, so there's no possibility tor will bind to any other address but 127.0.0.1. (Even if /etc/hosts maps 127.0.0.1 to some other address.)

If this is an issue, I need to know how tor's explicit request to bind to 127.0.0.1 ends up binding to another address, and how tor can detect that. It would also help to have ifconfig output from one of these hosts.

(I split off the fix for a loopback without 127.0.0.1 into #17991.)

comment:22 in reply to:  21 Changed 4 years ago by teor

Replying to teor:

Looking at the code in parse_port_config, tor converts the default address string "127.0.0.1" using tor_addr_parse, so there's no possibility tor will bind to any other address but 127.0.0.1. (Even if /etc/hosts maps 127.0.0.1 to some other address.)

If this is an issue, I need to know how tor's explicit request to bind to 127.0.0.1 ends up binding to another address, and how tor can detect that. It would also help to have ifconfig output from one of these hosts.

I've looked up some of the reference documentation for FreeBSD and OpenVZ:

  • FreeBSD: "Inside a jail, access to the loopback address 127.0.0.1 is redirected to the first IP address assigned to the jail."
    • https://www.freebsd.org/doc/handbook/jails-ezjail.html
    • we can probably detect this using getsockname after binding the listener(!) like the UDP socket hack does. It would be safer to run the UDP socket hack on 127.0.0.1 and see what result we get.
    • I suspect that "the first IP address assigned to the jail" will be on an interface without the loopback flag. That's another way of detecting this scenario.

And if 127.0.0.1 is missing entirely, with no redirect in place, we'll handle it in #17991. (It's not a security issue, tor will simply fail to bind the listener to 127.0.0.1.)

comment:23 Changed 4 years ago by teor

I've attached two small programs to check for the FreeBSD jail issue (binding to 127.0.0.1 actually binds to another address) and the OpenVZ issue (127.0.0.1 is not on a loopback interface).

I haven't had time to check if they're portable or check their results on an actual jail or OpenVZ config yet. But they were written on OS X, so they're likely to be close to BSD semantics.

Changed 4 years ago by teor

Attachment: test_loopback_sockname.c added

Test for FreeBSD jail bind to 127.0.0.1 gives non-loopback address

Changed 4 years ago by teor

Attachment: test_loopback_interface.c added

Test for OpenVZ 127.0.0.1 on non-loopback interface

comment:24 Changed 3 years ago by teor

bugzilla raised concerns about this ticket on #17949:

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.

This issue happens only on machines where binding to 127.0.0.1 doesn't bind to a loopback interface. This is non-standard OS behaviour / configuration. On standards-conformant OSs, binding to 127.0.0.1 reliably ensures that the port is not accessible outside the local machine, reducing the attack surface considerably. People who configure their OS any other way are vulnerable unless they take additional precautions. Tor can detect serious security issues like this, close the port, and warn the user. So we will do that, because it's more secure by default.

comment:25 Changed 3 years ago by bugzilla

This is non-standard OS behaviour / configuration.

Wrong. Loopback interface is not mandatory. There might be no 127.0.0.1 according to standards too. So, all XAMPP-like suites use localhost. Why? Standards give an answer:

An IPv4 or IPv6 address query for the name localhost must always resolve to the respective loopback address, which is specified in a separate standard.

Applications may resolve the name to a loopback address themselves, or pass it to the local name resolver mechanisms.

Resolve or pass&check are both suited. So, Tor will start resolve localhost for TBB! (Devs want to see their local services in Tor Browser too).

comment:26 Changed 3 years ago by s7r

Agree with teor here.

Even if it's not standard, I think we can safely call it at least common practice that 127.0.0.1 will ensure port is not accessible from outside that box. We can't have Tor rely on independent OS firewalls that need extra configuration. Detecting and closing the port in such cases unless the user explicitly confirms that he knows what he's doing seams like a good approach to me.

comment:27 Changed 3 years ago by nickm

Keywords: must-fix-before-028-rc added

Marking these as must-fix-before-028-rc.

Actually, some of them may not need to get 'fixed' before the rc, but I believe that they should either get fixed, or we should have a good explanation of why they don't need to get fixed.

comment:28 Changed 3 years ago by teor

Keywords: must-fix-before-028-rc removed

I'd like to get this fixed before 0.2.8-rc, but it might not make it.

Here's how I'd like to fix it:

  • put the attached sample code into two functions in tor to detect the two different non-standard configs;
  • when we open the ControlPort, check if either of those conditions applies, and close it with a warning if they do;
  • check it works on the BSD test environment from s7r;
  • merge in 0.2.8 if it's done in time;
  • refactor to use #17949 if we can in 0.2.9.

comment:29 Changed 3 years ago by nickm

Keywords: 029-proposed added; 027-backport 026-backport removed
Milestone: Tor: 0.2.8.x-finalTor: 0.2.???

comment:30 Changed 3 years ago by nickm

Points: medium

comment:31 Changed 3 years ago by nickm

Keywords: 029-nickm-says-no added

Marking these tickets as ones I propose we do not include in 029.

comment:32 Changed 3 years ago by nickm

Keywords: 029-proposed 029-nickm-says-no removed

Nobody objected to these, so they aren't going into 029 milestone. Please re-add 029-proposed if you disagree, and let me know why.

comment:33 Changed 3 years ago by nickm

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

Oh, wait, teor said he thinks this should go in.

I think we can take it if we do the a simple version where we exit if you don't have lo.

comment:34 Changed 3 years ago by teor

Status: needs_informationnew

The two checks are:

  • The interface for 127.0.0.1 must have the loopback flag set
  • Binding to 127.0.0.1 must actually bind to 127.0.0.1

(And I still have to confirm the second check works on an actual FreeBSD jail.)

comment:35 Changed 3 years ago by isabela

Points: medium3

comment:36 Changed 3 years ago by teor

Owner: teor deleted
Status: newassigned

I don't have time to revise this in 0.2.9.

comment:37 Changed 3 years ago by teor

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

While it would be nice to have this fixed, we need someone with FreeBSD / OpenVZ to develop and test it.

comment:38 Changed 3 years ago by s7r

I have a FreeBSD box with a fresh jail that has a public IP address assigned to it. I could add someone's ssh key to it if needed. Willing to help in testing as well. As for OpenVZ, I kind of hate it, but I'll check with gamambel and point him to this ticket as I recall they had some OpenVZ stuff in their infrastructure.

comment:39 Changed 3 years ago by mo

I have OpenVZ infrastructure to test this, let me know when you need me. Can set up a container for you to play with also.

comment:40 Changed 3 years ago by teor

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

Milestone renamed

comment:41 Changed 3 years 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:42 Changed 2 years ago by nickm

Keywords: tor-03-unspecified-201612 removed

Remove an old triaging keyword.

comment:43 Changed 2 years ago by nickm

Status: assignednew

Change the status of all assigned/accepted Tor tickets with owner="" to "new".

comment:44 Changed 2 years ago by nickm

Keywords: tor-control misconfiguration security easy added

comment:45 Changed 17 months ago by fristonio

If I got the issue right then, since ControlPort offers total control over tor we should not let it bind when the port is publicly exposed or when there is no loopback interface available. So to implement this we need to use the functions attached here by teor and then check if loopback address is available when we bind ControlPort using these functions, and close tor if check fails with a warning that it is dangerous.

Can I work on this?

comment:46 Changed 17 months ago by teor

You can, but please be aware that it might not get merged if we don't have anyone to test it.

We should do what we currrently do if the control port is set to a public IP address:

  • check if the control port will bind to localhost
  • if not, warn the user that the control port should not be public

.

comment:47 Changed 17 months ago by fristonio

Test it on FreeBSD and OpenVZ right?

I can attach a patch here anyway, it seems like an interesting issue and maybe if sr7 and mo can give me a container in their infrastructure I can test it there. :)

comment:48 Changed 17 months ago by teor

To answer your question on #tor-dev:

If you implement the new check as part of tor_addr_is_local(), add a for_listening flag like tor_addr_is_internal(), and put your new code behind that flag:
https://gitweb.torproject.org/tor.git/tree/src/common/address.c#n353

Edit: spurious escapes, spelling

Last edited 17 months ago by teor (previous) (diff)

comment:49 Changed 17 months ago by fristonio

teor, other than changing some basic functions like assert to tor_assert free to tor_free should I also segregate IPv4 and IPv6 checks you have implemented in test_loopback_sockname and test_loopback_interface into different functions like one for IPv4 addresses and one for IPv6 ones?

comment:50 in reply to:  49 Changed 17 months ago by teor

Replying to fristonio:

teor, other than changing some basic functions like assert to tor_assert

You should use a non-fatal assert, like if (BUG(condition)) { return -1; }.

free to tor_free should I also segregate IPv4 and IPv6 checks you have implemented in test_loopback_sockname and test_loopback_interface into different functions like one for IPv4 addresses and one for IPv6 ones?

You can do that if you'd like. We usually use an int family flag to choose between IPv4 and IPv6 code, so that we are not repeating code.

Please print a warning if the checks fail, and an info-level message if they succeed.

Also, try to re-use the existing Tor code that scans interfaces and checks socknames. Because that's where I copied this code from.

The test code was designed to inspect the interfaces on a machine, and print their addresses and flags. This patch stalled because I couldn't get anyone to run it in the relevant environments. I still haven't seen the output of the test code in any of the relevant environments. So there is no guarantee that it actually works. And I don't know which of the alternative sockname tests you should do.

Maybe you should wait until mo and s7r run the attached code and report the results?
Or maybe you could do it yourself if they give you a shell?

comment:51 Changed 17 months ago by yawning

Cc: yawning removed

comment:52 Changed 17 months ago by fristonio

Ok, I will wait until the code is tested and yes I would love to help test them. :)

Note: See TracTickets for help on using tickets.