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

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?

SELinux already identifies tasks through their creds (see e.g.
task_sid_obj()), and doesn't use the task security blob at all.
Apparmor also identifies tasks through their creds (see
aa_current_raw_label() and __aa_task_raw_label()), and just uses the
task blob to store information about other labels that the process may
transition from or to.

From what I can tell, the only LSM that actually identifies the
caller's security context through the task security blob is Tomoyo. As
far as I know, that means Tomoyo is kinda broken. (But does anyone
even use Tomoyo?)

I understand that there are no users of the binder driver
other than SELinux in Android upstream today. That's not
the issue. Two processes on a system with SELinux and AppArmor
together may be required to provide incompatible results
from security_secid_to_secctx()/security_secctx_to_secid().
If it's impossible to detect this incompatibility it's
impossible to prevent serious confusion.

The LSM stacking isn't upstream yet. But I hope to have it
there Real Soon Now. If there's another way to fix this that
doesn't remove the task_struct it would avoid my having to
put it back.

You fundamentally can't identify the recipient of a binder transaction
through its task_struct, because the recipient might have given the
binder FD to a child process and executed a setuid binary since it
opened /dev/binder. If you look at the credentials of the task on the
other side, you'll just see the setuid binary that doesn't even know
it has an open binder FD, and won't see the child process that is
actually going to receive the transaction.

You can't even usefully identify the opener of a file through its
task_struct - especially with io_uring, any userspace process can
cause kernel threads to open files and perform I/O on them *on behalf
of userspace* - and this "on behalf of" relationship is only visible
in the overridden credentials. (And yes, I do think that means Tomoyo
doesn't work properly on systems with io_uring.)