All of lore.kernel.org
 help / color / mirror / Atom feed
From: Paul Moore <paul@paul-moore.com>
To: Todd Kjos <tkjos@google.com>
Cc: gregkh@linuxfoundation.org, arve@android.com, tkjos@android.com,
	maco@android.com, christian@brauner.io,
	James Morris <jmorris@namei.org>, Serge Hallyn <serge@hallyn.com>,
	Stephen Smalley <stephen.smalley.work@gmail.com>,
	Eric Paris <eparis@parisplace.org>,
	keescook@chromium.org, jannh@google.com,
	Jeffrey Vander Stoep <jeffv@google.com>,
	zohar@linux.ibm.com, linux-security-module@vger.kernel.org,
	selinux@vger.kernel.org, devel@driverdev.osuosl.org,
	linux-kernel@vger.kernel.org, joel@joelfernandes.org,
	kernel-team@android.com, stable@vger.kernel.org
Subject: Re: [PATCH] binder: use cred instead of task for selinux checks
Date: Fri, 1 Oct 2021 10:38:12 -0400	[thread overview]
Message-ID: <CAHC9VhQ-uziaYRYWaah=RMmz7HUVvxGs+4F=g2sizVXR0ZSWVw@mail.gmail.com> (raw)
In-Reply-To: <20211001024506.3762647-1-tkjos@google.com>

On Thu, Sep 30, 2021 at 10:45 PM Todd Kjos <tkjos@google.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@google.com>
> Cc: stable@vger.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

  reply	other threads:[~2021-10-01 14:38 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-10-01  2:45 [PATCH] binder: use cred instead of task for selinux checks Todd Kjos
2021-10-01 14:38 ` Paul Moore [this message]
2021-10-01 14:44   ` Paul Moore
2021-10-01 17:14   ` Todd Kjos

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAHC9VhQ-uziaYRYWaah=RMmz7HUVvxGs+4F=g2sizVXR0ZSWVw@mail.gmail.com' \
    --to=paul@paul-moore.com \
    --cc=arve@android.com \
    --cc=christian@brauner.io \
    --cc=devel@driverdev.osuosl.org \
    --cc=eparis@parisplace.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=jannh@google.com \
    --cc=jeffv@google.com \
    --cc=jmorris@namei.org \
    --cc=joel@joelfernandes.org \
    --cc=keescook@chromium.org \
    --cc=kernel-team@android.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-security-module@vger.kernel.org \
    --cc=maco@android.com \
    --cc=selinux@vger.kernel.org \
    --cc=serge@hallyn.com \
    --cc=stable@vger.kernel.org \
    --cc=stephen.smalley.work@gmail.com \
    --cc=tkjos@android.com \
    --cc=tkjos@google.com \
    --cc=zohar@linux.ibm.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.