tor and capabilities
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.
Activity
changed milestone to %Tor: 0.2.8.x-final
Also cf. http://bugs.debian.org/700179
See also #5220 (moved)
Trac:
Username: kraai
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?
Trac:
Status: needs_review to needs_revision
Description: 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.to
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.
-
Trac:
captest.c 
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.
-
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.)
-
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!
-
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.