OpenVZ Forum - RDF feed
type: channel
items: [PATCH 1/1] capabilities: introduce per-process capability bounding set (v8)
items: Re: [PATCH 1/1] capabilities: introduce per-process capability bounding set (v8)

[PATCH 1/1] capabilities: introduce per-process capability bounding set (v8)
type: item
description: Andrew, this version follows all of your suggestions. Definately nicer userspace interface. thanks -serge >From b7c210160e3c210d63eca532289ca1c9caf1bd87 Mon Sep 17 00:00:00 2001 From: Serge E. Hallyn <serue@us.ibm.com> Date: Mon, 19 Nov 2007 13:54:05 -0500 Subject: [PATCH 1/1] capabilities: introduce per-process capability bounding set (v8) The capability bounding set is a set beyond which capabilities cannot grow. Currently cap_bset is per-system. It can be manipulated through sysctl, but only init can add capabilities. Root can remove capabilities. By default it includes all caps except CAP_SETPCAP. This patch makes the bounding set per-process when file capabilities are enabled. It is inherited at fork from parent. Noone can add elements, CAP_SETPCAP is required to remove them. One example use of this is to start a safer container. For instance, until device namespaces or per-container device whitelists are introduced, it is best to take CAP_MKNOD away from a container. The following test program will get and set the bounding set. For instance ./bset get (lists capabilities in bset) ./bset drop cap_net_raw (starts shell with new bset) (use capset, setuid binary, or binary with file capabilities to try to increase caps) ************************************************************ cap_bound.c ************************************************************ #include <sys/prctl.h> #include <linux/capability.h> #include <sys/types.h> #include <unistd.h> #include <stdio.h> #include <stdlib.h> #include <string.h> #ifndef PR_CAPBSET_READ #define PR_CAPBSET_READ 23 #endif #ifndef PR_CAPBSET_DROP #define PR_CAPBSET_DROP 24 #endif int usage(char *me) { printf("Usage: %s get\n", me); printf(" %s drop <capability>\n", me); return 1; } #define numcaps 32 char *captable[numcaps] = { "cap_chown", "cap_dac_override", "cap_dac_read_search", "cap_fowner", "cap_fsetid", "cap_kill", "cap_setgid", "cap_setuid", "cap_setpcap", "cap_linux_immutable", "cap_net_bind_service", "cap_net_broadca…
creator: serue
date: 2007-11-19T21:25:19-00:00

Re: [PATCH 1/1] capabilities: introduce per-process capability bounding set (v8)
type: item
description: -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Serge E. Hallyn wrote: > Andrew, this version follows all of your suggestions. Definately nicer > userspace interface. thanks [...] > > /* Allow ioperm/iopl access */ > @@ -314,6 +314,10 @@ typedef struct kernel_cap_struct { > > #define CAP_SETFCAP 31 > > +#define CAP_NUM_CAPS 32 > + > +#define cap_valid(x) ((x) >= 0 && (x) < CAP_NUM_CAPS) > + Could you change the name of CAP_NUM_CAPS? There is some libcap building code that does the following to automatically build the "cap_*" names for libcap, and this new define above messes that up! :-( sed -ne '/^#define[ \t]CAP[_A-Z]\+[ \t]\+[0-9]\+/{s/^#define \([^ \t]*\)[ \t]*\([^ \t]*\)/ \{ \2, \"\1\" \},/;y/ABCDEFGHIJKLMNOPQRSTUVWXYZ/abcdefghijklmnopqrstuvwxyz/;p;}' < $(KERNEL_HEADERS)/linux/capability.h | fgrep -v 0x > cap_names.sed Something like: #define CAP_NUM_CAPS (CAP_SETFCAP+1) will save me some hassle. :-) [...] > /* > * Bit location of each capability (used by user-space library and kernel) > */ > @@ -350,6 +354,17 @@ typedef struct kernel_cap_struct { > > #define CAP_INIT_INH_SET CAP_EMPTY_SET > Its kind of a pity to put a kernel config ifdef in a header file. Could you put the ifdef code in the c-files that uses these definitions? > +#ifdef CONFIG_SECURITY_FILE_CAPABILITIES In my experience when headers define things differently based on configuration #defines, other users of header files (apps, kernel modules etc.), never quite know what the current define is. If we can avoid conditional code like this in this header file, I'd be happier. > +#ifdef CONFIG_SECURITY_FILE_CAPABILITIES ditto. [...] > +extern long cap_prctl_drop(unsigned long cap); > +#else > +#include <linux/errno.h> > +static inline long cap_prctl_drop(unsigned long cap) > +{ return -EINVAL; } > +#endif > + > +long cap_prctl_drop(unsigned long cap) > +{ > + if (!capable(CAP_SETPCAP)) > + return -EPERM; > + if (!cap_valid(cap)) > + return -EINVAL; > + cap_lower(current->cap_bset, cap); I think the foll…
creator: Andrew Morgan
date: 2007-11-20T03:40:23-00:00

Re: [PATCH 1/1] capabilities: introduce per-process capability bounding set (v8)
type: item
description: -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Andrew Morgan wrote: >> + current->cap_effective = cap_intersect(current->cap_effective, >> + current->cap_bset); >> + current->cap_permitted = cap_intersect(current->cap_permitted, >> + current->cap_bset); >> + current->cap_inheritable = cap_intersect(current->cap_inheritable, >> + current->cap_bset); > > You might want to replace the above three lines with a restriction > elsewhere on what CAP_SETPCAP can newly set in > commoncap.c:cap_capset_check(). > > That is, CAP_SETPCAP permits the current process to raise 'any' pI > capability. I suspect that you'll want to prevent raising any bits not > masked by this: > > pI' & ~(pI | (pP & cap_bset)). On second thoughts, I really meant this: diff --git a/security/commoncap.c b/security/commoncap.c index 302e8d0..b28c0c1 100644 - --- a/security/commoncap.c +++ b/security/commoncap.c @@ -133,6 +133,12 @@ int cap_capset_check (struct task_struct *target, kernel_ca /* incapable of using this inheritable set */ return -EPERM; } + if (!cap_issubset(*inheritable, + cap_combine(target->cap_inheritable, + current->cap_bset))) { + /* no new pI capabilities outside bounding set */ + return -EPERM; + } /* verify restrictions on target's new Permitted set */ if (!cap_issubset (*permitted, Cheers Andrew -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.2.6 (GNU/Linux) iD8DBQFHQnJ8QheEq9QabfIRAl2rAJ4jH+l36N1+cHV+1A3DJpXs+UNsFgCgkg8H xOU/7dCrEq02xk9EgcRarg0= =FbqU -----END PGP SIGNATURE----- _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: Andrew Morgan
date: 2007-11-20T05:37:03-00:00

Re: [PATCH 1/1] capabilities: introduce per-process capability bounding set (v8)
type: item
description: Quoting Andrew Morgan (morgan@kernel.org): > -----BEGIN PGP SIGNED MESSAGE----- > Hash: SHA1 > > Serge E. Hallyn wrote: > > Andrew, this version follows all of your suggestions. Definately nicer > > userspace interface. thanks > [...] > > > > /* Allow ioperm/iopl access */ > > @@ -314,6 +314,10 @@ typedef struct kernel_cap_struct { > > > > #define CAP_SETFCAP 31 > > > > +#define CAP_NUM_CAPS 32 > > + > > +#define cap_valid(x) ((x) >= 0 && (x) < CAP_NUM_CAPS) > > + > > Could you change the name of CAP_NUM_CAPS? There is some libcap building > code that does the following to automatically build the "cap_*" names > for libcap, and this new define above messes that up! :-( > > sed -ne '/^#define[ \t]CAP[_A-Z]\+[ \t]\+[0-9]\+/{s/^#define \([^ > \t]*\)[ \t]*\([^ \t]*\)/ \{ \2, \"\1\" > \},/;y/ABCDEFGHIJKLMNOPQRSTUVWXYZ/abcdefghijklmnopqrstuvwxyz/;p;}' < > $(KERNEL_HEADERS)/linux/capability.h | fgrep -v 0x > cap_names.sed > > Something like: > > #define CAP_NUM_CAPS (CAP_SETFCAP+1) > > will save me some hassle. :-) Gotcha. Will change that. I worry that what you have is just a *touch* too busy so whoever adds capability #32 might forget to update CAP_NUM_CAPS, but it looks like #define CAP_LAST_CAP CAP_SETFCAP #define cap_valid(x) ((x) >= 0 && (x) <= CAP_LAST_CAP) should also be ok for libcap. > [...] > > > /* > > * Bit location of each capability (used by user-space library and kernel) > > */ > > @@ -350,6 +354,17 @@ typedef struct kernel_cap_struct { > > > > #define CAP_INIT_INH_SET CAP_EMPTY_SET > > > > Its kind of a pity to put a kernel config ifdef in a header file. Could > you put the ifdef code in the c-files that uses these definitions? Hmm, now that you mention it, I notice that the exact same block of code is still in commoncap.c. I must have lost the patch hunk dropping that some time ago... But at this point CAP_INIT_BSET is only used in include/linux/init_task.h. And I'd really rather not put the definition in there. Note that the conditional is under a #ifdef…
creator: serue
date: 2007-11-20T18:14:52-00:00

Re: [PATCH 1/1] capabilities: introduce per-process capability bounding set (v8)
type: item
description: Quoting Serge E. Hallyn (serue@us.ibm.com): > Quoting Andrew Morgan (morgan@kernel.org): > > -----BEGIN PGP SIGNED MESSAGE----- > > Hash: SHA1 > > > > Serge E. Hallyn wrote: > > > Andrew, this version follows all of your suggestions. Definately nicer > > > userspace interface. thanks > > [...] > > > > > > /* Allow ioperm/iopl access */ > > > @@ -314,6 +314,10 @@ typedef struct kernel_cap_struct { > > > > > > #define CAP_SETFCAP 31 > > > > > > +#define CAP_NUM_CAPS 32 > > > + > > > +#define cap_valid(x) ((x) >= 0 && (x) < CAP_NUM_CAPS) > > > + > > > > Could you change the name of CAP_NUM_CAPS? There is some libcap building > > code that does the following to automatically build the "cap_*" names > > for libcap, and this new define above messes that up! :-( > > > > sed -ne '/^#define[ \t]CAP[_A-Z]\+[ \t]\+[0-9]\+/{s/^#define \([^ > > \t]*\)[ \t]*\([^ \t]*\)/ \{ \2, \"\1\" > > \},/;y/ABCDEFGHIJKLMNOPQRSTUVWXYZ/abcdefghijklmnopqrstuvwxyz/;p;}' < > > $(KERNEL_HEADERS)/linux/capability.h | fgrep -v 0x > cap_names.sed > > > > Something like: > > > > #define CAP_NUM_CAPS (CAP_SETFCAP+1) > > > > will save me some hassle. :-) > > Gotcha. Will change that. > > I worry that what you have is just a *touch* too busy so whoever adds > capability #32 might forget to update CAP_NUM_CAPS, but it looks like > > #define CAP_LAST_CAP CAP_SETFCAP > > #define cap_valid(x) ((x) >= 0 && (x) <= CAP_LAST_CAP) > > should also be ok for libcap. > > > [...] > > > > > /* > > > * Bit location of each capability (used by user-space library and kernel) > > > */ > > > @@ -350,6 +354,17 @@ typedef struct kernel_cap_struct { > > > > > > #define CAP_INIT_INH_SET CAP_EMPTY_SET > > > > > > > Its kind of a pity to put a kernel config ifdef in a header file. Could > > you put the ifdef code in the c-files that uses these definitions? > > Hmm, now that you mention it, I notice that the exact same block of > code is still in commoncap.c. I must have lost the patch hunk dropping > that some time ago... > > But…
creator: serue
date: 2007-11-20T18:32:42-00:00

Re: [PATCH 1/1] capabilities: introduce per-process capability bounding set (v8)
type: item
description: Quoting Andrew Morgan (morgan@kernel.org): > -----BEGIN PGP SIGNED MESSAGE----- > Hash: SHA1 > > Serge E. Hallyn wrote: > > Andrew, this version follows all of your suggestions. Definately nicer > > userspace interface. thanks > [...] > > > > /* Allow ioperm/iopl access */ > > @@ -314,6 +314,10 @@ typedef struct kernel_cap_struct { > > > > #define CAP_SETFCAP 31 > > > > +#define CAP_NUM_CAPS 32 > > + > > +#define cap_valid(x) ((x) >= 0 && (x) < CAP_NUM_CAPS) > > + > > Could you change the name of CAP_NUM_CAPS? There is some libcap building > code that does the following to automatically build the "cap_*" names > for libcap, and this new define above messes that up! :-( > > sed -ne '/^#define[ \t]CAP[_A-Z]\+[ \t]\+[0-9]\+/{s/^#define \([^ > \t]*\)[ \t]*\([^ \t]*\)/ \{ \2, \"\1\" > \},/;y/ABCDEFGHIJKLMNOPQRSTUVWXYZ/abcdefghijklmnopqrstuvwxyz/;p;}' < > $(KERNEL_HEADERS)/linux/capability.h | fgrep -v 0x > cap_names.sed > > Something like: > > #define CAP_NUM_CAPS (CAP_SETFCAP+1) > > will save me some hassle. :-) > > [...] > > > /* > > * Bit location of each capability (used by user-space library and kernel) > > */ > > @@ -350,6 +354,17 @@ typedef struct kernel_cap_struct { > > > > #define CAP_INIT_INH_SET CAP_EMPTY_SET > > > > Its kind of a pity to put a kernel config ifdef in a header file. Could > you put the ifdef code in the c-files that uses these definitions? > > > +#ifdef CONFIG_SECURITY_FILE_CAPABILITIES > > In my experience when headers define things differently based on > configuration #defines, other users of header files (apps, kernel > modules etc.), never quite know what the current define is. If we can > avoid conditional code like this in this header file, I'd be happier. > > > +#ifdef CONFIG_SECURITY_FILE_CAPABILITIES > > ditto. > > [...] > > +extern long cap_prctl_drop(unsigned long cap); > > +#else > > +#include <linux/errno.h> > > +static inline long cap_prctl_drop(unsigned long cap) > > +{ return -EINVAL; } > > +#endif > > + > > +long cap_prctl_drop…
creator: serue
date: 2007-11-20T20:07:46-00:00

Re: [PATCH 1/1] capabilities: introduce per-process capability bounding set (v8)
type: item
description: Quoting Serge E. Hallyn (serue@us.ibm.com): > How about the following? Argh, with the following on top of it... -serge >From 470a68120cda83875a281354b897f3bda04b58fc Mon Sep 17 00:00:00 2001 From: Serge E. Hallyn <serue@us.ibm.com> Date: Tue, 20 Nov 2007 15:12:54 -0500 Subject: [PATCH 1/1] capbset: fix compilation when CONFIG_SECURITY=n without this patch, kernel/sys.c has undefined reference to cap_prctl_drop(). Signed-off-by: Serge E. Hallyn <serue@us.ibm.com> --- kernel/sys.c | 4 ++++ security/commoncap.c | 4 ---- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/kernel/sys.c b/kernel/sys.c index b528e75..efc495e 100644 --- a/kernel/sys.c +++ b/kernel/sys.c @@ -1747,7 +1747,11 @@ asmlinkage long sys_prctl(int option, unsigned long arg2, unsigned long arg3, return -EINVAL; return !!cap_raised(current->cap_bset, arg2); case PR_CAPBSET_DROP: +#ifdef CONFIG_SECURITY_FILE_CAPABILITIES return cap_prctl_drop(arg2); +#else + return -EINVAL; +#endif default: error = -EINVAL; diff --git a/security/commoncap.c b/security/commoncap.c index b72825e..e909f4f 100644 --- a/security/commoncap.c +++ b/security/commoncap.c @@ -594,10 +594,6 @@ int cap_task_kill(struct task_struct *p, struct siginfo *info, { return 0; } -long cap_prctl_drop(unsigned long cap) -{ - return -EINVAL; -} #endif void cap_task_reparent_to_init (struct task_struct *p) -- 1.5.1.1.GIT _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: serue
date: 2007-11-20T20:23:40-00:00

Re: [PATCH 1/1] capabilities: introduce per-process capability bounding set (v8)
type: item
description: -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Serge, 16d76d11d27f32487366a7cec6a52f6ec4fb1cbb + 470a68120cda83875a281354b897f3bda04b58fc Could you also modify the old comment above #define CAP_SETPCAP 8 "add any capability to the current process' inheritable set" to read "add any capability, from cap_bset, to the current process' inheritable set" Otherwise, this all looks good. Thanks Andrew Serge E. Hallyn wrote: > Quoting Serge E. Hallyn (serue@us.ibm.com): >> How about the following? > > Argh, with the following on top of it... > > -serge > > >>From 470a68120cda83875a281354b897f3bda04b58fc Mon Sep 17 00:00:00 2001 > From: Serge E. Hallyn <serue@us.ibm.com> > Date: Tue, 20 Nov 2007 15:12:54 -0500 > Subject: [PATCH 1/1] capbset: fix compilation when CONFIG_SECURITY=n > > without this patch, kernel/sys.c has undefined reference > to cap_prctl_drop(). > > Signed-off-by: Serge E. Hallyn <serue@us.ibm.com> > --- > kernel/sys.c | 4 ++++ > security/commoncap.c | 4 ---- > 2 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/kernel/sys.c b/kernel/sys.c > index b528e75..efc495e 100644 > --- a/kernel/sys.c > +++ b/kernel/sys.c > @@ -1747,7 +1747,11 @@ asmlinkage long sys_prctl(int option, unsigned long arg2, unsigned long arg3, > return -EINVAL; > return !!cap_raised(current->cap_bset, arg2); > case PR_CAPBSET_DROP: > +#ifdef CONFIG_SECURITY_FILE_CAPABILITIES > return cap_prctl_drop(arg2); > +#else > + return -EINVAL; > +#endif > > default: > error = -EINVAL; > diff --git a/security/commoncap.c b/security/commoncap.c > index b72825e..e909f4f 100644 > --- a/security/commoncap.c > +++ b/security/commoncap.c > @@ -594,10 +594,6 @@ int cap_task_kill(struct task_struct *p, struct siginfo *info, > { > return 0; > } > -long cap_prctl_drop(unsigned long cap) > -{ > - return -EINVAL; > -} > #endif > > void cap_task_reparent_to_init (struct task_struct *p) -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.2.6 (GNU/Linux) iD8DBQFHRSSmQheEq9QabfIRAk8zAJ9u+RFKmZroCUIZzyHDX9XIP…
creator: Andrew Morgan
date: 2007-11-22T06:41:45-00:00

Re: [PATCH 1/1] capabilities: introduce per-process capability bounding set (v8)
type: item
description: -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Serge E. Hallyn wrote: > I worry that what you have is just a *touch* too busy so whoever adds > capability #32 might forget to update CAP_NUM_CAPS, but it looks like > > #define CAP_LAST_CAP CAP_SETFCAP > > #define cap_valid(x) ((x) >= 0 && (x) <= CAP_LAST_CAP) > > should also be ok for libcap. FWIW libcap computes the upper limit itself in the process of consuming all that sed'ed stuff. You do need it for the kernel, and this seems like a fine mechanism. >>> +long cap_prctl_drop(unsigned long cap) >>> +{ >>> + if (!capable(CAP_SETPCAP)) >>> + return -EPERM; >>> + if (!cap_valid(cap)) >>> + return -EINVAL; >>> + cap_lower(current->cap_bset, cap); >> I think the following lines are overkill. Basically, the next exec() >> will perform the pP/pE clipping, and cap_bset should only interact with >> fP (and not fI). >> >> We already have a mechanism to manipulate pI, which in turn gates fI. >> And this same mechanism (libcap) can clip pE, pP if it is needed pre-exec(). >> >> So, if you want to drop a capability irrevocably, you drop it in bset, >> and separately in pI. The current process may continue to have the >> capability, but post-exec the working process tree has lost it. For >> things like login, this is desirable. > > Ok... > > I think this makes sense. It seems pretty subtle and complicated, and > therefore I'm a little worried that it will be fragile against future > code changes. Someone will think it's a good idea to slightly change > the capset() semantics and only a year later will we realize that the > bounding set is no longer working... We'll have to be diligent then :-) In truth, the whole model is not entirely unsubtle. > So this will all have to be very well documented (and tested). > > (Actually I notice that capabilities(7) manpage isn't in the libcap > sources. So an update to that is probably long overdue...) I don't believe it ever was. So far as I can tell this file has had its own life as part of t…
creator: Andrew Morgan
date: 2007-11-22T07:10:13-00:00