Re: [PATCH] binder: use cred instead of task for selinux checks

[Todd Kjos]

On Fri, Oct 1, 2021 at 7:38 AM Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
>
> On Thu, Sep 30, 2021 at 10:45 PM Todd Kjos <tkjos@xxxxxxxxxx> wrote:
> >
> > Save the struct cred associated with a binder process
> > at initial open to avoid potential race conditions
> > when converting to a security ID.
> >
> > Since binder was integrated with selinux, it has passed
> > 'struct task_struct' associated with the binder_proc
> > to represent the source and target of transactions.
> > The conversion of task to SID was then done in the hook
> > implementations. It turns out that there are race conditions
> > which can result in an incorrect security context being used.
> >
> > Fix by saving the 'struct cred' during binder_open and pass
> > it to the selinux subsystem.
> >
> > Fixes: 79af73079d75 ("Add security hooks to binder and implement the
> > hooks for SELinux.")
> > Signed-off-by: Todd Kjos <tkjos@xxxxxxxxxx>
> > Cc: stable@xxxxxxxxxxxxxxx # 5.14 (need backport for earlier stables)
> > ---
> >  drivers/android/binder.c          | 14 +++++----
> >  drivers/android/binder_internal.h |  3 ++
> >  include/linux/lsm_hook_defs.h     | 14 ++++-----
> >  include/linux/security.h          | 28 +++++++++---------
> >  security/security.c               | 14 ++++-----
> >  security/selinux/hooks.c          | 48 +++++++++----------------------
> >  6 files changed, 52 insertions(+), 69 deletions(-)
>
> Thanks Todd, I'm happy to see someone with a better understanding of
> binder than me pitch in to clean this up :)  A couple of quick
> comments/questions below ...
>
> > diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> > index 9edacc8b9768..ca599ebdea4a 100644
> > --- a/drivers/android/binder.c
> > +++ b/drivers/android/binder.c
> > @@ -5055,6 +5056,7 @@ static int binder_open(struct inode *nodp, struct file *filp)
> >         spin_lock_init(&proc->outer_lock);
> >         get_task_struct(current->group_leader);
> >         proc->tsk = current->group_leader;
> > +       proc->cred = get_cred(filp->f_cred);
>
> Is it *always* true that filp->f_cred is going to be the same as
> current->group_leader->cred?

Not necessarily -- it is current->cred of the task in binder_open()
(not group_leader). This is fine. We used to set proc->tsk to current,
but switched to group_leader a few years ago to make it easier to
detect the same process with multiple opens during mmap (to solve some
unrelated issues). We still use group_leader for that purpose, but for
the cred, the current cred in binder_open() is sufficient.

> Or rather does this help resolve the
> issue of wanting the subjective creds but not being able to access
> them mentioned in the task_sid_binder() comment?  If the latter, it
> might be nice to add something to the related comment in struct
> binder_ref (below).

Yes, we want the subjective cred so that is part of the point. I
started with "proc->cred = get_task_cred(current->group_leader)" and
got feedback that the "subjective" cred is preferred to avoid some
subtle races that could be introduced, for example, if /dev/binder is
opened through io_uring.

>
> >         INIT_LIST_HEAD(&proc->todo);
> >         init_waitqueue_head(&proc->freeze_wait);
> >         proc->default_priority = task_nice(current);
> > diff --git a/drivers/android/binder_internal.h b/drivers/android/binder_internal.h
> > index 402c4d4362a8..886fc327a534 100644
> > --- a/drivers/android/binder_internal.h
> > +++ b/drivers/android/binder_internal.h
> > @@ -364,6 +364,8 @@ struct binder_ref {
> >   *                        (invariant after initialized)
> >   * @tsk                   task_struct for group_leader of process
> >   *                        (invariant after initialized)
> > + * @cred                  struct cred for group_leader of process
> > + *                        (invariant after initialized)
>
> Related to the question above.  At the very least the comment should
> probably be changed to indicate to make it clear the creds are coming
> directly from the binder file/device and not always the group_leader.

Good catch. Will update the comment (it's actually struct binder_proc).

>
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index e7ebd45ca345..c8bf3db90c8b 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -255,29 +255,6 @@ static inline u32 task_sid_obj(const struct task_struct *task)
> >         return sid;
> >  }
> >
> > -/*
> > - * get the security ID of a task for use with binder
> > - */
> > -static inline u32 task_sid_binder(const struct task_struct *task)
> > -{
> > -       /*
> > -        * In many case where this function is used we should be using the
> > -        * task's subjective SID, but we can't reliably access the subjective
> > -        * creds of a task other than our own so we must use the objective
> > -        * creds/SID, which are safe to access.  The downside is that if a task
> > -        * is temporarily overriding it's creds it will not be reflected here;
> > -        * however, it isn't clear that binder would handle that case well
> > -        * anyway.
> > -        *
> > -        * If this ever changes and we can safely reference the subjective
> > -        * creds/SID of another task, this function will make it easier to
> > -        * identify the various places where we make use of the task SIDs in
> > -        * the binder code.  It is also likely that we will need to adjust
> > -        * the main drivers/android binder code as well.
> > -        */
> > -       return task_sid_obj(task);
> > -}
>
> --
> paul moore
> www.paul-moore.com

and from your next response:

> Ooops, I was a little over zealous when trimming my response and I
> accidentally cut off my comment that the associated comment blocks in
> include/linux/lsm_hooks.h should also be updated to reflect the binder
> LSM hook changes.

Thanks for pointing this out! I didn't notice these comment blocks.

-Todd