linux-kernel - Re: [PATCH v1] cgroup,bpf: Add access check for cgroup_get_from

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <57E1698F.1090106@digikod.net>
Date:   Tue, 20 Sep 2016 18:53:35 +0200
From:   Mickaël Salaün <mic@...ikod.net>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     linux-kernel@...r.kernel.org, Alexei Starovoitov <ast@...nel.org>,
        Andy Lutomirski <luto@...capital.net>,
        Daniel Borkmann <daniel@...earbox.net>,
        Daniel Mack <daniel@...que.org>,
        "David S . Miller" <davem@...emloft.net>,
        James Morris <james.l.morris@...cle.com>,
        Kees Cook <keescook@...omium.org>,
        Martin KaFai Lau <kafai@...com>, Tejun Heo <tj@...nel.org>,
        cgroups@...r.kernel.org
Subject: Re: [PATCH v1] cgroup,bpf: Add access check for cgroup_get_from_fd()

On 20/09/2016 02:30, Alexei Starovoitov wrote:
> On Tue, Sep 20, 2016 at 12:49:13AM +0200, Mickaël Salaün wrote:
>> Add security access check for cgroup backed FD. The "cgroup.procs" file
>> of the corresponding cgroup should be readable to identify the cgroup,
>> and writable to prove that the current process can manage this cgroup
>> (e.g. through delegation). This is similar to the check done by
>> cgroup_procs_write_permission().
>>
>> Fixes: 4ed8ec521ed5 ("cgroup: bpf: Add BPF_MAP_TYPE_CGROUP_ARRAY")
> 
> I don't understand what 'fixes' is about.
> Looks like new feature or tightening?
> Since cgroup was opened by the process and it got an fd,
> it had an access, so extra check here looks unnecessary.

It may not be a "fix", but this patch tighten the access control. The
current cgroup_get_from_fd() only rely on the access check done on the
passed FD. However, this FD come from a cgroup directory, not a
"cgroup.procs" (in this directory). The "cgroup.procs" is used for
cgroup delegation by cgroup_procs_write_permission(). Checking
"cgroup.procs" is then more consistent with access checks done by other
part of the cgroup code. Being able to open a cgroup directory only
means that the current process is able to list the cgroup hierarchy, not
necessarily to list the tasks in this cgroups.

A BPF_MAP_TYPE_CGROUP_ARRAY should then only contains cgroups readable
by the process that filled the map. It is currently possible to call
bpf_skb_in_cgroup() and know if a packet come from a task in a cgroup,
whereas the loading process may not be able to list this tasks.

Write access to a cgroup directory means to be able to create
sub-cgroups, not to add or remove tasks from that cgroup. This will be
important for future use like the Daniel Mack's patch (attach an eBPF
program to a cgroup). Indeed, with the current code, a process with
CAP_NET_ADMIN (but without the right to manage a cgroup) would be able
to attach programs to a cgroup. Similar thing goes for Landlock.

> 
>> -struct cgroup *cgroup_get_from_fd(int fd)
>> +struct cgroup *cgroup_get_from_fd(int fd, int access_mask)
>>  {
>>  	struct cgroup_subsys_state *css;
>>  	struct cgroup *cgrp;
>>  	struct file *f;
>> +	struct inode *inode;
>> +	int ret;
>>  
>>  	f = fget_raw(fd);
>>  	if (!f)
>>  		return ERR_PTR(-EBADF);
>>  
>>  	css = css_tryget_online_from_dir(f->f_path.dentry, NULL);
>> -	fput(f);
> 
> why move it down?

Because it is used by kernfs_get_inode().

> 
>> -	if (IS_ERR(css))
>> -		return ERR_CAST(css);
>> +	if (IS_ERR(css)) {
>> +		ret = PTR_ERR(css);
>> +		goto put_f;
>> +	}
>>  
>>  	cgrp = css->cgroup;
>>  	if (!cgroup_on_dfl(cgrp)) {
>> -		cgroup_put(cgrp);
>> -		return ERR_PTR(-EBADF);
>> +		ret = -EBADF;
>> +		goto put_cgrp;
>> +	}
>> +
>> +	ret = -ENOMEM;
>> +	inode = kernfs_get_inode(f->f_path.dentry->d_sb, cgrp->procs_file.kn);
>> +	if (inode) {
>> +		ret = inode_permission(inode, access_mask);
>> +		iput(inode);
>>  	}
>> +	if (ret)
>> +		goto put_cgrp;
>>  
>> +	fput(f);
>>  	return cgrp;
>> +
>> +put_cgrp:
>> +	cgroup_put(cgrp);
>> +put_f:
>> +	fput(f);
>> +	return ERR_PTR(ret);
>>  }
>>  EXPORT_SYMBOL_GPL(cgroup_get_from_fd);
>>  
>> -- 
>> 2.9.3
>>
> 

Download attachment "signature.asc" of type "application/pgp-signature" (456 bytes)