linux-integrity.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH RFC v2 00/12] Enroll kernel keys thru MOK
@ 2021-07-26 17:13 Eric Snowberg
  2021-07-26 17:13 ` [PATCH RFC v2 01/12] integrity: Introduce a Linux keyring for the Machine Owner Key (MOK) Eric Snowberg
                   ` (12 more replies)
  0 siblings, 13 replies; 25+ messages in thread
From: Eric Snowberg @ 2021-07-26 17:13 UTC (permalink / raw)
  To: keyrings, linux-integrity, zohar, dhowells, dwmw2, herbert,
	davem, jarkko, jmorris, serge
  Cc: eric.snowberg, keescook, gregkh, torvalds, scott.branden,
	weiyongjun1, nayna, ebiggers, ardb, nramas, lszubowi,
	linux-kernel, linux-crypto, linux-security-module,
	James.Bottomley, pjones, glin, konrad.wilk

Many UEFI Linux distributions boot using shim.  The UEFI shim provides
what is called Machine Owner Keys (MOK).  Shim uses both the UEFI Secure
Boot DB and MOK keys to validate the next step in the boot chain.  The
MOK facility can be used to import user generated keys.  These keys can
be used to sign an end-user development kernel build.  When Linux boots,
pre-boot keys (both UEFI Secure Boot DB and MOK keys) get loaded in the
Linux .platform keyring.  

Currently, pre-boot keys are not trusted within the Linux trust boundary
[1]. These platform keys can only be used for kexec. If an end-user
wants to use their own key within the Linux trust boundary, they must
either compile it into the kernel themselves or use the insert-sys-cert
script. Both options present a problem. Many end-users do not want to
compile their own kernels. With the insert-sys-cert option, there are
missing upstream changes [2].  Also, with the insert-sys-cert option,
the end-user must re-sign their kernel again with their own key, and
then insert that key into the MOK db. Another problem with
insert-sys-cert is that only a single key can be inserted into a
compressed kernel.

Having the ability to insert a key into the Linux trust boundary opens
up various possibilities.  The end-user can use a pre-built kernel and
sign their own kernel modules.  It also opens up the ability for an
end-user to more easily use digital signature based IMA-appraisal.  To
get a key into the ima keyring, it must be signed by a key within the
Linux trust boundary.

Downstream Linux distros try to have a single signed kernel for each
architecture.  Each end-user may use this kernel in entirely different
ways.  Some downstream kernels have chosen to always trust platform keys
within the Linux trust boundary for kernel module signing.  These
kernels have no way of using digital signature base IMA appraisal.

This series introduces a new Linux kernel keyring containing the Machine
Owner Keys (MOK) called .mok. It also adds a new MOK variable to shim.
This variable allows the end-user to decide if they want to trust keys
enrolled in the MOK within the Linux trust boundary.  By default,
nothing changes; MOK keys are not trusted within the Linux kernel.  They
are only trusted after the end-user makes the decision themselves.  The
end-user would set this through mokutil using a new --trust-mok option
[3]. This would work similar to how the kernel uses MOK variables to
enable/disable signature validation as well as use/ignore the db.

When shim boots, it mirrors the new MokTML Boot Services variable to a
new MokListTrustedRT Runtime Services variable and extends PCR14.
MokListTrustedRT is written without EFI_VARIABLE_NON_VOLATILE set,
preventing an end-user from setting it after booting and doing a kexec.

When the kernel boots, if MokListTrustedRT is set and
EFI_VARIABLE_NON_VOLATILE is not set, the MokListRT is loaded into the
mok keyring instead of the platform keyring. Mimi has suggested that
only CA keys or keys that can be vouched for by other kernel keys be
loaded into this keyring. All other certs will load into the platform
keyring instead.

The .mok keyring contains a new keyring permission that only allows CA
keys to be loaded. If the permission fails, the key is later loaded into
the platform keyring.  After all keys are added into the .mok keyring,
they are linked to either the builtin or secondary trusted keyring.
After the link is created, keys contained in the .mok keyring will
automatically be searched when searching either builtin or secondary
trusted keys.

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

I have included links to both the mokutil [3] and shim [4] changes I
have made to support this new functionality.

V2 changes:
- The .mok keyring persists past boot
- Removed the unrestricted move into the secondary keyring
- Removed the keyring move bypass patch
- Added restrictions to allow the .mok to be linked to either the
  builtin or secondary keyrings
- Secondary keyring dependency has been removed

[1] https://lore.kernel.org/lkml/1556221605.24945.3.camel@HansenPartnership.com/
[2] https://lore.kernel.org/patchwork/cover/902768/
[3] https://github.com/esnowberg/mokutil/tree/0.3.0-mokvars-v2
[4] https://github.com/esnowberg/shim/tree/mokvars-v2

Eric Snowberg (12):
  integrity: Introduce a Linux keyring for the Machine Owner Key (MOK)
  KEYS: CA link restriction
  integrity: Trust MOK keys if MokListTrustedRT found
  integrity: add add_to_mok_keyring
  integrity: restrict INTEGRITY_KEYRING_MOK to
    restrict_link_by_system_trusted_or_ca
  integrity: accessor function to get trust_moklist
  integrity: add new keyring handler for mok keys
  integrity: Suppress error message for keys added to the mok keyring
  KEYS: add a reference to mok keyring
  KEYS: link system_trusted_keys to mok_trusted_keys
  integrity: Do not allow mok keyring updates following init
  integrity: store reference to mok keyring

 certs/system_keyring.c                        | 47 ++++++++++
 crypto/asymmetric_keys/restrict.c             | 60 +++++++++++++
 include/crypto/public_key.h                   |  5 ++
 include/keys/system_keyring.h                 | 10 +++
 security/integrity/Makefile                   |  3 +-
 security/integrity/digsig.c                   | 16 +++-
 security/integrity/integrity.h                | 12 ++-
 .../platform_certs/keyring_handler.c          | 17 +++-
 .../platform_certs/keyring_handler.h          |  5 ++
 security/integrity/platform_certs/load_uefi.c |  4 +-
 .../integrity/platform_certs/mok_keyring.c    | 85 +++++++++++++++++++
 11 files changed, 256 insertions(+), 8 deletions(-)
 create mode 100644 security/integrity/platform_certs/mok_keyring.c


base-commit: ff1176468d368232b684f75e82563369208bc371
-- 
2.18.4


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH RFC v2 01/12] integrity: Introduce a Linux keyring for the Machine Owner Key (MOK)
  2021-07-26 17:13 [PATCH RFC v2 00/12] Enroll kernel keys thru MOK Eric Snowberg
@ 2021-07-26 17:13 ` Eric Snowberg
  2021-07-26 17:13 ` [PATCH RFC v2 02/12] KEYS: CA link restriction Eric Snowberg
                   ` (11 subsequent siblings)
  12 siblings, 0 replies; 25+ messages in thread
From: Eric Snowberg @ 2021-07-26 17:13 UTC (permalink / raw)
  To: keyrings, linux-integrity, zohar, dhowells, dwmw2, herbert,
	davem, jarkko, jmorris, serge
  Cc: eric.snowberg, keescook, gregkh, torvalds, scott.branden,
	weiyongjun1, nayna, ebiggers, ardb, nramas, lszubowi,
	linux-kernel, linux-crypto, linux-security-module,
	James.Bottomley, pjones, glin, konrad.wilk

Many UEFI Linux distributions boot using shim.  The UEFI shim provides
what is called Machine Owner Keys (MOK). Shim uses both the UEFI Secure
Boot DB and MOK keys to validate the next step in the boot chain.  The
MOK facility can be used to import user generated keys.  These keys can
be used to sign an end-users development kernel build.  When Linux
boots, both UEFI Secure Boot DB and MOK keys get loaded in the Linux
.platform keyring.

Add a new Linux keyring called .mok.  This keyring shall contain just
MOK keys and not the remaining keys in the platform keyring. This new
.mok keyring will be used in follow on patches.  Unlike keys in the
platform keyring, keys contained in the .mok keyring will be trusted
within the kernel if the end-user has chosen to do so.

Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
---
v1: Initial version
v2: Removed destory keyring code
---
 security/integrity/Makefile                   |  3 ++-
 security/integrity/digsig.c                   |  1 +
 security/integrity/integrity.h                |  3 ++-
 .../integrity/platform_certs/mok_keyring.c    | 21 +++++++++++++++++++
 4 files changed, 26 insertions(+), 2 deletions(-)
 create mode 100644 security/integrity/platform_certs/mok_keyring.c

diff --git a/security/integrity/Makefile b/security/integrity/Makefile
index 7ee39d66cf16..8e2e98cba1f6 100644
--- a/security/integrity/Makefile
+++ b/security/integrity/Makefile
@@ -9,7 +9,8 @@ integrity-y := iint.o
 integrity-$(CONFIG_INTEGRITY_AUDIT) += integrity_audit.o
 integrity-$(CONFIG_INTEGRITY_SIGNATURE) += digsig.o
 integrity-$(CONFIG_INTEGRITY_ASYMMETRIC_KEYS) += digsig_asymmetric.o
-integrity-$(CONFIG_INTEGRITY_PLATFORM_KEYRING) += platform_certs/platform_keyring.o
+integrity-$(CONFIG_INTEGRITY_PLATFORM_KEYRING) += platform_certs/platform_keyring.o \
+						  platform_certs/mok_keyring.o
 integrity-$(CONFIG_LOAD_UEFI_KEYS) += platform_certs/efi_parser.o \
 				      platform_certs/load_uefi.o \
 				      platform_certs/keyring_handler.o
diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
index 3b06a01bd0fd..e07334504ef1 100644
--- a/security/integrity/digsig.c
+++ b/security/integrity/digsig.c
@@ -30,6 +30,7 @@ static const char * const keyring_name[INTEGRITY_KEYRING_MAX] = {
 	".ima",
 #endif
 	".platform",
+	".mok",
 };
 
 #ifdef CONFIG_IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index 547425c20e11..e0e17ccba2e6 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -151,7 +151,8 @@ int integrity_kernel_read(struct file *file, loff_t offset,
 #define INTEGRITY_KEYRING_EVM		0
 #define INTEGRITY_KEYRING_IMA		1
 #define INTEGRITY_KEYRING_PLATFORM	2
-#define INTEGRITY_KEYRING_MAX		3
+#define INTEGRITY_KEYRING_MOK		3
+#define INTEGRITY_KEYRING_MAX		4
 
 extern struct dentry *integrity_dir;
 
diff --git a/security/integrity/platform_certs/mok_keyring.c b/security/integrity/platform_certs/mok_keyring.c
new file mode 100644
index 000000000000..b1ee45b77731
--- /dev/null
+++ b/security/integrity/platform_certs/mok_keyring.c
@@ -0,0 +1,21 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * MOK keyring routines.
+ *
+ * Copyright (c) 2021, Oracle and/or its affiliates.
+ */
+
+#include "../integrity.h"
+
+static __init int mok_keyring_init(void)
+{
+	int rc;
+
+	rc = integrity_init_keyring(INTEGRITY_KEYRING_MOK);
+	if (rc)
+		return rc;
+
+	pr_notice("MOK Keyring initialized\n");
+	return 0;
+}
+device_initcall(mok_keyring_init);
-- 
2.18.4


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH RFC v2 02/12] KEYS: CA link restriction
  2021-07-26 17:13 [PATCH RFC v2 00/12] Enroll kernel keys thru MOK Eric Snowberg
  2021-07-26 17:13 ` [PATCH RFC v2 01/12] integrity: Introduce a Linux keyring for the Machine Owner Key (MOK) Eric Snowberg
@ 2021-07-26 17:13 ` Eric Snowberg
  2021-08-05 14:00   ` Mimi Zohar
  2021-07-26 17:13 ` [PATCH RFC v2 03/12] integrity: Trust MOK keys if MokListTrustedRT found Eric Snowberg
                   ` (10 subsequent siblings)
  12 siblings, 1 reply; 25+ messages in thread
From: Eric Snowberg @ 2021-07-26 17:13 UTC (permalink / raw)
  To: keyrings, linux-integrity, zohar, dhowells, dwmw2, herbert,
	davem, jarkko, jmorris, serge
  Cc: eric.snowberg, keescook, gregkh, torvalds, scott.branden,
	weiyongjun1, nayna, ebiggers, ardb, nramas, lszubowi,
	linux-kernel, linux-crypto, linux-security-module,
	James.Bottomley, pjones, glin, konrad.wilk

Add a new link restriction.  Restrict the addition of keys in a keyring
based on the key to be added being a CA (self-signed) or by being
vouched for by a key in either the built-in or the secondary trusted
keyrings.

Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
---
v1: Initial version
v2: Removed secondary keyring references
---
 certs/system_keyring.c            | 21 +++++++++++
 crypto/asymmetric_keys/restrict.c | 60 +++++++++++++++++++++++++++++++
 include/crypto/public_key.h       |  5 +++
 include/keys/system_keyring.h     |  6 ++++
 4 files changed, 92 insertions(+)

diff --git a/certs/system_keyring.c b/certs/system_keyring.c
index 692365dee2bd..0a7b16c28a72 100644
--- a/certs/system_keyring.c
+++ b/certs/system_keyring.c
@@ -21,6 +21,9 @@
 static struct key *builtin_trusted_keys;
 #ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
 static struct key *secondary_trusted_keys;
+#define system_trusted_keys secondary_trusted_keys
+#else
+#define system_trusted_keys builtin_trusted_keys
 #endif
 #ifdef CONFIG_INTEGRITY_PLATFORM_KEYRING
 static struct key *platform_trusted_keys;
@@ -45,6 +48,24 @@ int restrict_link_by_builtin_trusted(struct key *dest_keyring,
 					  builtin_trusted_keys);
 }
 
+/**
+ * restrict_link_by_system_trusted_or_ca - Restrict keyring
+ *   addition by being a CA or vouched by the system trusted keyrings.
+ *
+ *  Restrict the addition of keys in a keyring based on the key-to-be-added
+ *  being a CA (self signed) or by being vouched for by a key in either
+ *  the built-in or the secondary system keyrings.
+ */
+int restrict_link_by_system_trusted_or_ca(
+	struct key *dest_keyring,
+	const struct key_type *type,
+	const union key_payload *payload,
+	struct key *restrict_key)
+{
+	return restrict_link_by_ca(dest_keyring, type, payload,
+				   system_trusted_keys);
+}
+
 #ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
 /**
  * restrict_link_by_builtin_and_secondary_trusted - Restrict keyring
diff --git a/crypto/asymmetric_keys/restrict.c b/crypto/asymmetric_keys/restrict.c
index 84cefe3b3585..75e4379226e8 100644
--- a/crypto/asymmetric_keys/restrict.c
+++ b/crypto/asymmetric_keys/restrict.c
@@ -108,6 +108,66 @@ int restrict_link_by_signature(struct key *dest_keyring,
 	return ret;
 }
 
+/**
+ * restrict_link_by_ca - Restrict additions to a ring of public keys
+ * based on it being a CA
+ * @dest_keyring: Keyring being linked to.
+ * @type: The type of key being added.
+ * @payload: The payload of the new key.
+ * @trusted: A key or ring of keys that can be used to vouch for the new cert.
+ *
+ * Check if the new certificate is a CA or if they key can be vouched for
+ * by keys already linked in the destination keyring or the trusted
+ * keyring.  If one of those is the signing key or it is self signed, then
+ * mark the new certificate as being ok to link.
+ *
+ * Returns 0 if the new certificate was accepted, -ENOKEY if we could not find
+ * a matching parent certificate in the trusted list.  -ENOPKG if the signature
+ * uses unsupported crypto, or some other error if there is a matching
+ * certificate  but the signature check cannot be performed.
+ */
+int restrict_link_by_ca(struct key *dest_keyring,
+			const struct key_type *type,
+			const union key_payload *payload,
+			struct key *trust_keyring)
+{
+	const struct public_key_signature *sig;
+	const struct public_key *pkey;
+	struct key *key;
+	int ret;
+
+	if (type != &key_type_asymmetric)
+		return -EOPNOTSUPP;
+
+	sig = payload->data[asym_auth];
+	if (!sig)
+		return -ENOPKG;
+
+	if (!sig->auth_ids[0] && !sig->auth_ids[1])
+		return -ENOKEY;
+
+	pkey = payload->data[asym_crypto];
+	if (!pkey)
+		return -ENOPKG;
+
+	ret = public_key_verify_signature(pkey, sig);
+	if (!ret)
+		return 0;
+
+	if (!trust_keyring)
+		return -ENOKEY;
+
+	key = find_asymmetric_key(trust_keyring,
+				  sig->auth_ids[0], sig->auth_ids[1],
+				  false);
+	if (IS_ERR(key))
+		return -ENOKEY;
+
+	ret = verify_signature(key, sig);
+	key_put(key);
+	return ret;
+}
+
 static bool match_either_id(const struct asymmetric_key_ids *pair,
 			    const struct asymmetric_key_id *single)
 {
diff --git a/include/crypto/public_key.h b/include/crypto/public_key.h
index 47accec68cb0..545af1ea57de 100644
--- a/include/crypto/public_key.h
+++ b/include/crypto/public_key.h
@@ -71,6 +71,11 @@ extern int restrict_link_by_key_or_keyring_chain(struct key *trust_keyring,
 						 const union key_payload *payload,
 						 struct key *trusted);
 
+extern int restrict_link_by_ca(struct key *dest_keyring,
+			       const struct key_type *type,
+			       const union key_payload *payload,
+			       struct key *trust_keyring);
+
 extern int query_asymmetric_key(const struct kernel_pkey_params *,
 				struct kernel_pkey_query *);
 
diff --git a/include/keys/system_keyring.h b/include/keys/system_keyring.h
index 6acd3cf13a18..2041254d74f4 100644
--- a/include/keys/system_keyring.h
+++ b/include/keys/system_keyring.h
@@ -28,6 +28,12 @@ static inline __init int load_module_cert(struct key *keyring)
 
 #endif
 
+extern int restrict_link_by_system_trusted_or_ca(
+	struct key *dest_keyring,
+	const struct key_type *type,
+	const union key_payload *payload,
+	struct key *restrict_key);
+
 #ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
 extern int restrict_link_by_builtin_and_secondary_trusted(
 	struct key *keyring,
-- 
2.18.4


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH RFC v2 03/12] integrity: Trust MOK keys if MokListTrustedRT found
  2021-07-26 17:13 [PATCH RFC v2 00/12] Enroll kernel keys thru MOK Eric Snowberg
  2021-07-26 17:13 ` [PATCH RFC v2 01/12] integrity: Introduce a Linux keyring for the Machine Owner Key (MOK) Eric Snowberg
  2021-07-26 17:13 ` [PATCH RFC v2 02/12] KEYS: CA link restriction Eric Snowberg
@ 2021-07-26 17:13 ` Eric Snowberg
  2021-07-26 17:13 ` [PATCH RFC v2 04/12] integrity: add add_to_mok_keyring Eric Snowberg
                   ` (9 subsequent siblings)
  12 siblings, 0 replies; 25+ messages in thread
From: Eric Snowberg @ 2021-07-26 17:13 UTC (permalink / raw)
  To: keyrings, linux-integrity, zohar, dhowells, dwmw2, herbert,
	davem, jarkko, jmorris, serge
  Cc: eric.snowberg, keescook, gregkh, torvalds, scott.branden,
	weiyongjun1, nayna, ebiggers, ardb, nramas, lszubowi,
	linux-kernel, linux-crypto, linux-security-module,
	James.Bottomley, pjones, glin, konrad.wilk

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 themself 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.

MOK variables are mirrored from Boot Services to Runtime Services.  When
shim sees the new MokTML BS variable, it will create a new variable
(before Exit Boot Services is called) called MokListTrustedRT without
EFI_VARIABLE_NON_VOLATILE set.  Following Exit Boot Services, UEFI
variables can only be set and created with SetVariable if both
EFI_VARIABLE_RUNTIME_ACCESS & EFI_VARIABLE_NON_VOLATILE are set.
Therefore, this can not be defeated by simply creating a
MokListTrustedRT variable from Linux, the existence of
EFI_VARIABLE_NON_VOLATILE will cause uefi_check_trust_mok_keys to return
false.

Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
---
v1: Initial version
v2: removed mok_keyring_trust_setup function
---
 .../integrity/platform_certs/mok_keyring.c    | 27 +++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/security/integrity/platform_certs/mok_keyring.c b/security/integrity/platform_certs/mok_keyring.c
index b1ee45b77731..fe4f2d336260 100644
--- a/security/integrity/platform_certs/mok_keyring.c
+++ b/security/integrity/platform_certs/mok_keyring.c
@@ -5,6 +5,7 @@
  * Copyright (c) 2021, Oracle and/or its affiliates.
  */
 
+#include <linux/efi.h>
 #include "../integrity.h"
 
 static __init int mok_keyring_init(void)
@@ -19,3 +20,29 @@ static __init int mok_keyring_init(void)
 	return 0;
 }
 device_initcall(mok_keyring_init);
+
+/*
+ * Try to load the MokListTrustedRT UEFI 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 kernel.
+ */
+static __init bool uefi_check_trust_mok_keys(void)
+{
+	efi_status_t status;
+	unsigned int mtrust = 0;
+	unsigned long size = sizeof(mtrust);
+	efi_guid_t guid = EFI_SHIM_LOCK_GUID;
+	u32 attr;
+
+	status = efi.get_variable(L"MokListTrustedRT", &guid, &attr, &size, &mtrust);
+
+	/*
+	 * The EFI_VARIABLE_NON_VOLATILE check is to verify MokListTrustedRT
+	 * was set thru shim mirrioring and not by a user from the host os.
+	 * According to the UEFI spec, once EBS is performed, SetVariable()
+	 * will succeed only when both EFI_VARIABLE_RUNTIME_ACCESS &
+	 * EFI_VARIABLE_NON_VOLATILE are set.
+	 */
+	return (status == EFI_SUCCESS && (!(attr & EFI_VARIABLE_NON_VOLATILE)));
+}
-- 
2.18.4


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH RFC v2 04/12] integrity: add add_to_mok_keyring
  2021-07-26 17:13 [PATCH RFC v2 00/12] Enroll kernel keys thru MOK Eric Snowberg
                   ` (2 preceding siblings ...)
  2021-07-26 17:13 ` [PATCH RFC v2 03/12] integrity: Trust MOK keys if MokListTrustedRT found Eric Snowberg
@ 2021-07-26 17:13 ` Eric Snowberg
  2021-07-26 17:13 ` [PATCH RFC v2 05/12] integrity: restrict INTEGRITY_KEYRING_MOK to restrict_link_by_system_trusted_or_ca Eric Snowberg
                   ` (8 subsequent siblings)
  12 siblings, 0 replies; 25+ messages in thread
From: Eric Snowberg @ 2021-07-26 17:13 UTC (permalink / raw)
  To: keyrings, linux-integrity, zohar, dhowells, dwmw2, herbert,
	davem, jarkko, jmorris, serge
  Cc: eric.snowberg, keescook, gregkh, torvalds, scott.branden,
	weiyongjun1, nayna, ebiggers, ardb, nramas, lszubowi,
	linux-kernel, linux-crypto, linux-security-module,
	James.Bottomley, pjones, glin, konrad.wilk

Add the ability to load Machine Owner Key (MOK) keys to the mok keyring.
If the permissions do not allow the key to be added to the mok keyring
this is not an error, add it to the platform keyring instead.

Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
---
v1: Initial version
v2: Unmodified from v1
---
 security/integrity/integrity.h                |  4 ++++
 .../integrity/platform_certs/mok_keyring.c    | 21 +++++++++++++++++++
 2 files changed, 25 insertions(+)

diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index e0e17ccba2e6..60d5c7ba05b2 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -278,9 +278,13 @@ integrity_audit_log_start(struct audit_context *ctx, gfp_t gfp_mask, int type)
 #ifdef CONFIG_INTEGRITY_PLATFORM_KEYRING
 void __init add_to_platform_keyring(const char *source, const void *data,
 				    size_t len);
+void __init add_to_mok_keyring(const char *source, const void *data, size_t len);
 #else
 static inline void __init add_to_platform_keyring(const char *source,
 						  const void *data, size_t len)
 {
 }
+void __init add_to_mok_keyring(const char *source, const void *data, size_t len)
+{
+}
 #endif
diff --git a/security/integrity/platform_certs/mok_keyring.c b/security/integrity/platform_certs/mok_keyring.c
index fe4f2d336260..f260edac0863 100644
--- a/security/integrity/platform_certs/mok_keyring.c
+++ b/security/integrity/platform_certs/mok_keyring.c
@@ -21,6 +21,27 @@ static __init int mok_keyring_init(void)
 }
 device_initcall(mok_keyring_init);
 
+void __init add_to_mok_keyring(const char *source, const void *data, size_t len)
+{
+	key_perm_t perm;
+	int rc;
+
+	perm = (KEY_POS_ALL & ~KEY_POS_SETATTR) | KEY_USR_VIEW;
+	rc = integrity_load_cert(INTEGRITY_KEYRING_MOK, source, data, len, perm);
+
+	/*
+	 * If the mok keyring restrictions prevented the cert from loading,
+	 * this is not an error.  Just load it into the platform keyring
+	 * instead.
+	 */
+	if (rc)
+		rc = integrity_load_cert(INTEGRITY_KEYRING_PLATFORM, source,
+					 data, len, perm);
+
+	if (rc)
+		pr_info("Error adding keys to mok keyring %s\n", source);
+}
+
 /*
  * Try to load the MokListTrustedRT UEFI variable to see if we should trust
  * the mok keys within the kernel. It is not an error if this variable
-- 
2.18.4


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH RFC v2 05/12] integrity: restrict INTEGRITY_KEYRING_MOK to restrict_link_by_system_trusted_or_ca
  2021-07-26 17:13 [PATCH RFC v2 00/12] Enroll kernel keys thru MOK Eric Snowberg
                   ` (3 preceding siblings ...)
  2021-07-26 17:13 ` [PATCH RFC v2 04/12] integrity: add add_to_mok_keyring Eric Snowberg
@ 2021-07-26 17:13 ` Eric Snowberg
  2021-07-26 17:13 ` [PATCH RFC v2 06/12] integrity: accessor function to get trust_moklist Eric Snowberg
                   ` (7 subsequent siblings)
  12 siblings, 0 replies; 25+ messages in thread
From: Eric Snowberg @ 2021-07-26 17:13 UTC (permalink / raw)
  To: keyrings, linux-integrity, zohar, dhowells, dwmw2, herbert,
	davem, jarkko, jmorris, serge
  Cc: eric.snowberg, keescook, gregkh, torvalds, scott.branden,
	weiyongjun1, nayna, ebiggers, ardb, nramas, lszubowi,
	linux-kernel, linux-crypto, linux-security-module,
	James.Bottomley, pjones, glin, konrad.wilk

Set the restriction check for INTEGRITY_KEYRING_MOK keys to
restrict_link_by_system_trusted_or_ca.  This will only allow keys
into the mok keyring that are either a CA or trusted by a key contained
within the builtin or secondary trusted keyring.

Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
---
v1: Initial version
v2: Added !IS_ENABLED(CONFIG_INTEGRITY_TRUSTED_KEYRING check so mok
    keyring gets created even when it isn't enabled
---
 security/integrity/digsig.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
index e07334504ef1..2f6898c89f60 100644
--- a/security/integrity/digsig.c
+++ b/security/integrity/digsig.c
@@ -132,7 +132,7 @@ int __init integrity_init_keyring(const unsigned int id)
 		goto out;
 	}
 
-	if (!IS_ENABLED(CONFIG_INTEGRITY_TRUSTED_KEYRING))
+	if (!IS_ENABLED(CONFIG_INTEGRITY_TRUSTED_KEYRING) && id != INTEGRITY_KEYRING_MOK)
 		return 0;
 
 	restriction = kzalloc(sizeof(struct key_restriction), GFP_KERNEL);
@@ -140,6 +140,11 @@ int __init integrity_init_keyring(const unsigned int id)
 		return -ENOMEM;
 
 	restriction->check = restrict_link_to_ima;
+	if (id == INTEGRITY_KEYRING_MOK)
+		restriction->check = restrict_link_by_system_trusted_or_ca;
+	else
+		restriction->check = restrict_link_to_ima;
+
 	perm |= KEY_USR_WRITE;
 
 out:
-- 
2.18.4


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH RFC v2 06/12] integrity: accessor function to get trust_moklist
  2021-07-26 17:13 [PATCH RFC v2 00/12] Enroll kernel keys thru MOK Eric Snowberg
                   ` (4 preceding siblings ...)
  2021-07-26 17:13 ` [PATCH RFC v2 05/12] integrity: restrict INTEGRITY_KEYRING_MOK to restrict_link_by_system_trusted_or_ca Eric Snowberg
@ 2021-07-26 17:13 ` Eric Snowberg
  2021-07-26 17:13 ` [PATCH RFC v2 07/12] integrity: add new keyring handler for mok keys Eric Snowberg
                   ` (6 subsequent siblings)
  12 siblings, 0 replies; 25+ messages in thread
From: Eric Snowberg @ 2021-07-26 17:13 UTC (permalink / raw)
  To: keyrings, linux-integrity, zohar, dhowells, dwmw2, herbert,
	davem, jarkko, jmorris, serge
  Cc: eric.snowberg, keescook, gregkh, torvalds, scott.branden,
	weiyongjun1, nayna, ebiggers, ardb, nramas, lszubowi,
	linux-kernel, linux-crypto, linux-security-module,
	James.Bottomley, pjones, glin, konrad.wilk

Add an accessor function to see if the mok list should be trusted.

Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
---
v1: Initial version
v2: Added trust_moklist function
---
 security/integrity/integrity.h                  |  5 +++++
 security/integrity/platform_certs/mok_keyring.c | 16 ++++++++++++++++
 2 files changed, 21 insertions(+)

diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index 60d5c7ba05b2..1fcefceb0da1 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -279,6 +279,7 @@ integrity_audit_log_start(struct audit_context *ctx, gfp_t gfp_mask, int type)
 void __init add_to_platform_keyring(const char *source, const void *data,
 				    size_t len);
 void __init add_to_mok_keyring(const char *source, const void *data, size_t len);
+bool __init trust_moklist(void);
 #else
 static inline void __init add_to_platform_keyring(const char *source,
 						  const void *data, size_t len)
@@ -287,4 +288,8 @@ static inline void __init add_to_platform_keyring(const char *source,
 void __init add_to_mok_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/mok_keyring.c b/security/integrity/platform_certs/mok_keyring.c
index f260edac0863..c7820d9136f3 100644
--- a/security/integrity/platform_certs/mok_keyring.c
+++ b/security/integrity/platform_certs/mok_keyring.c
@@ -8,6 +8,8 @@
 #include <linux/efi.h>
 #include "../integrity.h"
 
+bool trust_mok;
+
 static __init int mok_keyring_init(void)
 {
 	int rc;
@@ -67,3 +69,17 @@ static __init bool uefi_check_trust_mok_keys(void)
 	 */
 	return (status == EFI_SUCCESS && (!(attr & EFI_VARIABLE_NON_VOLATILE)));
 }
+
+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


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH RFC v2 07/12] integrity: add new keyring handler for mok keys
  2021-07-26 17:13 [PATCH RFC v2 00/12] Enroll kernel keys thru MOK Eric Snowberg
                   ` (5 preceding siblings ...)
  2021-07-26 17:13 ` [PATCH RFC v2 06/12] integrity: accessor function to get trust_moklist Eric Snowberg
@ 2021-07-26 17:13 ` Eric Snowberg
  2021-07-26 17:13 ` [PATCH RFC v2 08/12] integrity: Suppress error message for keys added to the mok keyring Eric Snowberg
                   ` (5 subsequent siblings)
  12 siblings, 0 replies; 25+ messages in thread
From: Eric Snowberg @ 2021-07-26 17:13 UTC (permalink / raw)
  To: keyrings, linux-integrity, zohar, dhowells, dwmw2, herbert,
	davem, jarkko, jmorris, serge
  Cc: eric.snowberg, keescook, gregkh, torvalds, scott.branden,
	weiyongjun1, nayna, ebiggers, ardb, nramas, lszubowi,
	linux-kernel, linux-crypto, linux-security-module,
	James.Bottomley, pjones, glin, konrad.wilk

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 mok 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 <eric.snowberg@oracle.com>
---
v1: Initial version
v2: Unmodified from v1
---
 .../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 5604bd57c990..1e15b65abc9f 100644
--- a/security/integrity/platform_certs/keyring_handler.c
+++ b/security/integrity/platform_certs/keyring_handler.c
@@ -66,7 +66,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)
 {
@@ -75,6 +75,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 (trust_moklist())
+			return add_to_mok_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


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH RFC v2 08/12] integrity: Suppress error message for keys added to the mok keyring
  2021-07-26 17:13 [PATCH RFC v2 00/12] Enroll kernel keys thru MOK Eric Snowberg
                   ` (6 preceding siblings ...)
  2021-07-26 17:13 ` [PATCH RFC v2 07/12] integrity: add new keyring handler for mok keys Eric Snowberg
@ 2021-07-26 17:13 ` Eric Snowberg
  2021-07-26 17:13 ` [PATCH RFC v2 09/12] KEYS: add a reference to " Eric Snowberg
                   ` (4 subsequent siblings)
  12 siblings, 0 replies; 25+ messages in thread
From: Eric Snowberg @ 2021-07-26 17:13 UTC (permalink / raw)
  To: keyrings, linux-integrity, zohar, dhowells, dwmw2, herbert,
	davem, jarkko, jmorris, serge
  Cc: eric.snowberg, keescook, gregkh, torvalds, scott.branden,
	weiyongjun1, nayna, ebiggers, ardb, nramas, lszubowi,
	linux-kernel, linux-crypto, linux-security-module,
	James.Bottomley, pjones, glin, konrad.wilk

Suppress the error message for keys added to the mok keyring. If an
error occurs, the key will be added to the platform keyring instead.

Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
---
v1: Initial version
v2: Unmodified from v1
---
 security/integrity/digsig.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
index 2f6898c89f60..be4860c596b9 100644
--- a/security/integrity/digsig.c
+++ b/security/integrity/digsig.c
@@ -165,7 +165,8 @@ static int __init integrity_add_key(const unsigned int id, const void *data,
 				   KEY_ALLOC_NOT_IN_QUOTA);
 	if (IS_ERR(key)) {
 		rc = PTR_ERR(key);
-		pr_err("Problem loading X.509 certificate %d\n", rc);
+		if (id != INTEGRITY_KEYRING_MOK)
+			pr_err("Problem loading X.509 certificate %d\n", rc);
 	} else {
 		pr_notice("Loaded X.509 cert '%s'\n",
 			  key_ref_to_ptr(key)->description);
-- 
2.18.4


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH RFC v2 09/12] KEYS: add a reference to mok keyring
  2021-07-26 17:13 [PATCH RFC v2 00/12] Enroll kernel keys thru MOK Eric Snowberg
                   ` (7 preceding siblings ...)
  2021-07-26 17:13 ` [PATCH RFC v2 08/12] integrity: Suppress error message for keys added to the mok keyring Eric Snowberg
@ 2021-07-26 17:13 ` Eric Snowberg
  2021-07-26 17:13 ` [PATCH RFC v2 10/12] KEYS: link system_trusted_keys to mok_trusted_keys Eric Snowberg
                   ` (3 subsequent siblings)
  12 siblings, 0 replies; 25+ messages in thread
From: Eric Snowberg @ 2021-07-26 17:13 UTC (permalink / raw)
  To: keyrings, linux-integrity, zohar, dhowells, dwmw2, herbert,
	davem, jarkko, jmorris, serge
  Cc: eric.snowberg, keescook, gregkh, torvalds, scott.branden,
	weiyongjun1, nayna, ebiggers, ardb, nramas, lszubowi,
	linux-kernel, linux-crypto, linux-security-module,
	James.Bottomley, pjones, glin, konrad.wilk

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

Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
---
v2: Initial version
---
 certs/system_keyring.c        | 5 +++++
 include/keys/system_keyring.h | 4 ++++
 2 files changed, 9 insertions(+)

diff --git a/certs/system_keyring.c b/certs/system_keyring.c
index 0a7b16c28a72..dcaf74102ab2 100644
--- a/certs/system_keyring.c
+++ b/certs/system_keyring.c
@@ -27,6 +27,7 @@ static struct key *secondary_trusted_keys;
 #endif
 #ifdef CONFIG_INTEGRITY_PLATFORM_KEYRING
 static struct key *platform_trusted_keys;
+static struct key *mok_trusted_keys;
 #endif
 
 extern __initconst const u8 system_certificate_list[];
@@ -317,4 +318,8 @@ void __init set_platform_trusted_keys(struct key *keyring)
 {
 	platform_trusted_keys = keyring;
 }
+void __init set_mok_trusted_keys(struct key *keyring)
+{
+	mok_trusted_keys = keyring;
+}
 #endif
diff --git a/include/keys/system_keyring.h b/include/keys/system_keyring.h
index 2041254d74f4..1adf78ddc035 100644
--- a/include/keys/system_keyring.h
+++ b/include/keys/system_keyring.h
@@ -94,10 +94,14 @@ static inline struct key *get_ima_blacklist_keyring(void)
 #if defined(CONFIG_INTEGRITY_PLATFORM_KEYRING) && \
 	defined(CONFIG_SYSTEM_TRUSTED_KEYRING)
 extern void __init set_platform_trusted_keys(struct key *keyring);
+extern void __init set_mok_trusted_keys(struct key *keyring);
 #else
 static inline void set_platform_trusted_keys(struct key *keyring)
 {
 }
+static void __init set_mok_trusted_keys(struct key *keyring)
+{
+}
 #endif
 
 #endif /* _KEYS_SYSTEM_KEYRING_H */
-- 
2.18.4


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH RFC v2 10/12] KEYS: link system_trusted_keys to mok_trusted_keys
  2021-07-26 17:13 [PATCH RFC v2 00/12] Enroll kernel keys thru MOK Eric Snowberg
                   ` (8 preceding siblings ...)
  2021-07-26 17:13 ` [PATCH RFC v2 09/12] KEYS: add a reference to " Eric Snowberg
@ 2021-07-26 17:13 ` Eric Snowberg
  2021-08-05 13:58   ` Mimi Zohar
  2021-07-26 17:13 ` [PATCH RFC v2 11/12] integrity: Do not allow mok keyring updates following init Eric Snowberg
                   ` (2 subsequent siblings)
  12 siblings, 1 reply; 25+ messages in thread
From: Eric Snowberg @ 2021-07-26 17:13 UTC (permalink / raw)
  To: keyrings, linux-integrity, zohar, dhowells, dwmw2, herbert,
	davem, jarkko, jmorris, serge
  Cc: eric.snowberg, keescook, gregkh, torvalds, scott.branden,
	weiyongjun1, nayna, ebiggers, ardb, nramas, lszubowi,
	linux-kernel, linux-crypto, linux-security-module,
	James.Bottomley, pjones, glin, konrad.wilk

Allow the .mok keyring to be linked to either the builtin_trusted_keys
or the secondary_trusted_keys. If CONFIG_SECONDARY_TRUSTED_KEYRING is
enabled, mok keys are linked to the secondary_trusted_keys.  Otherwise
they are linked to the builtin_trusted_keys.  After the link is created,
keys contained in the .mok keyring will automatically be searched when
searching either builtin_trusted_keys or secondary_trusted_keys.

Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
---
v2: Initial version
---
 certs/system_keyring.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/certs/system_keyring.c b/certs/system_keyring.c
index dcaf74102ab2..b27ae30eaadc 100644
--- a/certs/system_keyring.c
+++ b/certs/system_keyring.c
@@ -45,6 +45,15 @@ int restrict_link_by_builtin_trusted(struct key *dest_keyring,
 				     const union key_payload *payload,
 				     struct key *restriction_key)
 {
+	/* If the secondary trusted keyring is not enabled, we may link
+	 * through to the mok keyring and the search may follow that link.
+	 */
+	if (mok_trusted_keys && type == &key_type_keyring &&
+	    dest_keyring == builtin_trusted_keys &&
+	    payload == &mok_trusted_keys->payload)
+		/* Allow the mok keyring to be added to the builtin */
+		return 0;
+
 	return restrict_link_by_signature(dest_keyring, type, payload,
 					  builtin_trusted_keys);
 }
@@ -91,6 +100,15 @@ int restrict_link_by_builtin_and_secondary_trusted(
 		/* Allow the builtin keyring to be added to the secondary */
 		return 0;
 
+	/* If we have a secondary trusted keyring, it may contain a link
+	 * through to the mok keyring and the search may follow that link.
+	 */
+	if (mok_trusted_keys && type == &key_type_keyring &&
+	    dest_keyring == secondary_trusted_keys &&
+	    payload == &mok_trusted_keys->payload)
+		/* Allow the mok keyring to be added to the secondary */
+		return 0;
+
 	return restrict_link_by_signature(dest_keyring, type, payload,
 					  secondary_trusted_keys);
 }
@@ -321,5 +339,8 @@ void __init set_platform_trusted_keys(struct key *keyring)
 void __init set_mok_trusted_keys(struct key *keyring)
 {
 	mok_trusted_keys = keyring;
+
+	if (key_link(system_trusted_keys, mok_trusted_keys) < 0)
+		panic("Can't link (mok) trusted keyrings\n");
 }
 #endif
-- 
2.18.4


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH RFC v2 11/12] integrity: Do not allow mok keyring updates following init
  2021-07-26 17:13 [PATCH RFC v2 00/12] Enroll kernel keys thru MOK Eric Snowberg
                   ` (9 preceding siblings ...)
  2021-07-26 17:13 ` [PATCH RFC v2 10/12] KEYS: link system_trusted_keys to mok_trusted_keys Eric Snowberg
@ 2021-07-26 17:13 ` Eric Snowberg
  2021-07-26 17:13 ` [PATCH RFC v2 12/12] integrity: store reference to mok keyring Eric Snowberg
  2021-08-03 17:01 ` [PATCH RFC v2 00/12] Enroll kernel keys thru MOK Mimi Zohar
  12 siblings, 0 replies; 25+ messages in thread
From: Eric Snowberg @ 2021-07-26 17:13 UTC (permalink / raw)
  To: keyrings, linux-integrity, zohar, dhowells, dwmw2, herbert,
	davem, jarkko, jmorris, serge
  Cc: eric.snowberg, keescook, gregkh, torvalds, scott.branden,
	weiyongjun1, nayna, ebiggers, ardb, nramas, lszubowi,
	linux-kernel, linux-crypto, linux-security-module,
	James.Bottomley, pjones, glin, konrad.wilk

The mok keyring is setup during init.  No additional keys should be allowed
to be added afterwards.  Leave the permission as read only.

Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
---
v2: Initial version
---
 security/integrity/digsig.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
index be4860c596b9..3a12cc85b528 100644
--- a/security/integrity/digsig.c
+++ b/security/integrity/digsig.c
@@ -145,7 +145,8 @@ int __init integrity_init_keyring(const unsigned int id)
 	else
 		restriction->check = restrict_link_to_ima;
 
-	perm |= KEY_USR_WRITE;
+	if (id != INTEGRITY_KEYRING_MOK)
+		perm |= KEY_USR_WRITE;
 
 out:
 	return __integrity_init_keyring(id, perm, restriction);
-- 
2.18.4


^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH RFC v2 12/12] integrity: store reference to mok keyring
  2021-07-26 17:13 [PATCH RFC v2 00/12] Enroll kernel keys thru MOK Eric Snowberg
                   ` (10 preceding siblings ...)
  2021-07-26 17:13 ` [PATCH RFC v2 11/12] integrity: Do not allow mok keyring updates following init Eric Snowberg
@ 2021-07-26 17:13 ` Eric Snowberg
  2021-08-03 17:01 ` [PATCH RFC v2 00/12] Enroll kernel keys thru MOK Mimi Zohar
  12 siblings, 0 replies; 25+ messages in thread
From: Eric Snowberg @ 2021-07-26 17:13 UTC (permalink / raw)
  To: keyrings, linux-integrity, zohar, dhowells, dwmw2, herbert,
	davem, jarkko, jmorris, serge
  Cc: eric.snowberg, keescook, gregkh, torvalds, scott.branden,
	weiyongjun1, nayna, ebiggers, ardb, nramas, lszubowi,
	linux-kernel, linux-crypto, linux-security-module,
	James.Bottomley, pjones, glin, konrad.wilk

Store a reference to the mok keyring in system keyring code. The
reference is only set when trust_moklist is true.  This prevents the mok
keyring from linking to either the builtin or secondary trusted keyrings
with an empty mok list.

Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
---
v2: Initial version
---
 security/integrity/digsig.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
index 3a12cc85b528..cf13f4c56517 100644
--- a/security/integrity/digsig.c
+++ b/security/integrity/digsig.c
@@ -112,6 +112,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_MOK && trust_moklist())
+			set_mok_trusted_keys(keyring[id]);
 		if (id == INTEGRITY_KEYRING_IMA)
 			load_module_cert(keyring[id]);
 	}
-- 
2.18.4


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH RFC v2 00/12] Enroll kernel keys thru MOK
  2021-07-26 17:13 [PATCH RFC v2 00/12] Enroll kernel keys thru MOK Eric Snowberg
                   ` (11 preceding siblings ...)
  2021-07-26 17:13 ` [PATCH RFC v2 12/12] integrity: store reference to mok keyring Eric Snowberg
@ 2021-08-03 17:01 ` Mimi Zohar
  2021-08-03 19:52   ` Eric Snowberg
  12 siblings, 1 reply; 25+ messages in thread
From: Mimi Zohar @ 2021-08-03 17:01 UTC (permalink / raw)
  To: Eric Snowberg, keyrings, linux-integrity, dhowells, dwmw2,
	herbert, davem, jarkko, jmorris, serge
  Cc: keescook, gregkh, torvalds, scott.branden, weiyongjun1, nayna,
	ebiggers, ardb, nramas, lszubowi, linux-kernel, linux-crypto,
	linux-security-module, James.Bottomley, pjones, glin,
	konrad.wilk

Hi Eric,

On Mon, 2021-07-26 at 13:13 -0400, Eric Snowberg wrote:

> When the kernel boots, if MokListTrustedRT is set and
> EFI_VARIABLE_NON_VOLATILE is not set, the MokListRT is loaded into the
> mok keyring instead of the platform keyring. Mimi has suggested that
> only CA keys or keys that can be vouched for by other kernel keys be
> loaded into this keyring. All other certs will load into the platform
> keyring instead.

I suggested only loading the CA keys stored in the MOK db onto the MOK
keyring.  Like the builtin trusted keyring, the MOK keyring would also
be linked to the secondary keyring.   Assuming the secondary keyring is
defined, all other properly signed MOK db keys  - signed by keys on the
builtin, secondary or MOK keyring - would be loaded onto the secondary
keyring.

As previously discussed, this might require reading the MOK db twice -
once to load the CA keys on the MOK keyring, a second time to load the
remaining properly signed keys onto the secondary keyring.


thanks,

Mimi


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH RFC v2 00/12] Enroll kernel keys thru MOK
  2021-08-03 17:01 ` [PATCH RFC v2 00/12] Enroll kernel keys thru MOK Mimi Zohar
@ 2021-08-03 19:52   ` Eric Snowberg
  2021-08-04  1:14     ` Mimi Zohar
  0 siblings, 1 reply; 25+ messages in thread
From: Eric Snowberg @ 2021-08-03 19:52 UTC (permalink / raw)
  To: Mimi Zohar
  Cc: keyrings, linux-integrity, dhowells, dwmw2, herbert, davem,
	jarkko, jmorris, serge, keescook, gregkh, torvalds,
	scott.branden, weiyongjun1, nayna, ebiggers, ardb, nramas,
	lszubowi, linux-kernel, linux-crypto, linux-security-module,
	James.Bottomley, pjones, glin, konrad.wilk


> On Aug 3, 2021, at 11:01 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> 
> On Mon, 2021-07-26 at 13:13 -0400, Eric Snowberg wrote:
> 
>> When the kernel boots, if MokListTrustedRT is set and
>> EFI_VARIABLE_NON_VOLATILE is not set, the MokListRT is loaded into the
>> mok keyring instead of the platform keyring. Mimi has suggested that
>> only CA keys or keys that can be vouched for by other kernel keys be
>> loaded into this keyring. All other certs will load into the platform
>> keyring instead.
> 
> I suggested only loading the CA keys stored in the MOK db onto the MOK
> keyring.  Like the builtin trusted keyring, the MOK keyring would also
> be linked to the secondary keyring.   Assuming the secondary keyring is
> defined, all other properly signed MOK db keys  - signed by keys on the
> builtin, secondary or MOK keyring - would be loaded onto the secondary
> keyring.
> 
> As previously discussed, this might require reading the MOK db twice -
> once to load the CA keys on the MOK keyring, a second time to load the
> remaining properly signed keys onto the secondary keyring.

I’m only loading CA keys or keys that can be vouched for by other kernel 
keys into the new mok keyring.  Currently, I’m not doing another pass.  I 
could add another pass, but it would not solve the issue with someone trying 
to load an intermediate CA along with a leaf cert.  This would require yet 
a third pass.  I wasn’t sure if this added complexity was necessary.  

Currently, any CA contained within the MOK db would now be trusted by the 
kernel.  Someone using a kernel with the secondary keyring enabled could 
load the intermediate and leaf certs themselves following boot.  Taking 
this into account, if you’d like to see two passes, let me know and I’ll add 
that in v3.  If a second pass is done, do you really want these additional 
keys added to the secondary keyring or should they go into the mok keyring
instead?  I was under the impression the secondary should be empty until a
user adds their own keys into it. Thanks.


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH RFC v2 00/12] Enroll kernel keys thru MOK
  2021-08-03 19:52   ` Eric Snowberg
@ 2021-08-04  1:14     ` Mimi Zohar
  2021-08-04  2:56       ` Eric Snowberg
  0 siblings, 1 reply; 25+ messages in thread
From: Mimi Zohar @ 2021-08-04  1:14 UTC (permalink / raw)
  To: Eric Snowberg
  Cc: keyrings, linux-integrity, dhowells, dwmw2, herbert, davem,
	jarkko, jmorris, serge, keescook, gregkh, torvalds,
	scott.branden, weiyongjun1, nayna, ebiggers, ardb, nramas,
	lszubowi, linux-kernel, linux-crypto, linux-security-module,
	James.Bottomley, pjones, glin, konrad.wilk

Hi Eric,

On Tue, 2021-08-03 at 13:52 -0600, Eric Snowberg wrote:
> > On Aug 3, 2021, at 11:01 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> > 
> > On Mon, 2021-07-26 at 13:13 -0400, Eric Snowberg wrote:
> > 
> >> When the kernel boots, if MokListTrustedRT is set and
> >> EFI_VARIABLE_NON_VOLATILE is not set, the MokListRT is loaded into the
> >> mok keyring instead of the platform keyring. Mimi has suggested that
> >> only CA keys or keys that can be vouched for by other kernel keys be
> >> loaded into this keyring. All other certs will load into the platform
> >> keyring instead.
> > 
> > I suggested only loading the CA keys stored in the MOK db onto the MOK
> > keyring.  Like the builtin trusted keyring, the MOK keyring would also
> > be linked to the secondary keyring.   Assuming the secondary keyring is
> > defined, all other properly signed MOK db keys  - signed by keys on the
> > builtin, secondary or MOK keyring - would be loaded onto the secondary
> > keyring.
> > 
> > As previously discussed, this might require reading the MOK db twice -
> > once to load the CA keys on the MOK keyring, a second time to load the
> > remaining properly signed keys onto the secondary keyring.
> 
> I’m only loading CA keys or keys that can be vouched for by other kernel 
> keys into the new mok keyring.

The cover letter implies that this suggestion is coming from me, which
it definitely is not.  My preference, as I made clear from the very
beginning, is to load ONLY the MOK DB CA keys onto the mok
keyring.   (And even go one step farther, requiring the MOK DB CA
key(s) to be identified on the boot command line.)

> Currently, I’m not doing another pass.  I 
> could add another pass, but it would not solve the issue with someone trying 
> to load an intermediate CA along with a leaf cert.  This would require yet 
> a third pass.  I wasn’t sure if this added complexity was necessary.  
> 
> Currently, any CA contained within the MOK db would now be trusted by the 
> kernel.  Someone using a kernel with the secondary keyring enabled could 
> load the intermediate and leaf certs themselves following boot.

Correct, as previously discussed, the other signed MOK DB keys may be
loaded by userspace.   The only reason we're interested in any of the
other MOK DB keys is prevent a regression.  As you previously pointed
out all of the MOK DB keys are currently being loaded onto the platform
keyring.  So leave the existing code, which loads the MOK DB keys onto
the platform keyring, alone to prevent that regression.  It's already
being controlled by a UEFI variable.

> Taking 
> this into account, if you’d like to see two passes, let me know and I’ll add 
> that in v3.  If a second pass is done, do you really want these additional 
> keys added to the secondary keyring or should they go into the mok keyring
> instead?  I was under the impression the secondary should be empty until a
> user adds their own keys into it. Thanks.

Again, my preference would be to load ONLY the MOK DB CA keys onto the
mok keyring.

If YOU decide you want to load the signed keys stored in MOK DB, be my
guest.  However, they should be loaded onto the secondary keyring and a
new restriction defined, similar to
"restrict_link_by_builtin_and_secondary_trusted", which includes mok as
well.

thanks,

Mimi


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH RFC v2 00/12] Enroll kernel keys thru MOK
  2021-08-04  1:14     ` Mimi Zohar
@ 2021-08-04  2:56       ` Eric Snowberg
  2021-08-05 13:58         ` Mimi Zohar
  0 siblings, 1 reply; 25+ messages in thread
From: Eric Snowberg @ 2021-08-04  2:56 UTC (permalink / raw)
  To: Mimi Zohar
  Cc: keyrings, linux-integrity, David Howells, dwmw2, herbert, davem,
	jarkko, jmorris, serge, keescook, gregkh, torvalds,
	scott.branden, weiyongjun1, nayna, ebiggers, ardb,
	Lakshmi Ramasubramanian, lszubowi, linux-kernel, linux-crypto,
	linux-security-module, James.Bottomley@hansenpartnership.com,
	pjones, glin, Konrad Wilk



> On Aug 3, 2021, at 7:14 PM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> 
> On Tue, 2021-08-03 at 13:52 -0600, Eric Snowberg wrote:
>>> On Aug 3, 2021, at 11:01 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
>>> 
>>> On Mon, 2021-07-26 at 13:13 -0400, Eric Snowberg wrote:
>>> 
>>>> When the kernel boots, if MokListTrustedRT is set and
>>>> EFI_VARIABLE_NON_VOLATILE is not set, the MokListRT is loaded into the
>>>> mok keyring instead of the platform keyring. Mimi has suggested that
>>>> only CA keys or keys that can be vouched for by other kernel keys be
>>>> loaded into this keyring. All other certs will load into the platform
>>>> keyring instead.
>>> 
>>> I suggested only loading the CA keys stored in the MOK db onto the MOK
>>> keyring.  Like the builtin trusted keyring, the MOK keyring would also
>>> be linked to the secondary keyring.   Assuming the secondary keyring is
>>> defined, all other properly signed MOK db keys  - signed by keys on the
>>> builtin, secondary or MOK keyring - would be loaded onto the secondary
>>> keyring.
>>> 
>>> As previously discussed, this might require reading the MOK db twice -
>>> once to load the CA keys on the MOK keyring, a second time to load the
>>> remaining properly signed keys onto the secondary keyring.
>> 
>> I’m only loading CA keys or keys that can be vouched for by other kernel 
>> keys into the new mok keyring.
> 
> The cover letter implies that this suggestion is coming from me, which
> it definitely is not.  My preference, as I made clear from the very
> beginning, is to load ONLY the MOK DB CA keys onto the mok
> keyring.   (And even go one step farther, requiring the MOK DB CA
> key(s) to be identified on the boot command line.)

Ok, got it.  I guess I misunderstood and was thinking built-in should be 
referenced too for things going into the new mok keyring.

>> Currently, I’m not doing another pass.  I 
>> could add another pass, but it would not solve the issue with someone trying 
>> to load an intermediate CA along with a leaf cert.  This would require yet 
>> a third pass.  I wasn’t sure if this added complexity was necessary.  
>> 
>> Currently, any CA contained within the MOK db would now be trusted by the 
>> kernel.  Someone using a kernel with the secondary keyring enabled could 
>> load the intermediate and leaf certs themselves following boot.
> 
> Correct, as previously discussed, the other signed MOK DB keys may be
> loaded by userspace.   The only reason we're interested in any of the
> other MOK DB keys is prevent a regression.  As you previously pointed
> out all of the MOK DB keys are currently being loaded onto the platform
> keyring.  So leave the existing code, which loads the MOK DB keys onto
> the platform keyring, alone to prevent that regression.  It's already
> being controlled by a UEFI variable.

With this series, I do not believe a regression exists.  With a single pass, 
keys are either loaded into the mok or the platform keyring.  Since the mok
is linked to the secondary (or the built-in),  during kexec signature validation,
all keys are referenced. 

>> Taking 
>> this into account, if you’d like to see two passes, let me know and I’ll add 
>> that in v3.  If a second pass is done, do you really want these additional 
>> keys added to the secondary keyring or should they go into the mok keyring
>> instead?  I was under the impression the secondary should be empty until a
>> user adds their own keys into it. Thanks.
> 
> Again, my preference would be to load ONLY the MOK DB CA keys onto the
> mok keyring.

Ok, I’ll update the current code to just load CA keys into the mok in v3.  This would
simplify the new restrict_link_by_ca function. 

With that change, do you see any issues with how I’m doing the linking?  With the 
mok keyring linked to the secondary keyring, the platform keyring may only contain 
a subset of the keys it originally contained.  But, as I described above, I don’t believe
it will lead to a regression since all keys get referenced. Thanks.


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH RFC v2 00/12] Enroll kernel keys thru MOK
  2021-08-04  2:56       ` Eric Snowberg
@ 2021-08-05 13:58         ` Mimi Zohar
  0 siblings, 0 replies; 25+ messages in thread
From: Mimi Zohar @ 2021-08-05 13:58 UTC (permalink / raw)
  To: Eric Snowberg
  Cc: keyrings, linux-integrity, David Howells, dwmw2, herbert, davem,
	jarkko, jmorris, serge, keescook, gregkh, torvalds,
	scott.branden, weiyongjun1, nayna, ebiggers, ardb,
	Lakshmi Ramasubramanian, lszubowi, linux-kernel, linux-crypto,
	linux-security-module, James.Bottomley@hansenpartnership.com,
	pjones, glin, Konrad Wilk

On Wed, 2021-08-04 at 02:56 +0000, Eric Snowberg wrote:

> Ok, I’ll update the current code to just load CA keys into the mok in v3.  This would
> simplify the new restrict_link_by_ca function. 

Thank you!
> 
> With that change, do you see any issues with how I’m doing the linking?  With the 
> mok keyring linked to the secondary keyring, the platform keyring may only contain 
> a subset of the keys it originally contained.  But, as I described above, I don’t believe
> it will lead to a regression since all keys get referenced. Thanks.

I think there is a problem.  Only the builtin keys should ever be on
the builtin keyring.  The builtin keyring would need to be linked to
the mok keyring.  But in the secondary keyring case, the linking
should be the reverse, where the mok keyring would be linked to the
secondary keyring, similar to how the builtin keyring is linked to the
secondary keyring.

        if (key_link(secondary_trusted_keys, builtin_trusted_keys) < 0)
                panic("Can't link trusted keyrings\n");


thanks,

Mimi


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH RFC v2 10/12] KEYS: link system_trusted_keys to mok_trusted_keys
  2021-07-26 17:13 ` [PATCH RFC v2 10/12] KEYS: link system_trusted_keys to mok_trusted_keys Eric Snowberg
@ 2021-08-05 13:58   ` Mimi Zohar
  2021-08-06  1:29     ` Eric Snowberg
  0 siblings, 1 reply; 25+ messages in thread
From: Mimi Zohar @ 2021-08-05 13:58 UTC (permalink / raw)
  To: Eric Snowberg, keyrings, linux-integrity, dhowells, dwmw2,
	herbert, davem, jarkko, jmorris, serge
  Cc: keescook, gregkh, torvalds, scott.branden, weiyongjun1, nayna,
	ebiggers, ardb, nramas, lszubowi, linux-kernel, linux-crypto,
	linux-security-module, James.Bottomley, pjones, glin,
	konrad.wilk

On Mon, 2021-07-26 at 13:13 -0400, Eric Snowberg wrote:

> diff --git a/certs/system_keyring.c b/certs/system_keyring.c
> index dcaf74102ab2..b27ae30eaadc 100644
> --- a/certs/system_keyring.c
> +++ b/certs/system_keyring.c
> @@ -45,6 +45,15 @@ int restrict_link_by_builtin_trusted(struct key *dest_keyring,
>  				     const union key_payload *payload,
>  				     struct key *restriction_key)
>  {
> +	/* If the secondary trusted keyring is not enabled, we may link
> +	 * through to the mok keyring and the search may follow that link.
> +	 */

Refer to section "8) Commenting" of Documentation/process/coding-
style.rst for the format of multi line comments.

> +	if (mok_trusted_keys && type == &key_type_keyring &&
> +	    dest_keyring == builtin_trusted_keys &&
> +	    payload == &mok_trusted_keys->payload)
> +		/* Allow the mok keyring to be added to the builtin */
> +		return 0;
> +

Unless you're changing the meaning of the restriction, then a new
restriction needs to be defined.  In this case, please don't change the
meaning of restrict_link_by_builtin_trusted().  Instead define a new
restriction named restrict_link_by_builtin_and_ca_trusted().

>  	return restrict_link_by_signature(dest_keyring, type, payload,
>  					  builtin_trusted_keys);
>  }
> @@ -91,6 +100,15 @@ int restrict_link_by_builtin_and_secondary_trusted(
>  		/* Allow the builtin keyring to be added to the secondary */
>  		return 0;
>  
> +	/* If we have a secondary trusted keyring, it may contain a link
> +	 * through to the mok keyring and the search may follow that link.
> +	 */
> +	if (mok_trusted_keys && type == &key_type_keyring &&
> +	    dest_keyring == secondary_trusted_keys &&
> +	    payload == &mok_trusted_keys->payload)
> +		/* Allow the mok keyring to be added to the secondary */
> +		return 0;
> +

Similarly here, please define a new restriction maybe named
restrict_link_by_builtin_secondary_and_ca_trusted().   To avoid code
duplication, the new restriction could be a wrapper around the existing
function.

>  	return restrict_link_by_signature(dest_keyring, type, payload,
>  					  secondary_trusted_keys);
>  }
> @@ -321,5 +339,8 @@ void __init set_platform_trusted_keys(struct key *keyring)
>  void __init set_mok_trusted_keys(struct key *keyring)
>  {
>  	mok_trusted_keys = keyring;
> +
> +	if (key_link(system_trusted_keys, mok_trusted_keys) < 0)
> +		panic("Can't link (mok) trusted keyrings\n");
>  }

From the thread discussion on 00/12:

Only the builtin keys should ever be on the builtin keyring.  The
builtin keyring would need to be linked to the mok keyring.  But in the
secondary keyring case, the mok keyring would be linked to the
secondary keyring, similar to how the builtin keyring is linked to the
secondary keyring.

        if (key_link(secondary_trusted_keys, builtin_trusted_keys) < 0)
                panic("Can't link trusted keyrings\n");


thanks,

Mimi

>  #endif



^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH RFC v2 02/12] KEYS: CA link restriction
  2021-07-26 17:13 ` [PATCH RFC v2 02/12] KEYS: CA link restriction Eric Snowberg
@ 2021-08-05 14:00   ` Mimi Zohar
  0 siblings, 0 replies; 25+ messages in thread
From: Mimi Zohar @ 2021-08-05 14:00 UTC (permalink / raw)
  To: Eric Snowberg, keyrings, linux-integrity, dhowells, dwmw2,
	herbert, davem, jarkko, jmorris, serge
  Cc: keescook, gregkh, torvalds, scott.branden, weiyongjun1, nayna,
	ebiggers, ardb, nramas, lszubowi, linux-kernel, linux-crypto,
	linux-security-module, James.Bottomley, pjones, glin,
	konrad.wilk

On Mon, 2021-07-26 at 13:13 -0400, Eric Snowberg wrote:
> Add a new link restriction.  Restrict the addition of keys in a keyring
> based on the key to be added being a CA (self-signed) or by being
> vouched for by a key in either the built-in or the secondary trusted
> keyrings.
> 
> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>

As discussed, please remove "or by being vouched for by a key in
either the built-in or the secondary trusted keyrings."

If these keys were to be loaded onto a keyring other than the platform
keyring, they should be loaded onto the secondary keyring.  The
secondary restriction currently allows certificates signed by keys on
either the builtin or the secondary keyring, to be loaded onto the
secondary keyring.   A new restriction would also allow certificates
signed by keys on the ".mok" keyring.

> +/**
> + * restrict_link_by_ca - Restrict additions to a ring of public keys
> + * based on it being a CA
> + * @dest_keyring: Keyring being linked to.
> + * @type: The type of key being added.
> + * @payload: The payload of the new key.
> + * @trusted: A key or ring of keys that can be used to vouch for the new cert.
> + *
> + * Check if the new certificate is a CA or if they key can be vouched for
> + * by keys already linked in the destination keyring or the trusted
> + * keyring.  If one of those is the signing key or it is self signed, then
> + * mark the new certificate as being ok to link.
> + *
> + * Returns 0 if the new certificate was accepted, -ENOKEY if we could not find
> + * a matching parent certificate in the trusted list.  -ENOPKG if the signature
> + * uses unsupported crypto, or some other error if there is a matching
> + * certificate  but the signature check cannot be performed.
> + */

Please update the function description as discussed, removing "or if they
key can be vouched for by keys already linked in the destination
keyring or the trusted keyring."

The kernel doc "Brief description of function" should not wrap.  The
variable definition shouldn't be suffixed with a period.  Please refer
to Documentation/doc-guide/kernel-doc.rst.

> +int restrict_link_by_ca(struct key *dest_keyring,

The UEFI db CA keys should only be loaded on boot.  Should this be
annotated as __init?

> +			const struct key_type *type,
> +			const union key_payload *payload,
> +			struct key *trust_keyring)
> +{
> +	const struct public_key_signature *sig;
> +	const struct public_key *pkey;
> +	struct key *key;
> +	int ret;
> +
> +	if (type != &key_type_asymmetric)
> +		return -EOPNOTSUPP;
> +
> +	sig = payload->data[asym_auth];
> +	if (!sig)
> +		return -ENOPKG;
> +
> +	if (!sig->auth_ids[0] && !sig->auth_ids[1])
> +		return -ENOKEY;
> +
> +	pkey = payload->data[asym_crypto];
> +	if (!pkey)
> +		return -ENOPKG;
> +
> +	ret = public_key_verify_signature(pkey, sig);
> +	if (!ret)
> +		return 0;
> +
> +	if (!trust_keyring)
> +		return -ENOKEY;
> +
> +	key = find_asymmetric_key(trust_keyring,
> +				  sig->auth_ids[0], sig->auth_ids[1],
> +				  false);
> +	if (IS_ERR(key))
> +		return -ENOKEY;
> +
> +	ret = verify_signature(key, sig);
> +	key_put(key);
> +	return ret;
> +}
> +
>  static bool match_either_id(const struct asymmetric_key_ids *pair,
>  			    const struct asymmetric_key_id *single)
>  {


> +extern int restrict_link_by_system_trusted_or_ca(
> +	struct key *dest_keyring,
> +	const struct key_type *type,
> +	const union key_payload *payload,
> +	struct key *restrict_key);

After the discussed change, this shouldn't be needed.

thanks,

Mimi

> +
>  #ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
>  extern int restrict_link_by_builtin_and_secondary_trusted(
>  	struct key *keyring,



^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH RFC v2 10/12] KEYS: link system_trusted_keys to mok_trusted_keys
  2021-08-05 13:58   ` Mimi Zohar
@ 2021-08-06  1:29     ` Eric Snowberg
  2021-08-06  3:19       ` Mimi Zohar
  0 siblings, 1 reply; 25+ messages in thread
From: Eric Snowberg @ 2021-08-06  1:29 UTC (permalink / raw)
  To: Mimi Zohar
  Cc: keyrings, linux-integrity, David Howells, David Woodhouse,
	Herbert Xu, David S . Miller, Jarkko Sakkinen, James Morris,
	Serge E . Hallyn, keescook, gregkh, torvalds, scott.branden,
	weiyongjun1, nayna, ebiggers, ardb, nramas, lszubowi,
	linux-kernel, linux-crypto, linux-security-module,
	James.Bottomley, pjones, glin, konrad.wilk


> On Aug 5, 2021, at 7:58 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> 
> On Mon, 2021-07-26 at 13:13 -0400, Eric Snowberg wrote:
> 
>> diff --git a/certs/system_keyring.c b/certs/system_keyring.c
>> index dcaf74102ab2..b27ae30eaadc 100644
>> --- a/certs/system_keyring.c
>> +++ b/certs/system_keyring.c
>> @@ -45,6 +45,15 @@ int restrict_link_by_builtin_trusted(struct key *dest_keyring,
>> 				     const union key_payload *payload,
>> 				     struct key *restriction_key)
>> {
>> +	/* If the secondary trusted keyring is not enabled, we may link
>> +	 * through to the mok keyring and the search may follow that link.
>> +	 */
> 
> Refer to section "8) Commenting" of Documentation/process/coding-
> style.rst for the format of multi line comments.

Sure, I’ll fix this in the next version.

>> +	if (mok_trusted_keys && type == &key_type_keyring &&
>> +	    dest_keyring == builtin_trusted_keys &&
>> +	    payload == &mok_trusted_keys->payload)
>> +		/* Allow the mok keyring to be added to the builtin */
>> +		return 0;
>> +
> 
> Unless you're changing the meaning of the restriction, then a new
> restriction needs to be defined.  In this case, please don't change the
> meaning of restrict_link_by_builtin_trusted().  Instead define a new
> restriction named restrict_link_by_builtin_and_ca_trusted().


Along with this

>> 	return restrict_link_by_signature(dest_keyring, type, payload,
>> 					  builtin_trusted_keys);
>> }
>> @@ -91,6 +100,15 @@ int restrict_link_by_builtin_and_secondary_trusted(
>> 		/* Allow the builtin keyring to be added to the secondary */
>> 		return 0;
>> 
>> +	/* If we have a secondary trusted keyring, it may contain a link
>> +	 * through to the mok keyring and the search may follow that link.
>> +	 */
>> +	if (mok_trusted_keys && type == &key_type_keyring &&
>> +	    dest_keyring == secondary_trusted_keys &&
>> +	    payload == &mok_trusted_keys->payload)
>> +		/* Allow the mok keyring to be added to the secondary */
>> +		return 0;
>> +
> 
> Similarly here, please define a new restriction maybe named
> restrict_link_by_builtin_secondary_and_ca_trusted().   To avoid code
> duplication, the new restriction could be a wrapper around the existing
> function.

and this too.

> 
>> 	return restrict_link_by_signature(dest_keyring, type, payload,
>> 					  secondary_trusted_keys);
>> }
>> @@ -321,5 +339,8 @@ void __init set_platform_trusted_keys(struct key *keyring)
>> void __init set_mok_trusted_keys(struct key *keyring)
>> {
>> 	mok_trusted_keys = keyring;
>> +
>> +	if (key_link(system_trusted_keys, mok_trusted_keys) < 0)
>> +		panic("Can't link (mok) trusted keyrings\n");
>> }
> 
> From the thread discussion on 00/12:
> 
> Only the builtin keys should ever be on the builtin keyring.  The
> builtin keyring would need to be linked to the mok keyring.  But in the
> secondary keyring case, the mok keyring would be linked to the
> secondary keyring, similar to how the builtin keyring is linked to the
> secondary keyring.
> 
>        if (key_link(secondary_trusted_keys, builtin_trusted_keys) < 0)
>                panic("Can't link trusted keyrings\n");


This part is confusing me though.

Here are some of the tests I’m performing with the current series:

Initial setup:
Create and enroll my own key into the MOK.
Sign a kernel, kernel module and IMA key with my new CA key.
Boot with lockdown enabled (to enforce sig validation).

Kernel built with CONFIG_SECONDARY_TRUSTED_KEYRING=y

$ keyctl show %:.secondary_trusted_keys
Keyring
 530463486 ---lswrv      0     0  keyring: .secondary_trusted_keys
 411466727 ---lswrv      0     0   \_ keyring: .builtin_trusted_keys
 979167715 ---lswrv      0     0   |   \_ asymmetric: Build time autogenerated kernel key: 07a56e29cfa1e21379aff2c522efff7d1963202a
 534573591 ---lswrv      0     0   |   \_ asymmetric: Oracle-CA: Oracle certificate signing key: aeefb4bfde095cacaabff81dd266974b1b4e23b8
 968109018 ---lswrv      0     0   \_ keyring: .mok
 857795115 ---lswrv      0     0       \_ asymmetric: Erics-CA: UEK signing key: 9bfa6860483aa46bd83f7fa1289d9fc35799e93b

With this setup I can:
* load a kernel module signed with my CA key
* run "kexec -ls" with the kernel signed with my CA key
* run "kexec -ls" with a kernel signed by a key in the platform keyring
* load another key into the secondary trusted keyring that is signed by my CA key
* load a key into the ima keyring, signed by my CA key

Kernel built without CONFIG_SECONDARY_TRUSTED_KEYRING defined

$ keyctl show %:.builtin_trusted_keys
Keyring
 812785375 ---lswrv      0     0  keyring: .builtin_trusted_keys
 455418681 ---lswrv      0     0   \_ keyring: .mok
 910809006 ---lswrv      0     0   |   \_ asymmetric: Erics-CA: UEK signing key: 9bfa6860483aa46bd83f7fa1289d9fc35799e93b
 115345009 ---lswrv      0     0   \_ asymmetric: Oracle-CA: Oracle certificate signing key: aeefb4bfde095cacaabff81dd266974b1b4e23b8
 513131506 ---lswrv      0     0   \_ asymmetric: Build time autogenerated kernel key: 22353509f203b55b84f15d0aadeddc134b646185

With this setup I can:
* load a kernel module signed with my CA key
* run "kexec -ls" with the kernel signed with my CA key
* run "kexec -ls" with a kernel signed by a key in the platform keyring
* load a key into the ima keyring, signed by my CA key

So why would the linking need to be switched?  Is there a test I’m
missing?  Thanks.


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH RFC v2 10/12] KEYS: link system_trusted_keys to mok_trusted_keys
  2021-08-06  1:29     ` Eric Snowberg
@ 2021-08-06  3:19       ` Mimi Zohar
  2021-08-06 15:00         ` Eric Snowberg
  0 siblings, 1 reply; 25+ messages in thread
From: Mimi Zohar @ 2021-08-06  3:19 UTC (permalink / raw)
  To: Eric Snowberg
  Cc: keyrings, linux-integrity, David Howells, David Woodhouse,
	Herbert Xu, David S . Miller, Jarkko Sakkinen, James Morris,
	Serge E . Hallyn, keescook, gregkh, torvalds, scott.branden,
	weiyongjun1, nayna, ebiggers, ardb, nramas, lszubowi,
	linux-kernel, linux-crypto, linux-security-module,
	James.Bottomley, pjones, glin, konrad.wilk

On Thu, 2021-08-05 at 19:29 -0600, Eric Snowberg wrote:

> > From the thread discussion on 00/12:
> > 
> > Only the builtin keys should ever be on the builtin keyring.  The
> > builtin keyring would need to be linked to the mok keyring.  But in the
> > secondary keyring case, the mok keyring would be linked to the
> > secondary keyring, similar to how the builtin keyring is linked to the
> > secondary keyring.
> > 
> >        if (key_link(secondary_trusted_keys, builtin_trusted_keys) < 0)
> >                panic("Can't link trusted keyrings\n");
> 
> 
> This part is confusing me though.
> 
> Here are some of the tests I’m performing with the current series:
> 
> Initial setup:
> Create and enroll my own key into the MOK.
> Sign a kernel, kernel module and IMA key with my new CA key.
> Boot with lockdown enabled (to enforce sig validation).
> 
> Kernel built with CONFIG_SECONDARY_TRUSTED_KEYRING=y
> 
> $ keyctl show %:.secondary_trusted_keys
> Keyring
>  530463486 ---lswrv      0     0  keyring: .secondary_trusted_keys
>  411466727 ---lswrv      0     0   \_ keyring: .builtin_trusted_keys
>  979167715 ---lswrv      0     0   |   \_ asymmetric: Build time autogenerated kernel key: 07a56e29cfa1e21379aff2c522efff7d1963202a
>  534573591 ---lswrv      0     0   |   \_ asymmetric: Oracle-CA: Oracle certificate signing key: aeefb4bfde095cacaabff81dd266974b1b4e23b8
>  968109018 ---lswrv      0     0   \_ keyring: .mok
>  857795115 ---lswrv      0     0       \_ asymmetric: Erics-CA: UEK signing key: 9bfa6860483aa46bd83f7fa1289d9fc35799e93b
> 
> With this setup I can:
> * load a kernel module signed with my CA key
> * run "kexec -ls" with the kernel signed with my CA key
> * run "kexec -ls" with a kernel signed by a key in the platform keyring
> * load another key into the secondary trusted keyring that is signed by my CA key
> * load a key into the ima keyring, signed by my CA key
> 
> Kernel built without CONFIG_SECONDARY_TRUSTED_KEYRING defined
> 
> $ keyctl show %:.builtin_trusted_keys
> Keyring
>  812785375 ---lswrv      0     0  keyring: .builtin_trusted_keys
>  455418681 ---lswrv      0     0   \_ keyring: .mok
>  910809006 ---lswrv      0     0   |   \_ asymmetric: Erics-CA: UEK signing key: 9bfa6860483aa46bd83f7fa1289d9fc35799e93b
>  115345009 ---lswrv      0     0   \_ asymmetric: Oracle-CA: Oracle certificate signing key: aeefb4bfde095cacaabff81dd266974b1b4e23b8
>  513131506 ---lswrv      0     0   \_ asymmetric: Build time autogenerated kernel key: 22353509f203b55b84f15d0aadeddc134b646185
> 
> With this setup I can:
> * load a kernel module signed with my CA key
> * run "kexec -ls" with the kernel signed with my CA key
> * run "kexec -ls" with a kernel signed by a key in the platform keyring
> * load a key into the ima keyring, signed by my CA key
> 
> So why would the linking need to be switched?  Is there a test I’m
> missing?  Thanks.

It's a question of semantics.  The builtin keyring name is self
describing.  It should only contain the keys compiled into the kernel
or inserted post build into the reserved memory.  Not only the kernel
uses the builtin keyring, but userspace may as well[1].  Other users of
the builtin keyring might not want to trust the mok keyring as well.

thanks,

Mimi

[1] Refer to Mat Martineau's LSS 2019 talk titled "Using and
Implementing Keyring Restrictions in Userspace". 


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH RFC v2 10/12] KEYS: link system_trusted_keys to mok_trusted_keys
  2021-08-06  3:19       ` Mimi Zohar
@ 2021-08-06 15:00         ` Eric Snowberg
  2021-08-06 15:18           ` Mimi Zohar
  0 siblings, 1 reply; 25+ messages in thread
From: Eric Snowberg @ 2021-08-06 15:00 UTC (permalink / raw)
  To: Mimi Zohar
  Cc: keyrings, linux-integrity, David Howells, David Woodhouse,
	Herbert Xu, David S . Miller, Jarkko Sakkinen, James Morris,
	Serge E . Hallyn, keescook, gregkh, torvalds, scott.branden,
	weiyongjun1, nayna, ebiggers, ardb, Lakshmi Ramasubramanian,
	lszubowi, linux-kernel, linux-crypto, linux-security-module,
	James.Bottomley, pjones, glin, konrad.wilk


> On Aug 5, 2021, at 9:19 PM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> 
> On Thu, 2021-08-05 at 19:29 -0600, Eric Snowberg wrote:
> 
>>> From the thread discussion on 00/12:
>>> 
>>> Only the builtin keys should ever be on the builtin keyring.  The
>>> builtin keyring would need to be linked to the mok keyring.  But in the
>>> secondary keyring case, the mok keyring would be linked to the
>>> secondary keyring, similar to how the builtin keyring is linked to the
>>> secondary keyring.
>>> 
>>>       if (key_link(secondary_trusted_keys, builtin_trusted_keys) < 0)
>>>               panic("Can't link trusted keyrings\n");
>> 
>> 
>> This part is confusing me though.
>> 
>> Here are some of the tests I’m performing with the current series:
>> 
>> Initial setup:
>> Create and enroll my own key into the MOK.
>> Sign a kernel, kernel module and IMA key with my new CA key.
>> Boot with lockdown enabled (to enforce sig validation).
>> 
>> Kernel built with CONFIG_SECONDARY_TRUSTED_KEYRING=y
>> 
>> $ keyctl show %:.secondary_trusted_keys
>> Keyring
>> 530463486 ---lswrv      0     0  keyring: .secondary_trusted_keys
>> 411466727 ---lswrv      0     0   \_ keyring: .builtin_trusted_keys
>> 979167715 ---lswrv      0     0   |   \_ asymmetric: Build time autogenerated kernel key: 07a56e29cfa1e21379aff2c522efff7d1963202a
>> 534573591 ---lswrv      0     0   |   \_ asymmetric: Oracle-CA: Oracle certificate signing key: aeefb4bfde095cacaabff81dd266974b1b4e23b8
>> 968109018 ---lswrv      0     0   \_ keyring: .mok
>> 857795115 ---lswrv      0     0       \_ asymmetric: Erics-CA: UEK signing key: 9bfa6860483aa46bd83f7fa1289d9fc35799e93b
>> 
>> With this setup I can:
>> * load a kernel module signed with my CA key
>> * run "kexec -ls" with the kernel signed with my CA key
>> * run "kexec -ls" with a kernel signed by a key in the platform keyring
>> * load another key into the secondary trusted keyring that is signed by my CA key
>> * load a key into the ima keyring, signed by my CA key
>> 
>> Kernel built without CONFIG_SECONDARY_TRUSTED_KEYRING defined
>> 
>> $ keyctl show %:.builtin_trusted_keys
>> Keyring
>> 812785375 ---lswrv      0     0  keyring: .builtin_trusted_keys
>> 455418681 ---lswrv      0     0   \_ keyring: .mok
>> 910809006 ---lswrv      0     0   |   \_ asymmetric: Erics-CA: UEK signing key: 9bfa6860483aa46bd83f7fa1289d9fc35799e93b
>> 115345009 ---lswrv      0     0   \_ asymmetric: Oracle-CA: Oracle certificate signing key: aeefb4bfde095cacaabff81dd266974b1b4e23b8
>> 513131506 ---lswrv      0     0   \_ asymmetric: Build time autogenerated kernel key: 22353509f203b55b84f15d0aadeddc134b646185
>> 
>> With this setup I can:
>> * load a kernel module signed with my CA key
>> * run "kexec -ls" with the kernel signed with my CA key
>> * run "kexec -ls" with a kernel signed by a key in the platform keyring
>> * load a key into the ima keyring, signed by my CA key
>> 
>> So why would the linking need to be switched?  Is there a test I’m
>> missing?  Thanks.
> 
> It's a question of semantics.  The builtin keyring name is self
> describing.  It should only contain the keys compiled into the kernel
> or inserted post build into the reserved memory.  Not only the kernel
> uses the builtin keyring, but userspace may as well[1].  Other users of
> the builtin keyring might not want to trust the mok keyring as well.

Should this feature only work with kernels built with 
CONFIG_SECONDARY_TRUSTED_KEYRING defined?  If so, I could drop support in 
the next version for kernels built without it.  


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH RFC v2 10/12] KEYS: link system_trusted_keys to mok_trusted_keys
  2021-08-06 15:00         ` Eric Snowberg
@ 2021-08-06 15:18           ` Mimi Zohar
  2021-08-06 21:20             ` Eric Snowberg
  0 siblings, 1 reply; 25+ messages in thread
From: Mimi Zohar @ 2021-08-06 15:18 UTC (permalink / raw)
  To: Eric Snowberg
  Cc: keyrings, linux-integrity, David Howells, David Woodhouse,
	Herbert Xu, David S . Miller, Jarkko Sakkinen, James Morris,
	Serge E . Hallyn, keescook, gregkh, torvalds, scott.branden,
	weiyongjun1, nayna, ebiggers, ardb, Lakshmi Ramasubramanian,
	lszubowi, linux-kernel, linux-crypto, linux-security-module,
	James.Bottomley, pjones, glin, konrad.wilk

On Fri, 2021-08-06 at 09:00 -0600, Eric Snowberg wrote:
> > On Aug 5, 2021, at 9:19 PM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> > 
> > On Thu, 2021-08-05 at 19:29 -0600, Eric Snowberg wrote:
> > 
> >>> From the thread discussion on 00/12:
> >>> 
> >>> Only the builtin keys should ever be on the builtin keyring.  The
> >>> builtin keyring would need to be linked to the mok keyring.  But in the
> >>> secondary keyring case, the mok keyring would be linked to the
> >>> secondary keyring, similar to how the builtin keyring is linked to the
> >>> secondary keyring.
> >>> 
> >>>       if (key_link(secondary_trusted_keys, builtin_trusted_keys) < 0)
> >>>               panic("Can't link trusted keyrings\n");
> >> 
> >> 
> >> This part is confusing me though.
> >> 
> >> Here are some of the tests I’m performing with the current series:
> >> 
> >> Initial setup:
> >> Create and enroll my own key into the MOK.
> >> Sign a kernel, kernel module and IMA key with my new CA key.
> >> Boot with lockdown enabled (to enforce sig validation).
> >> 
> >> Kernel built with CONFIG_SECONDARY_TRUSTED_KEYRING=y
> >> 
> >> $ keyctl show %:.secondary_trusted_keys
> >> Keyring
> >> 530463486 ---lswrv      0     0  keyring: .secondary_trusted_keys
> >> 411466727 ---lswrv      0     0   \_ keyring: .builtin_trusted_keys
> >> 979167715 ---lswrv      0     0   |   \_ asymmetric: Build time autogenerated kernel key: 07a56e29cfa1e21379aff2c522efff7d1963202a
> >> 534573591 ---lswrv      0     0   |   \_ asymmetric: Oracle-CA: Oracle certificate signing key: aeefb4bfde095cacaabff81dd266974b1b4e23b8
> >> 968109018 ---lswrv      0     0   \_ keyring: .mok
> >> 857795115 ---lswrv      0     0       \_ asymmetric: Erics-CA: UEK signing key: 9bfa6860483aa46bd83f7fa1289d9fc35799e93b
> >> 
> >> With this setup I can:
> >> * load a kernel module signed with my CA key
> >> * run "kexec -ls" with the kernel signed with my CA key
> >> * run "kexec -ls" with a kernel signed by a key in the platform keyring
> >> * load another key into the secondary trusted keyring that is signed by my CA key
> >> * load a key into the ima keyring, signed by my CA key
> >> 
> >> Kernel built without CONFIG_SECONDARY_TRUSTED_KEYRING defined
> >> 
> >> $ keyctl show %:.builtin_trusted_keys
> >> Keyring
> >> 812785375 ---lswrv      0     0  keyring: .builtin_trusted_keys
> >> 455418681 ---lswrv      0     0   \_ keyring: .mok
> >> 910809006 ---lswrv      0     0   |   \_ asymmetric: Erics-CA: UEK signing key: 9bfa6860483aa46bd83f7fa1289d9fc35799e93b
> >> 115345009 ---lswrv      0     0   \_ asymmetric: Oracle-CA: Oracle certificate signing key: aeefb4bfde095cacaabff81dd266974b1b4e23b8
> >> 513131506 ---lswrv      0     0   \_ asymmetric: Build time autogenerated kernel key: 22353509f203b55b84f15d0aadeddc134b646185
> >> 
> >> With this setup I can:
> >> * load a kernel module signed with my CA key
> >> * run "kexec -ls" with the kernel signed with my CA key
> >> * run "kexec -ls" with a kernel signed by a key in the platform keyring
> >> * load a key into the ima keyring, signed by my CA key
> >> 
> >> So why would the linking need to be switched?  Is there a test I’m
> >> missing?  Thanks.
> > 
> > It's a question of semantics.  The builtin keyring name is self
> > describing.  It should only contain the keys compiled into the kernel
> > or inserted post build into the reserved memory.  Not only the kernel
> > uses the builtin keyring, but userspace may as well[1].  Other users of
> > the builtin keyring might not want to trust the mok keyring as well.
> 
> Should this feature only work with kernels built with 
> CONFIG_SECONDARY_TRUSTED_KEYRING defined?  If so, I could drop support in 
> the next version for kernels built without it.

Support for loading the CA keys stored in the MOK db onto the mok
keyring, only if the secondary keyring is configured would really
simplify the code.   Support for using the mok  keyring without the
secondary keyring being configured, could always be added later.  As
long as the other distros agree, I'm all for it.

thanks,

Mimi


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH RFC v2 10/12] KEYS: link system_trusted_keys to mok_trusted_keys
  2021-08-06 15:18           ` Mimi Zohar
@ 2021-08-06 21:20             ` Eric Snowberg
  0 siblings, 0 replies; 25+ messages in thread
From: Eric Snowberg @ 2021-08-06 21:20 UTC (permalink / raw)
  To: Mimi Zohar
  Cc: keyrings, linux-integrity, David Howells, David Woodhouse,
	Herbert Xu, David S . Miller, Jarkko Sakkinen, James Morris,
	Serge E . Hallyn, keescook, gregkh, torvalds, scott.branden,
	weiyongjun1, nayna, ebiggers, ardb, Lakshmi Ramasubramanian,
	lszubowi, linux-kernel, linux-crypto, linux-security-module,
	James Bottomley, pjones, glin, konrad.wilk


> On Aug 6, 2021, at 9:18 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> 
> On Fri, 2021-08-06 at 09:00 -0600, Eric Snowberg wrote:
>>> On Aug 5, 2021, at 9:19 PM, Mimi Zohar <zohar@linux.ibm.com> wrote:
>>> 
>>> On Thu, 2021-08-05 at 19:29 -0600, Eric Snowberg wrote:
>>> 
>>>>> From the thread discussion on 00/12:
>>>>> 
>>>>> Only the builtin keys should ever be on the builtin keyring.  The
>>>>> builtin keyring would need to be linked to the mok keyring.  But in the
>>>>> secondary keyring case, the mok keyring would be linked to the
>>>>> secondary keyring, similar to how the builtin keyring is linked to the
>>>>> secondary keyring.
>>>>> 
>>>>>      if (key_link(secondary_trusted_keys, builtin_trusted_keys) < 0)
>>>>>              panic("Can't link trusted keyrings\n");
>>>> 
>>>> 
>>>> This part is confusing me though.
>>>> 
>>>> Here are some of the tests I’m performing with the current series:
>>>> 
>>>> Initial setup:
>>>> Create and enroll my own key into the MOK.
>>>> Sign a kernel, kernel module and IMA key with my new CA key.
>>>> Boot with lockdown enabled (to enforce sig validation).
>>>> 
>>>> Kernel built with CONFIG_SECONDARY_TRUSTED_KEYRING=y
>>>> 
>>>> $ keyctl show %:.secondary_trusted_keys
>>>> Keyring
>>>> 530463486 ---lswrv      0     0  keyring: .secondary_trusted_keys
>>>> 411466727 ---lswrv      0     0   \_ keyring: .builtin_trusted_keys
>>>> 979167715 ---lswrv      0     0   |   \_ asymmetric: Build time autogenerated kernel key: 07a56e29cfa1e21379aff2c522efff7d1963202a
>>>> 534573591 ---lswrv      0     0   |   \_ asymmetric: Oracle-CA: Oracle certificate signing key: aeefb4bfde095cacaabff81dd266974b1b4e23b8
>>>> 968109018 ---lswrv      0     0   \_ keyring: .mok
>>>> 857795115 ---lswrv      0     0       \_ asymmetric: Erics-CA: UEK signing key: 9bfa6860483aa46bd83f7fa1289d9fc35799e93b
>>>> 
>>>> With this setup I can:
>>>> * load a kernel module signed with my CA key
>>>> * run "kexec -ls" with the kernel signed with my CA key
>>>> * run "kexec -ls" with a kernel signed by a key in the platform keyring
>>>> * load another key into the secondary trusted keyring that is signed by my CA key
>>>> * load a key into the ima keyring, signed by my CA key
>>>> 
>>>> Kernel built without CONFIG_SECONDARY_TRUSTED_KEYRING defined
>>>> 
>>>> $ keyctl show %:.builtin_trusted_keys
>>>> Keyring
>>>> 812785375 ---lswrv      0     0  keyring: .builtin_trusted_keys
>>>> 455418681 ---lswrv      0     0   \_ keyring: .mok
>>>> 910809006 ---lswrv      0     0   |   \_ asymmetric: Erics-CA: UEK signing key: 9bfa6860483aa46bd83f7fa1289d9fc35799e93b
>>>> 115345009 ---lswrv      0     0   \_ asymmetric: Oracle-CA: Oracle certificate signing key: aeefb4bfde095cacaabff81dd266974b1b4e23b8
>>>> 513131506 ---lswrv      0     0   \_ asymmetric: Build time autogenerated kernel key: 22353509f203b55b84f15d0aadeddc134b646185
>>>> 
>>>> With this setup I can:
>>>> * load a kernel module signed with my CA key
>>>> * run "kexec -ls" with the kernel signed with my CA key
>>>> * run "kexec -ls" with a kernel signed by a key in the platform keyring
>>>> * load a key into the ima keyring, signed by my CA key
>>>> 
>>>> So why would the linking need to be switched?  Is there a test I’m
>>>> missing?  Thanks.
>>> 
>>> It's a question of semantics.  The builtin keyring name is self
>>> describing.  It should only contain the keys compiled into the kernel
>>> or inserted post build into the reserved memory.  Not only the kernel
>>> uses the builtin keyring, but userspace may as well[1].  Other users of
>>> the builtin keyring might not want to trust the mok keyring as well.
>> 
>> Should this feature only work with kernels built with 
>> CONFIG_SECONDARY_TRUSTED_KEYRING defined?  If so, I could drop support in 
>> the next version for kernels built without it.
> 
> Support for loading the CA keys stored in the MOK db onto the mok
> keyring, only if the secondary keyring is configured would really
> simplify the code.   Support for using the mok  keyring without the
> secondary keyring being configured, could always be added later.  As
> long as the other distros agree, I'm all for it.

Agreed, it will simplify the series and there is nothing preventing the 
dropped code from being added in the future if a different distro finds
it necessary. I’ll work on this in the next version along with the other 
changes you identified.  Thanks for your review.


^ permalink raw reply	[flat|nested] 25+ messages in thread

end of thread, other threads:[~2021-08-06 21:21 UTC | newest]

Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-26 17:13 [PATCH RFC v2 00/12] Enroll kernel keys thru MOK Eric Snowberg
2021-07-26 17:13 ` [PATCH RFC v2 01/12] integrity: Introduce a Linux keyring for the Machine Owner Key (MOK) Eric Snowberg
2021-07-26 17:13 ` [PATCH RFC v2 02/12] KEYS: CA link restriction Eric Snowberg
2021-08-05 14:00   ` Mimi Zohar
2021-07-26 17:13 ` [PATCH RFC v2 03/12] integrity: Trust MOK keys if MokListTrustedRT found Eric Snowberg
2021-07-26 17:13 ` [PATCH RFC v2 04/12] integrity: add add_to_mok_keyring Eric Snowberg
2021-07-26 17:13 ` [PATCH RFC v2 05/12] integrity: restrict INTEGRITY_KEYRING_MOK to restrict_link_by_system_trusted_or_ca Eric Snowberg
2021-07-26 17:13 ` [PATCH RFC v2 06/12] integrity: accessor function to get trust_moklist Eric Snowberg
2021-07-26 17:13 ` [PATCH RFC v2 07/12] integrity: add new keyring handler for mok keys Eric Snowberg
2021-07-26 17:13 ` [PATCH RFC v2 08/12] integrity: Suppress error message for keys added to the mok keyring Eric Snowberg
2021-07-26 17:13 ` [PATCH RFC v2 09/12] KEYS: add a reference to " Eric Snowberg
2021-07-26 17:13 ` [PATCH RFC v2 10/12] KEYS: link system_trusted_keys to mok_trusted_keys Eric Snowberg
2021-08-05 13:58   ` Mimi Zohar
2021-08-06  1:29     ` Eric Snowberg
2021-08-06  3:19       ` Mimi Zohar
2021-08-06 15:00         ` Eric Snowberg
2021-08-06 15:18           ` Mimi Zohar
2021-08-06 21:20             ` Eric Snowberg
2021-07-26 17:13 ` [PATCH RFC v2 11/12] integrity: Do not allow mok keyring updates following init Eric Snowberg
2021-07-26 17:13 ` [PATCH RFC v2 12/12] integrity: store reference to mok keyring Eric Snowberg
2021-08-03 17:01 ` [PATCH RFC v2 00/12] Enroll kernel keys thru MOK Mimi Zohar
2021-08-03 19:52   ` Eric Snowberg
2021-08-04  1:14     ` Mimi Zohar
2021-08-04  2:56       ` Eric Snowberg
2021-08-05 13:58         ` Mimi Zohar

This is a public inbox, see mirroring instructions
on how to clone and mirror all data and code used for this inbox