| Message ID | 1481949827-23613-1-git-send-email-john.stultz@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, If I understand correctly, this patch is intended to add a delegation feature to cgroup v1, which does not really make sense for the v2 because of the clean cgroup-v2 delegation design. However, this new capability impact both versions. As Michael said, capabilities are a limited numbers of silos and we should try to use existing ones as much as possible but without falling into the trap of using the same capability for everything (e.g. CAP_SYS_ADMIN) [1]. The CAP_CGROUP looks a lot like another CAP_SYS_ADMIN. It is not tied to any particular privilege. Cgroups are not about resource limitation or any particular access control, they are just about managing a set of processes, which may then be subject to resource limitation or access control. This possible limitations only depend on cgroup controllers or netfilter rules or BPF programs. To avoid the current capability issue [1], I think it should be a good idea to reuse an existing capability (if one make sense) for each class of constraints a controller/eBPF program/netfilter rule can enforce. This may looks like CAP_NET_ADMIN (with network namespace handling) for netfilter and eBPF *socket* programs but CAP_SYS_RESOURCE for the CPU, memory and IO controllers. As Tejun said, it will be more complicated to handle such a case, but I don't see any other solution to keep a meaningful use of capabilities. However, even if a cgroup does not directly involve a limitation, it may be used to identify a group of processes for a security critical purpose (e.g. kill a group of process). It can then make sense to have a dedicated capability CAP_CGROUP to allow a process *without the right to write in cgroup.procs* to be allowed to move a process out of its current cgroup. This is similar to CAP_DAC_OVERRIDE but only for cgroup/controllers files (but not necessarily sufficient to modify all cgroups). This does not means that CAP_CGROUP should allow to move any process from any cgroup. The cgroup_procs_write_permission() should compose the checks for CAP_CGROUP and/or CAP_SYS_RESOURCE and/or CAP_SYS_ADMIN depending on the current use of the cgroup (i.e. cgroup controller, BPF program type, netfilter). Regards, Mickaël [1] https://forums.grsecurity.net/viewtopic.php?f=7&t=2522 On 17/12/2016 05:43, John Stultz wrote: > This patch adds CAP_GROUP and logic to allows a process to > migrate other tasks between cgroups. > > In Android (where this feature originated), the ActivityManager > tracks various application states (TOP_APP, FOREGROUND, > BACKGROUND, SYSTEM, etc), and then as applications change > states, the SchedPolicy logic will migrate the application tasks > between different cgroups used to control the different > application states (for example, there is a background cpuset > cgroup which can limit background tasks to stay on one low-power > cpu, and the bg_non_interactive cpuctrl cgroup can then further > limit those background tasks to a small percentage of that one > cpu's cpu time). > > However, for security reasons, Android doesn't want to make the > system_server (the process that runs the ActivityManager and > SchedPolicy logic), run as root. So in the Android common.git > kernel, they have some logic to allow cgroups to loosen their > permissions so CAP_SYS_NICE tasks can migrate other tasks between > cgroups. > > I feel the approach taken there overloads CAP_SYS_NICE a bit much > for non-android environments. Efforts to re-use CAP_SYS_RESOURCE > for this purpose (which Android has since adopted) was also > stymied by concerns about risks from future cgroups that could be > considered "dangerous" by how they might change system semantics. > > So to avoid overlapping usage, this patch adds a brand new > process capability flag (CAP_CGROUP), and uses it when checking > if a task can migrate other tasks between cgroups. > > I've tested this with AOSP master (though its a bit hacked in as > I still need to properly get the selinux userspace bits aware of > the new capability bit) with selinux set to permissive and it > seems to be working well. > > Thoughts and feedback would be appreciated! > > (Note, I'm going on holiday break after today, so I may not > respond to feedback immediately, but I figured it would be > better to give folks the chance to review this rather then sit > it for two weeks. I'll resend after the new-year, addressing any > feedback I do get.) > > Cc: Tejun Heo <tj@kernel.org> > Cc: Li Zefan <lizefan@huawei.com> > Cc: Jonathan Corbet <corbet@lwn.net> > Cc: cgroups@vger.kernel.org > Cc: Android Kernel Team <kernel-team@android.com> > Cc: Rom Lemarchand <romlem@android.com> > Cc: Colin Cross <ccross@android.com> > Cc: Dmitry Shmidt <dimitrysh@google.com> > Cc: Todd Kjos <tkjos@google.com> > Cc: Christian Poetzsch <christian.potzsch@imgtec.com> > Cc: Amit Pundir <amit.pundir@linaro.org> > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> > Cc: Kees Cook <keescook@chromium.org> > Cc: Serge E. Hallyn <serge@hallyn.com> > Cc: Andy Lutomirski <luto@amacapital.net> > Cc: linux-api@vger.kernel.org > Cc: Paul Moore <paul@paul-moore.com> > Cc: Stephen Smalley <sds@tycho.nsa.gov> > Cc: Eric Paris <eparis@parisplace.org> > Cc: James Morris <james.l.morris@oracle.com> > Cc: Jeff Vander Stoep <jeffv@google.com> > Cc: linux-security-module@vger.kernel.org > Cc: selinux@tycho.nsa.gov > Signed-off-by: John Stultz <john.stultz@linaro.org> > --- > v2: Renamed to just CAP_CGROUP_MIGRATE as reccomended by Tejun > v3: Switched to just using CAP_SYS_RESOURCE as suggested by Michael > v4: Send out properly folded down version of the patch. :P > v5: Switch back to CAP_CGROUP_MIGRATE due to concerns from Andy > v6: Rename to CAP_CGROUP, as it might be used for other purposes > in the future. Also added selinux mappings for the new cap. > --- > include/uapi/linux/capability.h | 5 ++++- > kernel/cgroup.c | 3 ++- > security/selinux/include/classmap.h | 4 ++-- > 3 files changed, 8 insertions(+), 4 deletions(-) > > diff --git a/include/uapi/linux/capability.h b/include/uapi/linux/capability.h > index 49bc062..726f767 100644 > --- a/include/uapi/linux/capability.h > +++ b/include/uapi/linux/capability.h > @@ -349,8 +349,11 @@ struct vfs_cap_data { > > #define CAP_AUDIT_READ 37 > > +/* Allow migration of other tasks between cgroups */ > > -#define CAP_LAST_CAP CAP_AUDIT_READ > +#define CAP_CGROUP 38 > + > +#define CAP_LAST_CAP CAP_CGROUP > > #define cap_valid(x) ((x) >= 0 && (x) <= CAP_LAST_CAP) > > diff --git a/kernel/cgroup.c b/kernel/cgroup.c > index 2ee9ec3..8b42ae3 100644 > --- a/kernel/cgroup.c > +++ b/kernel/cgroup.c > @@ -2856,7 +2856,8 @@ static int cgroup_procs_write_permission(struct task_struct *task, > */ > if (!uid_eq(cred->euid, GLOBAL_ROOT_UID) && > !uid_eq(cred->euid, tcred->uid) && > - !uid_eq(cred->euid, tcred->suid)) > + !uid_eq(cred->euid, tcred->suid) && > + !ns_capable(tcred->user_ns, CAP_CGROUP)) > ret = -EACCES; > > if (!ret && cgroup_on_dfl(dst_cgrp)) { > diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h > index e2d4ad3a..ee8c1ed 100644 > --- a/security/selinux/include/classmap.h > +++ b/security/selinux/include/classmap.h > @@ -22,9 +22,9 @@ > "audit_control", "setfcap" > > #define COMMON_CAP2_PERMS "mac_override", "mac_admin", "syslog", \ > - "wake_alarm", "block_suspend", "audit_read" > + "wake_alarm", "block_suspend", "audit_read", "cgroup" > > -#if CAP_LAST_CAP > CAP_AUDIT_READ > +#if CAP_LAST_CAP > CAP_CGROUP > #error New capability defined, please update COMMON_CAP2_PERMS. > #endif > >
Hello, On Sat, Dec 17, 2016 at 10:06:51PM +0100, Mickaël Salaün wrote: > If I understand correctly, this patch is intended to add a delegation > feature to cgroup v1, which does not really make sense for the v2 It's more about upstreaming a workaround for android somewhat like including binder into kernel. It isn't adding actual cgroup delegation to v1. It's just splitting a small piece of CAP_SYS_ADMIN to accomodate what android has been doing. > because of the clean cgroup-v2 delegation design. However, this new > capability impact both versions. In the same way but it's not about cgroup delegation. It's just allowing splitting up CAP_SYS_ADMIN so that "no extra restrictions on cgroup" can be given away in a safer way. > However, even if a cgroup does not directly involve a limitation, it may > be used to identify a group of processes for a security critical purpose > (e.g. kill a group of process). It can then make sense to have a > dedicated capability CAP_CGROUP to allow a process *without the right to > write in cgroup.procs* to be allowed to move a process out of its > current cgroup. This is similar to CAP_DAC_OVERRIDE but only for > cgroup/controllers files (but not necessarily sufficient to modify all > cgroups). This does not means that CAP_CGROUP should allow to move any > process from any cgroup. The cgroup_procs_write_permission() should > compose the checks for CAP_CGROUP and/or CAP_SYS_RESOURCE and/or > CAP_SYS_ADMIN depending on the current use of the cgroup (i.e. cgroup > controller, BPF program type, netfilter). There's no reason to invent a whole new set of security policies for cgroup. It already got one which follows the filesystem permissions with some extra restrictions. The CAP split is purely to accomodate android and that's it. If that isn't good enough a reason, then android should just keep carrying the patches it needs. This doesn't justify bolting on another permission model on cgroup in any way. Thanks.
diff --git a/include/uapi/linux/capability.h b/include/uapi/linux/capability.h index 49bc062..726f767 100644 --- a/include/uapi/linux/capability.h +++ b/include/uapi/linux/capability.h @@ -349,8 +349,11 @@ struct vfs_cap_data { #define CAP_AUDIT_READ 37 +/* Allow migration of other tasks between cgroups */ -#define CAP_LAST_CAP CAP_AUDIT_READ +#define CAP_CGROUP 38 + +#define CAP_LAST_CAP CAP_CGROUP #define cap_valid(x) ((x) >= 0 && (x) <= CAP_LAST_CAP) diff --git a/kernel/cgroup.c b/kernel/cgroup.c index 2ee9ec3..8b42ae3 100644 --- a/kernel/cgroup.c +++ b/kernel/cgroup.c @@ -2856,7 +2856,8 @@ static int cgroup_procs_write_permission(struct task_struct *task, */ if (!uid_eq(cred->euid, GLOBAL_ROOT_UID) && !uid_eq(cred->euid, tcred->uid) && - !uid_eq(cred->euid, tcred->suid)) + !uid_eq(cred->euid, tcred->suid) && + !ns_capable(tcred->user_ns, CAP_CGROUP)) ret = -EACCES; if (!ret && cgroup_on_dfl(dst_cgrp)) { diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h index e2d4ad3a..ee8c1ed 100644 --- a/security/selinux/include/classmap.h +++ b/security/selinux/include/classmap.h @@ -22,9 +22,9 @@ "audit_control", "setfcap" #define COMMON_CAP2_PERMS "mac_override", "mac_admin", "syslog", \ - "wake_alarm", "block_suspend", "audit_read" + "wake_alarm", "block_suspend", "audit_read", "cgroup" -#if CAP_LAST_CAP > CAP_AUDIT_READ +#if CAP_LAST_CAP > CAP_CGROUP #error New capability defined, please update COMMON_CAP2_PERMS. #endif
This patch adds CAP_GROUP and logic to allows a process to migrate other tasks between cgroups. In Android (where this feature originated), the ActivityManager tracks various application states (TOP_APP, FOREGROUND, BACKGROUND, SYSTEM, etc), and then as applications change states, the SchedPolicy logic will migrate the application tasks between different cgroups used to control the different application states (for example, there is a background cpuset cgroup which can limit background tasks to stay on one low-power cpu, and the bg_non_interactive cpuctrl cgroup can then further limit those background tasks to a small percentage of that one cpu's cpu time). However, for security reasons, Android doesn't want to make the system_server (the process that runs the ActivityManager and SchedPolicy logic), run as root. So in the Android common.git kernel, they have some logic to allow cgroups to loosen their permissions so CAP_SYS_NICE tasks can migrate other tasks between cgroups. I feel the approach taken there overloads CAP_SYS_NICE a bit much for non-android environments. Efforts to re-use CAP_SYS_RESOURCE for this purpose (which Android has since adopted) was also stymied by concerns about risks from future cgroups that could be considered "dangerous" by how they might change system semantics. So to avoid overlapping usage, this patch adds a brand new process capability flag (CAP_CGROUP), and uses it when checking if a task can migrate other tasks between cgroups. I've tested this with AOSP master (though its a bit hacked in as I still need to properly get the selinux userspace bits aware of the new capability bit) with selinux set to permissive and it seems to be working well. Thoughts and feedback would be appreciated! (Note, I'm going on holiday break after today, so I may not respond to feedback immediately, but I figured it would be better to give folks the chance to review this rather then sit it for two weeks. I'll resend after the new-year, addressing any feedback I do get.) Cc: Tejun Heo <tj@kernel.org> Cc: Li Zefan <lizefan@huawei.com> Cc: Jonathan Corbet <corbet@lwn.net> Cc: cgroups@vger.kernel.org Cc: Android Kernel Team <kernel-team@android.com> Cc: Rom Lemarchand <romlem@android.com> Cc: Colin Cross <ccross@android.com> Cc: Dmitry Shmidt <dimitrysh@google.com> Cc: Todd Kjos <tkjos@google.com> Cc: Christian Poetzsch <christian.potzsch@imgtec.com> Cc: Amit Pundir <amit.pundir@linaro.org> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> Cc: Kees Cook <keescook@chromium.org> Cc: Serge E. Hallyn <serge@hallyn.com> Cc: Andy Lutomirski <luto@amacapital.net> Cc: linux-api@vger.kernel.org Cc: Paul Moore <paul@paul-moore.com> Cc: Stephen Smalley <sds@tycho.nsa.gov> Cc: Eric Paris <eparis@parisplace.org> Cc: James Morris <james.l.morris@oracle.com> Cc: Jeff Vander Stoep <jeffv@google.com> Cc: linux-security-module@vger.kernel.org Cc: selinux@tycho.nsa.gov Signed-off-by: John Stultz <john.stultz@linaro.org> --- v2: Renamed to just CAP_CGROUP_MIGRATE as reccomended by Tejun v3: Switched to just using CAP_SYS_RESOURCE as suggested by Michael v4: Send out properly folded down version of the patch. :P v5: Switch back to CAP_CGROUP_MIGRATE due to concerns from Andy v6: Rename to CAP_CGROUP, as it might be used for other purposes in the future. Also added selinux mappings for the new cap. --- include/uapi/linux/capability.h | 5 ++++- kernel/cgroup.c | 3 ++- security/selinux/include/classmap.h | 4 ++-- 3 files changed, 8 insertions(+), 4 deletions(-)