diff mbox series

[v10,8/8] integrity: Only use machine keyring when uefi_check_trust_mok_keys is true

Message ID 20220126025834.255493-9-eric.snowberg@oracle.com
State Accepted
Commit 3d6ae1a5d0c2019d274284859f556dcb64aa98a7
Headers show
Series Enroll kernel keys thru MOK | expand

Commit Message

Eric Snowberg Jan. 26, 2022, 2:58 a.m. UTC
With the introduction of uefi_check_trust_mok_keys, it signifies the end-
user wants to trust the machine keyring as trusted keys.  If they have
chosen to trust the machine keyring, load the qualifying keys into it
during boot, then link it to the secondary keyring .  If the user has not
chosen to trust the machine keyring, it will be empty and not linked to
the secondary keyring.

Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
---
v4: Initial version
v5: Rename to machine keyring
v6: Unmodified from v5
v7: Made trust_mok static
v8: Unmodified from v7
v10: Added Jarkko's Reviewed-by
---
 security/integrity/digsig.c                      |  2 +-
 security/integrity/integrity.h                   |  5 +++++
 .../integrity/platform_certs/keyring_handler.c   |  2 +-
 .../integrity/platform_certs/machine_keyring.c   | 16 ++++++++++++++++
 4 files changed, 23 insertions(+), 2 deletions(-)

Comments

Michal Suchanek April 11, 2022, 11:06 a.m. UTC | #1
Hello,

On Tue, Jan 25, 2022 at 09:58:34PM -0500, Eric Snowberg wrote:
> With the introduction of uefi_check_trust_mok_keys, it signifies the end-

What value does such flag have?

The user is as much in control of the flag as the MOK keys.

> user wants to trust the machine keyring as trusted keys.  If they have
> chosen to trust the machine keyring, load the qualifying keys into it
> during boot, then link it to the secondary keyring .  If the user has not
> chosen to trust the machine keyring, it will be empty and not linked to
> the secondary keyring.

Why is importing the keys and using them linked together?

If later we get, say, machine keyring on powerpc managed by secvarctl
then it has its value to import the keyring and be able to list the
content with the same tools on EFI and powerpc.

It also makes sense to be able to configure the kernel to import the
keys and not use them. I don't see any value in configuring that in
shim, though. shim is both source of the key material and the flag so
the flag is redundant, it does not exist on existing shim versions
installed on user systems, and it's unlikely to exist on other
plaltforms, either.

Thanks

Michal

> 
> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
> Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
> ---
> v4: Initial version
> v5: Rename to machine keyring
> v6: Unmodified from v5
> v7: Made trust_mok static
> v8: Unmodified from v7
> v10: Added Jarkko's Reviewed-by
> ---
>  security/integrity/digsig.c                      |  2 +-
>  security/integrity/integrity.h                   |  5 +++++
>  .../integrity/platform_certs/keyring_handler.c   |  2 +-
>  .../integrity/platform_certs/machine_keyring.c   | 16 ++++++++++++++++
>  4 files changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
> index 7b719aa76188..c8c8a4a4e7a0 100644
> --- a/security/integrity/digsig.c
> +++ b/security/integrity/digsig.c
> @@ -112,7 +112,7 @@ static int __init __integrity_init_keyring(const unsigned int id,
>  	} else {
>  		if (id == INTEGRITY_KEYRING_PLATFORM)
>  			set_platform_trusted_keys(keyring[id]);
> -		if (id == INTEGRITY_KEYRING_MACHINE)
> +		if (id == INTEGRITY_KEYRING_MACHINE && trust_moklist())
>  			set_machine_trusted_keys(keyring[id]);
>  		if (id == INTEGRITY_KEYRING_IMA)
>  			load_module_cert(keyring[id]);
> diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
> index 730771eececd..2e214c761158 100644
> --- a/security/integrity/integrity.h
> +++ b/security/integrity/integrity.h
> @@ -287,9 +287,14 @@ static inline void __init add_to_platform_keyring(const char *source,
>  
>  #ifdef CONFIG_INTEGRITY_MACHINE_KEYRING
>  void __init add_to_machine_keyring(const char *source, const void *data, size_t len);
> +bool __init trust_moklist(void);
>  #else
>  static inline void __init add_to_machine_keyring(const char *source,
>  						  const void *data, size_t len)
>  {
>  }
> +static inline bool __init trust_moklist(void)
> +{
> +	return false;
> +}
>  #endif
> diff --git a/security/integrity/platform_certs/keyring_handler.c b/security/integrity/platform_certs/keyring_handler.c
> index 4872850d081f..1db4d3b4356d 100644
> --- a/security/integrity/platform_certs/keyring_handler.c
> +++ b/security/integrity/platform_certs/keyring_handler.c
> @@ -83,7 +83,7 @@ __init efi_element_handler_t get_handler_for_db(const efi_guid_t *sig_type)
>  __init efi_element_handler_t get_handler_for_mok(const efi_guid_t *sig_type)
>  {
>  	if (efi_guidcmp(*sig_type, efi_cert_x509_guid) == 0) {
> -		if (IS_ENABLED(CONFIG_INTEGRITY_MACHINE_KEYRING))
> +		if (IS_ENABLED(CONFIG_INTEGRITY_MACHINE_KEYRING) && trust_moklist())
>  			return add_to_machine_keyring;
>  		else
>  			return add_to_platform_keyring;
> diff --git a/security/integrity/platform_certs/machine_keyring.c b/security/integrity/platform_certs/machine_keyring.c
> index 09fd8f20c756..7aaed7950b6e 100644
> --- a/security/integrity/platform_certs/machine_keyring.c
> +++ b/security/integrity/platform_certs/machine_keyring.c
> @@ -8,6 +8,8 @@
>  #include <linux/efi.h>
>  #include "../integrity.h"
>  
> +static bool trust_mok;
> +
>  static __init int machine_keyring_init(void)
>  {
>  	int rc;
> @@ -59,3 +61,17 @@ static __init bool uefi_check_trust_mok_keys(void)
>  
>  	return false;
>  }
> +
> +bool __init trust_moklist(void)
> +{
> +	static bool initialized;
> +
> +	if (!initialized) {
> +		initialized = true;
> +
> +		if (uefi_check_trust_mok_keys())
> +			trust_mok = true;
> +	}
> +
> +	return trust_mok;
> +}
> -- 
> 2.18.4
>
Eric Snowberg April 11, 2022, 4:39 p.m. UTC | #2
> On Apr 11, 2022, at 5:06 AM, Michal Suchánek <msuchanek@suse.de> wrote:
> 
> Hello,
> 
> On Tue, Jan 25, 2022 at 09:58:34PM -0500, Eric Snowberg wrote:
>> With the introduction of uefi_check_trust_mok_keys, it signifies the end-
> 
> What value does such flag have?
> 
> The user is as much in control of the flag as the MOK keys.

The flag allows the system owner (not root) the ability to determine 
if they want to load MOKList into the machine keyring.  Keys contained 
in the machine keyring are then linked to the secondary.  The flag is no 
different than the '—ignore-db' currently available in shim, which then 
gets propagated to Linux (uefi_check_ignore_db).  These flags can be 
set by the system owner, who can prove physical presence.  

>> user wants to trust the machine keyring as trusted keys.  If they have
>> chosen to trust the machine keyring, load the qualifying keys into it
>> during boot, then link it to the secondary keyring .  If the user has not
>> chosen to trust the machine keyring, it will be empty and not linked to
>> the secondary keyring.
> 
> Why is importing the keys and using them linked together?
> 
> If later we get, say, machine keyring on powerpc managed by secvarctl
> then it has its value to import the keyring and be able to list the
> content with the same tools on EFI and powerpc.

The machine keyring is linked to the secondary keyring, exactly the same way 
the builtin is linked to it.  Linking this way should eliminate the need to change 
any user space tools to list the contents. 

> It also makes sense to be able to configure the kernel to import the
> keys and not use them. I don't see any value in configuring that in
> shim, though. shim is both source of the key material and the flag so
> the flag is redundant, it does not exist on existing shim versions
> installed on user systems, and it's unlikely to exist on other
> plaltforms, either.

I’m sure other solutions to enable it will be accepted as well.  I know Mimi was testing 
without shim using a different method.
Michal Suchanek April 11, 2022, 5:24 p.m. UTC | #3
On Mon, Apr 11, 2022 at 04:39:42PM +0000, Eric Snowberg wrote:
> 
> 
> > On Apr 11, 2022, at 5:06 AM, Michal Suchánek <msuchanek@suse.de> wrote:
> > 
> > Hello,
> > 
> > On Tue, Jan 25, 2022 at 09:58:34PM -0500, Eric Snowberg wrote:
> >> With the introduction of uefi_check_trust_mok_keys, it signifies the end-
> > 
> > What value does such flag have?
> > 
> > The user is as much in control of the flag as the MOK keys.
> 
> The flag allows the system owner (not root) the ability to determine 
> if they want to load MOKList into the machine keyring.  Keys contained 
> in the machine keyring are then linked to the secondary.  The flag is no 
> different than the '—ignore-db' currently available in shim, which then 
> gets propagated to Linux (uefi_check_ignore_db).  These flags can be 
> set by the system owner, who can prove physical presence.  

Managing the MOK keys requires physical presence equally.

Moreover, these keys are trusted for running code at ring0, in fact the
running kernel is expected to be signed by one of them, and can be
signed by any of them.

Then what exact purpose does this extra flag serve?

If such compile-time flag exists in the kernel it cannot be overriden by
the root once the kernel is signed, either.

> >> user wants to trust the machine keyring as trusted keys.  If they have
> >> chosen to trust the machine keyring, load the qualifying keys into it
> >> during boot, then link it to the secondary keyring .  If the user has not
> >> chosen to trust the machine keyring, it will be empty and not linked to
> >> the secondary keyring.
> > 
> > Why is importing the keys and using them linked together?
> > 
> > If later we get, say, machine keyring on powerpc managed by secvarctl
> > then it has its value to import the keyring and be able to list the
> > content with the same tools on EFI and powerpc.
> 
> The machine keyring is linked to the secondary keyring, exactly the same way 
> the builtin is linked to it.  Linking this way should eliminate the need to change 
> any user space tools to list the contents. 

That's answer to a completely different question, though.

You either import the keys and use them, or you don't use them and don't
import them. The option to import and not use is not available.

> > It also makes sense to be able to configure the kernel to import the
> > keys and not use them. I don't see any value in configuring that in
> > shim, though. shim is both source of the key material and the flag so
> > the flag is redundant, it does not exist on existing shim versions
> > installed on user systems, and it's unlikely to exist on other
> > plaltforms, either.
> 
> I’m sure other solutions to enable it will be accepted as well.  I know Mimi was testing 
> without shim using a different method.

Like not using that extra flag at all?

Thanks

Michal
Eric Snowberg April 11, 2022, 8:34 p.m. UTC | #4
> On Apr 11, 2022, at 11:24 AM, Michal Suchánek <msuchanek@suse.de> wrote:
> 
> On Mon, Apr 11, 2022 at 04:39:42PM +0000, Eric Snowberg wrote:
>> 
>> 
>>> On Apr 11, 2022, at 5:06 AM, Michal Suchánek <msuchanek@suse.de> wrote:
>>> 
>>> Hello,
>>> 
>>> On Tue, Jan 25, 2022 at 09:58:34PM -0500, Eric Snowberg wrote:
>>>> With the introduction of uefi_check_trust_mok_keys, it signifies the end-
>>> 
>>> What value does such flag have?
>>> 
>>> The user is as much in control of the flag as the MOK keys.
>> 
>> The flag allows the system owner (not root) the ability to determine 
>> if they want to load MOKList into the machine keyring.  Keys contained 
>> in the machine keyring are then linked to the secondary.  The flag is no 
>> different than the '—ignore-db' currently available in shim, which then 
>> gets propagated to Linux (uefi_check_ignore_db).  These flags can be 
>> set by the system owner, who can prove physical presence.  
> 
> Managing the MOK keys requires physical presence equally.
> 
> Moreover, these keys are trusted for running code at ring0, in fact the
> running kernel is expected to be signed by one of them, and can be
> signed by any of them.
> 
> Then what exact purpose does this extra flag serve?
> 
> If such compile-time flag exists in the kernel it cannot be overriden by
> the root once the kernel is signed, either.
> 
>>>> user wants to trust the machine keyring as trusted keys.  If they have
>>>> chosen to trust the machine keyring, load the qualifying keys into it
>>>> during boot, then link it to the secondary keyring .  If the user has not
>>>> chosen to trust the machine keyring, it will be empty and not linked to
>>>> the secondary keyring.
>>> 
>>> Why is importing the keys and using them linked together?
>>> 
>>> If later we get, say, machine keyring on powerpc managed by secvarctl
>>> then it has its value to import the keyring and be able to list the
>>> content with the same tools on EFI and powerpc.
>> 
>> The machine keyring is linked to the secondary keyring, exactly the same way 
>> the builtin is linked to it.  Linking this way should eliminate the need to change 
>> any user space tools to list the contents. 
> 
> That's answer to a completely different question, though.
> 
> You either import the keys and use them, or you don't use them and don't
> import them. The option to import and not use is not available.

Why import something into a keyring that can not be used?

MOKList keys get imported into one of two keyrings, either the machine or the 
platform.  If uefi_check_trust_mok_keys returns false, the MOKList keys are 
loaded into the platform along with the UEFI SB DB keys.  If true, they are loaded 
into the machine keyring.

>>> It also makes sense to be able to configure the kernel to import the
>>> keys and not use them. I don't see any value in configuring that in
>>> shim, though. shim is both source of the key material and the flag so
>>> the flag is redundant, it does not exist on existing shim versions
>>> installed on user systems, and it's unlikely to exist on other
>>> plaltforms, either.
>> 
>> I’m sure other solutions to enable it will be accepted as well.  I know Mimi was testing 
>> without shim using a different method.
> 
> Like not using that extra flag at all?

That would be up to the maintainers.

Otherwise, if you are using shim, you can build one with the flag on by default.
Michal Suchanek April 11, 2022, 9:35 p.m. UTC | #5
On Mon, Apr 11, 2022 at 08:34:55PM +0000, Eric Snowberg wrote:
> 
> 
> > On Apr 11, 2022, at 11:24 AM, Michal Suchánek <msuchanek@suse.de> wrote:
> > 
> > On Mon, Apr 11, 2022 at 04:39:42PM +0000, Eric Snowberg wrote:
> >> 
> >> 
> >>> On Apr 11, 2022, at 5:06 AM, Michal Suchánek <msuchanek@suse.de> wrote:
> >>> 
> >>> Hello,
> >>> 
> >>> On Tue, Jan 25, 2022 at 09:58:34PM -0500, Eric Snowberg wrote:
> >>>> With the introduction of uefi_check_trust_mok_keys, it signifies the end-
> >>> 
> >>> What value does such flag have?
> >>> 
> >>> The user is as much in control of the flag as the MOK keys.
> >> 
> >> The flag allows the system owner (not root) the ability to determine 
> >> if they want to load MOKList into the machine keyring.  Keys contained 
> >> in the machine keyring are then linked to the secondary.  The flag is no 
> >> different than the '—ignore-db' currently available in shim, which then 
> >> gets propagated to Linux (uefi_check_ignore_db).  These flags can be 
> >> set by the system owner, who can prove physical presence.  
> > 
> > Managing the MOK keys requires physical presence equally.
> > 
> > Moreover, these keys are trusted for running code at ring0, in fact the
> > running kernel is expected to be signed by one of them, and can be
> > signed by any of them.
> > 
> > Then what exact purpose does this extra flag serve?
> > 
> > If such compile-time flag exists in the kernel it cannot be overriden by
> > the root once the kernel is signed, either.
> > 
> >>>> user wants to trust the machine keyring as trusted keys.  If they have
> >>>> chosen to trust the machine keyring, load the qualifying keys into it
> >>>> during boot, then link it to the secondary keyring .  If the user has not
> >>>> chosen to trust the machine keyring, it will be empty and not linked to
> >>>> the secondary keyring.
> >>> 
> >>> Why is importing the keys and using them linked together?
> >>> 
> >>> If later we get, say, machine keyring on powerpc managed by secvarctl
> >>> then it has its value to import the keyring and be able to list the
> >>> content with the same tools on EFI and powerpc.
> >> 
> >> The machine keyring is linked to the secondary keyring, exactly the same way 
> >> the builtin is linked to it.  Linking this way should eliminate the need to change 
> >> any user space tools to list the contents. 
> > 
> > That's answer to a completely different question, though.
> > 
> > You either import the keys and use them, or you don't use them and don't
> > import them. The option to import and not use is not available.
> 
> Why import something into a keyring that can not be used?
> 
> MOKList keys get imported into one of two keyrings, either the machine or the 
> platform.  If uefi_check_trust_mok_keys returns false, the MOKList keys are 
> loaded into the platform along with the UEFI SB DB keys.  If true, they are loaded 
> into the machine keyring.

Ooh, such quirky and convoluted design. Not sure how anyone is supposed
to make any sense of this but hey, none of this can possibly change to
not break something.

Now I at least know.

Thanks

Michal
diff mbox series

Patch

diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
index 7b719aa76188..c8c8a4a4e7a0 100644
--- a/security/integrity/digsig.c
+++ b/security/integrity/digsig.c
@@ -112,7 +112,7 @@  static int __init __integrity_init_keyring(const unsigned int id,
 	} else {
 		if (id == INTEGRITY_KEYRING_PLATFORM)
 			set_platform_trusted_keys(keyring[id]);
-		if (id == INTEGRITY_KEYRING_MACHINE)
+		if (id == INTEGRITY_KEYRING_MACHINE && trust_moklist())
 			set_machine_trusted_keys(keyring[id]);
 		if (id == INTEGRITY_KEYRING_IMA)
 			load_module_cert(keyring[id]);
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index 730771eececd..2e214c761158 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -287,9 +287,14 @@  static inline void __init add_to_platform_keyring(const char *source,
 
 #ifdef CONFIG_INTEGRITY_MACHINE_KEYRING
 void __init add_to_machine_keyring(const char *source, const void *data, size_t len);
+bool __init trust_moklist(void);
 #else
 static inline void __init add_to_machine_keyring(const char *source,
 						  const void *data, size_t len)
 {
 }
+static inline bool __init trust_moklist(void)
+{
+	return false;
+}
 #endif
diff --git a/security/integrity/platform_certs/keyring_handler.c b/security/integrity/platform_certs/keyring_handler.c
index 4872850d081f..1db4d3b4356d 100644
--- a/security/integrity/platform_certs/keyring_handler.c
+++ b/security/integrity/platform_certs/keyring_handler.c
@@ -83,7 +83,7 @@  __init efi_element_handler_t get_handler_for_db(const efi_guid_t *sig_type)
 __init efi_element_handler_t get_handler_for_mok(const efi_guid_t *sig_type)
 {
 	if (efi_guidcmp(*sig_type, efi_cert_x509_guid) == 0) {
-		if (IS_ENABLED(CONFIG_INTEGRITY_MACHINE_KEYRING))
+		if (IS_ENABLED(CONFIG_INTEGRITY_MACHINE_KEYRING) && trust_moklist())
 			return add_to_machine_keyring;
 		else
 			return add_to_platform_keyring;
diff --git a/security/integrity/platform_certs/machine_keyring.c b/security/integrity/platform_certs/machine_keyring.c
index 09fd8f20c756..7aaed7950b6e 100644
--- a/security/integrity/platform_certs/machine_keyring.c
+++ b/security/integrity/platform_certs/machine_keyring.c
@@ -8,6 +8,8 @@ 
 #include <linux/efi.h>
 #include "../integrity.h"
 
+static bool trust_mok;
+
 static __init int machine_keyring_init(void)
 {
 	int rc;
@@ -59,3 +61,17 @@  static __init bool uefi_check_trust_mok_keys(void)
 
 	return false;
 }
+
+bool __init trust_moklist(void)
+{
+	static bool initialized;
+
+	if (!initialized) {
+		initialized = true;
+
+		if (uefi_check_trust_mok_keys())
+			trust_mok = true;
+	}
+
+	return trust_mok;
+}