capabilites, take 2

Implement optional working capability support. Try to avoid giving Andrew a heart attack. ;)

fs/exec.c renamed from fs/exec.c~caps +3 -1

@@ -882,8 +882,10 @@
 
 	if(!(bprm->file->f_vfsmnt->mnt_flags & MNT_NOSUID)) {
 		/* Set-uid? */
-		if (mode & S_ISUID)
+		if (mode & S_ISUID) {
 			bprm->e_uid = inode->i_uid;
+			bprm->secflags |= BINPRM_SEC_SETUID;
+		}
 
 		/* Set-gid? */
 		/*

@@ -891,10 +893,19 @@
 		 * is a candidate for mandatory locking, not a setgid
 		 * executable.

fs/exec.c renamed from fs/exec.c~caps +5 -0

+		cap_set_full(bprm->cap_permitted);
+	else
+		cap_clear(bprm->cap_permitted);
+
 	/* fill in binprm security blob */
 	retval = security_bprm_set(bprm);
 	if (retval)

@@ -1089,6 +1100,7 @@
 	bprm.loader = 0;
 	bprm.exec = 0;
 	bprm.security = NULL;
+	bprm.secflags = 0;
 	bprm.mm = mm_alloc();
 	retval = -ENOMEM;
 	if (!bprm.mm)

security/commoncap.c renamed from security/commoncap.c~caps +4 -0

+
+module_param(newcaps, int, 444);
+MODULE_PARM_DESC(newcaps, "Set newcaps=1 to enable experimental capabilities");
+
 int cap_capable (struct task_struct *tsk, int cap)
 {
 	/* Derived from include/linux/sched.h:capable. */

security/commoncap.c renamed from security/commoncap.c~caps +6 -0

+
 	if (!cap_issubset (child->cap_permitted, current->cap_permitted) &&
 	    !capable (CAP_SYS_PTRACE))
 		return -EPERM;

@@ -76,6 +86,11 @@
 		return -EPERM;
 	}
 
+	/* verify the _new_Permitted_ is a subset of the _new_Inheritable_ */
+	if (newcaps && !cap_issubset (*permitted, *inheritable)) {
+		return -EPERM;
+	}
+
 	return 0;
 }
 

security/commoncap.c renamed from security/commoncap.c~caps +4 -2

+
 	/* Copied from fs/exec.c:prepare_binprm. */
 
 	/* We don't have VFS support for capabilities yet */

@@ -115,10 +132,11 @@
 	return 0;
 }
 
-void cap_bprm_apply_creds (struct linux_binprm *bprm, int unsafe)
+static void cap_bprm_apply_creds_compat (struct linux_binprm *bprm, int unsafe)
 {
-	/* Derived from fs/exec.c:compute_creds. */
+	/* This function will hopefully die in 2.7. */
 	kernel_cap_t new_permitted, working;
+	static int fixed_init = 0;
 
 	new_permitted = cap_intersect (bprm->cap_permitted, cap_bset);
 	working = cap_intersect (bprm->cap_inheritable,

@@ -151,6 +169,15 @@
 		current->cap_permitted = new_permitted;
 		current->cap_effective =

security/commoncap.c renamed from security/commoncap.c~caps +115 -9

 	}
 
 	/* AUD: Audit candidate if current->cap_effective is set */

@@ -158,15 +185,103 @@
 	current->keep_capabilities = 0;
 }
 
+/*
+ * The rules of Linux capabilities (not POSIX!)
+ *
+ * What the masks mean:
+ *  pI = capabilities that this process or its children may have
+ *  pP = capabilities that this process has
+ *  pE = capabilities that this process has and are enabled
+ * (so pE <= pP <= pI)
+ *
+ * The capability evolution rules are:
+ *
+ *  pI' = pI & fI
+ *  pP' = ((fP & cap_bset) | pP) & pI' & Y
+ *  pE' = (setuid ? pP' : (pE & pP'))
+ *
+ *  X = cap_bset
+ *  Y is zero if uid!=0, euid==0, and setuid non-root
+ *
+ * Caveat: if (fP & ~pI'), there is no _theoretical_ problem, but
+ * this could introduce exploits in buggy programs.  Since programs
+ * that aren't capability-aware are insecure _anyway_ if pP!=0, this
+ * is OK.
+ *
+ * To allow pI != ~0 to be secure in the presence of buggy programs,
+ * we require full pI for setuid.
+ *
+ * The moral is that, if file capabilities are introduced, programs
+ * that are granted fP > 0 need to be aware of how to deal with it.
+ * Because the effective set is left alone on non-setuid fP>0,
+ * such a program should drop capabilities that were not in its initial
+ * effective set before running untrusted code.
+ *
+ */
+void cap_bprm_apply_creds (struct linux_binprm *bprm, int unsafe)
+{
+	kernel_cap_t new_pI, new_pP;
+	kernel_cap_t fI, fP;
+	int is_setuid, is_setgid;
+
+	if(!newcaps) {
+		cap_bprm_apply_creds_compat(bprm, unsafe);
+		return;
+	}
+
+	fI = bprm->cap_inheritable;
+	fP = bprm->cap_permitted;
+	is_setuid = (bprm->secflags & BINPRM_SEC_SETUID);
+	is_setgid = (bprm->secflags & BINPRM_SEC_SETGID);
+
+	new_pI = cap_intersect(current->cap_inheritable, fI);
+
+	/* Check that it's safe to elevate privileges */
+	if (unsafe & ~LSM_UNSAFE_PTRACE_CAP)
+		bprm->secflags |= BINPRM_SEC_NOELEVATE;
+
+	/* FIXME: Is this overly harsh on setgid? */
+	if ((bprm->secflags & (BINPRM_SEC_SETUID | BINPRM_SEC_SETGID)) &&
+	    new_pI != CAP_FULL_SET)
+			bprm->secflags |= BINPRM_SEC_NOELEVATE;
+
+	if (bprm->secflags & BINPRM_SEC_NOELEVATE) {
+		is_setuid = is_setgid = 0;
+		cap_clear(fP);
+	}
+
+	new_pP = cap_intersect(fP, cap_bset);
+	new_pP = cap_combine(new_pP, current->cap_permitted);
+	cap_mask(new_pP, new_pI);
+
+	/* setuid-nonroot is special. */
+	if (is_setuid && bprm->e_uid != 0 && current->uid != 0 &&
+	    current->euid == 0)
+		cap_clear(new_pP);
+
+	if(!cap_issubset(new_pP, current->cap_permitted))
+		bprm->secflags |= BINPRM_SEC_SECUREEXEC;
+
+	/* Apply new security state */
+	if (is_setuid) {
+	        current->suid = current->euid = current->fsuid = bprm->e_uid;
+		current->cap_effective = new_pP;
+	}
+	if (is_setgid)
+	        current->sgid = current->egid = current->fsgid = bprm->e_gid;
+
+	current->cap_inheritable = new_pI;
+	current->cap_permitted = new_pP;
+	cap_mask(current->cap_effective, new_pP);
+
+	current->keep_capabilities = 0;
+}
+
 int cap_bprm_secureexec (struct linux_binprm *bprm)
 {
-	/* If/when this module is enhanced to incorporate capability
-	   bits on files, the test below should be extended to also perform a 
-	   test between the old and new capability sets.  For now,
-	   it simply preserves the legacy decision algorithm used by
-	   the old userland. */
 	return (current->euid != current->uid ||
-		current->egid != current->gid);
+		current->egid != current->gid ||
+		(bprm->secflags & BINPRM_SEC_SECUREEXEC));
 }
 
 int cap_inode_setxattr(struct dentry *dentry, char *name, void *value,

@@ -280,9 +395,15 @@
 
 void cap_task_reparent_to_init (struct task_struct *p)
 {
-	p->cap_effective = CAP_INIT_EFF_SET;
-	p->cap_inheritable = CAP_INIT_INH_SET;
-	p->cap_permitted = CAP_FULL_SET;
+	if (newcaps) {
+		cap_set_full(p->cap_inheritable);
+		cap_set_full(p->cap_permitted);
+		cap_set_full(p->cap_effective);
+	} else {
+		p->cap_effective = CAP_OLD_INIT_EFF_SET;
+		p->cap_inheritable = CAP_OLD_INIT_INH_SET;
+		p->cap_permitted = CAP_FULL_SET;
+	}
 	p->keep_capabilities = 0;
 	return;
 }

@@ -367,6 +488,16 @@
 	return -ENOMEM;
 }
 
+static int __init commoncap_init (void)
+{
+	if (newcaps) {
+		printk(KERN_NOTICE "Experimental capability support is on\n");
+		cap_bset = CAP_FULL_SET;
+	}
+
+	return 0;
+}
+
 EXPORT_SYMBOL(cap_capable);
 EXPORT_SYMBOL(cap_ptrace);
 EXPORT_SYMBOL(cap_capget);

@@ -382,5 +513,7 @@
 EXPORT_SYMBOL(cap_syslog);
 EXPORT_SYMBOL(cap_vm_enough_memory);
 
+module_init(commoncap_init);
+
 MODULE_DESCRIPTION("Standard Linux Common Capabilities Security Module");
 MODULE_LICENSE("GPL");

kernel/capability.c renamed from kernel/capability.c~caps +1 -1

@@ -13,7 +13,7 @@
 #include <asm/uaccess.h>
 
 unsigned securebits = SECUREBITS_DEFAULT; /* systemwide security settings */
-kernel_cap_t cap_bset = CAP_INIT_EFF_SET;
+kernel_cap_t cap_bset = CAP_OLD_INIT_EFF_SET;
 int sysctl_mlock_group;
 
 EXPORT_SYMBOL(securebits);

include/linux/capability.h renamed from include/linux/capability.h~caps +4 -2

@@ -308,8 +308,10 @@
 
 #define CAP_EMPTY_SET       to_cap_t(0)
 #define CAP_FULL_SET        to_cap_t(~0)
-#define CAP_INIT_EFF_SET    to_cap_t(~0 & ~CAP_TO_MASK(CAP_SETPCAP))
-#define CAP_INIT_INH_SET    to_cap_t(0)
+
+/* For old-style capabilities, we use these. */
+#define CAP_OLD_INIT_EFF_SET    to_cap_t(~0 & ~CAP_TO_MASK(CAP_SETPCAP))
+#define CAP_OLD_INIT_INH_SET    to_cap_t(0)
 
 #define CAP_TO_MASK(x) (1 << (x))
 #define cap_raise(c, flag)   (cap_t(c) |=  CAP_TO_MASK(flag))

include/linux/binfmts.h renamed from include/linux/binfmts.h~caps +7 -1

@@ -20,6 +20,10 @@
 /*
  * This structure is used to hold the arguments that are used when loading binaries.
  */
+#define BINPRM_SEC_SETUID	1
+#define BINPRM_SEC_SETGID	2
+#define BINPRM_SEC_SECUREEXEC	4
+#define BINPRM_SEC_NOELEVATE	8
 struct linux_binprm{
 	char buf[BINPRM_BUF_SIZE];
 	struct page *page[MAX_ARG_PAGES];

@@ -28,7 +32,9 @@
 	int sh_bang;
 	struct file * file;
 	int e_uid, e_gid;
-	kernel_cap_t cap_inheritable, cap_permitted, cap_effective;
+	int secflags;
+	kernel_cap_t cap_inheritable, cap_permitted;
+	kernel_cap_t cap_effective; /* old caps -- do NOT use in new code */
 	void *security;
 	int argc, envc;
 	char * filename;	/* Name of binary as seen by procps */

I think it still needs more work. Default behavoiur is changed, like Inheritble is full rather than clear, setpcap is enabled, etc. Also, why do you change from Posix the way exec() updates capabilities? Sure, there is no filesystem bits present, so this changes the calculation, but I'm not convinced it's as secure this way. At least with newcaps=0.

Quoted 5 lines · Chris Wright

I think it still needs more work. Default behavoiur is changed, like Inheritble is full rather than clear, setpcap is enabled, etc. Also, why do you change from Posix the way exec() updates capabilities? Sure, there is no filesystem bits present, so this changes the calculation, but I'm not convinced it's as secure this way. At least with newcaps=0.

The last time the "capabilities" thread reared its head a while ago, Andy made a posting that pretty conclusively showed that the Posix way was totally b0rken if you ever intended to support filesystem bits. So if you wanted to ever have a snowball's chance of supporting something like:

chcap cap_net_raw+ep /bin/ping

Quoted 5 lines · Chris Wright

I think it still needs more work. Default behavoiur is changed, like Inheritble is full rather than clear, setpcap is enabled, etc. Also, why do you change from Posix the way exec() updates capabilities? Sure, there is no filesystem bits present, so this changes the calculation, but I'm not convinced it's as secure this way. At least with newcaps=0.

The last time the "capabilities" thread reared its head a while ago, Andy made a posting that pretty conclusively showed that the Posix way was totally b0rken if you ever intended to support filesystem bits. So if you wanted to ever have a snowball's chance of supporting something like:

chcap cap_net_raw+ep /bin/ping

2) Toss all the filesystems capabilities support out the window.

Seems like you're not aware of:
<http://www.olafdietsche.de/linux/capability/>

This supports filesystem capabilities with the current (POSIX?) implementation. So, whatever Andy has shown, it has at least one counter evidence q.e.d.

Quoted 5 lines · Olaf Dietsche

Seems like you're not aware of:
<http://www.olafdietsche.de/linux/capability/>

This supports filesystem capabilities with the current (POSIX?) implementation.

Yes.. I was aware of that.. and I just visited it.. and the VERY TOP it says:

"Filesystem capabilities for linux

This implementation is likely *not* POSIX compatible."

Now who should I believe, you or the author of the page? :)

Quoted 2 lines · * Andy Lutomirski ([email protected]) wrote:

Implement optional working capability support. Try to avoid giving Andrew a heart attack. ;)

I think it still needs more work. Default behavoiur is changed, like Inheritble is full rather than clear, setpcap is enabled, etc. ...

... Also,
why do you change from Posix the way exec() updates capabilities? Sure, there is no filesystem bits present, so this changes the calculation, but I'm not convinced it's as secure this way. At least with newcaps=0.

I believe we can get something functional with fewer changes, hence easier to understand the ramifications. In a nutshell, I'm still not comfortable with this.

Also, it breaks my tests which try to drop privs and keep caps across execve() which is really the only issue we're trying to solve ATM.

Quoted 25 lines

fs/exec.c renamed from fs/exec.c~caps +6 -2

@@ -882,8 +882,10 @@
 
 	if(!(bprm->file->f_vfsmnt->mnt_flags & MNT_NOSUID)) {
 		/* Set-uid? */
-		if (mode & S_ISUID)
+		if (mode & S_ISUID) {
 			bprm->e_uid = inode->i_uid;
+			bprm->secflags |= BINPRM_SEC_SETUID;
+		}
 
 		/* Set-gid? */
 		/*

@@ -891,10 +893,19 @@
 		 * is a candidate for mandatory locking, not a setgid
 		 * executable.
 		 */
-		if ((mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP))
+		if ((mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP)) {
 			bprm->e_gid = inode->i_gid;
+			bprm->secflags |= BINPRM_SEC_SETGID;
+		}
 	}
 

fs/exec.c line 903

Quoted 2 lines

fs/exec.c renamed from fs/exec.c~caps +2 -0

+	/* Pretend we have VFS capabilities */
+	cap_set_full(bprm->cap_inheritable);

This looks sketchy.

fs/exec.c line 904

Quoted 3 lines

fs/exec.c renamed from fs/exec.c~caps +1 -0

+	if((bprm->secflags & BINPRM_SEC_SETUID) && bprm->e_uid == 0)

CodingStyle: add space after keyword 'if'

	if ((...))

Quoted 21 lines

fs/exec.c renamed from fs/exec.c~caps +5 -0

+		cap_set_full(bprm->cap_permitted);
+	else
+		cap_clear(bprm->cap_permitted);
+
 	/* fill in binprm security blob */
 	retval = security_bprm_set(bprm);
 	if (retval)

@@ -1089,6 +1100,7 @@
 	bprm.loader = 0;
 	bprm.exec = 0;
 	bprm.security = NULL;
+	bprm.secflags = 0;
 	bprm.mm = mm_alloc();
 	retval = -ENOMEM;
 	if (!bprm.mm)

security/commoncap.c renamed from security/commoncap.c~caps +0 -0

@@ -24,6 +24,11 @@
 #include <linux/xattr.h>
 #include <linux/hugetlb.h>
 

security/commoncap.c line 27

Quoted 1 line

security/commoncap.c renamed from security/commoncap.c~caps +1 -0

+int newcaps = 0;

make this:
static int newcaps;

Quoted 12 lines

security/commoncap.c renamed from security/commoncap.c~caps +4 -0

+
+module_param(newcaps, int, 444);
+MODULE_PARM_DESC(newcaps, "Set newcaps=1 to enable experimental capabilities");
+
 int cap_capable (struct task_struct *tsk, int cap)
 {
 	/* Derived from include/linux/sched.h:capable. */

@@ -36,6 +41,11 @@
 int cap_ptrace (struct task_struct *parent, struct task_struct *child)
 {

security/commoncap.c line 47

Quoted 5 lines

security/commoncap.c renamed from security/commoncap.c~caps +4 -0

 	/* Derived from arch/i386/kernel/ptrace.c:sys_ptrace. */
+	/* CAP_SYS_PTRACE still can't bypass inheritable restrictions */
+	if (newcaps &&
+	    !cap_issubset (child->cap_inheritable, current->cap_inheritable))
+		return -EPERM;

Why no capable() override? In fact, is this check really necessary?

Quoted 20 lines

security/commoncap.c renamed from security/commoncap.c~caps +6 -0

+
 	if (!cap_issubset (child->cap_permitted, current->cap_permitted) &&
 	    !capable (CAP_SYS_PTRACE))
 		return -EPERM;

@@ -76,6 +86,11 @@
 		return -EPERM;
 	}
 
+	/* verify the _new_Permitted_ is a subset of the _new_Inheritable_ */
+	if (newcaps && !cap_issubset (*permitted, *inheritable)) {
+		return -EPERM;
+	}
+
 	return 0;
 }
 

@@ -89,6 +104,8 @@
 

security/commoncap.c line 107

Quoted 3 lines

security/commoncap.c renamed from security/commoncap.c~caps +1 -0

 int cap_bprm_set_security (struct linux_binprm *bprm)
 {
+	if (newcaps) return 0;

CodingStyle:

	if (newcaps)
		return 0;

Quoted 28 lines

security/commoncap.c renamed from security/commoncap.c~caps +8 -2

+
 	/* Copied from fs/exec.c:prepare_binprm. */
 
 	/* We don't have VFS support for capabilities yet */

@@ -115,10 +132,11 @@
 	return 0;
 }
 
-void cap_bprm_apply_creds (struct linux_binprm *bprm, int unsafe)
+static void cap_bprm_apply_creds_compat (struct linux_binprm *bprm, int unsafe)
 {
-	/* Derived from fs/exec.c:compute_creds. */
+	/* This function will hopefully die in 2.7. */
 	kernel_cap_t new_permitted, working;
+	static int fixed_init = 0;
 
 	new_permitted = cap_intersect (bprm->cap_permitted, cap_bset);
 	working = cap_intersect (bprm->cap_inheritable,

@@ -151,6 +169,15 @@
 		current->cap_permitted = new_permitted;
 		current->cap_effective =
 		    cap_intersect (new_permitted, bprm->cap_effective);
+	} else if (!fixed_init) {
+		/* This is not strictly correct, as it gives linuxrc more
+		 * permissions than it used to have.  It was the only way I
+		 * could think of to keep the resulting disaster contained,

security/commoncap.c line 180

Quoted 5 lines

security/commoncap.c renamed from security/commoncap.c~caps +5 -0

+		 * though.
+		 */
+		current->cap_effective = CAP_OLD_INIT_EFF_SET;
+		current->cap_inheritable = CAP_OLD_INIT_INH_SET;
+		fixed_init = 1;

Hrm...

Quoted 5 lines

include/linux/init_task.h renamed from include/linux/init_task.h~caps +0 -0

@@ -92,8 +92,8 @@
 		.function	= it_real_fn				\
 	},								\

include/linux/init_task.h line 96

Quoted 5 lines

include/linux/init_task.h renamed from include/linux/init_task.h~caps +2 -2

 	.group_info	= &init_groups,					\
-	.cap_effective	= CAP_INIT_EFF_SET,				\
-	.cap_inheritable = CAP_INIT_INH_SET,				\
+	.cap_effective	= CAP_FULL_SET,				\
+	.cap_inheritable = CAP_FULL_SET,				\

This was made unconditional. And how are you convinced it's safe?

include/linux/init_task.h line 99

Quoted 5 lines

include/linux/init_task.h renamed from include/linux/init_task.h~caps +0 -0

 	.cap_permitted	= CAP_FULL_SET,					\
 	.keep_capabilities = 0,						\
 	.rlim		= INIT_RLIMITS,					\

thanks,
-chris

Quoted 7 lines · Chris Wright

Quoted 2 lines · * Andy Lutomirski ([email protected]) wrote:

Implement optional working capability support. Try to avoid giving Andrew a heart attack. ;)

I think it still needs more work. Default behavoiur is changed, like Inheritble is full rather than clear, setpcap is enabled, etc. ...

In cap_bprm_apply_creds_compat:

security/commoncap.c renamed from security/commoncap.c~caps +9 -0

+	} else if (!fixed_init) {
+		/* This is not strictly correct, as it gives linuxrc more
+		 * permissions than it used to have.  It was the only way I
+		 * could think of to keep the resulting disaster contained,
+		 * though.
+		 */
+		current->cap_effective = CAP_OLD_INIT_EFF_SET;
+		current->cap_inheritable = CAP_OLD_INIT_INH_SET;
+		fixed_init = 1;

So that it gets changed back. Otherwise linuxrc ran without permissions and my drives never got mounted. Yah, it's ugly -- I'm open to suggestions to avoid this.

Quoted 4 lines

... Also,
why do you change from Posix the way exec() updates capabilities? Sure, there is no filesystem bits present, so this changes the calculation, but I'm not convinced it's as secure this way. At least with newcaps=0.

I'm not convinced that Posix's version makes any sense. Also, there are apparently a number of drafts around which disagree on what the right rules are. (My copy, for example, matches the old rules exactly, but the old rules caused the sendmail problem.) And, under Posix, what does the inheritable mask mean, anyway?

Also, I don't find the posix rules to be useful (why is there an inheritable mask if all it does is to cause caps to be dropped on exec, when the user could just manually drop them?).

Quoted 3 lines

I believe we can get something functional with fewer changes, hence easier to understand the ramifications. In a nutshell, I'm still not comfortable with this.

I'll play with it, but I think this is the shortest patch I've come up with. I'll admit that touching this stuff scares me too, but I'd rather redo it that try and patch it over again.

Quoted 2 lines

Also, it breaks my tests which try to drop privs and keep caps across execve() which is really the only issue we're trying to solve ATM.

Can you send me a sample of what breaks? I do:

[root@luto tmp]# cap -c = ls /home/andy
ls: /home/andy: Permission denied
[root@luto tmp]# echo test >foo
[root@luto tmp]# chmod 700 foo
[root@luto tmp]# su andy -c 'cat foo'
cat: foo: Permission denied
[root@luto tmp]# cap -c '= cap_dac_read_search=eip' -u andy cat foo test
[root@luto tmp]# cap -c '= cap_dac_read_search=eip' -u andy bash [andy@luto tmp]$ whoami
andy
[andy@luto tmp]$ dumpcap
Real Eff

User    500         500
Group   500         500

Caps: = cap_dac_read_search+eip
[andy@luto tmp]$ cat foo
test

Which looks exactly right to me.
(cap and dumpcap live at www.stanford.edu/~luto/cap/)

Quoted 25 lines

Quoted 25 lines

fs/exec.c renamed from fs/exec.c~caps +6 -2

@@ -882,8 +882,10 @@
 
 	if(!(bprm->file->f_vfsmnt->mnt_flags & MNT_NOSUID)) {
 		/* Set-uid? */
-		if (mode & S_ISUID)
+		if (mode & S_ISUID) {
 			bprm->e_uid = inode->i_uid;
+			bprm->secflags |= BINPRM_SEC_SETUID;
+		}
 
 		/* Set-gid? */
 		/*

@@ -891,10 +893,19 @@
 		 * is a candidate for mandatory locking, not a setgid
 		 * executable.
 		 */
-		if ((mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP))
+		if ((mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP)) {
 			bprm->e_gid = inode->i_gid;
+			bprm->secflags |= BINPRM_SEC_SETGID;
+		}
 	}
 

Quoted 5 lines

fs/exec.c line 903

Quoted 2 lines

fs/exec.c renamed from fs/exec.c~caps +2 -0

+	/* Pretend we have VFS capabilities */
+	cap_set_full(bprm->cap_inheritable);

This looks sketchy.

My concept of 'inheritable' is that caps that are _not_ inheritable may never be gained by this task or its children. So a process should normally have all caps inheritable.

Quoted 11 lines

fs/exec.c line 47

Quoted 8 lines

fs/exec.c renamed from fs/exec.c~caps +4 -0

@@ -36,6 +41,11 @@
 int cap_ptrace (struct task_struct *parent, struct task_struct *child)
 {
 	/* Derived from arch/i386/kernel/ptrace.c:sys_ptrace. */
+	/* CAP_SYS_PTRACE still can't bypass inheritable restrictions */
+	if (newcaps &&
+	    !cap_issubset (child->cap_inheritable, current->cap_inheritable))
+		return -EPERM;

Why no capable() override? In fact, is this check really necessary?

If task A has less inheritable caps than B, then A is somehow less trusted and has no business tracing B.

A concrete example: a system runs with very restricted inheritable caps on all processes except for a magic daemon. The magic daemon holds on to CAP_SYS_ADMIN to umount everything at shutdown. If the rest of the system gets rooted, it still shouldn't be possible to trace the daemon. (Yes, this is currently not workable -- I plan to add a sysctl that sets what inheritable caps a task must have for setuid to work. The blanket requirement that _all_ must be present is to avoid bugs in which a setuid program assumes it will be fully privileged.)

Quoted 11 lines

fs/exec.c line 92

Quoted 11 lines

fs/exec.c renamed from fs/exec.c~caps +4 -0

 	if (!cap_issubset (child->cap_permitted, current->cap_permitted) &&
 	    !capable (CAP_SYS_PTRACE))
 		return -EPERM;

@@ -76,6 +86,11 @@
 		return -EPERM;
 	}
 
+	/* verify the _new_Permitted_ is a subset of the _new_Inheritable_ */
+	if (newcaps && !cap_issubset (*permitted, *inheritable)) {
+		return -EPERM;
+	}

Quoted 25 lines

Quoted 25 lines

fs/exec.c renamed from fs/exec.c~caps +7 -2

 	return 0;
 }
 

@@ -115,10 +132,11 @@
 	return 0;
 }
 
-void cap_bprm_apply_creds (struct linux_binprm *bprm, int unsafe)
+static void cap_bprm_apply_creds_compat (struct linux_binprm *bprm, int unsafe)
 {
-	/* Derived from fs/exec.c:compute_creds. */
+	/* This function will hopefully die in 2.7. */
 	kernel_cap_t new_permitted, working;
+	static int fixed_init = 0;
 
 	new_permitted = cap_intersect (bprm->cap_permitted, cap_bset);
 	working = cap_intersect (bprm->cap_inheritable,

@@ -151,6 +169,15 @@
 		current->cap_permitted = new_permitted;
 		current->cap_effective =
 		    cap_intersect (new_permitted, bprm->cap_effective);
+	} else if (!fixed_init) {
+		/* This is not strictly correct, as it gives linuxrc more
+		 * permissions than it used to have.  It was the only way I
+		 * could think of to keep the resulting disaster contained,

Quoted 8 lines

fs/exec.c line 180

Quoted 5 lines

fs/exec.c renamed from fs/exec.c~caps +5 -0

+		 * though.
+		 */
+		current->cap_effective = CAP_OLD_INIT_EFF_SET;
+		current->cap_inheritable = CAP_OLD_INIT_INH_SET;
+		fixed_init = 1;

Hrm...

Yup. It sucks. I didn't want to touch the system startup code, though, and this is the closest LSM comes. It's too bad there's no LSM hook to do this right (although I suppose I could add one, but that would break capabilities as a module).

Speaking of which, this is a genuine problem if commoncap is a module and newcaps=0 -- this code will probably never run. Does it matter?

[lots of patch snipped]

Quoted 5 lines

Quoted 5 lines

include/linux/init_task.h renamed from include/linux/init_task.h~caps +0 -0

@@ -92,8 +92,8 @@
 		.function	= it_real_fn				\
 	},								\

Quoted 8 lines

include/linux/init_task.h line 96

Quoted 5 lines

include/linux/init_task.h renamed from include/linux/init_task.h~caps +2 -2

 	.group_info	= &init_groups,					\
-	.cap_effective	= CAP_INIT_EFF_SET,				\
-	.cap_inheritable = CAP_INIT_INH_SET,				\
+	.cap_effective	= CAP_FULL_SET,				\
+	.cap_inheritable = CAP_FULL_SET,				\

This was made unconditional. And how are you convinced it's safe?

Same as above. But even if it were really unconditional (instead of just sort-of unconditional), I still think it's safe, because (in newcaps=0 mode) only root can get CAP_SETPCAP. Root could always just insert a module to enable it. So granting it costs nothing.

On the other hand, safety is good, and I can't see any way in the old system for anything to get CAP_SETPCAP. I'll send in a new patch that just disables CAP_SETPCAP when newcaps=0.

Quoted 20 lines · * Andy Lutomirski ([email protected]) wrote:

Quoted 4 lines · Chris Wright

* Andy Lutomirski ([email protected]) wrote:

I think it still needs more work. Default behavoiur is changed, like Inheritble is full rather than clear, setpcap is enabled, etc. ...

In cap_bprm_apply_creds_compat:

security/commoncap.c renamed from security/commoncap.c~caps +9 -0

+	} else if (!fixed_init) {
+		/* This is not strictly correct, as it gives linuxrc more
+		 * permissions than it used to have.  It was the only way I
+		 * could think of to keep the resulting disaster contained,
+		 * though.
+		 */
+		current->cap_effective = CAP_OLD_INIT_EFF_SET;
+		current->cap_inheritable = CAP_OLD_INIT_INH_SET;
+		fixed_init = 1;

Quoted 3 lines

So that it gets changed back. Otherwise linuxrc ran without permissions and my drives never got mounted. Yah, it's ugly -- I'm open to suggestions to avoid this.

I tested as a module, and this doesn't run AFAICS

Quoted 4 lines

Quoted 2 lines

Also, it breaks my tests which try to drop privs and keep caps across execve() which is really the only issue we're trying to solve ATM.

Can you send me a sample of what breaks? I do:

Yes. It's something like this:

keepcaps
dropuid
drop caps
execve()

The caps that are left are like this. (effective == inheritable) which are a subset of permitted.

Quoted 10 lines

Quoted 3 lines

why do you change from Posix the way exec() updates capabilities? Sure, there is no filesystem bits present, so this changes the calculation, but I'm not convinced it's as secure this way. At least with newcaps=0.

I'm not convinced that Posix's version makes any sense. Also, there are apparently a number of drafts around which disagree on what the right rules are. (My copy, for example, matches the old rules exactly, but the old rules caused the sendmail problem.) And, under Posix, what does the inheritable mask mean, anyway?

Quoted 3 lines

Also, I don't find the posix rules to be useful (why is there an inheritable mask if all it does is to cause caps to be dropped on exec, when the user could just manually drop them?).

Not sure if it's defensible, but it allows passing on an inheritable capability through an intermediate process that simply can't inherit that capability. This is not unlike requiring an unprivileged process to ask a privileged process for it to do something on it's behalf. Certainly it's implicit that you trust the privileged process.

Quoted 9 lines

Quoted 5 lines

fs/exec.c line 903

Quoted 2 lines

fs/exec.c renamed from fs/exec.c~caps +2 -0

+	/* Pretend we have VFS capabilities */
+	cap_set_full(bprm->cap_inheritable);

This looks sketchy.

My concept of 'inheritable' is that caps that are _not_ inheritable may never be gained by this task or its children. So a process should normally have all caps inheritable.

This is the diff with Posix, which allows a process to inherit a capability that it can never exercise. However, it could pass the capablity on to someone else who could inherit it.

<snip>

Quoted 12 lines

Quoted 11 lines

security/commoncap.c line 47

Quoted 8 lines

security/commoncap.c renamed from security/commoncap.c~caps +4 -0

@@ -36,6 +41,11 @@
 int cap_ptrace (struct task_struct *parent, struct task_struct *child)
 {
 	/* Derived from arch/i386/kernel/ptrace.c:sys_ptrace. */
+	/* CAP_SYS_PTRACE still can't bypass inheritable restrictions */
+	if (newcaps &&
+	    !cap_issubset (child->cap_inheritable, current->cap_inheritable))
+		return -EPERM;

Why no capable() override? In fact, is this check really necessary?

Quoted 2 lines

If task A has less inheritable caps than B, then A is somehow less trusted and has no business tracing B.

But it's not less. It's just not a subset. Task B could have only one inheritable cap, while A could have all but the one cap that B has. In fact, that could be CAP_SYS_PTRACE. So the threat is task A tracing B, and using it to pass on capabilities that it wasn't allowed to pass on. Which is what the permitted test was for before, and what CAP_SYS_PTRACE was used to override.

Quoted 8 lines

A concrete example: a system runs with very restricted inheritable caps on all processes except for a magic daemon. The magic daemon holds on to CAP_SYS_ADMIN to umount everything at shutdown. If the rest of the system gets rooted, it still shouldn't be possible to trace the daemon. (Yes, this is currently not workable -- I plan to add a sysctl that sets what inheritable caps a task must have for setuid to work. The blanket requirement that _all_ must be present is to avoid bugs in which a setuid program assumes it will be fully privileged.)

I suppose this eliminates the usefulness of CAP_SYS_PTRACE.

Quoted 1 line

I tested as a module, and this doesn't run AFAICS

OK -- I give in. I'll redo it as a kernel (non-module) boot parameter. And touch some more files that have no business being capability-aware while I'm at it :(

The inheritable mask:

Quoted 8 lines

Quoted 1 line

Can you send me a sample of what breaks?

Yes. It's something like this:

keepcaps
dropuid
drop caps
execve()

Quoted 2 lines

The caps that are left are like this. (effective == inheritable) which are a subset of permitted.

Is (eff == inh) what happens or what should happen? If the former, then my patch is broken. If the latter, either I'm confused, or see below.

Quoted 7 lines

Quoted 3 lines

My concept of 'inheritable' is that caps that are _not_ inheritable may never be gained by this task or its children. So a process should normally have all caps inheritable.

This is the diff with Posix, which allows a process to inherit a capability that it can never exercise. However, it could pass the capablity on to someone else who could inherit it.

So here's where I think we disagree:

Posix (as interpreted by Linux 2.4/2.6) says:
pP' = (fP & cap_bset) | (fI & pI)

So (assuming that set_security did the "obvious" (but dangerous) thing):

pP := "task may enable and use these capabilities" pE := "task may use these capabilities _now_" pI := "task may gain these on exec from fI"

fP := "this program gets these caps (module cap_bset)" fI := "this program gets these caps if pI says so"

Which screams "overcomplicated." I imagine that the use is for a trusted daemon to run an untrusted helper (with pI>0) which then calls back into trusted land and gets its caps back. This is possibly convenient, but it seems to break (where break = scare me) when there are more than one such system on a given box. Then one's untrusted program (with fI>0) can call the other's trusted fI>0 helper. I suppose the point is that a random user's program (with fI==0) can't try to exploit anything, but, for this to be at all secure, both fI>0 programs need to be secure against attack from the other (unrelated) system, should it be compromised. Which means it might as well have set fP>0 and been done with it (I don't believe in security by inconvenience of exploit).

I see no particular invariants here, except for pE <= pP.

IRIX (thanks Valdis) says:

pI' = pI & fI
pP' = fP | (pI' & fP)

Which I interpret as

pP := "task may enable and use these capabilities" pE := "task may use these capabilities _now_" pI := ~"task _loses_ these on exec"

fP := "this program gets these caps"
fI := "this program may keep these caps"

This seems to want pP <= pI as an invariant.

This is what I always thought Linux capabilities meant to be. They don't make my brain hurt.

But I also think they're overengineered. Instead of:

drop_caps_from_inheritable()
exec()

a program could do:

drop_caps_from_permitted()
exec()

And I can't imagine what use fI != ~0 has, since it's trivially accomplished by a wrapper. It is also trivially bypassed by loading the program manually (with ld.so).

So, in my patch, I decided steal the inheritable mask to mean this:

pI := "this process may gain these caps"
fI := "this is an upper bound on pI"

In other words, if a process is extra-untrusted (e.g. it's a daemon that never needs a certain capability and has no business trying to gain it), it can drop it from pI. Then it cannot try to abuse programs with pP>0. The setuid override is just added paranoia. Another benefit is that it allows a securelevel-like scheme, where even root isn't quite trusted.

I suppose it might be inappropriate to steal this field like this, given that IRIX already has a (somewhat reasonable) use for it. I have no problem implementing IRIX-style capabilities and (if there is enough interest) adding a _fourth_ process field pM (process maximum capabilities) that does what my pI does.

As for the fE mask, I just don't see the point, although I _really_ don't like the way it's described in the IRIX manpage.

IRIX has pE = pP & fE. This breaks Posix non-capabilities compatibility for a program that's uid==0, euid!=0. It should have pE==0 and pP>0. But it calls exec() and gets pE>0. This is bad.

Assuming there's something else there to fix that case, then I still don't see the point. If a program is capability- aware, it can set its pE however it likes. If not, then it probably expects pE==pP. I guess there could be a trusted but dumb program that runs a trusted, cap-aware helper that needs capabilities. Then the admin sets fE==0 on the dump program and everything works. Seems a bit contrived, though.

On the other hand, I'm not wedded to my model of pE. It'll be harder to get IRIX's right for uid!=euid.

CAP_SYS_PTRACE:

Quoted 33 lines

Quoted 12 lines

Quoted 11 lines

security/commoncap.c line 47

Quoted 8 lines

security/commoncap.c renamed from security/commoncap.c~caps +4 -0

@@ -36,6 +41,11 @@
 int cap_ptrace (struct task_struct *parent, struct task_struct *child)
 {
 	/* Derived from arch/i386/kernel/ptrace.c:sys_ptrace. */
+	/* CAP_SYS_PTRACE still can't bypass inheritable restrictions */
+	if (newcaps &&
+	    !cap_issubset (child->cap_inheritable, current->cap_inheritable))
+		return -EPERM;

Why no capable() override? In fact, is this check really necessary?

Quoted 2 lines

If task A has less inheritable caps than B, then A is somehow less trusted and has no business tracing B.

But it's not less. It's just not a subset. Task B could have only one inheritable cap, while A could have all but the one cap that B has. In fact, that could be CAP_SYS_PTRACE. So the threat is task A tracing B, and using it to pass on capabilities that it wasn't allowed to pass on. Which is what the permitted test was for before, and what CAP_SYS_PTRACE was used to override.

Quoted 8 lines

A concrete example: a system runs with very restricted inheritable caps on all processes except for a magic daemon. The magic daemon holds on to CAP_SYS_ADMIN to umount everything at shutdown. If the rest of the system gets rooted, it still shouldn't be possible to trace the daemon. (Yes, this is currently not workable -- I plan to add a sysctl that sets what inheritable caps a task must have for setuid to work. The blanket requirement that _all_ must be present is to avoid bugs in which a setuid program assumes it will be fully privileged.)

Quoted 1 line

I suppose this eliminates the usefulness of CAP_SYS_PTRACE.

It lets one uid/gid trace another. If CAP_SYS_PTRACE allowed a process to arbitrarity steal another's capabilities, then the process with CAP_SYS_PTRACE might as well have been given those capabilities. That is, this should IMHO be disallowed

drop_all_but_CAP_SYS_PTRACE()
exec(slightly trusted debugger process)
ptrace(1) <--- but it was supposed to be "slightly trusted"!

So:

Should I redo this to keep IRIX's meaning of fI, should I keep mine, or should I do something else. Chris -- in your examples, you seem to have some idea of what should be happening. What do you think?

I'm not convinced that Posix's version makes any sense. Also, there are apparently a number of drafts around which disagree on what the right rules are. (My copy, for example, matches the old rules exactly, but the old rules caused the sendmail problem.)

And, under Posix, what does
the inheritable mask mean, anyway?

Also, I don't find the posix rules to be useful (why is there an inheritable mask if all it does is to cause caps to be dropped on exec, when the user could just manually drop them?).

As for Posix caps, is there any good reason to follow Posix? I don't know of any OS that has Posix caps except Linux, and they're broken. The spec was dropped, anyway.

Well, I wonder what IRIX does? They support capabilities and had a reasonable sized hand in the draft. No point in making it impossible to port apps. It's not that I'm a strong fan of Posix caps, but compatibility (even with a partially complete draft) with defacto standards is not entirely unreasonable.

Quoted 5 lines · Chris Wright

Well, I wonder what IRIX does? They support capabilities and had a reasonable sized hand in the draft. No point in making it impossible to port apps. It's not that I'm a strong fan of Posix caps, but compatibility (even with a partially complete draft) with defacto standards is not entirely unreasonable.

The IRIX 6.5 manpage says:

Note that Irix has *3* capability vectors attached to a file....

OK, this is precisely POSIX as I expected. No surprise given the folks involved.

security/commoncap.c line 116

Quoted 6 lines · Andy Lutomirski

security/commoncap.c +6 -0

+	/* Pretend we have VFS capabilities */
+	cap_set_full(bprm->cap_inheritable);
+	if ((bprm->secflags & BINPRM_SEC_SETUID) && bprm->e_uid == 0)
+		cap_set_full(bprm->cap_permitted);
+	else
+		cap_clear(bprm->cap_permitted);

I'd move this to security/commoncap.c:

Quoted 5 lines

(quoted diff) +5 -0

+	/* FIXME: Is this overly harsh on setgid? */
+	if ((bprm->secflags & (BINPRM_SEC_SETUID | BINPRM_SEC_SETGID)) &&
+	    new_pI != CAP_FULL_SET)
+			bprm->secflags |= BINPRM_SEC_NOELEVATE;
+

Review

Quoted 4 lines

(quoted diff) +4 -0

+	if (bprm->secflags & BINPRM_SEC_NOELEVATE) {
+		is_setuid = is_setgid = 0;
+		cap_clear(fP);
+	}

This prevents sendmail from being setuid mail and
cap_net_bind_service=ep.

Regards, Olaf.

BTW, in your capabilities implementation, why do you do:

If the answer's because that's how Linux cap evolution works, then I'd say that Linux cap evolution is broken.