[PATCH v2 4/4] security: binder: Add binder object flags to selinux_binder_transfer_file

[T.J. Mercier]

On Mon, Jan 23, 2023 at 2:04 PM T.J. Mercier <tjmercier at google.com> wrote:
>
>
>
> On Mon, Jan 23, 2023 at 1:36 PM Paul Moore <paul at paul-moore.com> wrote:
>>
>> On Mon, Jan 23, 2023 at 2:18 PM T.J. Mercier <tjmercier at google.com> wrote:
>> >
>> > Any process can cause a memory charge transfer to occur to any other
>> > process when transmitting a file descriptor through binder. This should
>> > only be possible for central allocator processes, so the binder object
>> > flags are added to the security_binder_transfer_file hook so that LSMs
>> > can enforce restrictions on charge transfers.
>> >
>> > Signed-off-by: T.J. Mercier <tjmercier at google.com>
>> > ---
>> >  drivers/android/binder.c            |  2 +-
>> >  include/linux/lsm_hook_defs.h       |  2 +-
>> >  include/linux/lsm_hooks.h           |  5 ++++-
>> >  include/linux/security.h            |  6 ++++--
>> >  security/security.c                 |  4 ++--
>> >  security/selinux/hooks.c            | 13 ++++++++++++-
>> >  security/selinux/include/classmap.h |  2 +-
>> >  7 files changed, 25 insertions(+), 9 deletions(-)
>>
>> ...
>>
>> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>> > index 3c5be76a9199..d4cfca3c9a3b 100644
>> > --- a/security/selinux/hooks.c
>> > +++ b/security/selinux/hooks.c
>> > @@ -88,6 +88,7 @@
>> >  #include <linux/bpf.h>
>> >  #include <linux/kernfs.h>
>> >  #include <linux/stringhash.h>  /* for hashlen_string() */
>> > +#include <uapi/linux/android/binder.h>
>> >  #include <uapi/linux/mount.h>
>> >  #include <linux/fsnotify.h>
>> >  #include <linux/fanotify.h>
>> > @@ -2029,7 +2030,8 @@ static int selinux_binder_transfer_binder(const struct cred *from,
>> >
>> >  static int selinux_binder_transfer_file(const struct cred *from,
>> >                                         const struct cred *to,
>> > -                                       struct file *file)
>> > +                                       struct file *file,
>> > +                                       u32 binder_object_flags)
>> >  {
>> >         u32 sid = cred_sid(to);
>> >         struct file_security_struct *fsec = selinux_file(file);
>> > @@ -2038,6 +2040,15 @@ static int selinux_binder_transfer_file(const struct cred *from,
>> >         struct common_audit_data ad;
>> >         int rc;
>> >
>> > +       if (binder_object_flags & BINDER_FD_FLAG_XFER_CHARGE) {
>> > +               rc = avc_has_perm(&selinux_state,
>> > +                           cred_sid(from), sid,
>> > +                           SECCLASS_BINDER, BINDER__TRANSFER_CHARGE,
>> > +                           NULL);
>> > +               if (rc)
>> > +                       return rc;
>> > +       }
>> > +
>> >         ad.type = LSM_AUDIT_DATA_PATH;
>> >         ad.u.path = file->f_path;
>> >
>> > diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
>> > index a3c380775d41..2eef180d10d7 100644
>> > --- a/security/selinux/include/classmap.h
>> > +++ b/security/selinux/include/classmap.h
>> > @@ -172,7 +172,7 @@ const struct security_class_mapping secclass_map[] = {
>> >         { "tun_socket",
>> >           { COMMON_SOCK_PERMS, "attach_queue", NULL } },
>> >         { "binder", { "impersonate", "call", "set_context_mgr", "transfer",
>> > -                     NULL } },
>> > +                     "transfer_charge", NULL } },
>> >         { "cap_userns",
>> >           { COMMON_CAP_PERMS, NULL } },
>> >         { "cap2_userns",
>>
>> My first take on reading these changes above is that you've completely
>> ignored my previous comments about SELinux access controls around
>> resource management.  You've leveraged the existing LSM/SELinux hook
>> as we discussed previously, that's good, but can you explain what
>> changes you've made to address my concerns about one-off resource
>> management controls?
>>
> It's been a couple of weeks since v1 so I've sent this update out now to incorporate all the other feedback so far to make sure it's headed in the right direction. I've tried opening up a discussion about this rather unique case, but there's been no activity on that yet.
>
Someone pointed out this didn't make it to the lists. Retrying.

>> --
>> paul-moore.com