Opened 3 years ago

Last modified 3 weeks ago

#8195 needs_review enhancement

tor and capabilities

Reported by: weasel Owned by:
Priority: Medium Milestone: Tor: 0.2.8.x-final
Component: Tor Version:
Severity: Normal Keywords: tor-relay, security, pre028-patch
Cc: Actual Points:
Parent ID: Points: small

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 3 years ago.
captest.c (2.6 KB) - added by nickm 2 years ago.

Download all attachments as: .zip

Change History (40)

comment:2 Changed 3 years ago by arma

See also #5220

comment:3 Changed 3 years ago by nickm

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

comment:4 Changed 3 years ago by nickm

  • Keywords tor-relay security added

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

comment:9 Changed 21 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! (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 21 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 21 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 21 months ago by arma

Neat! Thanks for working on this.

comment:13 Changed 21 months ago by nickm

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

              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 21 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 21 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 21 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 21 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 15 months ago by nickm

  • Keywords 026-triaged-1 026-deferrable added

comment:19 Changed 10 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.

comment:20 Changed 8 months ago by nickm

  • Keywords 027-triaged-1-out added

Marking triaged-out items from first round of 0.2.7 triage.

comment:21 Changed 8 months ago by nickm

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

Move *most* 0.2.7-triaged-1-out needs_revision items into 0.2.???. Keep a few based on my sense of the sensible.

comment:22 Changed 8 weeks ago by nickm

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

comment:23 Changed 8 weeks ago by nickm

  • Points set to small

comment:24 Changed 4 weeks ago by nickm

  • Keywords pre028-patch added

comment:25 Changed 3 weeks ago by nickm

  • Severity set to Normal

I'm wondering if this effort is worth pursuing. Linux is the only system that seems to have the capabilities support that we'd need here, and on Linux, the trend seems to be that when keeping root capabilities in a daemon, you just use systemd.

Are there relevant non-Linux systems that support capabilities like this? Among the non-systemd linux platforms, are there enough users who would want a feature like this to make it a good idea?

comment:26 Changed 3 weeks ago by yawning

FreeBSD has capsicum(4) ( as far as capabilities goes, but that's more along the lines of sandboxing than Linux capabilities. We should support that eventually but it's orthogonal to this, and none of the work here would carry over.

The existing state of PTs is somewhat better than it used to be since calling /usr/bin/setcap works for about half the transports as an alternative to port forwarding.

I'd vote to lorax this unless dgoulet is heavily invested in the code.

comment:27 Changed 3 weeks ago by nickm

  • Keywords 026-triaged-1 026-deferrable 027-triaged-1-out removed
  • Milestone changed from Tor: 0.2.8.x-final to Tor: unspecified
  • Priority changed from Medium to Very Low

Just heard from dgoulet, yawning, and athena: they all say this isn't a high priority, because of alternatives. (Also consider capsh.)

comment:28 Changed 3 weeks ago by nickm

  • Keywords pre028-patch removed

comment:29 Changed 3 weeks ago by nickm

  • Milestone changed from Tor: unspecified to Tor: 0.2.8.x-final
  • Priority changed from Very Low to Medium
  • Status changed from needs_revision to new

Weasel points out that "alternatives exist!" isn't so useful unless we document them.

So let's do that for a couple cases at least.

comment:31 Changed 3 weeks ago by nickm

The privbind documentation implies that capabilities won't work for this at all. :p

comment:32 Changed 3 weeks ago by nickm

In theory this should work for me as root:

capsh --keep=1 --secbits=0x14 --user=nickm --inh='cap_net_bind_service' \
   --caps='cap_net_bind_service=eip' -- -c "nc -l 1000"

but it doesn't. I wonder why.

comment:33 Changed 3 weeks ago by nickm

  • Keywords pre028-patch added

comment:34 Changed 3 weeks ago by nickm

Ugh. I've done my research here. It's a mess. You simply can't write a wrapper that does what I want without messing with the FS.

14:39 < nickm> To inherit any capability across an exec, the file being 
               exec()ed needs to have that capability listed in its 'inheritable' 
14:40 < nickm> exec does this: it clears every capability not in the 
               executable's inheritable set, then it grants every capability in 
               the executable's permitted set (subject to the system's bounding 
14:40 < nickm> obviously we wouldn't want to put anything in the permitted set; 
               that's basically setuid.
14:40 < nickm> (or a fraction of it.)
14:41 < nickm> but if we can't even put anything in the executable's inherited 
               set, then we can't do this with a sane external tool.
14:43 < nickm> (Digression: we could try to write a tool which forked, exec'd 
               the other process and THEN granted it capabilities, but I don't 
               see a way to avoid a race there.)
14:44 < nickm> ([To grant capabilities to another running process]
               You need the CAP_SETPCAP capability, and the documentation says 
               that really you shouldn't be holding on to that capability 
               unless you know what you're doing.)
14:44 < nickm> (I don't think I know what I'm doing enough to be safe there.)
14:45 < nickm> so option 1: mark the tor executable as allowed to inherit 
               CAP_NET_BIND_SERVICE.  THat means that if the calling process 
               intentionally passes that capability, tor can use it.
14:46 < nickm> option 2: do what #8195 envisioned, and teach Tor to retain this 
               capability across the setuid.
14:47 < nickm> [and for helpers]:
               option 1: mark the helper execuable as allowed to inherit the 
               capability, and teach tor how to pass it.
14:48 < nickm> option 2: let tor open the sockets and pass them across an 
               af_unix socket.
14:48 < nickm> option 3: i have no clue

comment:35 Changed 3 weeks ago by nickm

  • Status changed from new to needs_revision

comment:36 Changed 3 weeks ago by nickm

Looking at the code in david's branch and the code in my test code, I think I can get us something nice and compact here, and easy to test.

comment:37 Changed 3 weeks ago by nickm

I've done a minimalist implementation in my branch "bug8195_small". How's that?

(Tests forthcoming.)

comment:38 Changed 3 weeks ago by nickm

  • Status changed from needs_revision to needs_review

Now it has tests!

Note: See TracTickets for help on using tickets.