Opened 2 years ago

Last modified 2 months ago

#8195 needs_revision enhancement

tor and capabilities

Reported by: weasel Owned by:
Priority: normal Milestone: Tor: 0.2.7.x-final
Component: Tor Version:
Keywords: tor-relay security 026-triaged-1 026-deferrable Cc:
Actual Points: Parent ID:
Points:

Description (last modified by nickm)

We should figure out what it takes to keep the CAP_NET_BIND_SERVICE capability when changing the user away from root, so that we can re-open low listening ports later again.

Child Tickets

Attachments (2)

0001-Switch-user-before-opening-sockets-if-capable.patch (3.9 KB) - added by kraai 2 years ago.
captest.c (2.6 KB) - added by nickm 20 months ago.

Download all attachments as: .zip

Change History (21)

comment:2 Changed 2 years ago by arma

See also #5220

comment:3 Changed 2 years ago by nickm

  • Milestone set to Tor: 0.2.5.x-final
  • Status changed from new to needs_review

comment:4 Changed 2 years ago by nickm

  • Keywords tor-relay security added

comment:5 Changed 23 months ago by nickm

  • Description modified (diff)
  • Status changed from needs_review to needs_revision

Quick review, including some possibly stupid questions:

Needs a changes file.

Hm. We should add a comment to the config.c change that says that we're calling switch_user later on too, so that even if the first switch_id doesn't get called, we still change userid. We should also explain why we're trying the switch_id early.

Is there any way to do this with the supposedly more supposedly portable cap_set_proc() and cap_get_proc() interfaces, or will this forever be Linux-specific? The capget/capset manpage implies that the portable interface might be preferable.

On my host at least, the magic thing to include is <linux/capability.h>; though the manpage implies that sys/capability.h is supposed to be what works.

Is the ~ in " data.permitted &= ~CAP_TO_MASK(CAP_NET_BIND_SERVICE); " correct? It looks like it would leave every capability except CAP_NET_BIND_SERVICE.

Do we need to keep CAP_SETUID and CAP_SETGID so that the switch_id will work? Do we need to drop them afterward?

Can/should we clear KEEP_CAPS after switching the UID?

Should we use prctl(PR_SET_SECUREBITS) too to lock down the the environment?

comment:6 Changed 20 months ago by nickm

Hmm. We need to require (on some linuxes) that "libcap" be installed.

(Perhaps we should be using the cap_ functions rather than the capset/capget directly, if we can?)

comment:7 Changed 20 months ago by nickm

Also, cap_user_header_t and cap_user_data_t are defined (on my Linux) as pointers, not as structs.

What platform was this code tested on?

comment:8 Changed 20 months ago by nickm

After some hacking, I have a little test program, based on your code, that appears to actually work for my Linux. Attaching it now. It needs more auditing, inspection, testing and examination. It needs to be turned into a new patch. We should still consider many/all of the issues above.

Changed 20 months ago by nickm

comment:9 Changed 13 months ago by dgoulet

So here is a branch that uses libcap2 since it's a more portable API in terms of ABI changes from the kernel as stated in the man page.

It works fine right now and adds a "UseCapabilities 0|1" option to the torrc file for this. A first pass by anyone would be great!

https://github.com/dgoulet/tor.git (branch: bug8195)

Note that this does NOT drop any default existing capabilities but rather adds the bind service one. So, we might want to do that at some point in time and see exactly which capabilities Tor needs for all of its features.

No test though in that patch so that would be needed before any merge I guess.

comment:10 Changed 13 months ago by nickm

A good start! Some tests that we talked about are:

  • Make sure that we can't setuid() afterwards.
  • Make sure that we can't read some inaccessable file afterwards.
  • Make sure that we _can_ bind to some low port afterwards.
  • Make sure that we can't change capabilities afterwards.

And I think we thought that the sequence of actions should be:

  • Get ready to retain the binding-low-ports capability.
  • setuid.
  • Drop all capabilities besides binding low ports. (Including dropping all abilities that would allow us to regain other capabilities.)

comment:11 Changed 13 months ago by dgoulet

I hacked on it on the plane back from Island and I actually improve it quite a bit! The new version, not ready yet (but pushed on the same branch), now drops every capabilities except the one we need for the root process to setuid/setgid and then after only sets the two capabilities that I found that we need which are CAP_CHOWN and CAP_NET_BIND_SERVICE.

Once this is cleaned up I'll do a test that uses that compat API and make sure we have the expecting results like you mention.

I'll update back this when I have a patch that I'm satisfied with for review! Should be quite soon!

comment:12 Changed 13 months ago by arma

Neat! Thanks for working on this.

comment:13 Changed 13 months ago by nickm

Are you sure we need CAP_CHOWN? According to the capabilities(7) manpage:

       CAP_CHOWN
              Make arbitrary changes to file UIDs and GIDs (see chown(2)).

It sounds like that would allow the Tor process to do "chown nickm /etc/shadow" , which is probably not what we want. ;)

comment:14 Changed 13 months ago by dgoulet

Hrm, so of what I've seen Tor can chown() files *after* the boot time process. When the process goes to an unprivileged UID, the chown should only work on UID/GID it *owns* (behavior of chown(2)) thus the chown of /etc/shadow would be EPERM.

But now you've plant the "seed of doubt" in my head so I've looked in the Linux kernel and the check is actually done like this (fs/attr.c +49)

    /* Make sure a caller can chown. */
    if ((ia_valid & ATTR_UID) &&
        (!uid_eq(current_fsuid(), inode->i_uid) ||
         !uid_eq(attr->ia_uid, inode->i_uid)) &&
        !inode_capable(inode, CAP_CHOWN))
        return -EPERM;

Thus the check is done against the capability and the current UID with the file UID we are trying to change. I'll make a test just to be *sure* that I'm correct here unless I'm not completely wrong. Any case, the test will tell us.

comment:15 Changed 13 months ago by nickm

  • Milestone changed from Tor: 0.2.5.x-final to Tor: 0.2.6.x-final

Moving nearly all needs_revision tickets into 0.2.6, as now untimely for 0.2.5.

comment:16 Changed 13 months ago by dgoulet

Here is a "testing plan" to test capabilities in Tor.

As far as it's been investigated, Tor seems to need the NET_BIND_SERVICE and CHOWN capabilities to work. Pluggable transport spawned could be an issue here for missing capabilities but that would require individual testing with each supported PT.

The needed test would require root privileges (UID = 0). The target here is to test for each capabilities that we are interested in before setting it and after to ensure that it has been applied correctly.

Furthermore, for setuid(), Tor needs briefly the SETUID/SETGID capabilities thus this must be tested also with the current implementation.

*) List capabilities

1) List default capabilities and keep aside the list.
2) Set capabilities PRE setuid()
3) Match capabilities by listing the three sets (Effective, Permitted and Inheritable) and make sure we only have what's been set prior in the PRE step vis-a-vis the default cap. list.
4) Set capabilities POST setuid()
5) Match capabilities by listing the three sets (Effective, Permitted and Inheritable) and make sure we only have what's been set prior in the POST step vis-a-vis the default cap. list.

*) Bind privileged port with a unprivileged user.

1) Use bind() and make sure we are *unable* to do it expecting an EPERM.
2) Set capabilities PRE setuid()
3) setuid() to unpriv. user.
4) Set capabilities POST setuid()
5) Bind to privilege port and expect SUCCESS.

*) Chown file

1) Create 2 temporary file (A and B) with root:root and 660
2) Try to chown the A file to user:root, expect SUCCESS
3) Set capabilities PRE setuid()
4) setuid() to unpriv. user.
5) Set capabilities POST setuid()
6) Chown the B file to user:root expect EPERM
7) Chown the A file to user:user expert SUCCESS

(This capability needs to be confirmed if really needed.)

*) Setuid

1) Set PRE setuid() capabilities
2) Setuid() to unpriv user, expect SUCCESS
3) Set POST setuid() capabilities
4) Setuid() to unpriv *other* user, expect EPERM

*) Inheritance of capabilities for PT

1) Set Tor capabilities
2) fork() + execve()
3) List capabilities in the new process and make sure they match the expected once which should be NET BIND SERVICE and CHOWN for now.
4) Do bind + chown test inside the new process (see tests above).

*) Confirm that we don't have default capabilities enabled or some privileged one.

1) Create a fifo file (CAP_MKNOD). That should be enable by default. expect SUCCESS.
2) Create INET raw socket (CAP_NET_RAW), expect EPERM
3) Set Tor capabilities
4) Repeat fifo creation attempt, expect EPERM
5) Repeat raw socket, expect EPERM.

Quick draft for now on where I'm heading. Not sure yet how I can integrate that in the Tor tests repostiory especially with root but at least there is a basic plan now.

Nothing final here so please improve and add ideas! :)

comment:17 Changed 13 months ago by nickm

This all seems plausible. WRT integrating these into the rest of the tests when not running as root, I would suggest that it's okay to say that this test should just exit when you don't start it as root.

comment:18 Changed 7 months ago by nickm

  • Keywords 026-triaged-1 026-deferrable added

comment:19 Changed 2 months ago by dgoulet

  • Milestone changed from Tor: 0.2.6.x-final to Tor: 0.2.7.x-final

More work is needed to rebase that on 0.2.6, not sure I can make it for 0.2.6 feature freeze which would require much review. Also, PTs have a way to work without this so moving it to 0.2.7.

Note: See TracTickets for help on using tickets.