Re: [PATCH v5 1/8] LSM: Identify modules by more than name

[Paul Moore]

On Wed, Jan 11, 2023 at 7:05 PM Casey Schaufler <casey@xxxxxxxxxxxxxxxx> wrote:
> On 1/11/2023 1:00 PM, Paul Moore wrote:
> > On Mon, Jan 9, 2023 at 1:07 PM Casey Schaufler <casey@xxxxxxxxxxxxxxxx> wrote:
> >> Create a struct lsm_id to contain identifying information
> >> about Linux Security Modules (LSMs). At inception this contains
> >> the name of the module, an identifier associated with the security
> >> module and an integer member "attrs_used" which identifies the API
> >> related data associated with each security module. The initial set
> >> of features maps to information that has traditionaly been available
> >> in /proc/self/attr. They are documented in a new userspace-api file.
> >> Change the security_add_hooks() interface to use this structure.
> >> Change the individual modules to maintain their own struct lsm_id
> >> and pass it to security_add_hooks().
> >>
> >> The values are for LSM identifiers are defined in a new UAPI
> >> header file linux/lsm.h. Each existing LSM has been updated to
> >> include it's LSMID in the lsm_id.
> >>
> >> The LSM ID values are sequential, with the oldest module
> >> LSM_ID_CAPABILITY being the lowest value and the existing modules
> >> numbered in the order they were included in the main line kernel.
> >> This is an arbitrary convention for assigning the values, but
> >> none better presents itself. The value 0 is defined as being invalid.
> >> The values 1-99 are reserved for any special case uses which may
> >> arise in the future. This may include attributes of the LSM
> >> infrastructure itself, possibly related to namespacing or network
> >> attribute management. A special range is identified for such attributes
> >> to help reduce confusion for developers unfamiliar with LSMs.
> >>
> >> Signed-off-by: Casey Schaufler <casey@xxxxxxxxxxxxxxxx>
> >> ---
> >>  Documentation/userspace-api/index.rst |  1 +
> >>  Documentation/userspace-api/lsm.rst   | 55 +++++++++++++++++++++++++++
> >>  include/linux/lsm_hooks.h             | 18 ++++++++-
> >>  include/uapi/linux/lsm.h              | 55 +++++++++++++++++++++++++++
> >>  security/apparmor/lsm.c               |  9 ++++-
> >>  security/bpf/hooks.c                  | 13 ++++++-
> >>  security/commoncap.c                  |  8 +++-
> >>  security/landlock/cred.c              |  2 +-
> >>  security/landlock/fs.c                |  2 +-
> >>  security/landlock/ptrace.c            |  2 +-
> >>  security/landlock/setup.c             |  6 +++
> >>  security/landlock/setup.h             |  1 +
> >>  security/loadpin/loadpin.c            |  9 ++++-
> >>  security/lockdown/lockdown.c          |  8 +++-
> >>  security/safesetid/lsm.c              |  9 ++++-
> >>  security/security.c                   | 12 +++---
> >>  security/selinux/hooks.c              | 11 +++++-
> >>  security/smack/smack_lsm.c            |  9 ++++-
> >>  security/tomoyo/tomoyo.c              |  9 ++++-
> >>  security/yama/yama_lsm.c              |  8 +++-
> >>  20 files changed, 226 insertions(+), 21 deletions(-)
> >>  create mode 100644 Documentation/userspace-api/lsm.rst
> >>  create mode 100644 include/uapi/linux/lsm.h

...

> >> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> >> index 0a5ba81f7367..6f2cabb79ec4 100644
> >> --- a/include/linux/lsm_hooks.h
> >> +++ b/include/linux/lsm_hooks.h
> >> @@ -1708,7 +1722,7 @@ extern struct security_hook_heads security_hook_heads;
> >>  extern char *lsm_names;
> >>
> >>  extern void security_add_hooks(struct security_hook_list *hooks, int count,
> >> -                               const char *lsm);
> >> +                              struct lsm_id *lsmid);
> > We should be able to mark @lsmid as a const, right?
>
> At this point, yes, but the const would have to come off when
> the "slot" field is added to lsm_id in the upcoming stacking patches.
> I can mark it const for now if it is important.

Ah, right.  Okay, just leave it as-is then.

> >> diff --git a/include/uapi/linux/lsm.h b/include/uapi/linux/lsm.h
> >> new file mode 100644
> >> index 000000000000..61a91b7d946f
> >> --- /dev/null
> >> +++ b/include/uapi/linux/lsm.h
> >> @@ -0,0 +1,55 @@
> >> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> >> +/*
> >> + * Linux Security Modules (LSM) - User space API
> >> + *
> >> + * Copyright (C) 2022 Casey Schaufler <casey@xxxxxxxxxxxxxxxx>
> >> + * Copyright (C) 2022 Intel Corporation
> >> + */
> > This file should be added to the SECURITY SUBSYSTEM section in MAINTAINERS:
> >
> >   F: include/uapi/linux/lsm.h
>
> S'truth.
>
> >> +#ifndef _UAPI_LINUX_LSM_H
> >> +#define _UAPI_LINUX_LSM_H
> >> +
> >> +/*
> >> + * ID values to identify security modules.
> >> + * A system may use more than one security module.
> >> + *
> >> + * A value of 0 is considered invalid.
> >> + * Values 1-99 are reserved for future use.
> >> + * The interface is designed to extend to attributes beyond those which
> >> + * are active today. Currently all the attributes are specific to the
> >> + * individual modules. The LSM infrastructure itself has no variable state,
> >> + * but that may change. One proposal would allow loadable modules, in which
> >> + * case an attribute such as LSM_IS_LOADABLE might identify the dynamic
> >> + * modules. Another potential attribute could be which security modules is
> >> + * associated withnetwork labeling using netlabel. Another possible attribute
> >> + * could be related to stacking behavior in a namespaced environment.
> >> + * While it would be possible to intermingle the LSM infrastructure attribute
> >> + * values with the security module provided values, keeping them separate
> >> + * provides a clearer distinction.
> >> + */
> > As this is in a UAPI file, let's avoid speculation and stick to just
> > the current facts.  Anything we write here with respect to the future
> > is likely to be wrong so let's not tempt fate.
>
> Sure. I'll leave the rationale to the take message.
>
> > Once I reached patch 3/8 I also realized that we may want to have more
> > than just 0/invalid as a sentinel value, or at the very least we need
> > to redefine 0 as something slightly different if for no other reason
> > than we in fs/proc/base.c.  I know it seems a little trivial, but
> > since we're talking about values that will be used in the UAPI I think
> > we need to give it some thought and discussion.  The only think I can
> > think of right now is to redefine 0 as "undefined", which isn't that
> > far removed from "invalid" and will not look too terrible in patch 3/8
> > - thoughts?
>
> I originally had LSM_ID_INVALID for 0, but there was an objection.
> It's not really invalid or undefined, it is reserved as not being an LSM id.
> How about LSM_ID_NALSMID or LSM_ID_NOTALSMID for 0? sort of like NAN for
> Not A Number.
>
> > With all that in mind, I would suggest something like this:
> >
> >   /*
> >    * ID tokens to identify Linux Security Modules (LSMs)
> >    *
> >    * These token values are used to uniquely identify specific LSMs
> >    * in the kernel as well in the kernel's LSM userspace API.
> >    *
> >    * A value of zero/0 is considered undefined and should not be used
> >    * outside of the kernel, values 1-99 are reserved for potential
> >    * future use.
> >       */
> >   #define LSM_ID_UNDEF 0
>
> Fine, although I'd go with LSM_ID_NALSMID

Hmm, I had to think a little on that to figure out the NALSMID bit.
Of the two I would prefer LSM_ID_UNDEF as UNDEF tends to be a bit more
common in the kernel and would be more likely to get people thinking
in the right direction.

> >> +#define LSM_ID_CAPABILITY      100
> >> +#define LSM_ID_SELINUX         101
> >> +#define LSM_ID_SMACK           102
> >> +#define LSM_ID_TOMOYO          103
> >> +#define LSM_ID_IMA             104
> >> +#define LSM_ID_APPARMOR                105
> >> +#define LSM_ID_YAMA            106
> >> +#define LSM_ID_LOADPIN         107
> >> +#define LSM_ID_SAFESETID       108
> >> +#define LSM_ID_LOCKDOWN                109
> >> +#define LSM_ID_BPF             110
> >> +#define LSM_ID_LANDLOCK                111
> >> +
> >> +/*
> >> + * LSM_ATTR_XXX values identify the /proc/.../attr entry that the
> >> + * context represents. Not all security modules provide all of these
> >> + * values. Some security modules provide none of them.
> >> + */
> > I'd rather see text closer to this:
> >
> >   /*
> >    * LSM attributes
> >    *
> >    * The LSM_ATTR_XXX definitions identify different LSM
> >    * attributes which are used in the kernel's LSM userspace API.
> >    * Support for these attributes vary across the different LSMs,
> >    * none are required.
> >    */
>
> If you like that better I'm completely willing to adopt it.

Please.

> >> +#define LSM_ATTR_CURRENT       0x0001
> >> +#define LSM_ATTR_EXEC          0x0002
> >> +#define LSM_ATTR_FSCREATE      0x0004
> >> +#define LSM_ATTR_KEYCREATE     0x0008
> >> +#define LSM_ATTR_PREV          0x0010
> >> +#define LSM_ATTR_SOCKCREATE    0x0020
> >> +
> >> +#endif /* _UAPI_LINUX_LSM_H */
> >> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> >> index c6728a629437..63ea2a995987 100644
> >> --- a/security/apparmor/lsm.c
> >> +++ b/security/apparmor/lsm.c
> >> @@ -24,6 +24,7 @@
> >>  #include <linux/zstd.h>
> >>  #include <net/sock.h>
> >>  #include <uapi/linux/mount.h>
> >> +#include <uapi/linux/lsm.h>
> >>
> >>  #include "include/apparmor.h"
> >>  #include "include/apparmorfs.h"
> >> @@ -1217,6 +1218,12 @@ struct lsm_blob_sizes apparmor_blob_sizes __lsm_ro_after_init = {
> >>         .lbs_task = sizeof(struct aa_task_ctx),
> >>  };
> >>
> >> +static struct lsm_id apparmor_lsmid __lsm_ro_after_init = {
> >> +       .lsm = "apparmor",
> >> +       .id = LSM_ID_APPARMOR,
> >> +       .attrs_used = LSM_ATTR_CURRENT | LSM_ATTR_PREV | LSM_ATTR_EXEC,
> >> +};
> > Perhaps mark this as const in addition to ro_after_init?  This applies
> > to all the other @lsm_id defs in this patch too.
>
> As I mentioned above, the lsm_id will eventually get changed during the
> registration process. I can add the const for now, knowing full well that
> it will be removed before long.

No, that's okay.  When I was reviewing this I forgot that changes
would be required here for stacking.

-- 
paul-moore.com