Re: [PATCH v3 1/1] security: Add CONFIG_LSM_AUTO to handle default LSM stack ordering

[Casey Schaufler <casey@schaufler-ca.com>]

On 2/22/2021 7:06 AM, Mickaël Salaün wrote:
> From: Mickaël Salaün <mic@linux.microsoft.com>
>
> Add a new option CONFIG_LSM_AUTO to enable users to delegate default LSM
> stacking order to kernel developers.  This enable to keep a consistent
> order of enabled LSM when changing the LSM selection, especially when a
> new LSM is added to the kernel.

TL;DR - NAK

Do you think that we might have considered this when stacking was
introduced? Did you even consider the implications before sending
the patch? This only makes any sense if you want to compile in
AppArmor and/or Smack but always use SELinux. The existing Kconfig
model handles that perfectly well. Also, this will break when the
next phase of module stacking comes in, and all of a sudden
systems will automatically get AppArmor in addition to SELinux
or Smack.

I know that the CONFIG_LSM/lsm= mechanism is clumsy. But we spent
about a year discussing, proposing and implementing alternatives,
and if there's a better mechanism, we couldn't find it. Of course
we considered "just use the kernel order". It doesn't work for
generic kernels. I understand that adding a new LSM that you want
to be included by default is a tough problem. I also suggest
that silently adding an LSM to an existing configuration is likely
to violate the principle of least astonishment.

>
> CONFIG_LSM depends on !CONFIG_LSM_AUTO, which is backward compatible and
> gives the opportunity to users to select CONFIG_LSM_AUTO with a make
> oldconfig.
>
> CONFIG_LSM and CONFIG_LSM_AUTO depend on CONFIG_SECURITY, which makes
> sense because an LSM depends on the security framework.
>
> Cc: Casey Schaufler <casey@schaufler-ca.com>
> Cc: James Morris <jmorris@namei.org>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Serge E. Hallyn <serge@hallyn.com>
> Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com>
> Link: https://lore.kernel.org/r/20210222150608.808146-2-mic@digikod.net
> ---
>
> Changes since v2:
> * Revamp without virtual dependencies but a new option to automatically
>   enable all selected LSMs.
>
> Changes since v1:
> * Add CONFIG_SECURITY as a dependency of CONFIG_LSM.  This prevent an
>   error when building without any LSMs.
> ---
>  security/Kconfig    | 19 +++++++++++++++++++
>  security/security.c | 26 +++++++++++++++++++++++++-
>  2 files changed, 44 insertions(+), 1 deletion(-)
>
> diff --git a/security/Kconfig b/security/Kconfig
> index 7561f6f99f1d..fae083e9867d 100644
> --- a/security/Kconfig
> +++ b/security/Kconfig
> @@ -243,6 +243,7 @@ source "security/integrity/Kconfig"
>  
>  choice
>  	prompt "First legacy 'major LSM' to be initialized"
> +	depends on SECURITY
>  	default DEFAULT_SECURITY_SELINUX if SECURITY_SELINUX
>  	default DEFAULT_SECURITY_SMACK if SECURITY_SMACK
>  	default DEFAULT_SECURITY_TOMOYO if SECURITY_TOMOYO
> @@ -275,8 +276,26 @@ choice
>  
>  endchoice
>  
> +config LSM_AUTO
> +	bool "Automatically enable all selected LSMs at boot"
> +	depends on SECURITY
> +	default y
> +	help
> +	  This automatically configure the build-time selected LSMs to be
> +	  enabled at boot unless the "lsm=" parameter is provided.
> +
> +	  If this option is not selected, it will be required to configure and
> +	  maintained a static list of enabled LSMs that may become inconsistent
> +	  with future user configuration.  Indeed, this list will not be
> +	  automatically upgraded when selecting a new (future) LSM, e.g. with
> +	  make oldconfig.
> +
> +	  If you are unsure how to answer this question, answer Y.
> +
> +# This lists should be synchronized with LSM_ORDER defined in security/security.c .
>  config LSM
>  	string "Ordered list of enabled LSMs"
> +	depends on SECURITY && !LSM_AUTO
>  	default "lockdown,yama,loadpin,safesetid,integrity,smack,selinux,tomoyo,apparmor,bpf" if DEFAULT_SECURITY_SMACK
>  	default "lockdown,yama,loadpin,safesetid,integrity,apparmor,selinux,smack,tomoyo,bpf" if DEFAULT_SECURITY_APPARMOR
>  	default "lockdown,yama,loadpin,safesetid,integrity,tomoyo,bpf" if DEFAULT_SECURITY_TOMOYO
> diff --git a/security/security.c b/security/security.c
> index 401663b5b70e..defa1d2c40a3 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -82,7 +82,31 @@ static struct lsm_blob_sizes blob_sizes __lsm_ro_after_init;
>  static __initdata const char *chosen_lsm_order;
>  static __initdata const char *chosen_major_lsm;
>  
> -static __initconst const char * const builtin_lsm_order = CONFIG_LSM;
> +#ifdef CONFIG_LSM
> +#define LSM_ORDER	CONFIG_LSM
> +#else
> +
> +/*
> + * This lists should be synchronized with the default values of CONFIG_LSM
> + * defined in security/Kconfig .
> + */
> +#define LSM_ORDER_PRE	"lockdown,yama,loadpin,safesetid,integrity,"
> +
> +#if defined(CONFIG_DEFAULT_SECURITY_SMACK)
> +#define LSM_ORDER	LSM_ORDER_PRE "smack,selinux,tomoyo,apparmor,bpf"
> +#elif defined(CONFIG_DEFAULT_SECURITY_APPARMOR)
> +#define LSM_ORDER	LSM_ORDER_PRE "apparmor,selinux,smack,tomoyo,bpf"
> +#elif defined(CONFIG_DEFAULT_SECURITY_TOMOYO)
> +#define LSM_ORDER	LSM_ORDER_PRE "tomoyo,bpf"
> +#elif defined(CONFIG_DEFAULT_SECURITY_DAC)
> +#define LSM_ORDER	LSM_ORDER_PRE "bpf"
> +#else
> +#define LSM_ORDER	LSM_ORDER_PRE "selinux,smack,tomoyo,apparmor,bpf"
> +#endif
> +
> +#endif /* CONFIG_LSM */
> +
> +static __initconst const char * const builtin_lsm_order = LSM_ORDER;
>  
>  /* Ordered list of LSMs to initialize. */
>  static __initdata struct lsm_info **ordered_lsms;