| Message ID | Z9wDxeRQPhTi1EIS@gardel-login (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Revert "integrity: Do not load MOK and MOKx when secure boot be disabled" | expand |
On Thu, Mar 20, 2025 at 01:02:13PM +0100, Lennart Poettering wrote: > diff --git a/security/integrity/platform_certs/load_uefi.c b/security/integrity/platform_certs/load_uefi.c > index d1fdd113450a..7783bcacd26c 100644 > --- a/security/integrity/platform_certs/load_uefi.c > +++ b/security/integrity/platform_certs/load_uefi.c > @@ -7,7 +7,6 @@ > #include <linux/err.h> > #include <linux/efi.h> > #include <linux/slab.h> > -#include <linux/ima.h> > #include <keys/asymmetric-type.h> > #include <keys/system_keyring.h> > #include "../integrity.h" > @@ -211,10 +210,6 @@ static int __init load_uefi_certs(void) > kfree(dbx); > } > > - /* the MOK/MOKx can not be trusted when secure boot is disabled */ > - if (!arch_ima_get_secureboot()) > - return 0; > - > mokx = get_cert_list(L"MokListXRT", &mok_var, &mokxsize, &status); > if (!mokx) { > if (status == EFI_NOT_FOUND) The original commit message is foggy: " integrity: Do not load MOK and MOKx when secure boot be disabled The security of Machine Owner Key (MOK) relies on secure boot. When secure boot is disabled, EFI firmware will not verify binary code. Then arbitrary efi binary code can modify MOK when rebooting. This patch prevents MOK/MOKx be loaded when secure boot be disabled. " Given that I don't understand the problem it is trying to solve: Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org> > -- > 2.48.1 > > > Lennart > > -- > Lennart Poettering, Berlin > Jarkko
Hi Lennart, Lennart Poettering <mzxreary@0pointer.de> 於 2025年3月20日 週四 下午8:02寫道： > > This reverts commit 92ad19559ea9a8ec6f158480934ae26ebfe2c14f. > > This original commit this reverts creates a strange situation: it > ensures more restrictive behaviour if SecureBoot is off then when it > is on, which is the opposite of what one would expect. > > Typically, one would expect that if SB is off the validation of > resources during the pre-kernel and kernel initialization is less > restrictive, not more restrictive. But this check turned the world on > its head. > SB off means that the chain of trust is broken. Which means that all mechanisms rely on SB are non-secure. Meanwhile, if the integrity of kernel can be guaranteed by other mechanism (e.g. TPM), then mok should not be loaded when SB off. > I'd like to ask for this commit to be reverted. If SB is on all bets are > off regarding integrity of boot loaders and stuff, hence it makes no > sense to be restrictive here: you cannot regain integrity once you gave > it up once, hence if all bets are off anyway we might as well import any > Mok keys passed to us into the kernel keyring. > > Or to say this differently: if an attacker got control of the pre-kernel > boot phase they might as well patch around in the firmware apis to make > the kernel believe it is in SB mode even if it is not. Hence the check > carries no value. It doesn't protect anything in any effective way. > If this is the case, the check of MokListTrustedRT can also be removed. All mok can directly be added to machine keyring then link with secondary keyring. Because attacker can create MokListTrusted/MokListTrusted variables to cheat bootloader or kernel. The check of MokListTrustedRT is useless. > The reason i'd like this check to go is that I'd like a nice way to > insert keys from pre-boot into into the kernel keyring for use with > signed dm-verity, without requiring recompilation of the kernel, and > without SB database games. i.e. i'd like to use a regular, signed > distro kernel, and pass to it additional keys to insert into the > kernel keyring in a reasonable way. The mok stuff would be great for that, > except it all falls apart once SB is off. > > You might wonder what signed dm-verity gives me if I have SB off. If > we authenticate the boot phase up to Linux userspace via TPM-based PCR > policies (i.e. measured boot) we can be sure of the boot integrity > without having to rely on SB. But then we'd still like to use > dm-verity based code signing for userspace. hm... I am a bit confused. So, this patch can help the above scenario? > --- > security/integrity/platform_certs/load_uefi.c | 5 ----- > 1 file changed, 5 deletions(-) > > diff --git a/security/integrity/platform_certs/load_uefi.c b/security/integrity/platform_certs/load_uefi.c > index d1fdd113450a..7783bcacd26c 100644 > --- a/security/integrity/platform_certs/load_uefi.c > +++ b/security/integrity/platform_certs/load_uefi.c > @@ -7,7 +7,6 @@ > #include <linux/err.h> > #include <linux/efi.h> > #include <linux/slab.h> > -#include <linux/ima.h> > #include <keys/asymmetric-type.h> > #include <keys/system_keyring.h> > #include "../integrity.h" > @@ -211,10 +210,6 @@ static int __init load_uefi_certs(void) > kfree(dbx); > } > > - /* the MOK/MOKx can not be trusted when secure boot is disabled */ > - if (!arch_ima_get_secureboot()) > - return 0; > - > mokx = get_cert_list(L"MokListXRT", &mok_var, &mokxsize, &status); > if (!mokx) { > if (status == EFI_NOT_FOUND) > -- > 2.48.1 Thanks Joey Lee
On Fr, 21.03.25 15:13, lee joey (joeyli.kernel@gmail.com) wrote: > Hi Lennart, > > Lennart Poettering <mzxreary@0pointer.de> 於 2025年3月20日 週四 下午8:02寫道： > > > > This reverts commit 92ad19559ea9a8ec6f158480934ae26ebfe2c14f. > > > > This original commit this reverts creates a strange situation: it > > ensures more restrictive behaviour if SecureBoot is off then when it > > is on, which is the opposite of what one would expect. > > > > Typically, one would expect that if SB is off the validation of > > resources during the pre-kernel and kernel initialization is less > > restrictive, not more restrictive. But this check turned the world on > > its head. > > > > SB off means that the chain of trust is broken. Which means that all > mechanisms rely on SB are non-secure. Meanwhile, if the integrity of kernel > can be guaranteed by other mechanism (e.g. TPM), then mok should not > be loaded when SB off. Why not? as you say, chain of trust is broken: the kernel itself is not immediately integrity protected and neither is the firmware. Hence filtering out keys in this case is really pointless. > > I'd like to ask for this commit to be reverted. If SB is on all bets are > > off regarding integrity of boot loaders and stuff, hence it makes no > > sense to be restrictive here: you cannot regain integrity once you gave > > it up once, hence if all bets are off anyway we might as well import any > > Mok keys passed to us into the kernel keyring. > > > > Or to say this differently: if an attacker got control of the pre-kernel > > boot phase they might as well patch around in the firmware apis to make > > the kernel believe it is in SB mode even if it is not. Hence the check > > carries no value. It doesn't protect anything in any effective way. > > If this is the case, the check of MokListTrustedRT can also be > removed. I think it makes sense to honour explicit knobs, such as MokListTrustedRT: It has a very clear meaning: it indicates whether to import the keys or not. This is quite different from SB state, which is not explicit about importing keys at all, it just indicates whether firmware level validation of signatures is done or not. > All mok can directly be added to machine keyring then link with > secondary keyring. > Because attacker can create MokListTrusted/MokListTrusted variables to cheat > bootloader or kernel. The check of MokListTrustedRT is useless. Yeah, it does not carry immediate security value if SB is off. But it does allow finer-grained control by pre-boot code of how the kernel later sets up things. Hence I'd keep it. > > You might wonder what signed dm-verity gives me if I have SB off. If > > we authenticate the boot phase up to Linux userspace via TPM-based PCR > > policies (i.e. measured boot) we can be sure of the boot integrity > > without having to rely on SB. But then we'd still like to use > > dm-verity based code signing for userspace. > > hm... I am a bit confused. So, this patch can help the above > scenario? The revert I posted will allow us to populate the kernel keyring used for dm-verity signature checks from pre-boot even if SB is off. Via local or remote attestation during boot we can later validate this chosen boot path, and hence can a-posteriori validate that everything is OK even if the a-priori SB check is not done. But once we have validated that we then have the key in the kernel keyring for later dm-verity validations, which is all we wanted. Lennart -- Lennart Poettering, Berlin
On Fri, 2025-03-21 at 15:13 +0800, lee joey wrote: > Hi Lennart, > > Lennart Poettering <mzxreary@0pointer.de> 於 2025年3月20日 週四 下午8:02寫道： > > > > This reverts commit 92ad19559ea9a8ec6f158480934ae26ebfe2c14f. > > > > This original commit this reverts creates a strange situation: it > > ensures more restrictive behaviour if SecureBoot is off then when > > it is on, which is the opposite of what one would expect. > > > > Typically, one would expect that if SB is off the validation of > > resources during the pre-kernel and kernel initialization is less > > restrictive, not more restrictive. But this check turned the world > > on its head. > > > > SB off means that the chain of trust is broken. Which means that all > mechanisms rely on SB are non-secure. Meanwhile, if the integrity of > kernel can be guaranteed by other mechanism (e.g. TPM), then mok > should not be loaded when SB off. I think the point being made here is that there are other integrity mechanisms than secure boot which can protect the early boot environment. A second argument would be that we still load the UEFI certificates into the chain, and they have exactly the same early boot guarantees as the MOK ones, so we're not being consistent: we should load all of them all the time or none. The boot envelope still has some protections even without secure boot or anything else. The third must be the module one: we've trained users to insert module signing certificates into MOK. If they find that mechanism doesn't work under some possible circumstances they're going to be unhappy. Part of the problem with that last is with the lockdown creep we're increasing the chances that users will see turning off secure boot as the solution to fixing some lockdown problems (say they want hibernate for instance) so having the kernel be unable to load external modules in that case when they're trying to relax protections is highly counter intuitive. Regards, James
On Fri, Mar 21, 2025 at 09:39:55AM +0100, Lennart Poettering wrote: > On Fr, 21.03.25 15:13, lee joey (joeyli.kernel@gmail.com) wrote: > > > Hi Lennart, > > > > Lennart Poettering <mzxreary@0pointer.de> 於 2025年3月20日 週四 下午8:02寫道： > > > > > > This reverts commit 92ad19559ea9a8ec6f158480934ae26ebfe2c14f. > > > > > > This original commit this reverts creates a strange situation: it > > > ensures more restrictive behaviour if SecureBoot is off then when it > > > is on, which is the opposite of what one would expect. > > > > > > Typically, one would expect that if SB is off the validation of > > > resources during the pre-kernel and kernel initialization is less > > > restrictive, not more restrictive. But this check turned the world on > > > its head. > > > > > > > SB off means that the chain of trust is broken. Which means that all > > mechanisms rely on SB are non-secure. Meanwhile, if the integrity of kernel > > can be guaranteed by other mechanism (e.g. TPM), then mok should not > > be loaded when SB off. > > Why not? as you say, chain of trust is broken: the kernel itself is > not immediately integrity protected and neither is the firmware. Hence > filtering out keys in this case is really pointless. The way I look at this is that unless there is an actual threat scenario that we protect against by hiding MOK keys, then we should not hide those keys. Since I don't find any threat scenarios my reviewed-by holds. Pointless checks is security by obfuscation, which is not really security. BR, Jarkko
diff --git a/security/integrity/platform_certs/load_uefi.c b/security/integrity/platform_certs/load_uefi.c index d1fdd113450a..7783bcacd26c 100644 --- a/security/integrity/platform_certs/load_uefi.c +++ b/security/integrity/platform_certs/load_uefi.c @@ -7,7 +7,6 @@ #include <linux/err.h> #include <linux/efi.h> #include <linux/slab.h> -#include <linux/ima.h> #include <keys/asymmetric-type.h> #include <keys/system_keyring.h> #include "../integrity.h" @@ -211,10 +210,6 @@ static int __init load_uefi_certs(void) kfree(dbx); } - /* the MOK/MOKx can not be trusted when secure boot is disabled */ - if (!arch_ima_get_secureboot()) - return 0; - mokx = get_cert_list(L"MokListXRT", &mok_var, &mokxsize, &status); if (!mokx) { if (status == EFI_NOT_FOUND)