linux-kernel - Re: [PATCH v2] binder: use cred instead of task for selinux checks

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <736d2fab-311b-f663-cc69-d1febf0d019b@schaufler-ca.com>
Date:   Wed, 6 Oct 2021 12:49:36 -0700
From:   Casey Schaufler <casey@...aufler-ca.com>
To:     Jann Horn <jannh@...gle.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, Casey Schaufler <casey@...aufler-ca.com>
Subject: Re: [PATCH v2] binder: use cred instead of task for selinux checks

On 10/5/2021 7:27 PM, Jann Horn wrote:
> 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.)

It appears that I'll be better off using some other mechanism for
the recipient to identify the security module used by the sender
than the arguments to security_binder_transaction(). It's likely to
be more invasive to the binder driver, but that's the way things go.
At any rate, I'm no longer concerned about either the data type or
the source of what goes into the hook.