Date:   Fri, 1 Oct 2021 10:38:12 -0400
From:   Paul Moore <paul@...l-moore.com>
To:     Todd Kjos <tkjos@...gle.com>
Cc:     gregkh@...uxfoundation.org, arve@...roid.com, tkjos@...roid.com,
        maco@...roid.com, christian@...uner.io,
        James Morris <jmorris@...ei.org>,
        Serge Hallyn <serge@...lyn.com>,
        Stephen Smalley <stephen.smalley.work@...il.com>,
        Eric Paris <eparis@...isplace.org>, keescook@...omium.org,
        jannh@...gle.com, Jeffrey Vander Stoep <jeffv@...gle.com>,
        zohar@...ux.ibm.com, linux-security-module@...r.kernel.org,
        selinux@...r.kernel.org, devel@...verdev.osuosl.org,
        linux-kernel@...r.kernel.org, joel@...lfernandes.org,
        kernel-team@...roid.com, stable@...r.kernel.org
Subject: Re: [PATCH] binder: use cred instead of task for selinux checks

On Thu, Sep 30, 2021 at 10:45 PM Todd Kjos <tkjos@...gle.com> 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@...gle.com>
> Cc: stable@...r.kernel.org # 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?  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).

>         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.

> 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

Powered by blists - more mailing lists