linux-kernel - Re: [PATCH v2 4/4] security: binder: Add binder object flags to selinux_binder_transfer

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHC9VhQtbTWXXy8mNxNDdukaAp6YB4CX5Xa6dvSZ_r4DhM2EXg@mail.gmail.com>
Date:   Mon, 23 Jan 2023 16:36:32 -0500
From:   Paul Moore <paul@...l-moore.com>
To:     "T.J. Mercier" <tjmercier@...gle.com>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Arve Hjønnevåg <arve@...roid.com>,
        Todd Kjos <tkjos@...roid.com>,
        Martijn Coenen <maco@...roid.com>,
        Joel Fernandes <joel@...lfernandes.org>,
        Christian Brauner <brauner@...nel.org>,
        Carlos Llamas <cmllamas@...gle.com>,
        Suren Baghdasaryan <surenb@...gle.com>,
        James Morris <jmorris@...ei.org>,
        "Serge E. Hallyn" <serge@...lyn.com>,
        Stephen Smalley <stephen.smalley.work@...il.com>,
        Eric Paris <eparis@...isplace.org>, hannes@...xchg.org,
        daniel.vetter@...ll.ch, android-mm@...gle.com, jstultz@...gle.com,
        jeffv@...gle.com, linux-security-module@...r.kernel.org,
        selinux@...r.kernel.org, cgroups@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 4/4] security: binder: Add binder object flags to selinux_binder_transfer_file

On Mon, Jan 23, 2023 at 2:18 PM T.J. Mercier <tjmercier@...gle.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@...gle.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?

-- 
paul-moore.com