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
