Message-ID: <CAHC9VhRMUZvXxVezp+1AsHRiesW_wKcU+nzdMj2+nKDasphEpg@mail.gmail.com>
Date:   Wed, 9 Nov 2022 22:02:11 -0500
From:   Paul Moore <paul@...l-moore.com>
To:     Casey Schaufler <casey@...aufler-ca.com>
Cc:     casey.schaufler@...el.com, 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 v1 6/8] LSM: lsm_self_attr syscall for LSM self attributes

On Wed, Nov 9, 2022 at 8:32 PM Casey Schaufler <casey@...aufler-ca.com> wrote:
> On 11/9/2022 3:34 PM, Paul Moore wrote:
> > On Tue, Oct 25, 2022 at 2:48 PM Casey Schaufler <casey@...aufler-ca.com> wrote:
> >> Create a system call lsm_self_attr() to provide the security
> >> module maintained attributes of the current process. Historically
> >> these attributes have been exposed to user space via entries in
> >> procfs under /proc/self/attr.
> >>
> >> Attributes are provided as a collection of lsm_ctx structures
> >> which are placed into a user supplied buffer. Each structure
> >> identifys the security module providing the attribute, which
> >> of the possible attributes is provided, the size of the
> >> attribute, and finally the attribute value. The format of the
> >> attribute value is defined by the security module, but will
> >> always be \0 terminated. The ctx_len value will be larger than
> >> strlen(ctx).
> >>
> >>         ------------------------------
> >>         | unsigned int id            |
> >>         ------------------------------
> >>         | unsigned int flags         |
> >>         ------------------------------
> >>         | __kernel_size_t ctx_len    |
> >>         ------------------------------
> >>         | unsigned char ctx[ctx_len] |
> >>         ------------------------------
> >>         | unsigned int id            |
> >>         ------------------------------
> >>         | unsigned int flags         |
> >>         ------------------------------
> >>         | __kernel_size_t ctx_len    |
> >>         ------------------------------
> >>         | unsigned char ctx[ctx_len] |
> >>         ------------------------------
> >>
> >> Signed-off-by: Casey Schaufler <casey@...aufler-ca.com>
> >> ---
> >>  include/linux/syscalls.h |   2 +
> >>  include/uapi/linux/lsm.h |  21 ++++++
> >>  kernel/sys_ni.c          |   3 +
> >>  security/Makefile        |   1 +
> >>  security/lsm_syscalls.c  | 156 +++++++++++++++++++++++++++++++++++++++
> >>  5 files changed, 183 insertions(+)
> >>  create mode 100644 security/lsm_syscalls.c
> > ..
> >
> >> diff --git a/include/uapi/linux/lsm.h b/include/uapi/linux/lsm.h
> >> index 61e13b1b9ece..1d27fb5b7746 100644
> >> --- a/include/uapi/linux/lsm.h
> >> +++ b/include/uapi/linux/lsm.h
> >> @@ -9,6 +9,27 @@
> >>  #ifndef _UAPI_LINUX_LSM_H
> >>  #define _UAPI_LINUX_LSM_H
> >>
> >> +#include <linux/types.h>
> >> +#include <linux/unistd.h>
> >> +
> >> +/**
> >> + * struct lsm_ctx - LSM context
> >> + * @id: the LSM id number, see LSM_ID_XXX
> >> + * @flags: context specifier and LSM specific flags
> >> + * @ctx_len: the size of @ctx
> >> + * @ctx: the LSM context, a nul terminated string
> >> + *
> >> + * @ctx in a nul terminated string.
> >> + *     (strlen(@ctx) < @ctx_len) is always true.
> >> + *     (strlen(@ctx) == @ctx_len + 1) is not guaranteed.
> >> + */
> > We can do better than this, or rather we *should* do better than this.
> > One of the big advantages of creating a new API is we can fix some of
> > the silly things we have had to do in the past, including the
> > "sometimes" NUL terminator on strings.  For this LSM syscalls let's
> > make a promise that all human readable strings will be properly NUL
> > terminated;
>
> It requires effort and buffer management to ensure that ctx_len == strlen(ctx)+1,
> but if you think it's important, sure.

I do, yes.  A safe, familiar, and consistent API is worth a little
extra work.  Ensuring the human readable strings are always nul
terminated is familiar to most everyone who has sat in from of a code
editor, and making sure that the @ctx_len variable indicates the full
length of the @ctx buffer (both for strings and binary blobs) provides
a consistent way to manage the context, even if the application isn't
aware of the exact LSM-specific format.

> >  currently this includes all LSM contexts, and likely will
> > remain that way forever for various reasons, but let's leave the door
> > open for arbitrary blobs (see the "special" LSM ID discussion from
> > earlier in the patchset).  With that in mind I might suggest the
> > following:
> >
> >   /**
> >    * struct lsm_ctx - LSM context
> >    * @id: the LSM id number, see LSM_ID_XXX
> >    * @flags: context specifier and LSM specific flags
> >    * @ctx_len: the size of @ctx
> >    * @ctx: the LSM context, see description
> >    *
> >    * For LSMs which provide human readable contexts @ctx will be a nul
> >    * terminated string and @ctx_len will include the size of the string
> >    * and the nul terminator, e.g. 'ctx_len == strlen(ctx) + 1'.  For LSMs
> >    * which provide binary-only contexts @cts will be a binary blob with
> >    * @ctx_len being the exact value of the blob.  The type of the @ctx,
> >    * human readable string or binary, can be determined by inspecting
> >    * both @id and @flags.
>
> I'd go a touch further, defining LSM_ATTR_BINARY as a flag and demanding
> that any attribute that isn't a nul terminated string be thus identified,
> even if it is always binary. Thus, LSM_ATTR_CURRENT and LSM_ATTR_BINARY
> together would be an error.

No, the class/format of the context (string or binary, and the LSM
specific formatting for each) can be deduced from the LSM ID, @id, and
if necessary the @flags field.  I don't want this API to explicitly
prevent a binary LSM_ATTR_CURRENT if the rest of the system is
modified to support it in the future.

> >    *
> >    * If a given LSM @id does not define a set of values for use in the
> >    * @flags field, @flags MUST be set to zero.
> >    */
> >   struct lsm_ctx {
> >     __u32 id;
> >     __U32 flags;
> >     __kernel_size_t ctx_len;
> >     __u8 ctx[];
> >   };
> >
> >> +struct lsm_ctx {
> >> +       unsigned int            id;
> >> +       unsigned int            flags;
> >> +       __kernel_size_t         ctx_len;
> >> +       unsigned char           ctx[];
> >> +};
> > I agree with Greg, we should be explicit about variable sizing, let's
> > make sure everything in the UAPI header is defined in terms of
> > __uXX/__sXX.  This includes strings as __u8 arrays.
> >
> > Also, I sorta despite the 'let's line all the struct fields up
> > horizontally!' approach in struct/variable definitions.
>
> Kids. Got no respect for tradition.

I think you meant to say, "Kids.          Got no          respect
 for      tradition."

> >   I personally
> > think it looks horrible and it clutters up future patches.  Please
> > don't do this unless the individual file already does it, and since we
> > are creating this new please don't :)
> >
> >> diff --git a/security/lsm_syscalls.c b/security/lsm_syscalls.c
> >> new file mode 100644
> >> index 000000000000..da0fab7065e2
> >> --- /dev/null
> >> +++ b/security/lsm_syscalls.c
> >> @@ -0,0 +1,156 @@
> >> +// SPDX-License-Identifier: GPL-2.0-only
> >> +/*
> >> + * System calls implementing the Linux Security Module API.
> >> + *
> >> + *  Copyright (C) 2022 Casey Schaufler <casey@...aufler-ca.com>
> >> + *  Copyright (C) Intel Corporation
> >> + */
> >> +
> >> +#include <asm/current.h>
> >> +#include <linux/compiler_types.h>
> >> +#include <linux/err.h>
> >> +#include <linux/errno.h>
> >> +#include <linux/security.h>
> >> +#include <linux/stddef.h>
> >> +#include <linux/syscalls.h>
> >> +#include <linux/types.h>
> >> +#include <linux/lsm_hooks.h>
> >> +#include <uapi/linux/lsm.h>
> >> +
> >> +struct feature_map {
> >> +       char *name;
> >> +       int feature;
> >> +};
> >> +
> >> +static const struct feature_map lsm_attr_names[] = {
> >> +       { .name = "current",    .feature = LSM_ATTR_CURRENT, },
> >> +       { .name = "exec",       .feature = LSM_ATTR_EXEC, },
> >> +       { .name = "fscreate",   .feature = LSM_ATTR_FSCREATE, },
> >> +       { .name = "keycreate",  .feature = LSM_ATTR_KEYCREATE, },
> >> +       { .name = "prev",       .feature = LSM_ATTR_PREV, },
> >> +       { .name = "sockcreate", .feature = LSM_ATTR_SOCKCREATE, },
> >> +};
> >> +
> >> +/**
> >> + * lsm_self_attr - Return current task's security module attributes
> >> + * @ctx: the LSM contexts
> >> + * @size: size of @ctx, updated on return
> >> + * @flags: reserved for future use, must be zero
> >> + *
> >> + * Returns the calling task's LSM contexts. On success this
> >> + * function returns the number of @ctx array elements. This value
> >> + * may be zero if there are no LSM contexts assigned. If @size is
> >> + * insufficient to contain the return data -E2BIG is returned and
> >> + * @size is set to the minimum required size. In all other cases
> >> + * a negative value indicating the error is returned.
> >> + */
> >> +SYSCALL_DEFINE3(lsm_self_attr,
> >> +              struct lsm_ctx __user *, ctx,
> >> +              size_t __user *, size,
> >> +              int, flags)
> > See my comments above about UAPI types, let's change this to something
> > like this:
> >
> > [NOTE: I'm assuming it is safe to use __XXX types in syscall declarations?]
> >
> >   SYSCALL_DEFINE3(lsm_self_attr,
> >                  struct lsm_ctx __user *, ctx,
> >                  __kernel_size_t __user *, size,
> >                  __u32, flags)
> >
> >> +{
> >> +       struct lsm_ctx *final = NULL;
> >> +       struct lsm_ctx *interum;
> >> +       struct lsm_ctx *ip;
> >> +       void *curr;
> >> +       char **interum_ctx;
> >> +       char *cp;
> >> +       size_t total_size = 0;
> >> +       int count = 0;
> >> +       int attr;
> >> +       int len;
> >> +       int rc = 0;
> >> +       int i;
> > Ungh, reverse christmas trees ... I kinda hate it from a style
> > perspective, enough to mention it here, but I'm not going to be petty
> > enough to say "change it".
>
> I really don't care. Last I saw reverse christmas tree was the officially
> recommended style. I don't care one way or the other.

I think it is one of those per-subsystem oddities like it or not.

> >
> > However, if you did want to flip it upside down (normal christmas
> > tree?) during the respin I would be grateful ;)
> >

-- 
paul-moore.com

Powered by blists - more mailing lists