Message-ID: <7b67163a-9de1-313f-5b5a-8c720cef9b73@schaufler-ca.com>
Date:   Mon, 22 Feb 2021 12:31:36 -0800
From:   Casey Schaufler <casey@...aufler-ca.com>
To:     Mickaël Salaün <mic@...ikod.net>,
        James Morris <jmorris@...ei.org>,
        "Serge E . Hallyn" <serge@...lyn.com>
Cc:     Kees Cook <keescook@...omium.org>, linux-kernel@...r.kernel.org,
        linux-security-module@...r.kernel.org,
        Mickaël Salaün <mic@...ux.microsoft.com>,
        Casey Schaufler <casey@...aufler-ca.com>
Subject: Re: [PATCH v3 1/1] security: Add CONFIG_LSM_AUTO to handle default
 LSM stack ordering

On 2/22/2021 10:31 AM, Mickaël Salaün wrote:
> On 22/02/2021 17:51, Casey Schaufler wrote:
>> On 2/22/2021 7:06 AM, Mickaël Salaün wrote:
>>> From: Mickaël Salaün <mic@...ux.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?
> I didn't dig the detailed history of LSM stacking, but you are in Cc
> because I know that you know. I may have though that the main goal of
> the current LSM stacking implementation was to enable to stack existing
> LSMs, which works well with this CONFIG_LSM list, but doesn't work as
> well for new LSMs.

It works just fine for new LSMs if you treat them as significant
features which may have significant impact on the behavior of the
system.

>> Did you even consider the implications before sending
>> the patch?
> Yes, and it doesn't change much the current behavior without user
> interaction. However, it gives the choice to users to choose how they
> want their configuration to evolve.

Automatic inclusions of new LSMs would be counter to existing practice.
It won't work for "major" LSMs.


>> 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.
> This patch series doesn't change this behavior if the user doesn't want
> it to change.

Well, there's the question. If a distribution/system uses the new scheme
"users" are going to get new LSMs spontaniously. If they don't it's up to
the "user". Unsophisticated users won't want this, and the others don't
need it.

>> 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.
> What is the next phase of module stacking? What would be the consequences?

The next phase ( coming real soon now :) ) allows AppArmor and SELinux/Smack
at the same time. More generically, the number of interfaces that can't be
used by multiple LSMs are reduced.

> Systems will only get new LSMs if their configuration said so, either
> from Kconfig or from boot arguments. I think we should make easier to
> have a working, consistent and secure kernel configuration by default.
> If users want to have a non-default configuration, that's fine, fully
> supported, and they can do it.

That really only matters for distribution or product kernels.
Neither of those should use CONFIG_LSM_AUTO.


>> 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".
> This is indeed the intent of this patch, but this configuration is optional.

Anyone who should rationally chose this option doesn't need it.


>> It doesn't work for generic kernels.
> Why? 

Because you only ever get the first compiled in Major LSM, which
will always be SELinux. If you only ever get a particular LSM,
why compile in the others? OK, I'm ignoring boot options.

> Generic kernels can be configured with CONFIG_LSM or with
> CONFIG_LSM_AUTO. 

Sure, if they use SELinux. But why would they use CONFIG_LSM_AUTO?
They loss of control over system behavior is unreasonable for a
distribution or a product kernel.

> I agree that generic distros may want to not enable
> major LSMs such as SELinux, AppArmor, Smack and Tomoyo by default but
> support them in their generic kernel anyway to let users pick and
> configure an LSM thanks to the boot arguments, and that's totally fine.
> Moreover, distro maintainers will surely browse most of new options to
> identify if it is the best choice for their distro. The *default* choice
> (for LSMs enabled at boot) is in the hand of users configuring their
> kernel, and they are in the best position to choose if they want to
> follow new kernel options and their consequences (e.g. distro kernel
> maintainers, whose job is to follow kernel development), or to have an
> easier way to maintain an up-to-date kernel (e.g. sysadmins or
> hobbyists, who may not have so much time dedicated to follow kernel
> developments).

Users who configure their kernels don't need CONFIG_LSM_AUTO.
Users who don't configure their kernels shouldn't have it.


>> 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.
> Nothing is silently added to the user configuration with this patch. It
> is an optional (default) configuration, which I think makes more sense
> for users not expert in every kernel toggles.

That is exactly wrong. Users who are not expert on kernel configuration
should not get LSMs added to their configuration without their knowledge.

>>> 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@...aufler-ca.com>
>>> Cc: James Morris <jmorris@...ei.org>
>>> Cc: Kees Cook <keescook@...omium.org>
>>> Cc: Serge E. Hallyn <serge@...lyn.com>
>>> Signed-off-by: Mickaël Salaün <mic@...ux.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;

Powered by blists - more mailing lists