Re: [PATCH v6 04/11] LSM: syscalls for current process attributes

[Paul Moore]

On Wed, Mar 8, 2023 at 9:30 PM Casey Schaufler <casey@xxxxxxxxxxxxxxxx> wrote:
> On 3/7/2023 3:51 AM, Mickaël Salaün wrote:
> >
> > On 22/02/2023 21:08, Casey Schaufler 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
> >> 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@xxxxxxxxxxxxxxxx>
> >> ---
> >>   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            |   4 ++
> >>   include/uapi/linux/lsm.h            |  33 +++++++++
> >>   kernel/sys_ni.c                     |   4 ++
> >>   security/Makefile                   |   1 +
> >>   security/lsm_syscalls.c             | 104 ++++++++++++++++++++++++++++
> >>   security/security.c                 |  82 ++++++++++++++++++++++
> >>   10 files changed, 275 insertions(+)
> >>   create mode 100644 security/lsm_syscalls.c
> >>
> >
> > [...]
> >
> >> +/**
> >> + * security_setselfattr - Set an LSM attribute on the current process.
> >> + * @attr: which attribute to return
> >> + * @ctx: the user-space source for the information
> >> + * @size: the size of the data
> >> + *
> >> + * Set an LSM attribute for the current process. The LSM, attribute
> >> + * and new value are included in @ctx.
> >> + *
> >> + * Returns 0 on seccess, an LSM specific value on failure.
> >> + */
> >> +int security_setselfattr(u64 __user attr, struct lsm_ctx __user *ctx,
> >> +             size_t __user size)
> >> +{
> >> +    struct security_hook_list *hp;
> >> +    struct lsm_ctx lctx;
> >> +
> >> +    if (size < sizeof(*ctx))
> >
> > If the lsm_ctx struct could grow in the future, we should check the
> > size of the struct to the last field for compatibility reasons, see
> > Landlock's copy_min_struct_from_user().
>
> Because the lsm_ctx structure ends with the variable length context there's
> no way to append new fields to it. The structure can't grow.

The lsm_ctx can grow; that was one of the reasons for having both a
@len and @ctx_len field in the struct, the other being padding.  Of
course any LSM wanting to place information beyond the end of @ctx
will need to indicate that with a bit in the @flags field.

Having said that, there are probably other ways to pass other data via
a lsm_ctx struct, e.g. binary @ctx values, but I don't think we want
to rule anything out at this point.

Also, as a reminder, just because we *can* do something, doesn't mean
we will do something.  Any LSM that wants to pass something other than
a string @ctx value will face a *lot* of scrutiny.

-- 
paul-moore.com