Date:   Wed, 6 Oct 2021 04:27:22 +0200
From:   Jann Horn <jannh@...gle.com>
To:     Casey Schaufler <casey@...aufler-ca.com>
Cc:     Stephen Smalley <stephen.smalley.work@...il.com>,
        Todd Kjos <tkjos@...gle.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        arve@...roid.com, tkjos@...roid.com, maco@...roid.com,
        christian@...uner.io, James Morris <jmorris@...ei.org>,
        "Serge E. Hallyn" <serge@...lyn.com>,
        Paul Moore <paul@...l-moore.com>,
        Eric Paris <eparis@...isplace.org>,
        Kees Cook <keescook@...omium.org>,
        Jeffrey Vander Stoep <jeffv@...gle.com>,
        Mimi Zohar <zohar@...ux.ibm.com>,
        LSM List <linux-security-module@...r.kernel.org>,
        SElinux list <selinux@...r.kernel.org>,
        devel@...verdev.osuosl.org,
        linux-kernel <linux-kernel@...r.kernel.org>,
        "Joel Fernandes (Google)" <joel@...lfernandes.org>,
        "Cc: Android Kernel" <kernel-team@...roid.com>,
        stable@...r.kernel.org
Subject: Re: [PATCH v2] binder: use cred instead of task for selinux checks

On Tue, Oct 5, 2021 at 6:59 PM Casey Schaufler <casey@...aufler-ca.com> wrote:
> On 10/5/2021 8:21 AM, Stephen Smalley wrote:
> > On Mon, Oct 4, 2021 at 8:27 PM Jann Horn <jannh@...gle.com> wrote:
> >> On Tue, Oct 5, 2021 at 1:38 AM Casey Schaufler <casey@...aufler-ca.com> wrote:
> >>> On 10/4/2021 3:28 PM, Jann Horn wrote:
> >>>> You can't really attribute binder transactions to specific tasks that
> >>>> are actually involved in the specific transaction, neither on the
> >>>> sending side nor on the receiving side, because binder is built around
> >>>> passing data through memory mappings. Memory mappings can be accessed
> >>>> by multiple tasks, and even a task that does not currently have it
> >>>> mapped could e.g. map it at a later time. And on top of that you have
> >>>> the problem that the receiving task might also go through privileged
> >>>> execve() transitions.
> >>> OK. I'm curious now as to why the task_struct was being passed to the
> >>> hook in the first place.
> >> Probably because that's what most other LSM hooks looked like and the
> >> authors/reviewers of the patch didn't realize that this model doesn't
> >> really work for binder? FWIW, these hooks were added in commit
> >> 79af73079d75 ("Add security hooks to binder and implement the hooks
> >> for SELinux."). The commit message also just talks about "processes".
> > Note that in the same code path (binder_transaction), sender_euid is
> > set from proc->tsk and security_ctx is based on proc->tsk. If we are
> > changing the hooks to operate on the opener cred, then presumably we
> > should be doing that for sender_euid and replace the
> > security_task_getsecid_obj() call with security_cred_getsecid()?
> >
> > NB Mandatory Access Control doesn't allow uncontrolled delegation,
> > hence typically checks against the subject credential either at
> > delegation/transfer or use or both. That's true in other places too,
> > e.g. file_permission, socket_sendmsg.
>
> Terrific. Now I'm even less convinced that either the proposed change
> or the existing code make sense. It's also disturbing that the change
> log claims that the reason for the change is fix a race condition when
> in fact it changes the data being sent to the hook completely.

The race it's referring to is the one between
security_binder_transaction() (which checks for permission to send a
transaction and checks for delegation) and
security_task_getsecid_obj() (which tells the recipient what the
sender's security context is). (It's a good thing Paul noticed that
the v1 patch didn't actually change the security_task_getsecid_obj()
call... somehow I missed that.)

Powered by blists - more mailing lists