| 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
| ||
|
Message-ID: <1507210621.27146.7.camel@tycho.nsa.gov>
Date: Thu, 05 Oct 2017 09:37:01 -0400
From: Stephen Smalley <sds@...ho.nsa.gov>
To: Chenbo Feng <chenbofeng.kernel@...il.com>, netdev@...r.kernel.org,
SELinux <Selinux@...ho.nsa.gov>,
linux-security-module@...r.kernel.org
Cc: Chenbo Feng <fengc@...gle.com>,
Alexei Starovoitov <alexei.starovoitov@...il.com>,
Daniel Borkmann <daniel@...earbox.net>,
Lorenzo Colitti <lorenzo@...gle.com>
Subject: Re: [PATCH net-next 4/4] selinux: bpf: Add addtional check for bpf
object file receive
On Wed, 2017-10-04 at 11:29 -0700, Chenbo Feng wrote:
> From: Chenbo Feng <fengc@...gle.com>
>
> Introduce a bpf object related check when sending and receiving files
> through unix domain socket as well as binder. It checks if the
> receiving
> process have privilege to read/write the bpf map or use the bpf
> program.
> This check is necessary because the bpf maps and programs are using a
> anonymous inode as their shared inode so the normal way of checking
> the
> files and sockets when passing between processes cannot work properly
> on
> eBPF object. This check only works when the BPF_SYSCALL is
> configured.
>
> Signed-off-by: Chenbo Feng <fengc@...gle.com>
> ---
> include/linux/bpf.h | 3 +++
> kernel/bpf/syscall.c | 4 ++--
> security/selinux/hooks.c | 57
> +++++++++++++++++++++++++++++++++++++++++++++++-
> 3 files changed, 61 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index d757ea3f2228..ac8428a36d56 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -250,6 +250,9 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog,
> const union bpf_attr *kattr,
> #ifdef CONFIG_BPF_SYSCALL
> DECLARE_PER_CPU(int, bpf_prog_active);
>
> +extern const struct file_operations bpf_map_fops;
> +extern const struct file_operations bpf_prog_fops;
> +
> #define BPF_PROG_TYPE(_id, _ops) \
> extern const struct bpf_verifier_ops _ops;
> #define BPF_MAP_TYPE(_id, _ops) \
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 58ff769d58ab..5789a5359f0a 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -313,7 +313,7 @@ static ssize_t bpf_dummy_write(struct file *filp,
> const char __user *buf,
> return -EINVAL;
> }
>
> -static const struct file_operations bpf_map_fops = {
> +const struct file_operations bpf_map_fops = {
> #ifdef CONFIG_PROC_FS
> .show_fdinfo = bpf_map_show_fdinfo,
> #endif
> @@ -965,7 +965,7 @@ static void bpf_prog_show_fdinfo(struct seq_file
> *m, struct file *filp)
> }
> #endif
>
> -static const struct file_operations bpf_prog_fops = {
> +const struct file_operations bpf_prog_fops = {
> #ifdef CONFIG_PROC_FS
> .show_fdinfo = bpf_prog_show_fdinfo,
> #endif
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 41aba4e3d57c..381474ce3216 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -1847,6 +1847,7 @@ static int file_has_perm(const struct cred
> *cred,
>
> /* av is zero if only checking access to the descriptor. */
> rc = 0;
> +
> if (av)
> rc = inode_has_perm(cred, inode, av, &ad);
>
> @@ -2142,6 +2143,10 @@ static int
> selinux_binder_transfer_binder(struct task_struct *from,
> NULL);
> }
>
> +#ifdef CONFIG_BPF_SYSCALL
> +static int bpf_fd_pass(struct file *file, u32 sid);
> +#endif
> +
> static int selinux_binder_transfer_file(struct task_struct *from,
> struct task_struct *to,
> struct file *file)
> @@ -2165,6 +2170,12 @@ static int selinux_binder_transfer_file(struct
> task_struct *from,
> return rc;
> }
>
> +#ifdef CONFIG_BPF_SYSCALL
> + rc = bpf_fd_pass(file, sid);
> + if (rc)
> + return rc;
> +#endif
> +
> if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
> return 0;
>
> @@ -3735,8 +3746,18 @@ static int selinux_file_send_sigiotask(struct
> task_struct *tsk,
> static int selinux_file_receive(struct file *file)
> {
> const struct cred *cred = current_cred();
> + int rc;
> +
> + rc = file_has_perm(cred, file, file_to_av(file));
> + if (rc)
> + goto out;
> +
> +#ifdef CONFIG_BPF_SYSCALL
> + rc = bpf_fd_pass(file, cred_sid(sid));
> +#endif
>
> - return file_has_perm(cred, file, file_to_av(file));
> +out:
> + return rc;
> }
>
> static int selinux_file_open(struct file *file, const struct cred
> *cred)
> @@ -6288,6 +6309,40 @@ static u32 bpf_map_fmode_to_av(fmode_t fmode)
> return av;
> }
>
> +/* This function will check the file pass through unix socket or
> binder to see
> + * if it is a bpf related object. And apply correspinding checks on
> the bpf
> + * object based on the type. The bpf maps and programs, not like
> other files and
> + * socket, are using a shared anonymous inode inside the kernel as
> their inode.
> + * So checking that inode cannot identify if the process have
> privilege to
> + * access the bpf object and that's why we have to add this
> additional check in
> + * selinux_file_receive and selinux_binder_transfer_files.
> + */
> +static int bpf_fd_pass(struct file *file, u32 sid)
> +{
> + struct bpf_security_struct *bpfsec;
> + u32 sid = cred_sid(cred);
> + struct bpf_prog *prog;
> + struct bpf_map *map;
> + int ret;
> +
> + if (file->f_op == &bpf_map_fops) {
> + map = file->private_data;
> + bpfsec = map->security;
> + ret = avc_has_perm(sid, bpfsec->sid,
> SECCLASS_BPF_MAP,
> + bpf_map_fmode_to_av(file-
> >f_mode), NULL);
> + if (ret)
> + return ret;
> + } else if (file->f_op == &bpf_prog_fops) {
> + prog = file->private_data;
> + bpfsec = prog->aux->security;
> + ret = avc_has_perm(sid, bpfsec->sid,
> SECCLASS_BPF_PROG,
> + BPF_PROG__USE, NULL);
> + if (ret)
> + return ret;
> + }
> + return 0;
> +}
When the struct file is allocated for the bpf map and/or prog, you
could call a hook at that time passing both, and note the fact that it
is a bpf map/prog in the file_security_struct. Then, on
file_receive/binder_transfer_file, you could apply the appropriate
checking. Further, if we know that the file is always allocated at the
same point as the bpf map/prog, then they should have the same SID (i.e
fsec->sid should be the same as bpfsec->sid), so we shouldn't even need
to dereference the bpf map/prog. Unless I'm missing something.
Also, are we concerned about doing the same in
flush_unauthorized_files(), for inheriting descriptors across a
context-changing execve? Should this checking actually go into
file_has_perm() itself so it is always applied on any use of the struct
file?
Lastly, do we need/want these checks if sid == bpfsec->sid? We skip
FD__USE in the case where sid == fsec->sid, for example.
> +
> static int selinux_bpf_map(struct bpf_map *map, fmode_t fmode)
> {
> u32 sid = current_sid();
Powered by blists - more mailing lists