Re: Why does devices cgroup check for CAP_SYS_ADMIN explicitly?

[ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org (Eric W. Biederman)]

Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> writes:

> Hey, Eric.
>
> On Tue, Nov 06, 2012 at 07:34:07AM -0800, Eric W. Biederman wrote:
>> Having thought about this a little more I can give a definitive answer.
>> 
>> Adding a process to the device control group is equivalent to calling
>> mknod, as it allows that process to open device nodes, or equivalently
>> not open device nodes.  Therefore a capable check is absolutely
>> required.
>> 
>> Without a capability check it would be possible to remove access to
>> /dev/console for a suid root application keeping it from reporting
>> attempts to hack it for example.
>
> You understand that the whole thing is gated by VFS permission check,
> right?  I'm kinda lost what you're talking about.

mknod is gated by the vfs with a capability call.

open does not perform the CAP_MKNOD check.

Since the device cgroup prevents opening of device nodes adding
permission to access a new device node (update_access) is roughly
equivalent to mknod when the device cgroup does not exist.

To preserve the notion that only a privileged user can grant access to
device nodes we need a capability check.  Especially since the device
cgroup is designed to limit processes with uid == 0.

Without a capability check a process with CAP_DAC_OVERRIDE can go
shopping for a device control group that happens to have the device it
wants to use.

Similary without a capability check a process with CAP_DAD_OVERRIDE can
add or remove any device node into a device control group.

I don't see how the device control group can limit uid == 0 with the
device control group without making the operations require a capability
you don't give to ever user who has uid == 0.

>> The generic cgroup check in attach_task_by_pid to see if you can move
>> another process into a cgroup needs to be a capability call and not a
>> test for uid == 0.
>> 
>> static int attach_task_by_pid(struct cgroup *cgrp, u64 pid, bool threadgroup)
>> {
>> 	if (pid) {
>> 		tsk = find_task_by_vpid(pid);
>> 
>> 		/*
>> 		 * even if we're attaching all tasks in the thread group, we
>> 		 * only need to check permissions on one of them.
>> 		 */
>> 		tcred = __task_cred(tsk);
>> 		if (!uid_eq(cred->euid, GLOBAL_ROOT_UID) &&
>>                                         ^^^^^^^^^^^^^^^
>> 		    !uid_eq(cred->euid, tcred->uid) &&
>> 		    !uid_eq(cred->euid, tcred->suid)) {
>> 			rcu_read_unlock();
>> 			ret = -EACCES;
>> 			goto out_unlock_cgroup;
>
> This one isn't gated by VFS so we need to add CAP check to this
> function.  No?

correct.  The check should read something like:

 		tcred = __task_cred(tsk);
 		if (!uid_eq(cred->euid, tcred->uid) &&
 		    !uid_eq(cred->euid, tcred->suid) &&
                    !capable(CAP_SYS_ADMIN)) {
 			rcu_read_unlock();
 			ret = -EACCES;
 			goto out_unlock_cgroup;

Eric