2021-11-16 00:19:18

by Eric Snowberg

[permalink] [raw]
Subject: [PATCH v7 00/17] Enroll kernel keys thru MOK

Back in 2013 Linus requested a feature to allow end-users to have the
ability "to add their own keys and sign modules they trust". This was
his *second* order outlined here [1]. There have been many attempts
over the years to solve this problem, all have been rejected. Many
of the failed attempts loaded all preboot firmware keys into the kernel,
including the Secure Boot keys. Many distributions carry one of these
rejected attempts [2], [3], [4]. This series tries to solve this problem
with a solution that takes into account all the problems brought up in
the previous attempts.

On UEFI based systems, this series introduces a new Linux kernel keyring
containing the Machine Owner Keys (MOK) called machine. It also defines
a new MOK variable in shim. This variable allows the end-user to decide
if they want to load MOK keys into the machine keyring. Mimi has suggested
that only CA keys contained within the MOK be loaded into the machine
keyring. All other certs will load into the platform keyring instead.

By default, nothing changes; MOK keys are not loaded into the machine
keyring. They are only loaded after the end-user makes the decision
themselves. The end-user would set this through mokutil using a new
--trust-mok option [5]. This would work similar to how the kernel uses
MOK variables to enable/disable signature validation as well as use/ignore
the db. Any kernel operation that uses either the builtin or secondary
trusted keys as a trust source shall also reference the new machine
keyring as a trust source.

Secure Boot keys will never be loaded into the machine keyring. They
will always be loaded into the platform keyring. If an end-user wanted
to load one, they would need to enroll it into the MOK.

Steps required by the end user:

Sign kernel module with user created key:
$ /usr/src/kernels/$(uname -r)/scripts/sign-file sha512 \
machine_signing_key.priv machine_signing_key.x509 my_module.ko

Import the key into the MOK
$ mokutil --import machine_signing_key.x509

Setup the kernel to load MOK keys into the .machine keyring
$ mokutil --trust-mok

Then reboot, the MokManager will load and ask if you want to trust the
MOK key and enroll the MOK into the MOKList. Afterwards the signed kernel
module will load.

I have included a link to the mokutil [5] changes I have made to support
this new functionality. The shim changes have now been accepted
upstream [6].

[1] https://marc.info/?l=linux-kernel&m=136185386310140&w=2
[2] https://lore.kernel.org/lkml/[email protected]/
[3] https://lore.kernel.org/lkml/[email protected]/
[4] https://lore.kernel.org/linux-integrity/1e41f22b1f11784f1e943f32bf62034d4e054cdb.camel@HansenPartnership.com/
[5] https://github.com/esnowberg/mokutil/tree/mokvars-v3
[6] https://github.com/rhboot/shim/commit/4e513405b4f1641710115780d19dcec130c5208f

Eric Snowberg (17):
integrity: Introduce a Linux keyring called machine
integrity: Do not allow machine keyring updates following init
KEYS: Create static version of public_key_verify_signature
X.509: Parse Basic Constraints for CA
KEYS: CA link restriction
integrity: restrict INTEGRITY_KEYRING_MACHINE to restrict_link_by_ca
integrity: Fix warning about missing prototypes
integrity: add new keyring handler for mok keys
KEYS: Rename get_builtin_and_secondary_restriction
KEYS: add a reference to machine keyring
KEYS: Introduce link restriction for machine keys
KEYS: integrity: change link restriction to trust the machine keyring
KEYS: link secondary_trusted_keys to machine trusted keys
integrity: store reference to machine keyring
efi/mokvar: move up init order
integrity: Trust MOK keys if MokListTrustedRT found
integrity: Only use machine keyring when uefi_check_trust_mok_keys is
true

certs/system_keyring.c | 44 ++++++++++-
crypto/asymmetric_keys/restrict.c | 43 +++++++++++
crypto/asymmetric_keys/x509_cert_parser.c | 9 +++
drivers/firmware/efi/mokvar-table.c | 2 +-
include/crypto/public_key.h | 15 ++++
include/keys/system_keyring.h | 14 ++++
security/integrity/Kconfig | 12 +++
security/integrity/Makefile | 1 +
security/integrity/digsig.c | 23 +++++-
security/integrity/integrity.h | 17 +++-
.../platform_certs/keyring_handler.c | 18 ++++-
.../platform_certs/keyring_handler.h | 5 ++
security/integrity/platform_certs/load_uefi.c | 4 +-
.../platform_certs/machine_keyring.c | 77 +++++++++++++++++++
14 files changed, 273 insertions(+), 11 deletions(-)
create mode 100644 security/integrity/platform_certs/machine_keyring.c


base-commit: fa55b7dcdc43c1aa1ba12bca9d2dd4318c2a0dbf
--
2.18.4



2021-11-16 00:19:19

by Eric Snowberg

[permalink] [raw]
Subject: [PATCH v7 07/17] integrity: Fix warning about missing prototypes

make W=1 generates the following warning in keyring_handler.c

security/integrity/platform_certs/keyring_handler.c:71:30: warning: no previous prototype for get_handler_for_db [-Wmissing-prototypes]
__init efi_element_handler_t get_handler_for_db(const efi_guid_t *sig_type)
^~~~~~~~~~~~~~~~~~
security/integrity/platform_certs/keyring_handler.c:82:30: warning: no previous prototype for get_handler_for_dbx [-Wmissing-prototypes]
__init efi_element_handler_t get_handler_for_dbx(const efi_guid_t *sig_type)
^~~~~~~~~~~~~~~~~~~
Add the missing prototypes by including keyring_handler.h.

Signed-off-by: Eric Snowberg <[email protected]>
---
v7: Initial version
---
security/integrity/platform_certs/keyring_handler.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/security/integrity/platform_certs/keyring_handler.c b/security/integrity/platform_certs/keyring_handler.c
index 5604bd57c990..e9791be98fd9 100644
--- a/security/integrity/platform_certs/keyring_handler.c
+++ b/security/integrity/platform_certs/keyring_handler.c
@@ -9,6 +9,7 @@
#include <keys/asymmetric-type.h>
#include <keys/system_keyring.h>
#include "../integrity.h"
+#include "keyring_handler.h"

static efi_guid_t efi_cert_x509_guid __initdata = EFI_CERT_X509_GUID;
static efi_guid_t efi_cert_x509_sha256_guid __initdata =
--
2.18.4


2021-11-16 00:19:22

by Eric Snowberg

[permalink] [raw]
Subject: [PATCH v7 13/17] KEYS: link secondary_trusted_keys to machine trusted keys

Allow the .machine keyring to be linked to the secondary_trusted_keys.
After the link is created, keys contained in the .machine keyring will
automatically be searched when searching secondary_trusted_keys.

Signed-off-by: Eric Snowberg <[email protected]>
---
v3: Initial version
v4: Unmodified from v3
v5: Rename to machine keyring
v7: Unmodified from v5
---
certs/system_keyring.c | 3 +++
1 file changed, 3 insertions(+)

diff --git a/certs/system_keyring.c b/certs/system_keyring.c
index ba732856ebd0..2a2dc70b126c 100644
--- a/certs/system_keyring.c
+++ b/certs/system_keyring.c
@@ -101,6 +101,9 @@ static __init struct key_restriction *get_secondary_restriction(void)
void __init set_machine_trusted_keys(struct key *keyring)
{
machine_trusted_keys = keyring;
+
+ if (key_link(secondary_trusted_keys, machine_trusted_keys) < 0)
+ panic("Can't link (machine) trusted keyrings\n");
}

/**
--
2.18.4


2021-11-16 00:19:23

by Eric Snowberg

[permalink] [raw]
Subject: [PATCH v7 15/17] efi/mokvar: move up init order

Move up the init order so it can be used by the new machine keyring.

Signed-off-by: Eric Snowberg <[email protected]>
---
v7: Initial version
---
drivers/firmware/efi/mokvar-table.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/mokvar-table.c b/drivers/firmware/efi/mokvar-table.c
index 38722d2009e2..5ed0602c2f75 100644
--- a/drivers/firmware/efi/mokvar-table.c
+++ b/drivers/firmware/efi/mokvar-table.c
@@ -359,4 +359,4 @@ static int __init efi_mokvar_sysfs_init(void)
}
return err;
}
-device_initcall(efi_mokvar_sysfs_init);
+fs_initcall(efi_mokvar_sysfs_init);
--
2.18.4


2021-11-16 00:19:23

by Eric Snowberg

[permalink] [raw]
Subject: [PATCH v7 14/17] integrity: store reference to machine keyring

Store a reference to the machine keyring in system keyring code. The
system keyring code needs this to complete the keyring link to
to machine keyring.

Signed-off-by: Eric Snowberg <[email protected]>
---
v2: Initial version
v3: Unmodified from v2
v4: Removed trust_moklist check
v5: Rename to machine keyring
v7: Unmodified from v5
---
security/integrity/digsig.c | 2 ++
1 file changed, 2 insertions(+)

diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
index 74f73f7cc4fe..109b58840d45 100644
--- a/security/integrity/digsig.c
+++ b/security/integrity/digsig.c
@@ -116,6 +116,8 @@ 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)
+ set_machine_trusted_keys(keyring[id]);
if (id == INTEGRITY_KEYRING_IMA)
load_module_cert(keyring[id]);
}
--
2.18.4


2021-11-16 00:19:24

by Eric Snowberg

[permalink] [raw]
Subject: [PATCH v7 16/17] integrity: Trust MOK keys if MokListTrustedRT found

A new Machine Owner Key (MOK) variable called MokListTrustedRT has been
introduced in shim. When this UEFI variable is set, it indicates the
end-user has made the decision themselves that they wish to trust MOK keys
within the Linux trust boundary. It is not an error if this variable
does not exist. If it does not exist, the MOK keys should not be trusted
within the kernel.

Signed-off-by: Eric Snowberg <[email protected]>
---
v1: Initial version
v2: Removed mok_keyring_trust_setup function
v4: Unmodified from v2
v5: Rename to machine keyring
v6: Unmodified from v5
v7: Use mokvar table instead of EFI var (suggested by Peter Jones)
---
.../platform_certs/machine_keyring.c | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)

diff --git a/security/integrity/platform_certs/machine_keyring.c b/security/integrity/platform_certs/machine_keyring.c
index ea2ac2f9f2b5..09fd8f20c756 100644
--- a/security/integrity/platform_certs/machine_keyring.c
+++ b/security/integrity/platform_certs/machine_keyring.c
@@ -5,6 +5,7 @@
* Copyright (c) 2021, Oracle and/or its affiliates.
*/

+#include <linux/efi.h>
#include "../integrity.h"

static __init int machine_keyring_init(void)
@@ -40,3 +41,21 @@ void __init add_to_machine_keyring(const char *source, const void *data, size_t
if (rc)
pr_info("Error adding keys to machine keyring %s\n", source);
}
+
+/*
+ * Try to load the MokListTrustedRT MOK variable to see if we should trust
+ * the MOK keys within the kernel. It is not an error if this variable
+ * does not exist. If it does not exist, MOK keys should not be trusted
+ * within the machine keyring.
+ */
+static __init bool uefi_check_trust_mok_keys(void)
+{
+ struct efi_mokvar_table_entry *mokvar_entry;
+
+ mokvar_entry = efi_mokvar_entry_find("MokListTrustedRT");
+
+ if (mokvar_entry)
+ return true;
+
+ return false;
+}
--
2.18.4


2021-11-16 00:19:28

by Eric Snowberg

[permalink] [raw]
Subject: [PATCH v7 10/17] KEYS: add a reference to machine keyring

Expose the .machine keyring created in integrity code by adding
a reference. This makes the machine keyring accessible for keyring
restrictions in the future.

Signed-off-by: Eric Snowberg <[email protected]>
---
v2: Initial version
v3: set_mok_trusted_keys only available when secondary is enabled
v4: Moved code under CONFIG_INTEGRITY_MOK_KEYRING
v5: Rename to machine keyring
v7: Unmodified from v5
---
certs/system_keyring.c | 9 +++++++++
include/keys/system_keyring.h | 8 ++++++++
2 files changed, 17 insertions(+)

diff --git a/certs/system_keyring.c b/certs/system_keyring.c
index 8f1f87579819..bc7e44fc82c2 100644
--- a/certs/system_keyring.c
+++ b/certs/system_keyring.c
@@ -22,6 +22,9 @@ static struct key *builtin_trusted_keys;
#ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
static struct key *secondary_trusted_keys;
#endif
+#ifdef CONFIG_INTEGRITY_MACHINE_KEYRING
+static struct key *machine_trusted_keys;
+#endif
#ifdef CONFIG_INTEGRITY_PLATFORM_KEYRING
static struct key *platform_trusted_keys;
#endif
@@ -91,6 +94,12 @@ static __init struct key_restriction *get_secondary_restriction(void)
return restriction;
}
#endif
+#ifdef CONFIG_INTEGRITY_MACHINE_KEYRING
+void __init set_machine_trusted_keys(struct key *keyring)
+{
+ machine_trusted_keys = keyring;
+}
+#endif

/*
* Create the trusted keyrings
diff --git a/include/keys/system_keyring.h b/include/keys/system_keyring.h
index 6acd3cf13a18..98c9b10cdc17 100644
--- a/include/keys/system_keyring.h
+++ b/include/keys/system_keyring.h
@@ -38,6 +38,14 @@ extern int restrict_link_by_builtin_and_secondary_trusted(
#define restrict_link_by_builtin_and_secondary_trusted restrict_link_by_builtin_trusted
#endif

+#ifdef CONFIG_INTEGRITY_MACHINE_KEYRING
+extern void __init set_machine_trusted_keys(struct key *keyring);
+#else
+static inline void __init set_machine_trusted_keys(struct key *keyring)
+{
+}
+#endif
+
extern struct pkcs7_message *pkcs7;
#ifdef CONFIG_SYSTEM_BLACKLIST_KEYRING
extern int mark_hash_blacklisted(const char *hash);
--
2.18.4


2021-11-16 00:19:29

by Eric Snowberg

[permalink] [raw]
Subject: [PATCH v7 11/17] KEYS: Introduce link restriction for machine keys

Introduce a new link restriction that includes the trusted builtin,
secondary and machine keys. The restriction is based on the key to be
added being vouched for by a key in any of these three keyrings.

Suggested-by: Mimi Zohar <[email protected]>
Signed-off-by: Eric Snowberg <[email protected]>
---
v3: Initial version
v4: moved code under CONFIG_INTEGRITY_MOK_KEYRING
v5: Rename to machine keyring
v6: Change subject name (suggested by Mimi)
Rename restrict_link_by_builtin_secondary_and_ca_trusted
to restrict_link_by_builtin_secondary_and_machine (suggested by
Mimi)
v7: Unmodified from v6
---
certs/system_keyring.c | 23 +++++++++++++++++++++++
include/keys/system_keyring.h | 6 ++++++
2 files changed, 29 insertions(+)

diff --git a/certs/system_keyring.c b/certs/system_keyring.c
index bc7e44fc82c2..71a00add9805 100644
--- a/certs/system_keyring.c
+++ b/certs/system_keyring.c
@@ -99,6 +99,29 @@ void __init set_machine_trusted_keys(struct key *keyring)
{
machine_trusted_keys = keyring;
}
+
+/**
+ * restrict_link_by_builtin_secondary_and_machine
+ *
+ * Restrict the addition of keys into a keyring based on the key-to-be-added
+ * being vouched for by a key in either the built-in, the secondary, or
+ * the machine keyrings.
+ */
+int restrict_link_by_builtin_secondary_and_machine(
+ struct key *dest_keyring,
+ const struct key_type *type,
+ const union key_payload *payload,
+ struct key *restrict_key)
+{
+ if (machine_trusted_keys && type == &key_type_keyring &&
+ dest_keyring == secondary_trusted_keys &&
+ payload == &machine_trusted_keys->payload)
+ /* Allow the machine keyring to be added to the secondary */
+ return 0;
+
+ return restrict_link_by_builtin_and_secondary_trusted(dest_keyring, type,
+ payload, restrict_key);
+}
#endif

/*
diff --git a/include/keys/system_keyring.h b/include/keys/system_keyring.h
index 98c9b10cdc17..2419a735420f 100644
--- a/include/keys/system_keyring.h
+++ b/include/keys/system_keyring.h
@@ -39,8 +39,14 @@ extern int restrict_link_by_builtin_and_secondary_trusted(
#endif

#ifdef CONFIG_INTEGRITY_MACHINE_KEYRING
+extern int restrict_link_by_builtin_secondary_and_machine(
+ struct key *dest_keyring,
+ const struct key_type *type,
+ const union key_payload *payload,
+ struct key *restrict_key);
extern void __init set_machine_trusted_keys(struct key *keyring);
#else
+#define restrict_link_by_builtin_secondary_and_machine restrict_link_by_builtin_trusted
static inline void __init set_machine_trusted_keys(struct key *keyring)
{
}
--
2.18.4


2021-11-16 00:19:30

by Eric Snowberg

[permalink] [raw]
Subject: [PATCH v7 17/17] integrity: Only use machine keyring when uefi_check_trust_mok_keys is true

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 <[email protected]>
---
v4: Initial version
v5: Rename to machine keyring
v6: Unmodified from v5
v7: Made trust_mok static
---
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 109b58840d45..1de09c7b5f93 100644
--- a/security/integrity/digsig.c
+++ b/security/integrity/digsig.c
@@ -116,7 +116,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


2021-11-16 00:19:32

by Eric Snowberg

[permalink] [raw]
Subject: [PATCH v7 09/17] KEYS: Rename get_builtin_and_secondary_restriction

To improve clarity, rename get_builtin_and_secondary_restriction to
get_secondary_restriction.

Suggested-by: Mimi Zohar <[email protected]>
Signed-off-by: Eric Snowberg <[email protected]>
---
v6: Initial version
v7: Unmodified from v7
---
certs/system_keyring.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/certs/system_keyring.c b/certs/system_keyring.c
index 692365dee2bd..8f1f87579819 100644
--- a/certs/system_keyring.c
+++ b/certs/system_keyring.c
@@ -77,7 +77,7 @@ int restrict_link_by_builtin_and_secondary_trusted(
* Allocate a struct key_restriction for the "builtin and secondary trust"
* keyring. Only for use in system_trusted_keyring_init().
*/
-static __init struct key_restriction *get_builtin_and_secondary_restriction(void)
+static __init struct key_restriction *get_secondary_restriction(void)
{
struct key_restriction *restriction;

@@ -117,7 +117,7 @@ static __init int system_trusted_keyring_init(void)
KEY_USR_VIEW | KEY_USR_READ | KEY_USR_SEARCH |
KEY_USR_WRITE),
KEY_ALLOC_NOT_IN_QUOTA,
- get_builtin_and_secondary_restriction(),
+ get_secondary_restriction(),
NULL);
if (IS_ERR(secondary_trusted_keys))
panic("Can't allocate secondary trusted keyring\n");
--
2.18.4


2021-11-16 00:19:32

by Eric Snowberg

[permalink] [raw]
Subject: [PATCH v7 08/17] integrity: add new keyring handler for mok keys

Currently both Secure Boot DB and Machine Owner Keys (MOK) go through
the same keyring handler (get_handler_for_db). With the addition of the
new machine keyring, the end-user may choose to trust MOK keys.

Introduce a new keyring handler specific for MOK keys. If MOK keys are
trusted by the end-user, use the new keyring handler instead.

Signed-off-by: Eric Snowberg <[email protected]>
---
v1: Initial version
v3: Only change the keyring handler if the secondary is enabled
v4: Removed trust_moklist check
v5: Rename to machine keyring
v7: Unmodified from v5
---
.../integrity/platform_certs/keyring_handler.c | 17 ++++++++++++++++-
.../integrity/platform_certs/keyring_handler.h | 5 +++++
security/integrity/platform_certs/load_uefi.c | 4 ++--
3 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/security/integrity/platform_certs/keyring_handler.c b/security/integrity/platform_certs/keyring_handler.c
index e9791be98fd9..4872850d081f 100644
--- a/security/integrity/platform_certs/keyring_handler.c
+++ b/security/integrity/platform_certs/keyring_handler.c
@@ -67,7 +67,7 @@ static __init void uefi_revocation_list_x509(const char *source,

/*
* Return the appropriate handler for particular signature list types found in
- * the UEFI db and MokListRT tables.
+ * the UEFI db tables.
*/
__init efi_element_handler_t get_handler_for_db(const efi_guid_t *sig_type)
{
@@ -76,6 +76,21 @@ __init efi_element_handler_t get_handler_for_db(const efi_guid_t *sig_type)
return 0;
}

+/*
+ * Return the appropriate handler for particular signature list types found in
+ * the MokListRT tables.
+ */
+__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))
+ return add_to_machine_keyring;
+ else
+ return add_to_platform_keyring;
+ }
+ return 0;
+}
+
/*
* Return the appropriate handler for particular signature list types found in
* the UEFI dbx and MokListXRT tables.
diff --git a/security/integrity/platform_certs/keyring_handler.h b/security/integrity/platform_certs/keyring_handler.h
index 2462bfa08fe3..284558f30411 100644
--- a/security/integrity/platform_certs/keyring_handler.h
+++ b/security/integrity/platform_certs/keyring_handler.h
@@ -24,6 +24,11 @@ void blacklist_binary(const char *source, const void *data, size_t len);
*/
efi_element_handler_t get_handler_for_db(const efi_guid_t *sig_type);

+/*
+ * Return the handler for particular signature list types found in the mok.
+ */
+efi_element_handler_t get_handler_for_mok(const efi_guid_t *sig_type);
+
/*
* Return the handler for particular signature list types found in the dbx.
*/
diff --git a/security/integrity/platform_certs/load_uefi.c b/security/integrity/platform_certs/load_uefi.c
index f290f78c3f30..c1bfd1cd7cc3 100644
--- a/security/integrity/platform_certs/load_uefi.c
+++ b/security/integrity/platform_certs/load_uefi.c
@@ -94,7 +94,7 @@ static int __init load_moklist_certs(void)
rc = parse_efi_signature_list("UEFI:MokListRT (MOKvar table)",
mokvar_entry->data,
mokvar_entry->data_size,
- get_handler_for_db);
+ get_handler_for_mok);
/* All done if that worked. */
if (!rc)
return rc;
@@ -109,7 +109,7 @@ static int __init load_moklist_certs(void)
mok = get_cert_list(L"MokListRT", &mok_var, &moksize, &status);
if (mok) {
rc = parse_efi_signature_list("UEFI:MokListRT",
- mok, moksize, get_handler_for_db);
+ mok, moksize, get_handler_for_mok);
kfree(mok);
if (rc)
pr_err("Couldn't parse MokListRT signatures: %d\n", rc);
--
2.18.4


2021-11-16 00:19:34

by Eric Snowberg

[permalink] [raw]
Subject: [PATCH v7 03/17] KEYS: Create static version of public_key_verify_signature

The kernel test robot reports undefined reference to
public_key_verify_signature when CONFIG_ASYMMETRIC_PUBLIC_KEY_SUBTYPE is
not defined. Create a static version in this case and return -EINVAL.

Reported-by: kernel test robot <[email protected]>
Signed-off-by: Eric Snowberg <[email protected]>
---
v7: Initial version
---
include/crypto/public_key.h | 9 +++++++++
1 file changed, 9 insertions(+)

diff --git a/include/crypto/public_key.h b/include/crypto/public_key.h
index f603325c0c30..a9b2e600b7cc 100644
--- a/include/crypto/public_key.h
+++ b/include/crypto/public_key.h
@@ -80,7 +80,16 @@ extern int create_signature(struct kernel_pkey_params *, const void *, void *);
extern int verify_signature(const struct key *,
const struct public_key_signature *);

+#if IS_REACHABLE(CONFIG_ASYMMETRIC_PUBLIC_KEY_SUBTYPE)
int public_key_verify_signature(const struct public_key *pkey,
const struct public_key_signature *sig);
+#else
+static inline
+int public_key_verify_signature(const struct public_key *pkey,
+ const struct public_key_signature *sig)
+{
+ return -EINVAL;
+}
+#endif

#endif /* _LINUX_PUBLIC_KEY_H */
--
2.18.4


2021-11-16 16:00:40

by Jarkko Sakkinen

[permalink] [raw]
Subject: Re: [PATCH v7 00/17] Enroll kernel keys thru MOK

On Mon, 2021-11-15 at 19:15 -0500, Eric Snowberg wrote:
> Back in 2013 Linus requested a feature to allow end-users to have the
> ability "to add their own keys and sign modules they trust". This was
> his *second* order outlined here [1]. There have been many attempts
> over the years to solve this problem, all have been rejected.  Many
> of the failed attempts loaded all preboot firmware keys into the kernel,
> including the Secure Boot keys. Many distributions carry one of these
> rejected attempts [2], [3], [4]. This series tries to solve this problem
> with a solution that takes into account all the problems brought up in
> the previous attempts.
>
> On UEFI based systems, this series introduces a new Linux kernel keyring
> containing the Machine Owner Keys (MOK) called machine. It also defines
> a new MOK variable in shim. This variable allows the end-user to decide
> if they want to load MOK keys into the machine keyring. Mimi has suggested
> that only CA keys contained within the MOK be loaded into the machine
> keyring. All other certs will load into the platform keyring instead.
>
> By default, nothing changes; MOK keys are not loaded into the machine
> keyring.  They are only loaded after the end-user makes the decision
> themselves.  The end-user would set this through mokutil using a new
> --trust-mok option [5]. This would work similar to how the kernel uses
> MOK variables to enable/disable signature validation as well as use/ignore
> the db. Any kernel operation that uses either the builtin or secondary
> trusted keys as a trust source shall also reference the new machine
> keyring as a trust source.
>
> Secure Boot keys will never be loaded into the machine keyring.  They
> will always be loaded into the platform keyring.  If an end-user wanted
> to load one, they would need to enroll it into the MOK.
>
> Steps required by the end user:
>
> Sign kernel module with user created key:
> $ /usr/src/kernels/$(uname -r)/scripts/sign-file sha512 \
>    machine_signing_key.priv machine_signing_key.x509 my_module.ko
>
> Import the key into the MOK
> $ mokutil --import machine_signing_key.x509
>
> Setup the kernel to load MOK keys into the .machine keyring
> $ mokutil --trust-mok
>
> Then reboot, the MokManager will load and ask if you want to trust the
> MOK key and enroll the MOK into the MOKList.  Afterwards the signed kernel
> module will load.
>
> I have included  a link to the mokutil [5] changes I have made to support
> this new functionality.  The shim changes have now been accepted
> upstream [6].
>
> [1] https://marc.info/?l=linux-kernel&m=136185386310140&w=2
> [2] https://lore.kernel.org/lkml/[email protected]/
> [3] https://lore.kernel.org/lkml/[email protected]/
> [4] https://lore.kernel.org/linux-integrity/1e41f22b1f11784f1e943f32bf62034d4e054cdb.camel@HansenPartnership.com/
> [5] https://github.com/esnowberg/mokutil/tree/mokvars-v3
> [6] https://github.com/rhboot/shim/commit/4e513405b4f1641710115780d19dcec130c5208f
>
> Eric Snowberg (17):
>   integrity: Introduce a Linux keyring called machine
>   integrity: Do not allow machine keyring updates following init
>   KEYS: Create static version of public_key_verify_signature
>   X.509: Parse Basic Constraints for CA
>   KEYS: CA link restriction
>   integrity: restrict INTEGRITY_KEYRING_MACHINE to restrict_link_by_ca
>   integrity: Fix warning about missing prototypes
>   integrity: add new keyring handler for mok keys
>   KEYS: Rename get_builtin_and_secondary_restriction
>   KEYS: add a reference to machine keyring
>   KEYS: Introduce link restriction for machine keys
>   KEYS: integrity: change link restriction to trust the machine keyring
>   KEYS: link secondary_trusted_keys to machine trusted keys
>   integrity: store reference to machine keyring
>   efi/mokvar: move up init order
>   integrity: Trust MOK keys if MokListTrustedRT found
>   integrity: Only use machine keyring when uefi_check_trust_mok_keys is
>     true
>
>  certs/system_keyring.c                        | 44 ++++++++++-
>  crypto/asymmetric_keys/restrict.c             | 43 +++++++++++
>  crypto/asymmetric_keys/x509_cert_parser.c     |  9 +++
>  drivers/firmware/efi/mokvar-table.c           |  2 +-
>  include/crypto/public_key.h                   | 15 ++++
>  include/keys/system_keyring.h                 | 14 ++++
>  security/integrity/Kconfig                    | 12 +++
>  security/integrity/Makefile                   |  1 +
>  security/integrity/digsig.c                   | 23 +++++-
>  security/integrity/integrity.h                | 17 +++-
>  .../platform_certs/keyring_handler.c          | 18 ++++-
>  .../platform_certs/keyring_handler.h          |  5 ++
>  security/integrity/platform_certs/load_uefi.c |  4 +-
>  .../platform_certs/machine_keyring.c          | 77 +++++++++++++++++++
>  14 files changed, 273 insertions(+), 11 deletions(-)
>  create mode 100644 security/integrity/platform_certs/machine_keyring.c
>
>
> base-commit: fa55b7dcdc43c1aa1ba12bca9d2dd4318c2a0dbf

Does shim have the necessary features in a release?

/Jarkko

2021-11-16 16:19:16

by Konrad Rzeszutek Wilk

[permalink] [raw]
Subject: Re: [PATCH v7 00/17] Enroll kernel keys thru MOK

> > I have included? a link to the mokutil [5] changes I have made to support
> > this new functionality.? The shim changes have now been accepted
> > upstream [6].

..snip..
> > [6] https://github.com/rhboot/shim/commit/4e513405b4f1641710115780d19dcec130c5208f

..snip..
>
> Does shim have the necessary features in a release?

Hi!

It has been accepted in the upstream shim. If you are looking
for a distribution having rolled out a shim with this feature (so signed
by MSF) I fear that distributions are not that fast with shim releases.

Also these:
https://github.com/rhboot/shim/pulls
https://github.com/rhboot/shim/issues

do mean some extra work would need to go in before an official
release is cut.

Hope this helps?

2021-11-16 16:25:00

by Jarkko Sakkinen

[permalink] [raw]
Subject: Re: [PATCH v7 00/17] Enroll kernel keys thru MOK

On Tue, 2021-11-16 at 11:18 -0500, Konrad Rzeszutek Wilk wrote:
> > > I have included  a link to the mokutil [5] changes I have made to support
> > > this new functionality.  The shim changes have now been accepted
> > > upstream [6].
>
> ..snip..
> > > [6] https://github.com/rhboot/shim/commit/4e513405b4f1641710115780d19dcec130c5208f
>
> ..snip..
> >
> > Does shim have the necessary features in a release?
>
> Hi!
>
> It has been accepted in the upstream shim. If you are looking
> for a distribution having rolled out a shim with this feature (so signed
> by MSF) I fear that distributions are not that fast with shim releases.
>
> Also these:
> https://github.com/rhboot/shim/pulls
> https://github.com/rhboot/shim/issues
>
> do mean some extra work would need to go in before an official
> release is cut.
>
> Hope this helps?

Yes. I'll hold with this up until there is an official release. Thank you.

/Jarkko

2021-11-16 16:41:38

by Konrad Rzeszutek Wilk

[permalink] [raw]
Subject: Re: [PATCH v7 00/17] Enroll kernel keys thru MOK

On Tue, Nov 16, 2021 at 06:24:52PM +0200, Jarkko Sakkinen wrote:
> On Tue, 2021-11-16 at 11:18 -0500, Konrad Rzeszutek Wilk wrote:
> > > > I have included? a link to the mokutil [5] changes I have made to support
> > > > this new functionality.? The shim changes have now been accepted
> > > > upstream [6].
> >
> > ..snip..
> > > > [6] https://github.com/rhboot/shim/commit/4e513405b4f1641710115780d19dcec130c5208f
> >
> > ..snip..
> > >
> > > Does shim have the necessary features in a release?
> >
> > Hi!
> >
> > It has been accepted in the upstream shim. If you are looking
> > for a distribution having rolled out a shim with this feature (so signed
> > by MSF) I fear that distributions are not that fast with shim releases.
> >
> > Also these:
> > https://github.com/rhboot/shim/pulls
> > https://github.com/rhboot/shim/issues
> >
> > do mean some extra work would need to go in before an official
> > release is cut.
> >
> > Hope this helps?
>
> Yes. I'll hold with this up until there is an official release. Thank you.

Not sure I understand - but what are the concerns you have with shim
code that has been accepted?


2021-11-17 07:50:39

by Jarkko Sakkinen

[permalink] [raw]
Subject: Re: [PATCH v7 00/17] Enroll kernel keys thru MOK

On Tue, 2021-11-16 at 11:39 -0500, Konrad Rzeszutek Wilk wrote:
> On Tue, Nov 16, 2021 at 06:24:52PM +0200, Jarkko Sakkinen wrote:
> > On Tue, 2021-11-16 at 11:18 -0500, Konrad Rzeszutek Wilk wrote:
> > > > > I have included  a link to the mokutil [5] changes I have made to support
> > > > > this new functionality.  The shim changes have now been accepted
> > > > > upstream [6].
> > >
> > > ..snip..
> > > > > [6] https://github.com/rhboot/shim/commit/4e513405b4f1641710115780d19dcec130c5208f
> > >
> > > ..snip..
> > > >
> > > > Does shim have the necessary features in a release?
> > >
> > > Hi!
> > >
> > > It has been accepted in the upstream shim. If you are looking
> > > for a distribution having rolled out a shim with this feature (so signed
> > > by MSF) I fear that distributions are not that fast with shim releases.
~~~

Should that be MS, or what does MSF mean?

> > >
> > > Also these:
> > > https://github.com/rhboot/shim/pulls
> > > https://github.com/rhboot/shim/issues
> > >
> > > do mean some extra work would need to go in before an official
> > > release is cut.
> > >
> > > Hope this helps?
> >
> > Yes. I'll hold with this up until there is an official release. Thank you.
>
> Not sure I understand - but what are the concerns you have with shim
> code that has been accepted?

Maybe my concern is that none of the patches have a tested-by?

Probably would be easier to get a test coverage, e.g. for people like
me who do not even know how to self-compile Shim, how to setup user
space using the product and so forth.

I don't demand a release, if the changes have been accepted, but 17
patches do need to be tested.

/Jarkko



2021-11-17 07:51:30

by Jarkko Sakkinen

[permalink] [raw]
Subject: Re: [PATCH v7 00/17] Enroll kernel keys thru MOK

On Wed, 2021-11-17 at 09:50 +0200, Jarkko Sakkinen wrote:
> On Tue, 2021-11-16 at 11:39 -0500, Konrad Rzeszutek Wilk wrote:
> > On Tue, Nov 16, 2021 at 06:24:52PM +0200, Jarkko Sakkinen wrote:
> > > On Tue, 2021-11-16 at 11:18 -0500, Konrad Rzeszutek Wilk wrote:
> > > > > > I have included  a link to the mokutil [5] changes I have made to support
> > > > > > this new functionality.  The shim changes have now been accepted
> > > > > > upstream [6].
> > > >
> > > > ..snip..
> > > > > > [6] https://github.com/rhboot/shim/commit/4e513405b4f1641710115780d19dcec130c5208f
> > > >
> > > > ..snip..
> > > > >
> > > > > Does shim have the necessary features in a release?
> > > >
> > > > Hi!
> > > >
> > > > It has been accepted in the upstream shim. If you are looking
> > > > for a distribution having rolled out a shim with this feature (so signed
> > > > by MSF) I fear that distributions are not that fast with shim releases.
>          ~~~
>
> Should that be MS, or what does MSF mean?
>
> > > >
> > > > Also these:
> > > > https://github.com/rhboot/shim/pulls
> > > > https://github.com/rhboot/shim/issues
> > > >
> > > > do mean some extra work would need to go in before an official
> > > > release is cut.
> > > >
> > > > Hope this helps?
> > >
> > > Yes. I'll hold with this up until there is an official release. Thank you.
> >
> > Not sure I understand - but what are the concerns you have with shim
> > code that has been accepted?
>
> Maybe my concern is that none of the patches have a tested-by?
>
> Probably would be easier to get a test coverage, e.g. for people like
> me who do not even know how to self-compile Shim, how to setup user
> space using the product and so forth.
~~~~~~~~~~~~~~~~~

for the end product

/Jarkko




2021-11-17 13:32:59

by Mimi Zohar

[permalink] [raw]
Subject: Re: [PATCH v7 03/17] KEYS: Create static version of public_key_verify_signature

On Mon, 2021-11-15 at 19:15 -0500, Eric Snowberg wrote:
> The kernel test robot reports undefined reference to
> public_key_verify_signature when CONFIG_ASYMMETRIC_PUBLIC_KEY_SUBTYPE is
> not defined. Create a static version in this case and return -EINVAL.
>
> Reported-by: kernel test robot <[email protected]>
> Signed-off-by: Eric Snowberg <[email protected]>

Reviewed-by: Mimi Zohar <[email protected]>


2021-11-17 13:54:39

by Mimi Zohar

[permalink] [raw]
Subject: Re: [PATCH v7 03/17] KEYS: Create static version of public_key_verify_signature

On Wed, 2021-11-17 at 08:32 -0500, Mimi Zohar wrote:
> On Mon, 2021-11-15 at 19:15 -0500, Eric Snowberg wrote:
> > The kernel test robot reports undefined reference to
> > public_key_verify_signature when CONFIG_ASYMMETRIC_PUBLIC_KEY_SUBTYPE is
> > not defined. Create a static version in this case and return -EINVAL.
> >
> > Reported-by: kernel test robot <[email protected]>
> > Signed-off-by: Eric Snowberg <[email protected]>
>
> Reviewed-by: Mimi Zohar <[email protected]>

Perhaps this patch wouldn't be needed if

+config INTEGRITY_MACHINE_KEYRING
+ bool "Provide a keyring to which CA Machine Owner Keys may be
added"
+ depends on SECONDARY_TRUSTED_KEYRING
+ depends on INTEGRITY_ASYMMETRIC_KEYS

depends on ASYMMETRIC_PUBLIC_KEY_SUBTYPE=y

+ depends on SYSTEM_BLACKLIST_KEYRING
+ depends on LOAD_UEFI_KEYS

Mimi


2021-11-17 15:16:58

by Mimi Zohar

[permalink] [raw]
Subject: Re: [PATCH v7 07/17] integrity: Fix warning about missing prototypes

Hi Eric,

On Mon, 2021-11-15 at 19:15 -0500, Eric Snowberg wrote:
> make W=1 generates the following warning in keyring_handler.c
>
> security/integrity/platform_certs/keyring_handler.c:71:30: warning: no previous prototype for get_handler_for_db [-Wmissing-prototypes]
> __init efi_element_handler_t get_handler_for_db(const efi_guid_t *sig_type)
> ^~~~~~~~~~~~~~~~~~
> security/integrity/platform_certs/keyring_handler.c:82:30: warning: no previous prototype for get_handler_for_dbx [-Wmissing-prototypes]
> __init efi_element_handler_t get_handler_for_dbx(const efi_guid_t *sig_type)
> ^~~~~~~~~~~~~~~~~~~
> Add the missing prototypes by including keyring_handler.h.
>
> Signed-off-by: Eric Snowberg <[email protected]>

These sorts of fixes, which aren't really dependent on the patch set,
could be moved to the begining of the patch set.

Reviewed-by: Mimi Zohar <[email protected]>


2021-11-17 17:03:17

by Konrad Rzeszutek Wilk

[permalink] [raw]
Subject: Re: [PATCH v7 00/17] Enroll kernel keys thru MOK

On Wed, Nov 17, 2021 at 09:51:25AM +0200, Jarkko Sakkinen wrote:
> On Wed, 2021-11-17 at 09:50 +0200, Jarkko Sakkinen wrote:
> > On Tue, 2021-11-16 at 11:39 -0500, Konrad Rzeszutek Wilk wrote:
> > > On Tue, Nov 16, 2021 at 06:24:52PM +0200, Jarkko Sakkinen wrote:
> > > > On Tue, 2021-11-16 at 11:18 -0500, Konrad Rzeszutek Wilk wrote:
> > > > > > > I have included? a link to the mokutil [5] changes I have made to support
> > > > > > > this new functionality.? The shim changes have now been accepted
> > > > > > > upstream [6].
> > > > >
> > > > > ..snip..
> > > > > > > [6] https://github.com/rhboot/shim/commit/4e513405b4f1641710115780d19dcec130c5208f
> > > > >
> > > > > ..snip..
> > > > > >
> > > > > > Does shim have the necessary features in a release?
> > > > >
> > > > > Hi!
> > > > >
> > > > > It has been accepted in the upstream shim. If you are looking
> > > > > for a distribution having rolled out a shim with this feature (so signed
> > > > > by MSF) I fear that distributions are not that fast with shim releases.
> > ???????? ~~~
> >
> > Should that be MS, or what does MSF mean?

Microsoft :-)

> >
> > > > >
> > > > > Also these:
> > > > > https://github.com/rhboot/shim/pulls
> > > > > https://github.com/rhboot/shim/issues
> > > > >
> > > > > do mean some extra work would need to go in before an official
> > > > > release is cut.
> > > > >
> > > > > Hope this helps?
> > > >
> > > > Yes. I'll hold with this up until there is an official release. Thank you.
> > >
> > > Not sure I understand - but what are the concerns you have with shim
> > > code that has been accepted?
> >
> > Maybe my concern is that none of the patches have a tested-by?
> >
> > Probably would be easier to get a test coverage, e.g. for people like
> > me who do not even know how to self-compile Shim, how to setup user
> > space using the product and so forth.
> ~~~~~~~~~~~~~~~~~
>
> for the end product

<nods> That makes total sense. Thanks for the explanation, let me double
check whether

https://github.com/rhboot/shim/blob/main/BUILDING

is still correct.
>
> /Jarkko
>
>
>

2021-11-17 17:21:39

by Eric Snowberg

[permalink] [raw]
Subject: Re: [PATCH v7 00/17] Enroll kernel keys thru MOK



> On Nov 17, 2021, at 10:02 AM, Konrad Wilk <[email protected]> wrote:
>
> On Wed, Nov 17, 2021 at 09:51:25AM +0200, Jarkko Sakkinen wrote:
>> On Wed, 2021-11-17 at 09:50 +0200, Jarkko Sakkinen wrote:
>>> On Tue, 2021-11-16 at 11:39 -0500, Konrad Rzeszutek Wilk wrote:
>>>> On Tue, Nov 16, 2021 at 06:24:52PM +0200, Jarkko Sakkinen wrote:
>>>>> On Tue, 2021-11-16 at 11:18 -0500, Konrad Rzeszutek Wilk wrote:
>>>>>>>> I have included a link to the mokutil [5] changes I have made to support
>>>>>>>> this new functionality. The shim changes have now been accepted
>>>>>>>> upstream [6].
>>>>>>
>>>>>> ..snip..
>>>>>>>> [6] https://github.com/rhboot/shim/commit/4e513405b4f1641710115780d19dcec130c5208f
>>>>>>
>>>>>> ..snip..
>>>>>>>
>>>>>>> Does shim have the necessary features in a release?
>>>>>>
>>>>>> Hi!
>>>>>>
>>>>>> It has been accepted in the upstream shim. If you are looking
>>>>>> for a distribution having rolled out a shim with this feature (so signed
>>>>>> by MSF) I fear that distributions are not that fast with shim releases.
>>> ~~~
>>>
>>> Should that be MS, or what does MSF mean?
>
> Microsoft :-)

Correct, I’ll fix that in the next round.

>>>>>>
>>>>>> Also these:
>>>>>> https://github.com/rhboot/shim/pulls
>>>>>> https://github.com/rhboot/shim/issues
>>>>>>
>>>>>> do mean some extra work would need to go in before an official
>>>>>> release is cut.
>>>>>>
>>>>>> Hope this helps?
>>>>>
>>>>> Yes. I'll hold with this up until there is an official release. Thank you.
>>>>
>>>> Not sure I understand - but what are the concerns you have with shim
>>>> code that has been accepted?
>>>
>>> Maybe my concern is that none of the patches have a tested-by?
>>>
>>> Probably would be easier to get a test coverage, e.g. for people like
>>> me who do not even know how to self-compile Shim, how to setup user
>>> space using the product and so forth.
>> ~~~~~~~~~~~~~~~~~
>>
>> for the end product
>
> <nods> That makes total sense. Thanks for the explanation, let me double
> check whether
>
> https://github.com/rhboot/shim/blob/main/BUILDING
>
> is still correct.

Those are the steps I use for building. I then move over mmx64.efi and
shimx64.efi to the ESP. I can add the shim build/install instructions to the next
cover letter If you think that would be appropriate.




2021-11-18 03:14:17

by Jarkko Sakkinen

[permalink] [raw]
Subject: Re: [PATCH v7 00/17] Enroll kernel keys thru MOK

On Wed, 2021-11-17 at 17:20 +0000, Eric Snowberg wrote:
>
>
> > On Nov 17, 2021, at 10:02 AM, Konrad Wilk <[email protected]> wrote:
> >
> > On Wed, Nov 17, 2021 at 09:51:25AM +0200, Jarkko Sakkinen wrote:
> > > On Wed, 2021-11-17 at 09:50 +0200, Jarkko Sakkinen wrote:
> > > > On Tue, 2021-11-16 at 11:39 -0500, Konrad Rzeszutek Wilk wrote:
> > > > > On Tue, Nov 16, 2021 at 06:24:52PM +0200, Jarkko Sakkinen wrote:
> > > > > > On Tue, 2021-11-16 at 11:18 -0500, Konrad Rzeszutek Wilk wrote:
> > > > > > > > > I have included  a link to the mokutil [5] changes I have made to support
> > > > > > > > > this new functionality.  The shim changes have now been accepted
> > > > > > > > > upstream [6].
> > > > > > >
> > > > > > > ..snip..
> > > > > > > > > [6] https://github.com/rhboot/shim/commit/4e513405b4f1641710115780d19dcec130c5208f
> > > > > > >
> > > > > > > ..snip..
> > > > > > > >
> > > > > > > > Does shim have the necessary features in a release?
> > > > > > >
> > > > > > > Hi!
> > > > > > >
> > > > > > > It has been accepted in the upstream shim. If you are looking
> > > > > > > for a distribution having rolled out a shim with this feature (so signed
> > > > > > > by MSF) I fear that distributions are not that fast with shim releases.
> > > >          ~~~
> > > >
> > > > Should that be MS, or what does MSF mean?
> >
> > Microsoft :-)
>
> Correct, I’ll fix that in the next round.
>
> > > > > > >
> > > > > > > Also these:
> > > > > > > https://github.com/rhboot/shim/pulls
> > > > > > > https://github.com/rhboot/shim/issues
> > > > > > >
> > > > > > > do mean some extra work would need to go in before an official
> > > > > > > release is cut.
> > > > > > >
> > > > > > > Hope this helps?
> > > > > >
> > > > > > Yes. I'll hold with this up until there is an official release. Thank you.
> > > > >
> > > > > Not sure I understand - but what are the concerns you have with shim
> > > > > code that has been accepted?
> > > >
> > > > Maybe my concern is that none of the patches have a tested-by?
> > > >
> > > > Probably would be easier to get a test coverage, e.g. for people like
> > > > me who do not even know how to self-compile Shim, how to setup user
> > > > space using the product and so forth.
> > >        ~~~~~~~~~~~~~~~~~
> > >
> > > for the end product
> >
> > <nods> That makes total sense. Thanks for the explanation, let me double
> > check whether
> >
> > https://github.com/rhboot/shim/blob/main/BUILDING
> >
> > is still correct.
>
> Those are the steps I use for building.   I then move over mmx64.efi and 
> shimx64.efi to the ESP.  I can add the shim build/install instructions to the next
> cover letter If you think that would be appropriate.

Yeah, that would be great. I'll try to setup VM for that purpose. I have
already a script to build UEFI enabled archlinux VM's, which I use to
test SGX patches. I can probably tailor that for this purpose.

/Jarkko


2021-11-18 12:33:05

by Mimi Zohar

[permalink] [raw]
Subject: Re: [PATCH v7 13/17] KEYS: link secondary_trusted_keys to machine trusted keys

Hi Eric,

Is the subject line left over from the original patch? Shouldn't it
be "link machine trusted keys to secondary_trusted_keys".

On Mon, 2021-11-15 at 19:15 -0500, Eric Snowberg wrote:
> Allow the .machine keyring to be linked to the secondary_trusted_keys.
> After the link is created, keys contained in the .machine keyring will
> automatically be searched when searching secondary_trusted_keys.
>
> Signed-off-by: Eric Snowberg <[email protected]>
> ---
> v3: Initial version
> v4: Unmodified from v3
> v5: Rename to machine keyring
> v7: Unmodified from v5
> ---
> certs/system_keyring.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/certs/system_keyring.c b/certs/system_keyring.c
> index ba732856ebd0..2a2dc70b126c 100644
> --- a/certs/system_keyring.c
> +++ b/certs/system_keyring.c
> @@ -101,6 +101,9 @@ static __init struct key_restriction *get_secondary_restriction(void)
> void __init set_machine_trusted_keys(struct key *keyring)
> {
> machine_trusted_keys = keyring;
> +
> + if (key_link(secondary_trusted_keys, machine_trusted_keys) < 0)
> + panic("Can't link (machine) trusted keyrings\n");
> }
>
> /**

In general is the ordering of the patches "bisect safe"[1]? Only in
the next patch is machine_trusted_keys set. In this case, either
merge the two patches or reverse their order.

thanks,

Mimi

[1] Refer to the section "Separate your changes" in
Documentation/process/submitting-patches.rst.


2021-11-18 21:37:57

by Eric Snowberg

[permalink] [raw]
Subject: Re: [PATCH v7 13/17] KEYS: link secondary_trusted_keys to machine trusted keys



> On Nov 18, 2021, at 5:32 AM, Mimi Zohar <[email protected]> wrote:
>
> Hi Eric,
>
> Is the subject line left over from the original patch? Shouldn't it
> be "link machine trusted keys to secondary_trusted_keys".

Yes, you are right, this was left over from the original patch. I’ll update
the heading in the next round.

> On Mon, 2021-11-15 at 19:15 -0500, Eric Snowberg wrote:
>> Allow the .machine keyring to be linked to the secondary_trusted_keys.
>> After the link is created, keys contained in the .machine keyring will
>> automatically be searched when searching secondary_trusted_keys.
>>
>> Signed-off-by: Eric Snowberg <[email protected]>
>> ---
>> v3: Initial version
>> v4: Unmodified from v3
>> v5: Rename to machine keyring
>> v7: Unmodified from v5
>> ---
>> certs/system_keyring.c | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/certs/system_keyring.c b/certs/system_keyring.c
>> index ba732856ebd0..2a2dc70b126c 100644
>> --- a/certs/system_keyring.c
>> +++ b/certs/system_keyring.c
>> @@ -101,6 +101,9 @@ static __init struct key_restriction *get_secondary_restriction(void)
>> void __init set_machine_trusted_keys(struct key *keyring)
>> {
>> machine_trusted_keys = keyring;
>> +
>> + if (key_link(secondary_trusted_keys, machine_trusted_keys) < 0)
>> + panic("Can't link (machine) trusted keyrings\n");
>> }
>>
>> /**
>
> In general is the ordering of the patches "bisect safe"[1]? Only in
> the next patch is machine_trusted_keys set. In this case, either
> merge the two patches or reverse their order.

I’ll also reverse the ordering in the next round too. Thanks.

2021-11-19 00:06:27

by Mimi Zohar

[permalink] [raw]
Subject: Re: [PATCH v7 08/17] integrity: add new keyring handler for mok keys

On Mon, 2021-11-15 at 19:15 -0500, Eric Snowberg wrote:
> Currently both Secure Boot DB and Machine Owner Keys (MOK) go through
> the same keyring handler (get_handler_for_db). With the addition of the
> new machine keyring, the end-user may choose to trust MOK keys.
>
> Introduce a new keyring handler specific for MOK keys. If MOK keys are
> trusted by the end-user, use the new keyring handler instead.
>
> Signed-off-by: Eric Snowberg <[email protected]>

Reviewed-by: Mimi Zohar <[email protected]>


2021-11-19 00:06:29

by Mimi Zohar

[permalink] [raw]
Subject: Re: [PATCH v7 09/17] KEYS: Rename get_builtin_and_secondary_restriction

Hi Eric,

On Mon, 2021-11-15 at 19:15 -0500, Eric Snowberg wrote:
> To improve clarity, rename get_builtin_and_secondary_restriction to
> get_secondary_restriction.

The existing name clarity is fine. Perhaps instead prefix the above
sentence with "In preparation for returning either the existing
restrict_link_by_builtin_and_secondary_trusted or the new restriction
that includes the trusted builtin, secondary and machine keys, ..."
>
> Suggested-by: Mimi Zohar <[email protected]>
> Signed-off-by: Eric Snowberg <[email protected]>

Otherwise,
Reviewed-by: Mimi Zohar <[email protected]>


2021-11-19 00:20:38

by Mimi Zohar

[permalink] [raw]
Subject: Re: [PATCH v7 11/17] KEYS: Introduce link restriction for machine keys

Hi Eric,

On Mon, 2021-11-15 at 19:15 -0500, Eric Snowberg wrote:
> Introduce a new link restriction that includes the trusted builtin,
> secondary and machine keys. The restriction is based on the key to be
> added being vouched for by a key in any of these three keyrings.
>
> Suggested-by: Mimi Zohar <[email protected]>
> Signed-off-by: Eric Snowberg <[email protected]>
> ---
> v3: Initial version
> v4: moved code under CONFIG_INTEGRITY_MOK_KEYRING
> v5: Rename to machine keyring
> v6: Change subject name (suggested by Mimi)
> Rename restrict_link_by_builtin_secondary_and_ca_trusted
> to restrict_link_by_builtin_secondary_and_machine (suggested by
> Mimi)
> v7: Unmodified from v6
> ---
> certs/system_keyring.c | 23 +++++++++++++++++++++++
> include/keys/system_keyring.h | 6 ++++++
> 2 files changed, 29 insertions(+)
>
> diff --git a/certs/system_keyring.c b/certs/system_keyring.c
> index bc7e44fc82c2..71a00add9805 100644
> --- a/certs/system_keyring.c
> +++ b/certs/system_keyring.c
> @@ -99,6 +99,29 @@ void __init set_machine_trusted_keys(struct key *keyring)
> {
> machine_trusted_keys = keyring;
> }
> +
> +/**

This begins the start of kernel doc.

> + * restrict_link_by_builtin_secondary_and_machine

Missing are the parameter defintions. Please refer to
Documentation/doc-guide/kernel-doc.rst for details.

Mimi

> + *
> + * Restrict the addition of keys into a keyring based on the key-to-be-added
> + * being vouched for by a key in either the built-in, the secondary, or
> + * the machine keyrings.
> + */
> +int restrict_link_by_builtin_secondary_and_machine(
> + struct key *dest_keyring,
> + const struct key_type *type,
> + const union key_payload *payload,
> + struct key *restrict_key)
> +{
> + if (machine_trusted_keys && type == &key_type_keyring &&
> + dest_keyring == secondary_trusted_keys &&
> + payload == &machine_trusted_keys->payload)
> + /* Allow the machine keyring to be added to the secondary */
> + return 0;
> +
> + return restrict_link_by_builtin_and_secondary_trusted(dest_keyring, type,
> + payload, restrict_key);
> +}
> #endif
>
> /*
> diff --git a/include/keys/system_keyring.h b/include/keys/system_keyring.h
> index 98c9b10cdc17..2419a735420f 100644
> --- a/include/keys/system_keyring.h
> +++ b/include/keys/system_keyring.h
> @@ -39,8 +39,14 @@ extern int restrict_link_by_builtin_and_secondary_trusted(
> #endif
>
> #ifdef CONFIG_INTEGRITY_MACHINE_KEYRING
> +extern int restrict_link_by_builtin_secondary_and_machine(
> + struct key *dest_keyring,
> + const struct key_type *type,
> + const union key_payload *payload,
> + struct key *restrict_key);
> extern void __init set_machine_trusted_keys(struct key *keyring);
> #else
> +#define restrict_link_by_builtin_secondary_and_machine restrict_link_by_builtin_trusted
> static inline void __init set_machine_trusted_keys(struct key *keyring)
> {
> }



2021-11-19 02:53:28

by Eric Snowberg

[permalink] [raw]
Subject: Re: [PATCH v7 11/17] KEYS: Introduce link restriction for machine keys



> On Nov 18, 2021, at 5:20 PM, Mimi Zohar <[email protected]> wrote:
>
> Hi Eric,
>
> On Mon, 2021-11-15 at 19:15 -0500, Eric Snowberg wrote:
>> Introduce a new link restriction that includes the trusted builtin,
>> secondary and machine keys. The restriction is based on the key to be
>> added being vouched for by a key in any of these three keyrings.
>>
>> Suggested-by: Mimi Zohar <[email protected]>
>> Signed-off-by: Eric Snowberg <[email protected]>
>> ---
>> v3: Initial version
>> v4: moved code under CONFIG_INTEGRITY_MOK_KEYRING
>> v5: Rename to machine keyring
>> v6: Change subject name (suggested by Mimi)
>> Rename restrict_link_by_builtin_secondary_and_ca_trusted
>> to restrict_link_by_builtin_secondary_and_machine (suggested by
>> Mimi)
>> v7: Unmodified from v6
>> ---
>> certs/system_keyring.c | 23 +++++++++++++++++++++++
>> include/keys/system_keyring.h | 6 ++++++
>> 2 files changed, 29 insertions(+)
>>
>> diff --git a/certs/system_keyring.c b/certs/system_keyring.c
>> index bc7e44fc82c2..71a00add9805 100644
>> --- a/certs/system_keyring.c
>> +++ b/certs/system_keyring.c
>> @@ -99,6 +99,29 @@ void __init set_machine_trusted_keys(struct key *keyring)
>> {
>> machine_trusted_keys = keyring;
>> }
>> +
>> +/**
>
> This begins the start of kernel doc.
>
>> + * restrict_link_by_builtin_secondary_and_machine
>
> Missing are the parameter defintions. Please refer to
> Documentation/doc-guide/kernel-doc.rst for details.

I’ll add this in the next round, thanks.