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

[Casey Schaufler]

On 10/1/2021 12:50 PM, Jann Horn wrote:
> On Fri, Oct 1, 2021 at 9:36 PM Jann Horn <jannh@xxxxxxxxxx> wrote:
>> On Fri, Oct 1, 2021 at 8:46 PM Casey Schaufler <casey@xxxxxxxxxxxxxxxx> wrote:
>>> On 10/1/2021 10:55 AM, Todd Kjos 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.
>>> In the LSM stacking patch set I've been posting for a while
>>> (on version 29 now) I use information from the task structure
>>> to ensure that the security information passed via the binder
>>> interface is agreeable to both sides. Passing the cred will
>>> make it impossible to do this check. The task information
>>> required is not appropriate to have in the cred.
>> Why not? Why can't you put the security identity of the task into the creds?
> Ah, I get it now, you're concerned about different processes wanting
> to see security contexts formatted differently (e.g. printing the
> SELinux label vs printing the AppArmor label), right?

That is correct.

> But still, I don't think you can pull that information from the
> receiving task. Maybe the easiest solution would be to also store that
> in the creds? Or you'd have to manually grab that information when
> /dev/binder is opened.

I'm storing the information in the task security blob because that's
the appropriate scope. Today the LSM hook is given both task_struct's.
It's easy to compare to make sure the tasks are compatible. Adding the
information to the cred would be yet another case where the scope of
security information is wrong.