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

[Andy Lutomirski]

Chris Wright wrote:
> * Andy Lutomirski (luto@xxxxxxxxxxxxx) 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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/