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

[Casey Schaufler]

On 10/5/2021 7:27 PM, Jann Horn wrote:
> On Tue, Oct 5, 2021 at 6:59 PM Casey Schaufler <casey@xxxxxxxxxxxxxxxx> wrote:
>> On 10/5/2021 8:21 AM, Stephen Smalley wrote:
>>> On Mon, Oct 4, 2021 at 8:27 PM Jann Horn <jannh@xxxxxxxxxx> wrote:
>>>> On Tue, Oct 5, 2021 at 1:38 AM Casey Schaufler <casey@xxxxxxxxxxxxxxxx> 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.