Message-ID: <CAHC9VhSoecHCmuxCeiTvYxjRb+D=jw4TXyYs5P9hnXOjjGGDQQ@mail.gmail.com>
Date:   Thu, 30 Mar 2023 19:22:34 -0400
From:   Paul Moore <paul@...l-moore.com>
To:     Casey Schaufler <casey@...aufler-ca.com>
Cc:     linux-security-module@...r.kernel.org, jmorris@...ei.org,
        keescook@...omium.org, john.johansen@...onical.com,
        penguin-kernel@...ove.sakura.ne.jp, stephen.smalley.work@...il.com,
        linux-kernel@...r.kernel.org, linux-api@...r.kernel.org,
        mic@...ikod.net
Subject: Re: [PATCH v7 04/11] LSM: syscalls for current process attributes

On Thu, Mar 30, 2023 at 4:00 PM Casey Schaufler <casey@...aufler-ca.com> wrote:
> On 3/29/2023 6:12 PM, Paul Moore wrote:
> > On Wed, Mar 15, 2023 at 6:48 PM Casey Schaufler <casey@...aufler-ca.com> wrote:
> >> Create a system call lsm_get_self_attr() to provide the security
> >> module maintained attributes of the current process.
> >> Create a system call lsm_set_self_attr() to set a security
> >> module maintained attribute of the current process.
> >> Historically these attributes have been exposed to user space via
> >> entries in procfs under /proc/self/attr.
> >>
> >> The attribute value is provided in a lsm_ctx structure. The structure
> >> identifys the size of the attribute, and the attribute value. The format
> > "identifies"
> >
> >> of the attribute value is defined by the security module. A flags field
> >> is included for LSM specific information. It is currently unused and must
> >> be 0. The total size of the data, including the lsm_ctx structure and any
> >> padding, is maintained as well.
> >>
> >> struct lsm_ctx {
> >>         __u64   id;
> >>         __u64   flags;
> >>         __u64   len;
> >>         __u64   ctx_len;
> >>         __u8    ctx[];
> >> };
> >>
> >> Two new LSM hooks are used to interface with the LSMs.
> >> security_getselfattr() collects the lsm_ctx values from the
> >> LSMs that support the hook, accounting for space requirements.
> >> security_setselfattr() identifies which LSM the attribute is
> >> intended for and passes it along.
> >>
> >> Signed-off-by: Casey Schaufler <casey@...aufler-ca.com>
> >> ---
> >>  Documentation/userspace-api/lsm.rst | 15 +++++
> >>  include/linux/lsm_hook_defs.h       |  4 ++
> >>  include/linux/lsm_hooks.h           |  9 +++
> >>  include/linux/security.h            | 19 ++++++
> >>  include/linux/syscalls.h            |  5 ++
> >>  include/uapi/linux/lsm.h            | 33 ++++++++++
> >>  kernel/sys_ni.c                     |  4 ++
> >>  security/Makefile                   |  1 +
> >>  security/lsm_syscalls.c             | 55 ++++++++++++++++
> >>  security/security.c                 | 97 +++++++++++++++++++++++++++++
> >>  10 files changed, 242 insertions(+)
> >>  create mode 100644 security/lsm_syscalls.c

...

> >> +       int count = 0;
> >> +       int rc;
> >> +
> >> +       if (attr == 0)
> >> +               return -EINVAL;
> >> +       if (flags != 0)
> >> +               return -EINVAL;
> >> +       if (size == NULL)
> >> +               return -EINVAL;
> >> +       if (get_user(left, size))
> >> +               return -EFAULT;
> >> +
> >> +       hlist_for_each_entry(hp, &security_hook_heads.getselfattr, list) {
> >> +               this = left;
> >> +               if (base)
> >> +                       ctx = (struct lsm_ctx __user *)(base + total);
> > Pointer math on void pointers always makes me nervous.  Why not set
> > @base's type to a 'u8' just to remove any concerns?
>
> I can do that. I made it a void pointer to reflect the notion that
> the attributes aren't necessarily strings. Making it a u8 may suggest that
> the data is a string to some developers.

That's a fair concern, but there is plenty of precedence of binary
blobs being stored in 'unsigned char' arrays to make it easier to
pluck data out at random byte offsets.

> >> +               rc = hp->hook.getselfattr(attr, ctx, &this, flags);
> >> +               switch (rc) {
> >> +               case -EOPNOTSUPP:
> >> +                       rc = 0;
> >> +                       continue;
> >> +               case -E2BIG:
> >> +                       istoobig = true;
> >> +                       left = 0;
> >> +                       break;
> >> +               case 0:
> >> +                       left -= this;
> >> +                       break;
> >> +               default:
> >> +                       return rc;
> > I think the @getselfattr hook should behave similarly to the
> > associated syscall, returning a non-negative number should indicate
> > that @rc entries have been added to the @ctx array.  Right now all the
> > LSMs would just be adding one entry to the array, but we might as well
> > code this up to be flexible.
>
> Yes, some LSM may decide to have multiple "contexts".
>
> >> +               }
> >> +               total += this;
> >> +               count++;
> >> +       }
> >> +       if (count == 0)
> >> +               return LSM_RET_DEFAULT(getselfattr);
> >> +       if (put_user(total, size))
> >> +               return -EFAULT;
> >> +       if (rc)
> >> +               return rc;
> > Is the 'if (rc)' check needed here?  Shouldn't the switch-statement
> > after the hook catch everything that this check would catch?
>
> It's necessary because of BPF, which doesn't follow the LSM rules.

I thought if it made it this far in the function the LSM, BPF or not,
would still have gone through the switch statement above which would
have returned early if the the value was something other than one of
the accepted return codes ... right?

> >> +       if (istoobig)
> >> +               return -E2BIG;
> >> +       return count;
> >> +}
> >> +
> >> +/**
> >> + * security_setselfattr - Set an LSM attribute on the current process.
> >> + * @attr: which attribute to set
> >> + * @ctx: the user-space source for the information
> >> + * @size: the size of the data
> >> + * @flags: reserved for future use, must be 0
> >> + *
> >> + * Set an LSM attribute for the current process. The LSM, attribute
> >> + * and new value are included in @ctx.
> >> + *
> >> + * Returns 0 on success, an LSM specific value on failure.
> >> + */
> >> +int security_setselfattr(unsigned int __user attr, struct lsm_ctx __user *ctx,
> >> +                        size_t __user size, u32 __user flags)
> >> +{
> >> +       struct security_hook_list *hp;
> >> +       struct lsm_ctx lctx;
> > Shouldn't we check @attr for valid values and return -EINVAL if bogus?
>
> Sure.
>
> >> +       if (flags != 0)
> >> +               return -EINVAL;
> >> +       if (size < sizeof(*ctx))
> >> +               return -EINVAL;
> > If we're only going to support on 'lsm_ctx' entry in this function we
> > should verify that the 'len' and 'ctx_len' fields are sane.  Although
> > more on this below ...
>
> The LSM is going to have to do its own version of sanity checking. Having
> sanity checking here as well seems excessive.

Yes, the LSM will probably need to do some checks, but we can safely
do the length checking here so we might as well do it simply so every
LSM doesn't have to duplicate the length checks.

> >> +       if (copy_from_user(&lctx, ctx, sizeof(*ctx)))
> >> +               return -EFAULT;
> >> +
> >> +       hlist_for_each_entry(hp, &security_hook_heads.setselfattr, list)
> >> +               if ((hp->lsmid->id) == lctx.id)
> >> +                       return hp->hook.setselfattr(attr, ctx, size, flags);
> > Can anyone think of any good reason why we shouldn't support setting
> > multiple LSMs in one call, similar to what we do with
> > security_getselfattr()?  It seems like it might be a nice thing to
> > have ...
>
> If you're setting the context for multiple LSMs ...

See my follow-up to my original reply sent earlier today.

-- 
paul-moore.com

Powered by blists - more mailing lists