On Mon, Oct 4, 2021 at 6:19 PM Casey Schaufler [off-list ref] wrote:

On 10/1/2021 3:58 PM, Jann Horn wrote:

quoted

On Fri, Oct 1, 2021 at 10:10 PM Casey Schaufler [off-list ref] wrote:

quoted

On 10/1/2021 12:50 PM, Jann Horn wrote:

quoted

On Fri, Oct 1, 2021 at 9:36 PM Jann Horn [off-list ref] wrote:

quoted

On Fri, Oct 1, 2021 at 8:46 PM Casey Schaufler [off-list ref] wrote:

quoted

On 10/1/2021 10:55 AM, Todd Kjos wrote:

quoted

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.

quoted

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?

Yeah, basically - but luckily the actual impact this has is limited by
the transitions that SELinux permits. If domain1 has no way to
transition to domain2, then it can't abuse this bug to pretend to be
domain2. I do have a reproducer that lets Android's "shell" domain
send a binder transaction that appears to come from "runas", but
luckily "runas" has no interesting privileges with regards to binder,
so that's not exploitable.

How does passing the creds from the wrong tasks "fix" the problem?

This patch is not passing the creds from the "wrong" tasks at all. It
relies on the basic idea that when a security context opens a
resource, and then hands that resource to another context for
read/write operations, then you can effectively treat this as a
delegation of privileges from the original opener, and perform access
checks against the credentials using which the resource was opened.

In particular, we already have those semantics in the core kernel for
->read() and ->write() VFS operations - they are *not allowed* to look
at the credentials of the caller, and if they want to make security
checks, they have to instead check against file->f_cred, which are the
credentials using which the file was originally opened. (Yes, some
places still get that wrong.) Passing a file descriptor to another
task is a delegation of access, and the other task can then call
syscalls like read() / write() / mmap() on the file descriptor without
needing to have any access to the underlying file.

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.

quoted

quoted

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.

quoted

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.

But it is data that AFAICS needs to be preserved in the same places
where the creds need to be preserved, and it is also related to
security labels, so isn't "struct cred" a logical place to stuff it
anyway?