| Message ID | 20221013223654.659758-1-keescook@chromium.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | integrity: Move hooks into LSM | expand |
On 14/10/2022 00:36, Kees Cook wrote: > Move "integrity" LSM to the end of the Kconfig list and prepare for > having ima and evm LSM initialization called from the top-level > "integrity" LSM. > > Cc: Paul Moore <paul@paul-moore.com> > Cc: James Morris <jmorris@namei.org> > Cc: "Serge E. Hallyn" <serge@hallyn.com> > Cc: Mimi Zohar <zohar@linux.ibm.com> > Cc: Dmitry Kasatkin <dmitry.kasatkin@gmail.com> > Cc: "Mickaël Salaün" <mic@digikod.net> > Cc: linux-security-module@vger.kernel.org > Cc: linux-integrity@vger.kernel.org > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > security/Kconfig | 10 +++++----- > security/integrity/evm/evm_main.c | 4 ++++ > security/integrity/iint.c | 17 +++++++++++++---- > security/integrity/ima/ima_main.c | 4 ++++ > security/integrity/integrity.h | 6 ++++++ > 5 files changed, 32 insertions(+), 9 deletions(-) > > diff --git a/security/Kconfig b/security/Kconfig > index e6db09a779b7..d472e87a2fc4 100644 > --- a/security/Kconfig > +++ b/security/Kconfig > @@ -246,11 +246,11 @@ endchoice > > config LSM > string "Ordered list of enabled LSMs" > - default "landlock,lockdown,yama,loadpin,safesetid,integrity,smack,selinux,tomoyo,apparmor,bpf" if DEFAULT_SECURITY_SMACK > - default "landlock,lockdown,yama,loadpin,safesetid,integrity,apparmor,selinux,smack,tomoyo,bpf" if DEFAULT_SECURITY_APPARMOR > - default "landlock,lockdown,yama,loadpin,safesetid,integrity,tomoyo,bpf" if DEFAULT_SECURITY_TOMOYO > - default "landlock,lockdown,yama,loadpin,safesetid,integrity,bpf" if DEFAULT_SECURITY_DAC > - default "landlock,lockdown,yama,loadpin,safesetid,integrity,selinux,smack,tomoyo,apparmor,bpf" > + default "landlock,lockdown,yama,loadpin,safesetid,smack,selinux,tomoyo,apparmor,bpf,integrity" if DEFAULT_SECURITY_SMACK > + default "landlock,lockdown,yama,loadpin,safesetid,apparmor,selinux,smack,tomoyo,bpf,integrity" if DEFAULT_SECURITY_APPARMOR > + default "landlock,lockdown,yama,loadpin,safesetid,tomoyo,bpf,integrity" if DEFAULT_SECURITY_TOMOYO > + default "landlock,lockdown,yama,loadpin,safesetid,bpf,integrity" if DEFAULT_SECURITY_DAC > + default "landlock,lockdown,yama,loadpin,safesetid,selinux,smack,tomoyo,apparmor,bpf,integrity" This is not backward compatible, but can easily be fixed thanks to DEFINE_LSM().order Side node: I proposed an alternative to that but it was Nacked: https://lore.kernel.org/all/20210222150608.808146-1-mic@digikod.net/ > help > A comma-separated list of LSMs, in initialization order. > Any LSMs left off this list will be ignored. This can be > diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c > index 2e6fb6e2ffd2..1ef965089417 100644 > --- a/security/integrity/evm/evm_main.c > +++ b/security/integrity/evm/evm_main.c > @@ -904,3 +904,7 @@ static int __init init_evm(void) > } > > late_initcall(init_evm); > + > +void __init integrity_lsm_evm_init(void) > +{ > +} > diff --git a/security/integrity/iint.c b/security/integrity/iint.c > index 8638976f7990..4f322324449d 100644 > --- a/security/integrity/iint.c > +++ b/security/integrity/iint.c > @@ -18,7 +18,6 @@ > #include <linux/file.h> > #include <linux/uaccess.h> > #include <linux/security.h> > -#include <linux/lsm_hooks.h> > #include "integrity.h" > > static struct rb_root integrity_iint_tree = RB_ROOT; > @@ -172,19 +171,29 @@ static void init_once(void *foo) > mutex_init(&iint->mutex); > } > > -static int __init integrity_iintcache_init(void) > +void __init integrity_add_lsm_hooks(struct security_hook_list *hooks, > + int count) > +{ > + security_add_hooks(hooks, count, "integrity"); > +} > + > +static int __init integrity_lsm_init(void) > { > iint_cache = > kmem_cache_create("iint_cache", sizeof(struct integrity_iint_cache), > 0, SLAB_PANIC, init_once); > + > + integrity_lsm_ima_init(); > + integrity_lsm_evm_init(); > + > return 0; > } > + > DEFINE_LSM(integrity) = { > .name = "integrity", > - .init = integrity_iintcache_init, > + .init = integrity_lsm_init, For backward compatibility, there should be an ".order = LSM_ORDER_FIRST," here.
On Fri, Oct 14, 2022 at 04:40:01PM +0200, Mickaël Salaün wrote: > This is not backward compatible Why? Nothing will be running LSM hooks until init finishes, at which point the integrity inode cache will be allocated. And ima and evm don't start up until lateinit. >, but can easily be fixed thanks to > DEFINE_LSM().order That forces the LSM to be enabled, which may not be desired? > Side node: I proposed an alternative to that but it was Nacked: > https://lore.kernel.org/all/20210222150608.808146-1-mic@digikod.net/ Yeah, for the reasons pointed out -- that can't work. The point is to not have The Default LSM. I do think Casey's NAK was rather prickly, though. ;)
On 14/10/2022 19:59, Kees Cook wrote: > On Fri, Oct 14, 2022 at 04:40:01PM +0200, Mickaël Salaün wrote: >> This is not backward compatible > > Why? Nothing will be running LSM hooks until init finishes, at which > point the integrity inode cache will be allocated. And ima and evm don't > start up until lateinit. > >> , but can easily be fixed thanks to >> DEFINE_LSM().order > > That forces the LSM to be enabled, which may not be desired? This is not backward compatible because currently IMA is enabled independently of the "lsm=" cmdline, which means that for all installed systems using IMA and also with a custom "lsm=" cmdline, updating the kernel with this patch will (silently) disable IMA. Using ".order = LSM_ORDER_FIRST," should keep this behavior. BTW, I think we should set such order (but maybe rename it) for LSMs that do nothing unless configured (e.g. Yama, Landlock). > >> Side node: I proposed an alternative to that but it was Nacked: >> https://lore.kernel.org/all/20210222150608.808146-1-mic@digikod.net/ > > Yeah, for the reasons pointed out -- that can't work. The point is to > not have The Default LSM. I do think Casey's NAK was rather prickly, > though. ;) I don't agree, there is no "the default LSM", and this new behavior is under an LSM_AUTO configuration option.
On Mon, Oct 17, 2022 at 11:26:44AM +0200, Mickaël Salaün wrote: > > On 14/10/2022 19:59, Kees Cook wrote: > > On Fri, Oct 14, 2022 at 04:40:01PM +0200, Mickaël Salaün wrote: > > > This is not backward compatible > > > > Why? Nothing will be running LSM hooks until init finishes, at which > > point the integrity inode cache will be allocated. And ima and evm don't > > start up until lateinit. > > > > > , but can easily be fixed thanks to > > > DEFINE_LSM().order > > > > That forces the LSM to be enabled, which may not be desired? > > This is not backward compatible because currently IMA is enabled > independently of the "lsm=" cmdline, which means that for all installed > systems using IMA and also with a custom "lsm=" cmdline, updating the kernel > with this patch will (silently) disable IMA. Using ".order = > LSM_ORDER_FIRST," should keep this behavior. > > BTW, I think we should set such order (but maybe rename it) for LSMs that do > nothing unless configured (e.g. Yama, Landlock). Ah yeah, good point. the .enabled stuff will need to be correctly wired up. Anyway, it's a good starting point for the conversion, so I'm hoping it can be carried forward by someone who is not me. :) (Hint hint to the integrity folks...) > > > Side node: I proposed an alternative to that but it was Nacked: > > > https://lore.kernel.org/all/20210222150608.808146-1-mic@digikod.net/ > > > > Yeah, for the reasons pointed out -- that can't work. The point is to > > not have The Default LSM. I do think Casey's NAK was rather prickly, > > though. ;) > > I don't agree, there is no "the default LSM", and this new behavior is under > an LSM_AUTO configuration option. The "config it twice" aspect of the current situation is suboptimal, yes. Let me go comment on the old thread...
On Thu, 2022-10-13 at 15:36 -0700, Kees Cook wrote: > Move "integrity" LSM to the end of the Kconfig list and prepare for > having ima and evm LSM initialization called from the top-level > "integrity" LSM. The securityfs integrity directory and the "iint_cache" are shared IMA/EVM resources. Just because the "iint_cache" was on an LSM hook, it should never have been treated as an LSM on its own. IMA maintains and verifies file data integrity, while EVM maintains and verifies file metadata integrity. IMA and EVM may both be configured and enabled, or independently of each other. However, only if either IMA or EVM are configured and enabled, should the iint_cache be created. There is absolutely no need for an independent "integrity" LSM.
On Wed, Oct 19, 2022 at 10:34:08AM -0400, Mimi Zohar wrote: > On Thu, 2022-10-13 at 15:36 -0700, Kees Cook wrote: > > Move "integrity" LSM to the end of the Kconfig list and prepare for > > having ima and evm LSM initialization called from the top-level > > "integrity" LSM. > > The securityfs integrity directory and the "iint_cache" are shared > IMA/EVM resources. Just because the "iint_cache" was on an LSM hook, > it should never have been treated as an LSM on its own. IMA maintains > and verifies file data integrity, while EVM maintains and verifies file > metadata integrity. IMA and EVM may both be configured and enabled, or > independently of each other. However, only if either IMA or EVM are > configured and enabled, should the iint_cache be created. There is > absolutely no need for an independent "integrity" LSM. The purpose of this patch was to tie ima and evm into integrity, since the iint_cache is used by both. It's been true since 4.20 that using ima and evm requires that the LSM named "integrity" has been initialized. Since ima and evm have separate indicators for "am I active?" (much like apparmor, etc), it seemed sensible to make ima and evm part of the LSM named "integrity". Other solutions are totally fine! I do note, however, this patch needs to be tweaked for the case where CONFIG_IMA or CONFIG_EVM are not set.
On Mon, Oct 17, 2022 at 11:26:44AM +0200, Mickaël Salaün wrote: > > On 14/10/2022 19:59, Kees Cook wrote: > > On Fri, Oct 14, 2022 at 04:40:01PM +0200, Mickaël Salaün wrote: > > > This is not backward compatible > > > > Why? Nothing will be running LSM hooks until init finishes, at which > > point the integrity inode cache will be allocated. And ima and evm don't > > start up until lateinit. > > > > > , but can easily be fixed thanks to > > > DEFINE_LSM().order > > > > That forces the LSM to be enabled, which may not be desired? > > This is not backward compatible because currently IMA is enabled > independently of the "lsm=" cmdline, which means that for all installed > systems using IMA and also with a custom "lsm=" cmdline, updating the kernel > with this patch will (silently) disable IMA. Using ".order = > LSM_ORDER_FIRST," should keep this behavior. This isn't true. If "integrity" is removed from the lsm= line today, IMA will immediately panic: process_measurement(): integrity_inode_get(): if (!iint_cache) panic("%s: lsm=integrity required.\n", __func__); and before v5.12 (where the panic was added), it would immediately NULL deref. (And it took 3 years to even notice.)
On Wed, 2022-10-19 at 11:33 -0700, Kees Cook wrote: > On Mon, Oct 17, 2022 at 11:26:44AM +0200, Mickaël Salaün wrote: > > > > On 14/10/2022 19:59, Kees Cook wrote: > > > On Fri, Oct 14, 2022 at 04:40:01PM +0200, Mickaël Salaün wrote: > > > > This is not backward compatible > > > > > > Why? Nothing will be running LSM hooks until init finishes, at which > > > point the integrity inode cache will be allocated. And ima and evm don't > > > start up until lateinit. > > > > > > > , but can easily be fixed thanks to > > > > DEFINE_LSM().order > > > > > > That forces the LSM to be enabled, which may not be desired? > > > > This is not backward compatible because currently IMA is enabled > > independently of the "lsm=" cmdline, which means that for all installed > > systems using IMA and also with a custom "lsm=" cmdline, updating the kernel > > with this patch will (silently) disable IMA. Using ".order = > > LSM_ORDER_FIRST," should keep this behavior. > > This isn't true. If "integrity" is removed from the lsm= line today, IMA > will immediately panic: > > process_measurement(): > integrity_inode_get(): > if (!iint_cache) > panic("%s: lsm=integrity required.\n", __func__); > > and before v5.12 (where the panic was added), it would immediately NULL > deref. (And it took 3 years to even notice.) Most people were/are still using the "security=" boot command line option, not "lsm=". This previously wasn't a problem with "security=", but became a problem with "lsm=". I should have been aware of the change from "security=" to "lsm=", but unfortunately wasn't. It took me totally by surprise. All of sudden "integrity" went from being a common IMA/EVM resource to an LSM. The correct solution would have been to move it a different initcall. (It's not too late to fix it.)
On Wed, Oct 19, 2022 at 03:13:34PM -0400, Mimi Zohar wrote: > Most people were/are still using the "security=" boot command line > option, not "lsm=". This previously wasn't a problem with "security=", > but became a problem with "lsm=". How are people still using "security=" for IMA/EVM? It only interacts with LSMs marked with LSM_FLAG_LEGACY_MAJOR.
diff --git a/security/Kconfig b/security/Kconfig index e6db09a779b7..d472e87a2fc4 100644 --- a/security/Kconfig +++ b/security/Kconfig @@ -246,11 +246,11 @@ endchoice config LSM string "Ordered list of enabled LSMs" - default "landlock,lockdown,yama,loadpin,safesetid,integrity,smack,selinux,tomoyo,apparmor,bpf" if DEFAULT_SECURITY_SMACK - default "landlock,lockdown,yama,loadpin,safesetid,integrity,apparmor,selinux,smack,tomoyo,bpf" if DEFAULT_SECURITY_APPARMOR - default "landlock,lockdown,yama,loadpin,safesetid,integrity,tomoyo,bpf" if DEFAULT_SECURITY_TOMOYO - default "landlock,lockdown,yama,loadpin,safesetid,integrity,bpf" if DEFAULT_SECURITY_DAC - default "landlock,lockdown,yama,loadpin,safesetid,integrity,selinux,smack,tomoyo,apparmor,bpf" + default "landlock,lockdown,yama,loadpin,safesetid,smack,selinux,tomoyo,apparmor,bpf,integrity" if DEFAULT_SECURITY_SMACK + default "landlock,lockdown,yama,loadpin,safesetid,apparmor,selinux,smack,tomoyo,bpf,integrity" if DEFAULT_SECURITY_APPARMOR + default "landlock,lockdown,yama,loadpin,safesetid,tomoyo,bpf,integrity" if DEFAULT_SECURITY_TOMOYO + default "landlock,lockdown,yama,loadpin,safesetid,bpf,integrity" if DEFAULT_SECURITY_DAC + default "landlock,lockdown,yama,loadpin,safesetid,selinux,smack,tomoyo,apparmor,bpf,integrity" help A comma-separated list of LSMs, in initialization order. Any LSMs left off this list will be ignored. This can be diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c index 2e6fb6e2ffd2..1ef965089417 100644 --- a/security/integrity/evm/evm_main.c +++ b/security/integrity/evm/evm_main.c @@ -904,3 +904,7 @@ static int __init init_evm(void) } late_initcall(init_evm); + +void __init integrity_lsm_evm_init(void) +{ +} diff --git a/security/integrity/iint.c b/security/integrity/iint.c index 8638976f7990..4f322324449d 100644 --- a/security/integrity/iint.c +++ b/security/integrity/iint.c @@ -18,7 +18,6 @@ #include <linux/file.h> #include <linux/uaccess.h> #include <linux/security.h> -#include <linux/lsm_hooks.h> #include "integrity.h" static struct rb_root integrity_iint_tree = RB_ROOT; @@ -172,19 +171,29 @@ static void init_once(void *foo) mutex_init(&iint->mutex); } -static int __init integrity_iintcache_init(void) +void __init integrity_add_lsm_hooks(struct security_hook_list *hooks, + int count) +{ + security_add_hooks(hooks, count, "integrity"); +} + +static int __init integrity_lsm_init(void) { iint_cache = kmem_cache_create("iint_cache", sizeof(struct integrity_iint_cache), 0, SLAB_PANIC, init_once); + + integrity_lsm_ima_init(); + integrity_lsm_evm_init(); + return 0; } + DEFINE_LSM(integrity) = { .name = "integrity", - .init = integrity_iintcache_init, + .init = integrity_lsm_init, }; - /* * integrity_kernel_read - read data from the file * diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index 040b03ddc1c7..e617863af5ff 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -1076,3 +1076,7 @@ static int __init init_ima(void) } late_initcall(init_ima); /* Start IMA after the TPM is available */ + +void __init integrity_lsm_ima_init(void) +{ +} diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h index 7167a6e99bdc..3707349271c9 100644 --- a/security/integrity/integrity.h +++ b/security/integrity/integrity.h @@ -18,6 +18,7 @@ #include <crypto/hash.h> #include <linux/key.h> #include <linux/audit.h> +#include <linux/lsm_hooks.h> /* iint action cache flags */ #define IMA_MEASURE 0x00000001 @@ -191,6 +192,11 @@ extern struct dentry *integrity_dir; struct modsig; +void __init integrity_lsm_ima_init(void); +void __init integrity_lsm_evm_init(void); +void __init integrity_add_lsm_hooks(struct security_hook_list *hooks, + int count); + #ifdef CONFIG_INTEGRITY_SIGNATURE int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
Move "integrity" LSM to the end of the Kconfig list and prepare for having ima and evm LSM initialization called from the top-level "integrity" LSM. Cc: Paul Moore <paul@paul-moore.com> Cc: James Morris <jmorris@namei.org> Cc: "Serge E. Hallyn" <serge@hallyn.com> Cc: Mimi Zohar <zohar@linux.ibm.com> Cc: Dmitry Kasatkin <dmitry.kasatkin@gmail.com> Cc: "Mickaël Salaün" <mic@digikod.net> Cc: linux-security-module@vger.kernel.org Cc: linux-integrity@vger.kernel.org Signed-off-by: Kees Cook <keescook@chromium.org> --- security/Kconfig | 10 +++++----- security/integrity/evm/evm_main.c | 4 ++++ security/integrity/iint.c | 17 +++++++++++++---- security/integrity/ima/ima_main.c | 4 ++++ security/integrity/integrity.h | 6 ++++++ 5 files changed, 32 insertions(+), 9 deletions(-)