Re: [PATCH] scaled-back caps, take 4

Re: [PATCH] scaled-back caps, take 4

[Andy Lutomirski]

Chris Wright wrote:
> * Olaf Dietsche (olaf+list.linux-kernel@olafdietsche.de) wrote:
> 
>>Andy Lutomirski <luto@myrealbox.com> writes:
>>
>>
>>>cap_2.6.6-mm2_4.patch: New stripped-back capabilities.
>>>
>>> fs/exec.c               |   15 ++++-
>>> include/linux/binfmts.h |    9 ++-
>>> security/commoncap.c    |  130 ++++++++++++++++++++++++++++++++++++++++++------
>>> 3 files changed, 136 insertions(+), 18 deletions(-)
>>
>>[patch]
>>
>>Why don't you provide this as a configurable andycap.c module?
>>I think, this is the whole point of LSM.
> 
> 
> I agree, if we can't find a clean way to do it.  However, note this
> includes changes to core.  And it's nice to fix this for the base case.

On the other hand, this version has minimal changes to core (it adds a new 
field to linux_binprm and makes fs/exec.c fill in some extra information). 
  These changes shouldn't break any existing code, as the current behavior 
is for bprm->cap_* to be undefined when bprm_set_security is called.  None 
of this is strictly necessary for my patch, but it makes it a lot cleaner.

So, if the core changes were merged, my caps semantics could be maintained 
as a (fairly simple) separate LSM.  That prevents it working with SELinux 
or other (non-stacking) LSMs loaded.

--Andy

Re: [PATCH] scaled-back caps, take 4

[Stephen Smalley]

On Mon, 2004-05-17 at 03:19, Andy Lutomirski wrote:
> So, if the core changes were merged, my caps semantics could be maintained 
> as a (fairly simple) separate LSM.  That prevents it working with SELinux 
> or other (non-stacking) LSMs loaded.

SELinux supports stacking with the existing capability module (SELinux
registers first, then the capability module registers as a secondary
module under it).

-- 
Stephen Smalley <sds@epoch.ncsc.mil>
National Security Agency

Re: [PATCH] scaled-back caps, take 4

[Olaf Dietsche]

Andy Lutomirski <luto@myrealbox.com> writes:

> [sorry if this is a resend -- i don't think it worked the first
> time.]
>
> Olaf Dietsche wrote:
>> Andy Lutomirski <luto@myrealbox.com> writes:
>
>>>First, CAP_SETPCAP is never obtainable (by anything).
>>>Since cap_bset never has this bit set, nothing can inherit it
>>>from fP.  capset_check prevents it from getting set in pI.
>> # mv /sbin/init /sbin/init.bin
>> # cat >/sbin/init
>> #! /bin/sh
>> if test $$ -eq 1; then
>>         mount /proc
>>         echo -1 >/proc/sys/kernel/cap-bound
>> fi
>> exec /sbin/init.bin "$@"
>> ^D
>> # chmod 755 /sbin/init
>> # reboot
>
> Wow -- I missed that.  Does anyone actually do this?  And is there a
> reason why it should work like this?

Because in kernel/sysctl.c:
int proc_dointvec_bset(ctl_table *table, int write, struct file *filp,
			void __user *buffer, size_t *lenp)
allows init only to set cap_bset.
You can write a module to set cap_bset, of course, or patch the kernel
to define CAP_INIT_EFF_SET to ~0.

>>>cap_bprm_set_security does:
>>>fP = fI = (new_uid == 0 || new_euid == 0)
>>>fE = (new_euid == 0)
>> Only if (!issecure (SECURE_NOROOT))
>> [...]
>
> I don't see any way to change securebits.

I thought there has been a /proc way, to set securebits, but maybe I
confused this with cap_bset. Anyway, here's the easy way out:

diff -urN a/include/linux/securebits.h b/include/linux/securebits.h
--- a/include/linux/securebits.h        Sat Oct  5 18:42:33 2002
+++ b/include/linux/securebits.h        Sun May 23 22:38:02 2004
@@ -1,7 +1,7 @@
 #ifndef _LINUX_SECUREBITS_H
 #define _LINUX_SECUREBITS_H 1
 
-#define SECUREBITS_DEFAULT 0x00000000
+#define SECUREBITS_DEFAULT (SECURE_NO_SETUID_FIXUP | SECURE_NOROOT)
 
 extern unsigned securebits;
 
>> Please, don't get me wrong. For me, it's just a matter of maintaining
>> a slightly bigger fscaps patch. But I don't think capabilities in
>> Linux are really broken, only because some proponents of SELinux claim
>> so.
>
>
> I find caps to be broken, and I don't use SELinux.  I want to be able
> to run programs as non-root with limited caps, which I currently can't
> do without modifying each program to start as root, then drop caps,
> then set KEEPCAPS, then drop root.  And even with that change, these
> programs can't usefully exec themselves, which could be useful.

This is, where filesystem capabilities come into play. You implemented
them yourself. Execing is still a problem, though. However, if you
activate SECURE_NO_SETUID_FIXUP this issue is gone, too.

> And no, I don't think this patch is necessary, or that it should be
> applied or used by itself.  I think it makes a good starting point to
> fix caps
> (which a lot of people seem to think are broken).

Well, I know, that I don't have a strong following. :-)

Regards, Olaf.

Re: [PATCH] scaled-back caps, take 4

[Martin Schlemmer]

[-- Attachment #1: Type: text/plain, Size: 777 bytes --]


> > And no, I don't think this patch is necessary, or that it should be
> > applied or used by itself.  I think it makes a good starting point to
> > fix caps
> > (which a lot of people seem to think are broken).
> 
> Well, I know, that I don't have a strong following. :-)
> 

It might just be that not many are familiar with the code, or really
care.  If your patch really have 100% the same behaviour as the
original, and maybe some comments might be added as in how things
work, it can really help future debugging/additions/whatever, as
its so much easier (IMHO, not really caring much myself, but somebody
looking is better than nobody).  You might check with Andrew if he
wants to kick it around a bit in -mm ... ?


Cheers,

-- 
Martin Schlemmer

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] capabilites, take 2

[Andy Lutomirski]

Stephen Smalley wrote:

> On Fri, 2004-05-14 at 11:57, Andy Lutomirski wrote:
> 
>>Thanks -- turning brain back on, SELinux is obviously better than any
>>fine-grained capability scheme I can imagine.
>>
>>So unless anyone convinces me you're wrong, I'll stick with just
>>fixing up capabilities to work without making them finer-grained.
> 
> 
> Great, thanks.  Fixing capabilities to work is definitely useful and
> desirable.  Significantly expanding them in any manner is a poor use of
> limited resources, IMHO; I'd much rather see people work on applying
> SELinux to the problem and solving it more effectively for the future.
> 

Does this mean I should trash my 'maximum' mask?

(I like 'cap -c = sftp-server' so it can't try to run setuid/fP apps.)
OTOH, since SELinux accomplishes this better, it may not be worth the
effort.

--Andy

Re: [PATCH] capabilites, take 2

[Chris Wright]

* Andy Lutomirski (luto@stanford.edu) wrote:
> Stephen Smalley wrote:
> > On Fri, 2004-05-14 at 11:57, Andy Lutomirski wrote:
> > > Thanks -- turning brain back on, SELinux is obviously better than any
> > > fine-grained capability scheme I can imagine.
> > > 
> > > So unless anyone convinces me you're wrong, I'll stick with just
> > > fixing up capabilities to work without making them finer-grained.
> > 
> > Great, thanks.  Fixing capabilities to work is definitely useful and
> > desirable.  Significantly expanding them in any manner is a poor use of
> > limited resources, IMHO; I'd much rather see people work on applying
> > SELinux to the problem and solving it more effectively for the future.
> 
> Does this mean I should trash my 'maximum' mask?
> 
> (I like 'cap -c = sftp-server' so it can't try to run setuid/fP apps.)
> OTOH, since SELinux accomplishes this better, it may not be worth the
> effort.

Let's just get back to the simplest task.  Allow execve() to do smth.
reasonable with capabilities.

thanks,
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net

[PATCH] scaled-back caps, take 4 (was Re: [PATCH] capabilites, take 2)

[Andy Lutomirski]

On Friday 14 May 2004 11:07, Chris Wright wrote:
> * Andy Lutomirski (luto@stanford.edu) wrote:
> > Stephen Smalley wrote:
> > > On Fri, 2004-05-14 at 11:57, Andy Lutomirski wrote:
> > > > Thanks -- turning brain back on, SELinux is obviously better than any
> > > > fine-grained capability scheme I can imagine.
> > > > 
> > > > So unless anyone convinces me you're wrong, I'll stick with just
> > > > fixing up capabilities to work without making them finer-grained.
> > > 
> > > Great, thanks.  Fixing capabilities to work is definitely useful and
> > > desirable.  Significantly expanding them in any manner is a poor use of
> > > limited resources, IMHO; I'd much rather see people work on applying
> > > SELinux to the problem and solving it more effectively for the future.
> > 
> > Does this mean I should trash my 'maximum' mask?
> > 
> > (I like 'cap -c = sftp-server' so it can't try to run setuid/fP apps.)
> > OTOH, since SELinux accomplishes this better, it may not be worth the
> > effort.
> 
> Let's just get back to the simplest task.  Allow execve() to do smth.
> reasonable with capabilities.

Sold.

This version throws out the inheritable mask.  There is no change in
behavior with newcaps=0.

All that's left in newcaps=1 mode is:

fP := permitted (app gains these)
pP := permitted (app can make them effective)
pE := effective

There is no inheritable mask :)  So we can't argue about what it's
supposed to / not supposed to.

It's not too well tested yet.  Enjoy.

--Andy

cap_2.6.6-mm2_4.patch: New stripped-back capabilities.

 fs/exec.c               |   15 ++++-
 include/linux/binfmts.h |    9 ++-
 security/commoncap.c    |  130 ++++++++++++++++++++++++++++++++++++++++++------
 3 files changed, 136 insertions(+), 18 deletions(-)

--- linux-2.6.6-mm2/fs/exec.c~caps	2004-05-13 11:42:26.000000000 -0700
+++ linux-2.6.6-mm2/fs/exec.c	2004-05-14 12:36:17.000000000 -0700
@@ -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,18 @@
 		 * 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;
+		}
 	}
 
+	/* Pretend we have VFS capabilities */
+	if ((bprm->secflags & BINPRM_SEC_SETUID) && bprm->e_uid == 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 +1099,7 @@
 	bprm.loader = 0;
 	bprm.exec = 0;
 	bprm.security = NULL;
+	bprm.secflags = 0;
 	bprm.mm = mm_alloc();
 	retval = -ENOMEM;
 	if (!bprm.mm)
--- linux-2.6.6-mm2/security/commoncap.c~caps	2004-05-13 11:42:26.000000000 -0700
+++ linux-2.6.6-mm2/security/commoncap.c	2004-05-14 13:24:45.000000000 -0700
@@ -24,6 +24,11 @@
 #include <linux/xattr.h>
 #include <linux/hugetlb.h>
 
+static int newcaps = 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. */
@@ -48,17 +53,19 @@
 {
 	/* Derived from kernel/capability.c:sys_capget. */
 	*effective = cap_t (target->cap_effective);
-	*inheritable = cap_t (target->cap_inheritable);
 	*permitted = cap_t (target->cap_permitted);
+	if (newcaps)
+		*inheritable = CAP_EMPTY_SET;
+	else
+		*inheritable = cap_t (target->cap_inheritable);
 	return 0;
 }
 
 int cap_capset_check (struct task_struct *target, kernel_cap_t *effective,
 		      kernel_cap_t *inheritable, kernel_cap_t *permitted)
 {
-	/* Derived from kernel/capability.c:sys_capset. */
 	/* verify restrictions on target's new Inheritable set */
-	if (!cap_issubset (*inheritable,
+	if (!newcaps && !cap_issubset (*inheritable,
 			   cap_combine (target->cap_inheritable,
 					current->cap_permitted))) {
 		return -EPERM;
@@ -83,12 +90,16 @@
 		     kernel_cap_t *inheritable, kernel_cap_t *permitted)
 {
 	target->cap_effective = *effective;
-	target->cap_inheritable = *inheritable;
 	target->cap_permitted = *permitted;
+	if (!newcaps)
+		target->cap_inheritable = *inheritable;
 }
 
 int cap_bprm_set_security (struct linux_binprm *bprm)
 {
+	if (newcaps)
+		return 0;
+
 	/* Copied from fs/exec.c:prepare_binprm. */
 
 	/* We don't have VFS support for capabilities yet */
@@ -115,9 +126,9 @@
 	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;
 
 	new_permitted = cap_intersect (bprm->cap_permitted, cap_bset);
@@ -158,15 +169,88 @@
 	current->keep_capabilities = 0;
 }
 
+/*
+ * The rules of Linux capabilities (not POSIX!)
+ *
+ * What the masks mean:
+ *  pP = capabilities that this process has
+ *  pE = capabilities that this process has and are enabled
+ * (so pE <= pP)
+ *
+ * The capability evolution rules are:
+ *
+ *  pP' = ((fP & cap_bset) | pP) & Y
+ *  pE' = (pE | fP) & pP'
+ *
+ *  X = cap_bset
+ *  Y is zero if uid!=0, euid==0, and setuid non-root
+ *
+ *  Exception: if setuid-nonroot, then pE' is reset to 0.
+ *
+ * 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_pP, new_pE, fP;
+	int is_setuid, is_setgid;
+
+	if(!newcaps) {
+		cap_bprm_apply_creds_compat(bprm, unsafe);
+		return;
+	}
+
+	fP = bprm->cap_permitted;
+	is_setuid = (bprm->secflags & BINPRM_SEC_SETUID);
+	is_setgid = (bprm->secflags & BINPRM_SEC_SETGID);
+
+	/* Check that it's safe to elevate privileges */
+	if (unsafe & ~LSM_UNSAFE_PTRACE_CAP)
+		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);
+
+	/* 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;
+
+	new_pE = cap_combine(current->cap_effective, fP);
+	cap_mask(new_pE, new_pP);
+	if (is_setuid && bprm->e_uid != 0)
+		cap_clear(new_pE);
+
+	/* Apply new security state */
+	if (is_setuid) {
+	        current->suid = current->euid = current->fsuid = bprm->e_uid;
+	}
+	if (is_setgid)
+	        current->sgid = current->egid = current->fsgid = bprm->e_gid;
+
+	current->cap_permitted = new_pP;
+	current->cap_effective = new_pE;
+
+	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 +364,14 @@
 
 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_permitted);
+		cap_set_full(p->cap_effective);
+	} else {
+		p->cap_effective = CAP_INIT_EFF_SET;
+		p->cap_inheritable = CAP_INIT_INH_SET;
+		p->cap_permitted = CAP_FULL_SET;
+	}
 	p->keep_capabilities = 0;
 	return;
 }
@@ -367,6 +456,15 @@
 	return -ENOMEM;
 }
 
+static int __init commoncap_init (void)
+{
+	if (newcaps) {
+		printk(KERN_NOTICE "Experimental capability support is on\n");
+	}
+
+	return 0;
+}
+
 EXPORT_SYMBOL(cap_capable);
 EXPORT_SYMBOL(cap_ptrace);
 EXPORT_SYMBOL(cap_capget);
@@ -382,5 +480,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");
--- linux-2.6.6-mm2/include/linux/binfmts.h~caps	2004-05-13 11:42:26.000000000 -0700
+++ linux-2.6.6-mm2/include/linux/binfmts.h	2004-05-14 10:28:05.000000000 -0700
@@ -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,10 @@
 	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_permitted;
+	/* old caps -- do NOT use in new code */
+	kernel_cap_t cap_inheritable, cap_effective;
 	void *security;
 	int argc, envc;
 	char * filename;	/* Name of binary as seen by procps */

Re: [PATCH] scaled-back caps, take 4

[Olaf Dietsche]

Andy Lutomirski <luto@myrealbox.com> writes:

> cap_2.6.6-mm2_4.patch: New stripped-back capabilities.
>
>  fs/exec.c               |   15 ++++-
>  include/linux/binfmts.h |    9 ++-
>  security/commoncap.c    |  130 ++++++++++++++++++++++++++++++++++++++++++------
>  3 files changed, 136 insertions(+), 18 deletions(-)
[patch]

Why don't you provide this as a configurable andycap.c module?
I think, this is the whole point of LSM.

Regards, Olaf.

Re: [PATCH] scaled-back caps, take 4

[Albert Cahalan]

On Fri, 2004-05-14 at 20:06, Olaf Dietsche wrote:
> Andy Lutomirski <luto@myrealbox.com> writes:
> 
> > cap_2.6.6-mm2_4.patch: New stripped-back capabilities.
> >
> >  fs/exec.c               |   15 ++++-
> >  include/linux/binfmts.h |    9 ++-
> >  security/commoncap.c    |  130 ++++++++++++++++++++++++++++++++++++++++++------
> >  3 files changed, 136 insertions(+), 18 deletions(-)
> [patch]
> 
> Why don't you provide this as a configurable andycap.c module?

**GROAN**

For those that don't know, this is Andy Cap:
http://picosel.free.fr/andycap/andy_billard.jpg

Re: [PATCH] scaled-back caps, take 4

[Chris Wright]

* Olaf Dietsche (olaf+list.linux-kernel@olafdietsche.de) wrote:
> Andy Lutomirski <luto@myrealbox.com> writes:
> 
> > cap_2.6.6-mm2_4.patch: New stripped-back capabilities.
> >
> >  fs/exec.c               |   15 ++++-
> >  include/linux/binfmts.h |    9 ++-
> >  security/commoncap.c    |  130 ++++++++++++++++++++++++++++++++++++++++++------
> >  3 files changed, 136 insertions(+), 18 deletions(-)
> [patch]
> 
> Why don't you provide this as a configurable andycap.c module?
> I think, this is the whole point of LSM.

I agree, if we can't find a clean way to do it.  However, note this
includes changes to core.  And it's nice to fix this for the base case.

thanks,
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net

Re: [PATCH] scaled-back caps, take 4 (was Re: [PATCH] capabilites, take 2)

[Andy Lutomirski]

Chris Wright wrote:
> * Andy Lutomirski (luto@myrealbox.com) wrote:
> 
>>This version throws out the inheritable mask.  There is no change in
>>behavior with newcaps=0.
> 
> 
> Alright, I tried to write up my expectations for all the various modes
> w.r.t dropping privs, keeping them, setuid apps, etc.  I then wrote some
> tests to get a baseline, and well as a way to compare results.  Finally
> I wrote a patch that meets the requirements I laid out, and compared it
> against yours.  With one minor exception, the capabilities bits match
> up.  You drop effective caps when a non-root users execs a non-root
> setuid app, and I the caps alone.  ...

Paranoia.  There are legacy setuid programs out there (presumably even 
setuid-nonroot).  Let's make them behave as closely to the way they 
currently do as possible.


 > ... One side note, you're changes effect
> the user/group saved ids unexpectedly.

Oops.  That's a trivial fix.

> 
> Here's a bunch of test cases:

> # Still w/out changing inheritable and with KEEPCAPS set
> # 10 Root process does setuid(!0), effective caps are dropped
> # 11 Root process does seteuid(!0), effective caps are dropped
> # 12 Nonroot process restores uid 0, effective restored to permitted

I still want some variant on KEEPCAPS that causes setxuid to leave caps 
completely alone.  Oh, well.

> # 18 Non-root execs setuid-nonroot process, new caps bound by inheritable

What new caps?

>>cap_2.6.6-mm2_4.patch: New stripped-back capabilities.
>>
>> fs/exec.c               |   15 ++++-
>> include/linux/binfmts.h |    9 ++-
>> security/commoncap.c    |  130 ++++++++++++++++++++++++++++++++++++++++++------
>> 3 files changed, 136 insertions(+), 18 deletions(-)
>>
>>--- linux-2.6.6-mm2/fs/exec.c~caps	2004-05-13 11:42:26.000000000 -0700
>>+++ linux-2.6.6-mm2/fs/exec.c	2004-05-14 12:36:17.000000000 -0700
>>@@ -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;
>>+		}
> 
> 
> No strong objection, but I don't think it's necessary.

I wanted to distinguish between setuid-me and non-setuid in apply_creds. 
That one doesn't matter much, though.

> 
> 
>> 
>> 		/* Set-gid? */
>> 		/*
>>@@ -891,10 +893,18 @@
>> 		 * 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;
>>+		}
>> 	}
>> 
>>+	/* Pretend we have VFS capabilities */
>>+	if ((bprm->secflags & BINPRM_SEC_SETUID) && bprm->e_uid == 0)
>>+		cap_set_full(bprm->cap_permitted);
>>+	else
>>+		cap_clear(bprm->cap_permitted);
>>+
> 
> 
> Thus far we've kept all this stuff out of core.  I believe we should
> keep it that way.  This could easily be left in bprm_set().

True.  But as long as linux_binprm has fields for this stuff, intuitively 
it should always be filled in (i.e. not just in commoncap).  That way we 
can say that commoncap doesn't get special treatment :)

Also, this seems like the right place to check for VFS caps.  This way we can.

> 
> 
>>--- linux-2.6.6-mm2/security/commoncap.c~caps	2004-05-13 11:42:26.000000000 -0700
>>+++ linux-2.6.6-mm2/security/commoncap.c	2004-05-14 13:24:45.000000000 -0700
>>@@ -24,6 +24,11 @@
>> #include <linux/xattr.h>
>> #include <linux/hugetlb.h>
>> 
>>+static int newcaps = 0;
>>+
>>+module_param(newcaps, int, 444);
>>+MODULE_PARM_DESC(newcaps, "Set newcaps=1 to enable experimental capabilities");
> 
> 
> It would be really nice to have a one size fits all solution.  Esp.
> since the legacy mode is what one gets when leaving inheritable mask
> untouched.

I agree.  Andrew specifically asked for this, though, at least until this 
stuff is well-tested and everyone likes it.

> 
> 
>> int cap_bprm_set_security (struct linux_binprm *bprm)
>> {
>>+	if (newcaps)
>>+		return 0;
>>+
> 
> 
> That setup could go here instead of in core.
> 
> 

[snip]
>> 
>>+/*
>>+ * The rules of Linux capabilities (not POSIX!)
>>+ *
>>+ * What the masks mean:
>>+ *  pP = capabilities that this process has
>>+ *  pE = capabilities that this process has and are enabled
>>+ * (so pE <= pP)
>>+ *
>>+ * The capability evolution rules are:
>>+ *
>>+ *  pP' = ((fP & cap_bset) | pP) & Y
>>+ *  pE' = (pE | fP) & pP'
>>+ *
>>+ *  X = cap_bset
>>+ *  Y is zero if uid!=0, euid==0, and setuid non-root
>>+ *
>>+ *  Exception: if setuid-nonroot, then pE' is reset to 0.
> 
> 
> While this works fine, I was interested to see what we could do to
> leave the old algorithm.  Seems both work out fine.
> 
> 
>>+	/* setuid-nonroot is special. */
>>+	if (is_setuid && bprm->e_uid != 0 && current->uid != 0 &&
>>+	    current->euid == 0)
>>+		cap_clear(new_pP);
> 
> 
> setuid-nonroot from a non-root user needs to clear effective?

Yes.  Say user 500 runs a setuid-root program, which goes back and runs a 
setuid-500 program.  uid==euid==500 now, so the program expects to be 
unprivileged.  This makes that work.  (I'm assuming you meant permitted. 
Effective gets cleared in cap_mask(new_pE, new_pP)).


The reason I killed the old algorithm is because it's scary (in the sense 
of being complicated and subtle for no good reason) and because I don't see 
the point of inheritable caps.  I doubt anything uses them currently on a 
vanilla kernel because they don't _do_ anything.  So I killed them.


--Andy

Re: [PATCH] scaled-back caps, take 4 (was Re: [PATCH] capabilites, take 2)

[Chris Wright]

* Andy Lutomirski (luto@stanford.edu) wrote:
> Chris Wright wrote:
> > Alright, I tried to write up my expectations for all the various modes
> > w.r.t dropping privs, keeping them, setuid apps, etc.  I then wrote some
> > tests to get a baseline, and well as a way to compare results.  Finally
> > I wrote a patch that meets the requirements I laid out, and compared it
> > against yours.  With one minor exception, the capabilities bits match
> > up.  You drop effective caps when a non-root users execs a non-root
> > setuid app, and I the caps alone.  ...
> 
> Paranoia.  There are legacy setuid programs out there (presumably even 
> setuid-nonroot).  Let's make them behave as closely to the way they 
> currently do as possible.

Yes, that's the goal I have.  Although, the above scenario, they've
already been limited (IOW, if nothing's been touched, all behaves the
same).  Starting with limited inheritable (as say uid 500), execute
a non-root, setuid (say uid 99) program, is this expected to change
effective set?  Currently it's a transition for 0 caps to 0 caps, not
very interesting.  Given they are both unprivelged uids, I kept
the (limited) effective.

> > # Still w/out changing inheritable and with KEEPCAPS set
> > # 10 Root process does setuid(!0), effective caps are dropped
> > # 11 Root process does seteuid(!0), effective caps are dropped
> > # 12 Nonroot process restores uid 0, effective restored to permitted
> 
> I still want some variant on KEEPCAPS that causes setxuid to leave caps 
> completely alone.  Oh, well.

Yeah, digs hole deeper though.

> > # 18 Non-root execs setuid-nonroot process, new caps bound by inheritable
> 
> What new caps?

The caps in the newly exec'd process.  IOW, effective aren't dropped,
but as you'd expect inheritable provides limit.

> >>+	/* Pretend we have VFS capabilities */
> >>+	if ((bprm->secflags & BINPRM_SEC_SETUID) && bprm->e_uid == 0)
> >>+		cap_set_full(bprm->cap_permitted);
> >>+	else
> >>+		cap_clear(bprm->cap_permitted);
> >>+
> > 
> > 
> > Thus far we've kept all this stuff out of core.  I believe we should
> > keep it that way.  This could easily be left in bprm_set().
> 
> True.  But as long as linux_binprm has fields for this stuff, intuitively 
> it should always be filled in (i.e. not just in commoncap).  That way we 
> can say that commoncap doesn't get special treatment :)
> 
> Also, this seems like the right place to check for VFS caps.  This way we can.

This does change the current notion of layering.  I see your point though, 
likening it to say reading inode and finding S_ISUID bit.

> >>+ * The rules of Linux capabilities (not POSIX!)
> >>+ *
> >>+ * What the masks mean:
> >>+ *  pP = capabilities that this process has
> >>+ *  pE = capabilities that this process has and are enabled
> >>+ * (so pE <= pP)
> >>+ *
> >>+ * The capability evolution rules are:
> >>+ *
> >>+ *  pP' = ((fP & cap_bset) | pP) & Y
> >>+ *  pE' = (pE | fP) & pP'
> >>+ *
> >>+ *  X = cap_bset
> >>+ *  Y is zero if uid!=0, euid==0, and setuid non-root
> >>+ *
> >>+ *  Exception: if setuid-nonroot, then pE' is reset to 0.
> > 
> > While this works fine, I was interested to see what we could do to
> > leave the old algorithm.  Seems both work out fine.
> > 
> >>+	/* setuid-nonroot is special. */
> >>+	if (is_setuid && bprm->e_uid != 0 && current->uid != 0 &&
> >>+	    current->euid == 0)
> >>+		cap_clear(new_pP);
> > 
> > 
> > setuid-nonroot from a non-root user needs to clear effective?
> 
> Yes.  Say user 500 runs a setuid-root program, which goes back and runs a 
> setuid-500 program.  uid==euid==500 now, so the program expects to be 
> unprivileged.  This makes that work.  (I'm assuming you meant permitted. 
> Effective gets cleared in cap_mask(new_pE, new_pP)).

Yup, I see.  This works in my patch as well.  I'll add this test.  Also
added test to check disabling priv escalation during ptrace of setuid
app.

> The reason I killed the old algorithm is because it's scary (in the sense 
> of being complicated and subtle for no good reason) and because I don't see 
> the point of inheritable caps.  I doubt anything uses them currently on a 
> vanilla kernel because they don't _do_ anything.  So I killed them.

This does break all those caps aware apps (yeah, tongue-in-cheek ;-)
that actually have the idea to widen the effective set, yet limit the
inheriable set.  Seriously, I don't know how much this matters.

thanks,
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net

Re: [PATCH] scaled-back caps, take 4

[Andy Lutomirski]

Chris Wright wrote:

> * Andy Lutomirski (luto@stanford.edu) wrote:
> 
>>Chris Wright wrote:
>>
>>>
>>>
>>>Thus far we've kept all this stuff out of core.  I believe we should
>>>keep it that way.  This could easily be left in bprm_set().
>>
>>True.  But as long as linux_binprm has fields for this stuff, intuitively 
>>it should always be filled in (i.e. not just in commoncap).  That way we 
>>can say that commoncap doesn't get special treatment :)
>>
>>Also, this seems like the right place to check for VFS caps.  This way we can.
> 
> 
> This does change the current notion of layering.  I see your point though, 
> likening it to say reading inode and finding S_ISUID bit.

On the other hand, no one would put reading of a SELinux security label 
here.  But we already have fields in binprm specifically for commoncap.  I 
have no strong preference.


>>The reason I killed the old algorithm is because it's scary (in the sense 
>>of being complicated and subtle for no good reason) and because I don't see 
>>the point of inheritable caps.  I doubt anything uses them currently on a 
>>vanilla kernel because they don't _do_ anything.  So I killed them.
> 
> 
> This does break all those caps aware apps (yeah, tongue-in-cheek ;-)
> that actually have the idea to widen the effective set, yet limit the
> inheriable set.  Seriously, I don't know how much this matters.

Yah, they're broken anyway right now if that's what they're doing.

The reason I didn't go for something like your approach (other than not 
thinking of it) was that, as long as we're changing the semantics, we might 
as well make them as clean as possible.  I also didn't mind ripping out 
lots of old code :).  If the inheritable mask stays, then programs need to 
be audited for what happens if they are run with different inheritable 
masks.  I'd rather just eliminate that complication and the corresponding 
blob of commoncap code.  Obviously my patch fails a lot of your tests as a 
result.

So do we arm-wrestle over whose implementation wins? :)  I'd say mine wins 
on readability (not your fault -- the old code was pretty bad to begin 
with) and some simplicity, but yours has the benefit of being less intrusive.

--Andy

Re: [PATCH] scaled-back caps, take 4

[Chris Wright]

* Andy Lutomirski (luto@myrealbox.com) wrote:
> Chris Wright wrote:
> > This does change the current notion of layering.  I see your point though, 
> > likening it to say reading inode and finding S_ISUID bit.
> 
> On the other hand, no one would put reading of a SELinux security label 
> here.  But we already have fields in binprm specifically for commoncap.  I 
> have no strong preference.

Yes, I stopped short of that argument only because capabilities fall
into a bit more of a gray zone than other modules.  But I do prefer
leaving it in the module.

> >>The reason I killed the old algorithm is because it's scary (in the sense 
> >>of being complicated and subtle for no good reason) and because I don't see 
> >>the point of inheritable caps.  I doubt anything uses them currently on a 
> >>vanilla kernel because they don't _do_ anything.  So I killed them.
> > 
> > This does break all those caps aware apps (yeah, tongue-in-cheek ;-)
> > that actually have the idea to widen the effective set, yet limit the
> > inheriable set.  Seriously, I don't know how much this matters.
> 
> Yah, they're broken anyway right now if that's what they're doing.

On Linux they are.  On IRIX they aren't.  This is part of the issue as I
see it.

> The reason I didn't go for something like your approach (other than not 
> thinking of it) was that, as long as we're changing the semantics, we might 
> as well make them as clean as possible.  I also didn't mind ripping out 
> lots of old code :).  If the inheritable mask stays, then programs need to 
> be audited for what happens if they are run with different inheritable 
> masks.  I'd rather just eliminate that complication and the corresponding 
> blob of commoncap code.  Obviously my patch fails a lot of your tests as a 
> result.

Actually the only glaring difference (aside from the uid/suid and non-root
execs nonroot-yet-diff-id-setuid-app issue I mentioned earlier) is in
something like "=ep cap_setpcap-ep cap_ipc_lock+i" IIRC.

I have the feeling we both are after the same thing, which is introducing
the ability to keep some caps through exec and still being able to sleep
at night w/ confidence that there isn't some subtle new hole lurking.
This is why I aimed to change as little code as possible.

> So do we arm-wrestle over whose implementation wins? :)  I'd say mine wins 
> on readability (not your fault -- the old code was pretty bad to begin 
> with) and some simplicity, but yours has the benefit of being less intrusive.

Hehe, arm wrestling could be entertaining ;-)  I'm in favor of the most
conservative change, which I feel is in my patch.  But I'm game to
continue to pick on each.

thanks,
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net

Re: [PATCH] scaled-back caps, take 4

[Andy Lutomirski]

On Wednesday 19 May 2004 00:30, Chris Wright wrote:
> * Andy Lutomirski (luto@myrealbox.com) wrote:
> > Chris Wright wrote:

> > The reason I didn't go for something like your approach (other than not 
> > thinking of it) was that, as long as we're changing the semantics, we might 
> > as well make them as clean as possible.  I also didn't mind ripping out 
> > lots of old code :).  If the inheritable mask stays, then programs need to 
> > be audited for what happens if they are run with different inheritable 
> > masks.  I'd rather just eliminate that complication and the corresponding 
> > blob of commoncap code.  Obviously my patch fails a lot of your tests as a 
> > result.
> 
> Actually the only glaring difference (aside from the uid/suid and non-root
> execs nonroot-yet-diff-id-setuid-app issue I mentioned earlier) is in
> something like "=ep cap_setpcap-ep cap_ipc_lock+i" IIRC.
> 
> I have the feeling we both are after the same thing, which is introducing
> the ability to keep some caps through exec and still being able to sleep
> at night w/ confidence that there isn't some subtle new hole lurking.
> This is why I aimed to change as little code as possible.
> 
> > So do we arm-wrestle over whose implementation wins? :)  I'd say mine wins 
> > on readability (not your fault -- the old code was pretty bad to begin 
> > with) and some simplicity, but yours has the benefit of being less intrusive.
> 
> Hehe, arm wrestling could be entertaining ;-)  I'm in favor of the most
> conservative change, which I feel is in my patch.  But I'm game to
> continue to pick on each.
> 

You don't like my patch because it rips out a bunch of code and it's
not clear it won't break stuff.

I don't like your patch because it takes a bunch of incomprehensible
code that does almost nothing and tweaks it slightly to do something
useful.  (That's not to say it's does the wrong thing; I just don't
think the code is clear.)

So I decided to figure out what was going on with the original code.

First, CAP_SETPCAP is never obtainable (by anything).
Since cap_bset never has this bit set, nothing can inherit it
from fP.  capset_check prevents it from getting set in pI.

Second, cap_bset is broken.  For one thing, there's no way
to remove the caps you want to restrict from already-running
tasks.  So I don't think it matters if we break/change it.


cap_bprm_set_security does:
fP = fI = (new_uid == 0 || new_euid == 0)
fE = (new_euid == 0)

cap_bprm_apply_creds then does:
1. pP' = (fP & X) | (fI & pI)
2. unsafeness stuff
3. fix up uids
4. pE' = fE & pP'

Now for the fun part:

Since fP == fI, pP' = fP & (X | pI)
But X | pI == X (since pI can't have CAP_SETPCAP and X==~CAP_SETPCAP)
So pP' = fP & X

pE' == fE & pP' == fE & fP & X
But fE & fP == fE, so:
pE' = fE & X

The whole result is just
pP' = (uid == 0 || euid==0) & X
pE' = (euid == 0) & X


This patch implements this.  It should be invisible to userspace
(unless userspace (ab)uses cap_bset).  It also adds a secureexec
flag, which we both need.

First, did I get this right?  It seems to work :)

Second, do you have any objection to both of us redoing our
patches against this one?  It should make them nicer-looking
at least.

--Andy

--- 2.6.6-mm4/fs/exec.c~cap_cleanup	2004-05-19 10:37:25.000000000 -0700
+++ 2.6.6-mm4/fs/exec.c	2004-05-21 19:03:16.000000000 -0700
@@ -1093,6 +1093,7 @@
 	bprm.loader = 0;
 	bprm.exec = 0;
 	bprm.security = NULL;
+	bprm.secflags = 0;
 	bprm.mm = mm_alloc();
 	retval = -ENOMEM;
 	if (!bprm.mm)
--- 2.6.6-mm4/security/commoncap.c~cap_cleanup	2004-05-21 18:51:42.000000000 -0700
+++ 2.6.6-mm4/security/commoncap.c	2004-05-23 01:42:51.000000000 -0700
@@ -89,44 +89,20 @@
 
 int cap_bprm_set_security (struct linux_binprm *bprm)
 {
-	/* Copied from fs/exec.c:prepare_binprm. */
-
-	/* We don't have VFS support for capabilities yet */
-	cap_clear (bprm->cap_inheritable);
-	cap_clear (bprm->cap_permitted);
-	cap_clear (bprm->cap_effective);
-
-	/*  To support inheritance of root-permissions and suid-root
-	 *  executables under compatibility mode, we raise all three
-	 *  capability sets for the file.
-	 *
-	 *  If only the real uid is 0, we only raise the inheritable
-	 *  and permitted sets of the executable file.
-	 */
-
-	if (!issecure (SECURE_NOROOT)) {
-		if (bprm->e_uid == 0 || current->uid == 0) {
-			cap_set_full (bprm->cap_inheritable);
-			cap_set_full (bprm->cap_permitted);
-		}
-		if (bprm->e_uid == 0)
-			cap_set_full (bprm->cap_effective);
-	}
 	return 0;
 }
 
 void cap_bprm_apply_creds (struct linux_binprm *bprm, int unsafe)
 {
-	/* Derived from fs/exec.c:compute_creds. */
-	kernel_cap_t new_permitted, working;
+	kernel_cap_t new_pP, new_pE;
 
-	new_permitted = cap_intersect (bprm->cap_permitted, cap_bset);
-	working = cap_intersect (bprm->cap_inheritable,
-				 current->cap_inheritable);
-	new_permitted = cap_combine (new_permitted, working);
+	if (bprm->e_uid == 0 || current->uid == 0)
+		new_pP = cap_bset;
+	else
+		cap_clear(new_pP);
 
 	if (bprm->e_uid != current->uid || bprm->e_gid != current->gid ||
-	    !cap_issubset (new_permitted, current->cap_permitted)) {
+	    !cap_issubset (new_pP, current->cap_permitted)) {
 		current->mm->dumpable = 0;
 
 		if (unsafe & ~LSM_UNSAFE_PTRACE_CAP) {
@@ -134,13 +110,16 @@
 				bprm->e_uid = current->uid;
 				bprm->e_gid = current->gid;
 			}
-			if (!capable (CAP_SETPCAP)) {
-				new_permitted = cap_intersect (new_permitted,
-							current->cap_permitted);
-			}
+			new_pP = cap_intersect (new_pP,
+						current->cap_permitted);
 		}
 	}
 
+	if (bprm->e_uid == 0)
+		new_pE = new_pP;
+	else
+		cap_clear(new_pE);
+
 	current->suid = current->euid = current->fsuid = bprm->e_uid;
 	current->sgid = current->egid = current->fsgid = bprm->e_gid;
 
@@ -148,9 +127,8 @@
 	 * in the init_task struct. Thus we skip the usual
 	 * capability rules */
 	if (current->pid != 1) {
-		current->cap_permitted = new_permitted;
-		current->cap_effective =
-		    cap_intersect (new_permitted, bprm->cap_effective);
+		current->cap_permitted = new_pP;
+		current->cap_effective = new_pE;
 	}
 
 	/* AUD: Audit candidate if current->cap_effective is set */
@@ -160,13 +138,9 @@
 
 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,
--- 2.6.6-mm4/include/linux/binfmts.h~cap_cleanup	2004-05-19 10:37:25.000000000 -0700
+++ 2.6.6-mm4/include/linux/binfmts.h	2004-05-23 02:24:31.034876220 -0700
@@ -20,6 +20,7 @@
 /*
  * This structure is used to hold the arguments that are used when loading binaries.
  */
+#define BINPRM_SEC_SECUREEXEC	1
 struct linux_binprm{
 	char buf[BINPRM_BUF_SIZE];
 	struct page *page[MAX_ARG_PAGES];
@@ -28,6 +29,7 @@
 	int sh_bang;
 	struct file * file;
 	int e_uid, e_gid;
+	int secflags;
 	kernel_cap_t cap_inheritable, cap_permitted, cap_effective;
 	void *security;
 	int argc, envc;

Re: [PATCH] scaled-back caps, take 4

[Olaf Dietsche]

Andy Lutomirski <luto@myrealbox.com> writes:

> You don't like my patch because it rips out a bunch of code and it's
> not clear it won't break stuff.
>
> I don't like your patch because it takes a bunch of incomprehensible
> code that does almost nothing and tweaks it slightly to do something
> useful.  (That's not to say it's does the wrong thing; I just don't
> think the code is clear.)
>
> So I decided to figure out what was going on with the original code.
>
> First, CAP_SETPCAP is never obtainable (by anything).
> Since cap_bset never has this bit set, nothing can inherit it
> from fP.  capset_check prevents it from getting set in pI.

# mv /sbin/init /sbin/init.bin
# cat >/sbin/init
#! /bin/sh

if test $$ -eq 1; then
        mount /proc
        echo -1 >/proc/sys/kernel/cap-bound
fi

exec /sbin/init.bin "$@"
^D
# chmod 755 /sbin/init
# reboot

> Second, cap_bset is broken.  For one thing, there's no way
> to remove the caps you want to restrict from already-running
> tasks.  So I don't think it matters if we break/change it.

Maybe I don't understand this, but I think this is what sys_capset()
is for.

> cap_bprm_set_security does:
> fP = fI = (new_uid == 0 || new_euid == 0)
> fE = (new_euid == 0)

Only if (!issecure (SECURE_NOROOT))

[...]
> The whole result is just
> pP' = (uid == 0 || euid==0) & X
> pE' = (euid == 0) & X
>
> This patch implements this.  It should be invisible to userspace
> (unless userspace (ab)uses cap_bset).  It also adds a secureexec
> flag, which we both need.
>
> First, did I get this right?  It seems to work :)

With this patch you effectively revert all capable() calls back to
suser() tests. If this is what you intended, your patch looks fine.

> Second, do you have any objection to both of us redoing our
> patches against this one?  It should make them nicer-looking
> at least.

I didn't scrutinize capabilities as thoroughly as you did, but I still
don't see why your patch is necessary, besides the changes in
fs/exec.c and include/binfmts.h, maybe.

$ cp commoncap.c lutocap.c
modify it to your liking
# insmod lutocap

same goes for chriscap.c

Please, don't get me wrong. For me, it's just a matter of maintaining
a slightly bigger fscaps patch. But I don't think capabilities in
Linux are really broken, only because some proponents of SELinux claim
so.

If you want a simpler - setuid like - capabilities model, throw out
the inheritable _and_ permitted set and use the effective set alone.

Regards, Olaf.