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

[Stephen Smalley <stephen.smalley.work@gmail.com>]

On Tue, Oct 5, 2021 at 12:49 PM Todd Kjos <tkjos@google.com> wrote:
>
> On Tue, Oct 5, 2021 at 8:21 AM Stephen Smalley
> <stephen.smalley.work@gmail.com> wrote:
> >
> > On Mon, Oct 4, 2021 at 8:27 PM Jann Horn <jannh@google.com> wrote:
> > >
> > > On Tue, Oct 5, 2021 at 1:38 AM Casey Schaufler <casey@schaufler-ca.com> wrote:
> > > > On 10/4/2021 3:28 PM, Jann Horn wrote:
> > > > > On Mon, Oct 4, 2021 at 6:19 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> > > > >> On 10/1/2021 3:58 PM, Jann Horn wrote:
> > > > >>> On Fri, Oct 1, 2021 at 10:10 PM Casey Schaufler <casey@schaufler-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@google.com> wrote:
> > > > >>>>>> On Fri, Oct 1, 2021 at 8:46 PM Casey Schaufler <casey@schaufler-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?
> > > > > Yeah, basically
> > > >
> > > > Well, hot biscuits and gravy!
> > > >
> > > > >  - 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.
> > > >
> > > > You're counting on the peculiarities of the SELinux policy you're
> > > > assuming is used to mask the fact that the hook isn't really doing
> > > > what it is supposed to?  Ouch.
> > >
> > > I'm not saying I like the current situation - I do think that this
> > > needs to change. I'm just saying it probably isn't *exploitable*, and
> > > exploitability often hinges on these little circumstantial details.
> > >
> > > > >> 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.
> > > >
> > > > OK. I can understand that without endorsing it.
> > > >
> > > > > 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.
> > > >
> > > > A mechanism sufficiently entrenched.
> > >
> > > It's not just "entrenched", it is a fundamental requirement for being
> > > able to use file descriptor passing with syscalls like write(). If
> > > task A gives a file descriptor to task B, then task B must be able to
> > > write() to that FD without having to worry that the FD actually refers
> > > to some sort of special file that interprets the written data as some
> > > type of command, or something like that, and that this leads to task B
> > > unknowingly passing through access checks.
> > >
> > > > > 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.
> > > >
> > > > OK. I'm curious now as to why the task_struct was being passed to the
> > > > hook in the first place.
> > >
> > > Probably because that's what most other LSM hooks looked like and the
> > > authors/reviewers of the patch didn't realize that this model doesn't
> > > really work for binder? FWIW, these hooks were added in commit
> > > 79af73079d75 ("Add security hooks to binder and implement the hooks
> > > for SELinux."). The commit message also just talks about "processes".
> >
> > Note that in the same code path (binder_transaction), sender_euid is
> > set from proc->tsk and security_ctx is based on proc->tsk. If we are
> > changing the hooks to operate on the opener cred, then presumably we
> > should be doing that for sender_euid and replace the
> > security_task_getsecid_obj() call with security_cred_getsecid()?
>
> Stephan, do you want that to be included in this patch? Or should I
> follow this up with another patch for the sender_euid case?

Either way is fine with me. Fixing sender_euid arguably is a fix that
should go all the way back to the introduction of binder unless I
misunderstand; it is independent of SELinux. Fixing the
security_task_getsecid -> cred_secid only goes back to
ec74136ded792deed80780a2f8baf3521eeb72f9. So having it as 3 separate
patches may help with the different Fixes tags and back-porting
purposes.