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 for Android: free password hash cracker in your pocket

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

Message-ID: <e0c1fab9-cb97-d5af-1f4b-f15b6b2097fd@schaufler-ca.com>
Date:   Mon, 4 Oct 2021 09:19:02 -0700
From:   Casey Schaufler <casey@...aufler-ca.com>
To:     Jann Horn <jannh@...gle.com>
Cc:     Todd Kjos <tkjos@...gle.com>, gregkh@...uxfoundation.org,
        arve@...roid.com, tkjos@...roid.com, maco@...roid.com,
        christian@...uner.io, jmorris@...ei.org, serge@...lyn.com,
        paul@...l-moore.com, stephen.smalley.work@...il.com,
        eparis@...isplace.org, keescook@...omium.org, jeffv@...gle.com,
        zohar@...ux.ibm.com, linux-security-module@...r.kernel.org,
        selinux@...r.kernel.org, devel@...verdev.osuosl.org,
        linux-kernel@...r.kernel.org, joel@...lfernandes.org,
        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/1/2021 3:58 PM, Jann Horn wrote:
> On Fri, Oct 1, 2021 at 10:10 PM Casey Schaufler <casey@...aufler-ca.com> wrote:
>> On 10/1/2021 12:50 PM, Jann Horn wrote:
>>> On Fri, Oct 1, 2021 at 9:36 PM Jann Horn <jannh@...gle.com> wrote:
>>>> On Fri, Oct 1, 2021 at 8:46 PM Casey Schaufler <casey@...aufler-ca.com> 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.
> Which is wrong, because you have no idea who the semantic "recipient
> task" is - any task that has a mapping of the binder fd can
> effectively receive transactions from it.
>
> (And the current "sender task" is also wrong, because binder looks at
> the task that opened the binder device, not the task currently
> performing the action.)

I'm confused. Are you saying that the existing binder code is
completely broken? Are you saying that neither "task" is correct?
How does passing the creds from the wrong tasks "fix" the problem?

>> It's easy to compare to make sure the tasks are compatible.
> It would be, if you actually had a pair of tasks that accurately
> represent the sender and the recipient.
>
>> Adding the
>> information to the cred would be yet another case where the scope of
>> security information is wrong.
> Can you elaborate on why you think that?

The information identifies how the task is going to display
the security "context". It isn't used in access checks.