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? 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.)
