I think that would be great have CONFIG_SECURITY_LANDLOCK enabled by default in gentoo-sources and wherever possible. We have it enabled in sys-kernel/gentoo-kernel-bin since 5.15 and there wasn't reports so far What do you think? References: https://landlock.io/ https://docs.kernel.org/userspace-api/landlock.html#kernel-support https://www.openwall.com/lists/oss-security/2022/08/17/1
Landlock is now part of the recommended KSPP settings: https://kernsec.org/wiki/index.php/Kernel_Self_Protection_Project/Recommended_Settings It would make sense to update CONFIG_GENTOO_KERNEL_SELF_PROTECTION accordingly. I guess CONFIG_LSM can be ignored and get the default value, which already includes Landlock.
(In reply to Mickaël Salaün from comment #1) > Landlock is now part of the recommended KSPP settings: > https://kernsec.org/wiki/index.php/Kernel_Self_Protection_Project/ > Recommended_Settings > > It would make sense to update CONFIG_GENTOO_KERNEL_SELF_PROTECTION > accordingly. I guess CONFIG_LSM can be ignored and get the default value, > which already includes Landlock. # Provide userspace with Landlock MAC interface. # Make sure that "landlock" is also present in the "CONFIG_LSM=landlock,..." list. CONFIG_SECURITY_LANDLOCK= landlock in CONFIG_LSM does not appear in any of my configs
Here is relevant part from /usr/src/linux/security/Kconfig if it helps: 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" help A comma-separated list of LSMs, in initialization order. Any LSMs left off this list will be ignored. This can be controlled at boot with the "lsm=" parameter. If unsure, leave this as the default. Problem could be that if user once changed this, there is no way back to defaults and "landlock" must inserted by user, if it could not set by our distro/Kconfig.
I agree on adding config security landlocker to the CONFIG_GENTOO_KERNEL_SELF_PROTECTION the only concern for me is about possible problems with LANDLOCK_ACCESS_FS_REFER on ABI update as wrote in the newsletter. https://lore.kernel.org/landlock/441bd1cd-03fd-8e30-c370-3d0f0263d564@digikod.net/
(In reply to Alice Ferrazzi from comment #4) > I agree on adding config security landlocker to the > CONFIG_GENTOO_KERNEL_SELF_PROTECTION > > the only concern for me is about possible problems with > LANDLOCK_ACCESS_FS_REFER on ABI update as wrote in the newsletter. > > https://lore.kernel.org/landlock/441bd1cd-03fd-8e30-c370- > 3d0f0263d564@digikod.net/ What is your concern? Landlock is an incremental development and will support new access rights over time. The Landlock ABI version bump is the normal way to inform user space that the running kernel supports Landlock features up to a specific set, but everything is backward compatible (as should be any kernel interface). Landlock is designed to make it easy for developers to follow a best-effort security approach in a least-surprising way.
(In reply to Peter from comment #3) > Here is relevant part from /usr/src/linux/security/Kconfig if it helps: > > 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" > help > A comma-separated list of LSMs, in initialization order. > Any LSMs left off this list will be ignored. This can be > controlled at boot with the "lsm=" parameter. > > If unsure, leave this as the default. > > > Problem could be that if user once changed this, there is no way back to > defaults and "landlock" must inserted by user, if it could not set by our > distro/Kconfig. Correct. Unfortunately we cannot do much about that, except warn users if their configuration has a modified CONFIG_LSM which doesn't include Landlock.
(In reply to Mickaël Salaün from comment #6) > (In reply to Peter from comment #3) > > Problem could be that if user once changed this, there is no way back to > > defaults and "landlock" must inserted by user, if it could not set by our > > distro/Kconfig. > > Correct. Unfortunately we cannot do much about that, except warn users if > their configuration has a modified CONFIG_LSM which doesn't include Landlock. Well, we could prepend "landlock,yama" by default like this: diff --git a/security/security.c b/security/security.c index 14d30fec8a00..b9696c0d198e 100644 --- a/security/security.c +++ b/security/security.c @@ -85,7 +85,11 @@ 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; +#ifdef CONFIG_GENTOO_KERNEL_SELF_PROTECTION +static __initconst const char * const builtin_lsm_order = "landlock,yama," CONFIG_LSM; +#else static __initconst const char * const builtin_lsm_order = CONFIG_LSM; +#endif /* Ordered list of LSMs to initialize. */ static __initdata struct lsm_info **ordered_lsms; It will not affect users that explicitly define the "lsm=" kernel cmdline argument though, which seems reasonable.
(In reply to Mickaël Salaün from comment #7) > (In reply to Mickaël Salaün from comment #6) > > (In reply to Peter from comment #3) > > > > Problem could be that if user once changed this, there is no way back to > > > defaults and "landlock" must inserted by user, if it could not set by our > > > distro/Kconfig. > > > > Correct. Unfortunately we cannot do much about that, except warn users if > > their configuration has a modified CONFIG_LSM which doesn't include Landlock. > > Well, we could prepend "landlock,yama" by default like this: > > diff --git a/security/security.c b/security/security.c > index 14d30fec8a00..b9696c0d198e 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -85,7 +85,11 @@ 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; > > +#ifdef CONFIG_GENTOO_KERNEL_SELF_PROTECTION > +static __initconst const char * const builtin_lsm_order = "landlock,yama," > CONFIG_LSM; > +#else > static __initconst const char * const builtin_lsm_order = CONFIG_LSM; > +#endif > > /* Ordered list of LSMs to initialize. */ > static __initdata struct lsm_info **ordered_lsms; > > > It will not affect users that explicitly define the "lsm=" kernel cmdline > argument though, which seems reasonable. Thanks for you input, my only concern here is that we now take that piece of the kernel configurable settings and remove that ability and line of site from the user. They modify their config, but we overrode the source code. Maybe a warning is enough ?
(In reply to Mike Pagano from comment #8) > (In reply to Mickaël Salaün from comment #7) > > (In reply to Mickaël Salaün from comment #6) > > > (In reply to Peter from comment #3) > > > > > > Problem could be that if user once changed this, there is no way back to > > > > defaults and "landlock" must inserted by user, if it could not set by our > > > > distro/Kconfig. > > > > > > Correct. Unfortunately we cannot do much about that, except warn users if > > > their configuration has a modified CONFIG_LSM which doesn't include Landlock. > > > > Well, we could prepend "landlock,yama" by default like this: > > > > diff --git a/security/security.c b/security/security.c > > index 14d30fec8a00..b9696c0d198e 100644 > > --- a/security/security.c > > +++ b/security/security.c > > @@ -85,7 +85,11 @@ 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; > > > > +#ifdef CONFIG_GENTOO_KERNEL_SELF_PROTECTION > > +static __initconst const char * const builtin_lsm_order = "landlock,yama," > > CONFIG_LSM; > > +#else > > static __initconst const char * const builtin_lsm_order = CONFIG_LSM; > > +#endif > > > > /* Ordered list of LSMs to initialize. */ > > static __initdata struct lsm_info **ordered_lsms; > > > > > > It will not affect users that explicitly define the "lsm=" kernel cmdline > > argument though, which seems reasonable. > > > Thanks for you input, my only concern here is that we now take that piece of > the kernel configurable settings and remove that ability and line of site > from the user. They modify their config, but we overrode the source code. > > Maybe a warning is enough ? I understand your concern, but from a user point of view how willingly selects CONFIG_GENTOO_KERNEL_SELF_PROTECTION, I would expect the KSPP configuration to be applied, whatever other configuration I choose (or forgot I previously chose). If I want to cherry-pick some of the CONFIG_GENTOO_KERNEL_SELF_PROTECTION dependencies, I'm on my own to select them myself. With this patch, the user configuration (CONFIG_LSM) is still taken into account (Bugzilla wrapped a line), including the selection of Yama and Landlock.
still from the newsletter ``` This comes with a second Landlock ABI version that should be checked to leverage Landlock in a best-effort way. See the updated documentation: https://docs.kernel.org/userspace-api/landlock.html If developers don't change their ruleset's handled access rights, a sandboxed application will not change. If they add the new LANDLOCK_ACCESS_FS_REFER right, then they should first check the ABI version to make sure it will work as expected: int abi = landlock_create_ruleset(NULL, 0, LANDLOCK_CREATE_RULESET_VERSION); if (abi < 2) { ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_REFER; } path_beneath_attr.allowed_access &= ruleset_attr.handled_access_fs; ``` from what I'm understanding adding the new LANDLOCK_ACCESS_FS_REFER could break with application using older ABI and the correct ABI need to be checked. I'm understanding this correctly? >I understand your concern, but from a user point of view how willingly selects >CONFIG_GENTOO_KERNEL_SELF_PROTECTION, I would expect the KSPP configuration to >be applied, whatever other configuration I choose (or forgot I previously >chose). If I want to cherry-pick some of the >CONFIG_GENTOO_KERNEL_SELF_PROTECTION dependencies, I'm on my own to select them >myself. With this patch, the user configuration (CONFIG_LSM) is still taken >into account (Bugzilla wrapped a line), including the selection of Yama and >Landlock. Are you also suggesting to add it to CONFIG_GENTOO_KERNEL_SELF_PROTECTION ?
(In reply to Mike Pagano from comment #8) > (In reply to Mickaël Salaün from comment #7) > > (In reply to Mickaël Salaün from comment #6) > > > (In reply to Peter from comment #3) > > > > > > Problem could be that if user once changed this, there is no way back to > > > > defaults and "landlock" must inserted by user, if it could not set by our > > > > distro/Kconfig. > > > > > > Correct. Unfortunately we cannot do much about that, except warn users if > > > their configuration has a modified CONFIG_LSM which doesn't include Landlock. > > > > Well, we could prepend "landlock,yama" by default like this: > > > > diff --git a/security/security.c b/security/security.c > > index 14d30fec8a00..b9696c0d198e 100644 > > --- a/security/security.c > > +++ b/security/security.c > > @@ -85,7 +85,11 @@ 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; > > > > +#ifdef CONFIG_GENTOO_KERNEL_SELF_PROTECTION > > +static __initconst const char * const builtin_lsm_order = "landlock,yama," > > CONFIG_LSM; > > +#else > > static __initconst const char * const builtin_lsm_order = CONFIG_LSM; > > +#endif > > > > /* Ordered list of LSMs to initialize. */ > > static __initdata struct lsm_info **ordered_lsms; > > > > > > It will not affect users that explicitly define the "lsm=" kernel cmdline > > argument though, which seems reasonable. > > > Thanks for you input, my only concern here is that we now take that piece of > the kernel configurable settings and remove that ability and line of site > from the user. They modify their config, but we overrode the source code. > > Maybe a warning is enough ? I also think it should need a warning. (In reply to Mike Pagano from comment #8) > (In reply to Mickaël Salaün from comment #7) > > (In reply to Mickaël Salaün from comment #6) > > > (In reply to Peter from comment #3) > > > > > > Problem could be that if user once changed this, there is no way back to > > > > defaults and "landlock" must inserted by user, if it could not set by our > > > > distro/Kconfig. > > > > > > Correct. Unfortunately we cannot do much about that, except warn users if > > > their configuration has a modified CONFIG_LSM which doesn't include Landlock. > > > > Well, we could prepend "landlock,yama" by default like this: > > > > diff --git a/security/security.c b/security/security.c > > index 14d30fec8a00..b9696c0d198e 100644 > > --- a/security/security.c > > +++ b/security/security.c > > @@ -85,7 +85,11 @@ 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; > > > > +#ifdef CONFIG_GENTOO_KERNEL_SELF_PROTECTION > > +static __initconst const char * const builtin_lsm_order = "landlock,yama," > > CONFIG_LSM; > > +#else > > static __initconst const char * const builtin_lsm_order = CONFIG_LSM; > > +#endif > > > > /* Ordered list of LSMs to initialize. */ > > static __initdata struct lsm_info **ordered_lsms; > > > > > > It will not affect users that explicitly define the "lsm=" kernel cmdline > > argument though, which seems reasonable. > > > Thanks for you input, my only concern here is that we now take that piece of > the kernel configurable settings and remove that ability and line of site > from the user. They modify their config, but we overrode the source code. > > Maybe a warning is enough ? I also agree to have at least a warning.
(In reply to Alice Ferrazzi from comment #10) > still from the newsletter > ``` > This comes with a second Landlock ABI version that should be checked to > leverage Landlock in a best-effort way. See the updated documentation: > https://docs.kernel.org/userspace-api/landlock.html > If developers don't change their ruleset's handled access rights, a > sandboxed application will not change. If they add the new > LANDLOCK_ACCESS_FS_REFER right, then they should first check the ABI > version to make sure it will work as expected: > > int abi = landlock_create_ruleset(NULL, 0, LANDLOCK_CREATE_RULESET_VERSION); > if (abi < 2) { > ruleset_attr.handled_access_fs &= ~LANDLOCK_ACCESS_FS_REFER; > } > path_beneath_attr.allowed_access &= ruleset_attr.handled_access_fs; > ``` > from what I'm understanding adding the new LANDLOCK_ACCESS_FS_REFER could > break with application using older ABI and the correct ABI need to be > checked. > > I'm understanding this correctly? I don't think so. Using LANDLOCK_ACCESS_FS_REFER or any future access right is the choice of the developer. If the application code doesn't change, there is no reason for the sandboxing behavior to change nor break. Developers don't directly select a specific Landlock ABI version, they use a set of features from which a subset may only be supported starting from a specific ABI version (i.e. a kernel version wrt. backports), hence the ABI version check to know which features are supported by the running kernel. When developers want to use a newer Landlock features (e.g. a new access right), as for any new code to their application, they need to make sure using this new code work fine with their application. In the case of Landlock, developers willing to support a newer feature (e.g. LANDLOCK_ACCESS_FS_REFER) need to test that this updated security policy fit well with their application legitimate use. To say it another way, when restricting an application, we still want this application to work as expected (except when it get exploited by a malicious actor). TL;DR: software development as usual, nothing to worry about. ;) > > I understand your concern, but from a user point of view how willingly s/how/who/ > > selects CONFIG_GENTOO_KERNEL_SELF_PROTECTION, I would expect the KSPP > > configuration to be applied, whatever other configuration I choose (or > > forgot I previously chose). If I want to cherry-pick some of the > > CONFIG_GENTOO_KERNEL_SELF_PROTECTION dependencies, I'm on my own to select > > them myself. With this patch, the user configuration (CONFIG_LSM) is still > > taken into account (Bugzilla wrapped a line), including the selection of > > Yama and Landlock. > > Are you also suggesting to add it to CONFIG_GENTOO_KERNEL_SELF_PROTECTION ? To add Yama? It is already part of CONFIG_GENTOO_KERNEL_SELF_PROTECTION. Landlock is also now part of the KSPP recommendation, so yes it should also be part of CONFIG_GENTOO_KERNEL_SELF_PROTECTION.
I dont think patching security.c is a good idea: +#ifdef CONFIG_GENTOO_KERNEL_SELF_PROTECTION +static __initconst const char * const builtin_lsm_order = "landlock,yama," What if CONFIG_LSM already contains "landlock, ... , yama, ..." ? Result would be: Both are doubled in builtin_lsm_order - used as init sequence. Has anybody tested how kernel will react ? (I am using "lsm.debug" as add. kernel command line parm to see what is happening while loading all sec.-modules; I am using ima and apparmor also) I think best would be to inform user (after emerge new kernel) to check this line IF KSPP settings is used and add "landlock" at first place if not already present.
(In reply to Peter from comment #13) > I dont think patching security.c is a good idea: > > +#ifdef CONFIG_GENTOO_KERNEL_SELF_PROTECTION > +static __initconst const char * const builtin_lsm_order = "landlock,yama," > > What if CONFIG_LSM already contains "landlock, ... , yama, ..." ? Result > would be: Both are doubled in builtin_lsm_order - used as init sequence. Has > anybody tested how kernel will react ? There is no issue, the kernel ignores duplicates: https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/security/security.c#n150
(In reply to Peter from comment #13) > I dont think patching security.c is a good idea: > > +#ifdef CONFIG_GENTOO_KERNEL_SELF_PROTECTION > +static __initconst const char * const builtin_lsm_order = "landlock,yama," Bugzilla wrapped the patch line but there is an extra CONFIG_LSM at the end of this line, so it only enable Landlock and Yama if they are not already set in CONFIG_LSM (and ignores duplicates).
Have you tested this ? (for my understanding it adds string "..." to CONFIG_LSM)
(In reply to Peter from comment #16) > Have you tested this ? (for my understanding it adds string "..." to > CONFIG_LSM) Yes I tested it and it works as expected: the duplicated entries are silently ignored because of the exists_ordered_lsm() check. Where does the "..." string come from?
The bug has been referenced in the following commit(s): https://gitweb.gentoo.org/proj/linux-patches.git/commit/?id=27a3d3432243c1bd89ef3c68330f8d31da45ba34 commit 27a3d3432243c1bd89ef3c68330f8d31da45ba34 Author: Mike Pagano <mpagano@gentoo.org> AuthorDate: 2022-08-25 17:36:30 +0000 Commit: Mike Pagano <mpagano@gentoo.org> CommitDate: 2022-08-25 17:36:30 +0000 Add CONFIG_LANDLOCK to KSPP and RANDSTRUCT fix Bug: https://bugs.gentoo.org/865685 Signed-off-by: Mike Pagano <mpagano@gentoo.org> 4567_distro-Gentoo-Kconfig.patch | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-)
Sorry, I was to lazy; I have meant with "..." "landlock,yama," Ok, thanks for your test; great it works, but I dont know why I dont like it; for me it is not a clean solution (if I have two words doubled in a string). Luckily its not me who decide it ;-)
This is now in 5.19.5
