Message-ID: <CAMOXUJn_vbzNJcdf6rnj=PPBFQUe=sNwt2AgCa1ai_1hboc8_Q@mail.gmail.com>
Date:   Fri, 6 Oct 2017 14:10:11 -0700
From:   Chenbo Feng <fengc@...gle.com>
To:     Stephen Smalley <sds@...ho.nsa.gov>
Cc:     Chenbo Feng <chenbofeng.kernel@...il.com>, netdev@...r.kernel.org,
        SELinux <Selinux@...ho.nsa.gov>,
        linux-security-module@...r.kernel.org,
        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 Thu, Oct 5, 2017 at 11:26 AM, Stephen Smalley <sds@...ho.nsa.gov> wrote:
> On Thu, 2017-10-05 at 09:37 -0400, Stephen Smalley wrote:
>> 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.
>
> BTW, the prog use check seems slightly redundant in that we will
> already check fd use permission.  So we only really need it if you want
> to allow fd use but deny prog use.  The map read/write checks are more
> granular than fd use, so I guess we can't get rid of those.
>
But it seems fd use doesn't check what object is behind that fd. So if
we don't want a process to run the eBPF program, then we still need a
additional check for it. Maybe use prog_run instead of prog_use should
be more appropriate.
>>
>> > +
>> >  static int selinux_bpf_map(struct bpf_map *map, fmode_t fmode)
>> >  {
>> >     u32 sid = current_sid();

Powered by blists - more mailing lists