[PATCH 0/7] Add CA enforcement keyring restrictions

[PATCH 0/7] Add CA enforcement keyring restrictions

[Eric Snowberg]

A key added to the ima keyring must be signed by a key contained within 
either the builtin trusted or secondary trusted keyrings. Currently, there are 
CA restrictions described in IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY,
but these restrictions are not enforced within code. Therefore, keys within 
either the builtin or secondary may not be a CA and could be used to
vouch for an ima key.

The machine keyring can not be used as another trust anchor for adding keys 
to the ima keyring, since CA enforcement does not currently exist [1]. This 
would expand the current integrity gap.

Introduce a new root of trust key flag to close this integrity gap for
all keyrings.  The first key type to use this is X.509.  When a X.509 
certificate is self signed, contains kernCertSign Key Usage and contains 
the CA bit, the new flag is set.  Introduce new keyring restrictions 
that not only validates a key is signed by a key contained within the 
keyring, but also validates the key has the new root of trust key flag 
set.  Use this new restriction for keys added to the ima keyring.  Now 
that we have CA enforcement, allow the machine keyring to be used as another 
trust anchor for the ima keyring.

To recap, all keys that previously loaded into the builtin, secondary or
machine keyring will still load after applying this series.  Keys
contained within these keyrings may carry the root of trust flag. The
ima keyring will use the new root of trust restriction to validate
CA enforcement. Other keyrings that require a root of trust could also 
use this in the future.

[1] https://lore.kernel.org/lkml/2d681148b6ea57241f6a7c518dd331068a5f47b0.camel@linux.ibm.com/

Eric Snowberg (7):
  KEYS: Create static version of public_key_verify_signature
  KEYS: X.509: Parse Basic Constraints for CA
  KEYS: X.509: Parse Key Usage
  KEYS: Introduce a builtin root of trust key flag
  KEYS: Introduce sig restriction that validates root of trust
  KEYS: X.509: Flag Intermediate CA certs as built in
  integrity: Use root of trust signature restriction

 certs/system_keyring.c                    | 18 ++++++++++
 crypto/asymmetric_keys/restrict.c         | 42 +++++++++++++++++++++++
 crypto/asymmetric_keys/x509_cert_parser.c | 29 ++++++++++++++++
 crypto/asymmetric_keys/x509_parser.h      |  2 ++
 crypto/asymmetric_keys/x509_public_key.c  | 12 +++++++
 include/crypto/public_key.h               |  9 +++++
 include/keys/system_keyring.h             | 17 ++++++++-
 include/linux/ima.h                       | 16 +++++++++
 include/linux/key-type.h                  |  3 ++
 include/linux/key.h                       |  2 ++
 security/integrity/Kconfig                |  1 -
 security/integrity/digsig.c               |  4 +--
 security/keys/key.c                       | 13 +++++++
 13 files changed, 164 insertions(+), 4 deletions(-)


base-commit: 3123109284176b1532874591f7c81f3837bbdc17
-- 
2.27.0

[PATCH 1/7] KEYS: Create static version of public_key_verify_signature

[Eric Snowberg]

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

Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
---
 include/crypto/public_key.h | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/include/crypto/public_key.h b/include/crypto/public_key.h
index 68f7aa2a7e55..6d61695e1cde 100644
--- a/include/crypto/public_key.h
+++ b/include/crypto/public_key.h
@@ -80,7 +80,16 @@ extern int create_signature(struct kernel_pkey_params *, const void *, void *);
 extern int verify_signature(const struct key *,
 			    const struct public_key_signature *);
 
+#if IS_REACHABLE(CONFIG_ASYMMETRIC_PUBLIC_KEY_SUBTYPE)
 int public_key_verify_signature(const struct public_key *pkey,
 				const struct public_key_signature *sig);
+#else
+static inline
+int public_key_verify_signature(const struct public_key *pkey,
+				const struct public_key_signature *sig)
+{
+	return -EINVAL;
+}
+#endif
 
 #endif /* _LINUX_PUBLIC_KEY_H */
-- 
2.27.0

[PATCH 2/7] KEYS: X.509: Parse Basic Constraints for CA

[Eric Snowberg]

Parse the X.509 Basic Constraints.  The basic constraints extension
identifies whether the subject of the certificate is a CA.

BasicConstraints ::= SEQUENCE {
        cA                      BOOLEAN DEFAULT FALSE,
        pathLenConstraint       INTEGER (0..MAX) OPTIONAL }

If the CA is true, store it in the x509_certificate.  This will be used
in a follow on patch that requires knowing if the public key is a CA.

Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
---
 crypto/asymmetric_keys/x509_cert_parser.c | 9 +++++++++
 crypto/asymmetric_keys/x509_parser.h      | 1 +
 2 files changed, 10 insertions(+)

diff --git a/crypto/asymmetric_keys/x509_cert_parser.c b/crypto/asymmetric_keys/x509_cert_parser.c
index 2899ed80bb18..30f7374ea9c0 100644
--- a/crypto/asymmetric_keys/x509_cert_parser.c
+++ b/crypto/asymmetric_keys/x509_cert_parser.c
@@ -583,6 +583,15 @@ int x509_process_extension(void *context, size_t hdrlen,
 		return 0;
 	}
 
+	if (ctx->last_oid == OID_basicConstraints) {
+		if (vlen < 2 || v[0] != (ASN1_CONS_BIT | ASN1_SEQ))
+			return -EBADMSG;
+		if (v[1] != vlen - 2)
+			return -EBADMSG;
+		if (vlen >= 4 && v[1] != 0 && v[2] == ASN1_BOOL && v[3] == 1)
+			ctx->cert->is_root_ca = true;
+	}
+
 	return 0;
 }
 
diff --git a/crypto/asymmetric_keys/x509_parser.h b/crypto/asymmetric_keys/x509_parser.h
index 97a886cbe01c..dc45df9f6594 100644
--- a/crypto/asymmetric_keys/x509_parser.h
+++ b/crypto/asymmetric_keys/x509_parser.h
@@ -38,6 +38,7 @@ struct x509_certificate {
 	bool		self_signed;		/* T if self-signed (check unsupported_sig too) */
 	bool		unsupported_sig;	/* T if signature uses unsupported crypto */
 	bool		blacklisted;
+	bool		is_root_ca;		/* T if basic constraints CA is set */
 };
 
 /*
-- 
2.27.0

[PATCH 3/7] KEYS: X.509: Parse Key Usage

[Eric Snowberg]

Parse the X.509 Key Usage.  The key usage extension defines the purpose of
the key contained in the certificate.

   id-ce-keyUsage OBJECT IDENTIFIER ::=  { id-ce 15 }

      KeyUsage ::= BIT STRING {
           digitalSignature        (0),
           contentCommitment       (1),
           keyEncipherment         (2),
           dataEncipherment        (3),
           keyAgreement            (4),
           keyCertSign             (5),
           cRLSign                 (6),
           encipherOnly            (7),
           decipherOnly            (8) }

If the keyCertSign is set, store it in the x509_certificate structure.
This will be used in a follow on patch that requires knowing the
certificate key usage type.

Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
---
 crypto/asymmetric_keys/x509_cert_parser.c | 20 ++++++++++++++++++++
 crypto/asymmetric_keys/x509_parser.h      |  1 +
 2 files changed, 21 insertions(+)

diff --git a/crypto/asymmetric_keys/x509_cert_parser.c b/crypto/asymmetric_keys/x509_cert_parser.c
index 30f7374ea9c0..a89f1e0c8a0f 100644
--- a/crypto/asymmetric_keys/x509_cert_parser.c
+++ b/crypto/asymmetric_keys/x509_cert_parser.c
@@ -576,6 +576,26 @@ int x509_process_extension(void *context, size_t hdrlen,
 		return 0;
 	}
 
+	if (ctx->last_oid == OID_keyUsage) {
+		/*
+		 * Get hold of the keyUsage bit string to validate keyCertSign
+		 * v[1] is the encoding size
+		 *       (Expect either 0x02 or 0x03, making it 1 or 2 bytes)
+		 * v[2] is the number of unused bits in the bit string
+		 *       (If >= 3 keyCertSign is missing)
+		 * v[3] and possibly v[4] contain the bit string
+		 * 0x04 is where KeyCertSign lands in this bit string (from
+		 *      RFC 5280 4.2.1.3)
+		 */
+		if (v[0] != ASN1_BTS || vlen < 4)
+			return -EBADMSG;
+		if (v[1] == 0x02 && v[2] <= 2 && (v[3] & 0x04))
+			ctx->cert->is_kcs_set = true;
+		else if (vlen > 4 && v[1] == 0x03 && (v[3] & 0x04))
+			ctx->cert->is_kcs_set = true;
+		return 0;
+	}
+
 	if (ctx->last_oid == OID_authorityKeyIdentifier) {
 		/* Get hold of the CA key fingerprint */
 		ctx->raw_akid = v;
diff --git a/crypto/asymmetric_keys/x509_parser.h b/crypto/asymmetric_keys/x509_parser.h
index dc45df9f6594..d6ac0985d8a5 100644
--- a/crypto/asymmetric_keys/x509_parser.h
+++ b/crypto/asymmetric_keys/x509_parser.h
@@ -39,6 +39,7 @@ struct x509_certificate {
 	bool		unsupported_sig;	/* T if signature uses unsupported crypto */
 	bool		blacklisted;
 	bool		is_root_ca;		/* T if basic constraints CA is set */
+	bool		is_kcs_set;		/* T if keyCertSign is set */
 };
 
 /*
-- 
2.27.0

[PATCH 4/7] KEYS: Introduce a builtin root of trust key flag

[Eric Snowberg]

Some subsystems are interested in knowing if keys within a keyring could
be used as a foundation of a root of trust.  Introduce a new builtin root
of trust key flag.

The first type of key to use this is X.509.  When a X.509 certificate
is self signed, has the kernCertSign Key Usage set and contains the
CA bit set this new flag is set.

Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
---
 crypto/asymmetric_keys/x509_public_key.c | 2 ++
 include/linux/key-type.h                 | 2 ++
 include/linux/key.h                      | 2 ++
 security/keys/key.c                      | 8 ++++++++
 4 files changed, 14 insertions(+)

diff --git a/crypto/asymmetric_keys/x509_public_key.c b/crypto/asymmetric_keys/x509_public_key.c
index 91a4ad50dea2..7290e765f46b 100644
--- a/crypto/asymmetric_keys/x509_public_key.c
+++ b/crypto/asymmetric_keys/x509_public_key.c
@@ -215,6 +215,8 @@ static int x509_key_preparse(struct key_preparsed_payload *prep)
 	prep->payload.data[asym_auth] = cert->sig;
 	prep->description = desc;
 	prep->quotalen = 100;
+	if (cert->is_kcs_set && cert->self_signed && cert->is_root_ca)
+		prep->payload_flags |= KEY_ALLOC_ROT;
 
 	/* We've finished with the certificate */
 	cert->pub = NULL;
diff --git a/include/linux/key-type.h b/include/linux/key-type.h
index 7d985a1dfe4a..ed0aaad3849b 100644
--- a/include/linux/key-type.h
+++ b/include/linux/key-type.h
@@ -36,6 +36,8 @@ struct key_preparsed_payload {
 	size_t		datalen;	/* Raw datalen */
 	size_t		quotalen;	/* Quota length for proposed payload */
 	time64_t	expiry;		/* Expiry time of key */
+	unsigned int	payload_flags;  /* Proposed payload flags */
+#define KEY_ALLOC_ROT	0x0001		/* Proposed Root of Trust (ROT) key */
 } __randomize_layout;
 
 typedef int (*request_key_actor_t)(struct key *auth_key, void *aux);
diff --git a/include/linux/key.h b/include/linux/key.h
index 7febc4881363..97f6a1f86a27 100644
--- a/include/linux/key.h
+++ b/include/linux/key.h
@@ -230,6 +230,7 @@ struct key {
 #define KEY_FLAG_ROOT_CAN_INVAL	7	/* set if key can be invalidated by root without permission */
 #define KEY_FLAG_KEEP		8	/* set if key should not be removed */
 #define KEY_FLAG_UID_KEYRING	9	/* set if key is a user or user session keyring */
+#define KEY_FLAG_BUILTIN_ROT	10	/* set if key is a builtin Root of Trust key */
 
 	/* the key type and key description string
 	 * - the desc is used to match a key against search criteria
@@ -290,6 +291,7 @@ extern struct key *key_alloc(struct key_type *type,
 #define KEY_ALLOC_BYPASS_RESTRICTION	0x0008	/* Override the check on restricted keyrings */
 #define KEY_ALLOC_UID_KEYRING		0x0010	/* allocating a user or user session keyring */
 #define KEY_ALLOC_SET_KEEP		0x0020	/* Set the KEEP flag on the key/keyring */
+#define KEY_ALLOC_BUILT_IN_ROT		0x0040  /* Add builtin root of trust key */
 
 extern void key_revoke(struct key *key);
 extern void key_invalidate(struct key *key);
diff --git a/security/keys/key.c b/security/keys/key.c
index c45afdd1dfbb..732bb837fc51 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -305,6 +305,8 @@ struct key *key_alloc(struct key_type *type, const char *desc,
 		key->flags |= 1 << KEY_FLAG_UID_KEYRING;
 	if (flags & KEY_ALLOC_SET_KEEP)
 		key->flags |= 1 << KEY_FLAG_KEEP;
+	if (flags & KEY_ALLOC_BUILT_IN_ROT)
+		key->flags |= 1 << KEY_FLAG_BUILTIN_ROT;
 
 #ifdef KEY_DEBUGGING
 	key->magic = KEY_DEBUG_MAGIC;
@@ -929,6 +931,12 @@ key_ref_t key_create_or_update(key_ref_t keyring_ref,
 			perm |= KEY_POS_WRITE;
 	}
 
+	/* Only allow KEY_ALLOC_BUILT_IN_ROT flag to be set by preparser contents */
+	if (prep.payload_flags & KEY_ALLOC_ROT)
+		flags |= KEY_ALLOC_BUILT_IN_ROT;
+	else
+		flags &= ~KEY_ALLOC_BUILT_IN_ROT;
+
 	/* allocate a new key */
 	key = key_alloc(index_key.type, index_key.description,
 			cred->fsuid, cred->fsgid, cred, perm, flags, NULL);
-- 
2.27.0

[PATCH 5/7] KEYS: Introduce sig restriction that validates root of trust

[Eric Snowberg]

The current keyring restrictions validate if a key can be vouched for by
another key already contained in a keyring.  Add a new restriction called
restrict_link_by_rot_and_signature that both vouches for the new key and
validates the vouching key contains the builtin root of trust flag.

Two new system keyring restrictions are added to use
restrict_link_by_rot_and_signature.  The first restriction called
restrict_link_by_rot_builtin_trusted  uses the builtin_trusted_keys as
the restricted keyring.  The second system keyring restriction called
restrict_link_by_rot_builtin_and_secondary_trusted uses the
secondary_trusted_keys as the restricted keyring.  Should the machine
keyring be defined, it shall be validated too, since it is linked to
the secondary_trusted_keys keyring.

Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
---
 certs/system_keyring.c            | 18 +++++++++++++
 crypto/asymmetric_keys/restrict.c | 42 +++++++++++++++++++++++++++++++
 include/keys/system_keyring.h     | 17 ++++++++++++-
 3 files changed, 76 insertions(+), 1 deletion(-)

diff --git a/certs/system_keyring.c b/certs/system_keyring.c
index 05b66ce9d1c9..a8b53446ec25 100644
--- a/certs/system_keyring.c
+++ b/certs/system_keyring.c
@@ -48,6 +48,14 @@ int restrict_link_by_builtin_trusted(struct key *dest_keyring,
 					  builtin_trusted_keys);
 }
 
+int restrict_link_by_rot_builtin_trusted(struct key *dest_keyring,
+					 const struct key_type *type,
+					 const union key_payload *payload,
+					 struct key *unused)
+{
+	return restrict_link_by_rot_and_signature(dest_keyring, type, payload,
+						  builtin_trusted_keys);
+}
 #ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
 /**
  * restrict_link_by_builtin_and_secondary_trusted - Restrict keyring
@@ -76,6 +84,16 @@ int restrict_link_by_builtin_and_secondary_trusted(
 					  secondary_trusted_keys);
 }
 
+int restrict_link_by_rot_builtin_and_secondary_trusted(
+	struct key *dest_keyring,
+	const struct key_type *type,
+	const union key_payload *payload,
+	struct key *unused)
+{
+	return restrict_link_by_rot_and_signature(dest_keyring, type, payload,
+						  secondary_trusted_keys);
+}
+
 /**
  * Allocate a struct key_restriction for the "builtin and secondary trust"
  * keyring. Only for use in system_trusted_keyring_init().
diff --git a/crypto/asymmetric_keys/restrict.c b/crypto/asymmetric_keys/restrict.c
index 6b1ac5f5896a..840ea302b40a 100644
--- a/crypto/asymmetric_keys/restrict.c
+++ b/crypto/asymmetric_keys/restrict.c
@@ -108,6 +108,48 @@ int restrict_link_by_signature(struct key *dest_keyring,
 	return ret;
 }
 
+int restrict_link_by_rot_and_signature(struct key *dest_keyring,
+				       const struct key_type *type,
+				       const union key_payload *payload,
+				       struct key *trust_keyring)
+{
+	const struct public_key_signature *sig;
+	struct key *key;
+	int ret;
+
+	if (!trust_keyring)
+		return -ENOKEY;
+
+	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] && !sig->auth_ids[2])
+		return -ENOKEY;
+
+	if (ca_keyid && !asymmetric_key_id_partial(sig->auth_ids[1], ca_keyid))
+		return -EPERM;
+
+	/* See if we have a key that signed this one. */
+	key = find_asymmetric_key(trust_keyring,
+				  sig->auth_ids[0], sig->auth_ids[1],
+				  sig->auth_ids[2], false);
+	if (IS_ERR(key))
+		return -ENOKEY;
+
+	if (!test_bit(KEY_FLAG_BUILTIN_ROT, &key->flags))
+		ret = -ENOKEY;
+	else if (use_builtin_keys && !test_bit(KEY_FLAG_BUILTIN, &key->flags))
+		ret = -ENOKEY;
+	else
+		ret = verify_signature(key, sig);
+	key_put(key);
+	return ret;
+}
+
+
 static bool match_either_id(const struct asymmetric_key_id **pair,
 			    const struct asymmetric_key_id *single)
 {
diff --git a/include/keys/system_keyring.h b/include/keys/system_keyring.h
index 2419a735420f..2c1241042f1f 100644
--- a/include/keys/system_keyring.h
+++ b/include/keys/system_keyring.h
@@ -17,9 +17,18 @@ extern int restrict_link_by_builtin_trusted(struct key *keyring,
 					    const union key_payload *payload,
 					    struct key *restriction_key);
 extern __init int load_module_cert(struct key *keyring);
-
+extern int restrict_link_by_rot_builtin_trusted(struct key *keyring,
+						const struct key_type *type,
+						const union key_payload *payload,
+						struct key *unused);
+extern int restrict_link_by_rot_and_signature(struct key *dest_keyring,
+					      const struct key_type *type,
+					      const union key_payload *payload,
+					      struct key *unused);
 #else
 #define restrict_link_by_builtin_trusted restrict_link_reject
+#define restrict_link_by_rot_and_signature restrict_link_reject
+#define restrict_link_by_rot_builtin_trusted restrict_link_reject
 
 static inline __init int load_module_cert(struct key *keyring)
 {
@@ -34,8 +43,14 @@ extern int restrict_link_by_builtin_and_secondary_trusted(
 	const struct key_type *type,
 	const union key_payload *payload,
 	struct key *restriction_key);
+extern int restrict_link_by_rot_builtin_and_secondary_trusted(
+	struct key *dest_keyring,
+	const struct key_type *type,
+	const union key_payload *payload,
+	struct key *restrict_key);
 #else
 #define restrict_link_by_builtin_and_secondary_trusted restrict_link_by_builtin_trusted
+#define restrict_link_by_rot_builtin_and_secondary_trusted restrict_link_by_builtin_trusted
 #endif
 
 #ifdef CONFIG_INTEGRITY_MACHINE_KEYRING
-- 
2.27.0

[PATCH 6/7] KEYS: X.509: Flag Intermediate CA certs as built in

[Eric Snowberg]

Currently X.509 Intermediate CA certs do not have the builtin root of trust
key flag set. Allow intermediate CA certs to be added.  Requirements for an
intermediate CA include: Usage extension defined as keyCertSign, Basic
Constrains for CA is false, and Intermediate CA cert is signed by a current
builtin ROT key.

Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
---
 crypto/asymmetric_keys/x509_public_key.c | 14 ++++++++++++--
 include/linux/ima.h                      | 16 ++++++++++++++++
 include/linux/key-type.h                 |  1 +
 security/keys/key.c                      |  5 +++++
 4 files changed, 34 insertions(+), 2 deletions(-)

diff --git a/crypto/asymmetric_keys/x509_public_key.c b/crypto/asymmetric_keys/x509_public_key.c
index 7290e765f46b..9052dd761ea3 100644
--- a/crypto/asymmetric_keys/x509_public_key.c
+++ b/crypto/asymmetric_keys/x509_public_key.c
@@ -215,8 +215,18 @@ static int x509_key_preparse(struct key_preparsed_payload *prep)
 	prep->payload.data[asym_auth] = cert->sig;
 	prep->description = desc;
 	prep->quotalen = 100;
-	if (cert->is_kcs_set && cert->self_signed && cert->is_root_ca)
-		prep->payload_flags |= KEY_ALLOC_ROT;
+	if (cert->is_kcs_set) {
+		if (cert->self_signed && cert->is_root_ca)
+			prep->payload_flags |= KEY_ALLOC_ROT;
+		/*
+		 * In this case it could be an Intermediate CA.  Set
+		 * KEY_MAYBE_ROT for now.  If the restriction check
+		 * passes later, the key will be allocated with the
+		 * correct ROT flag.
+		 */
+		else if (!cert->self_signed && !cert->is_root_ca)
+			prep->payload_flags |= KEY_MAYBE_ROT;
+	}
 
 	/* We've finished with the certificate */
 	cert->pub = NULL;
diff --git a/include/linux/ima.h b/include/linux/ima.h
index 426b1744215e..3f23bccf880a 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -12,6 +12,7 @@
 #include <linux/security.h>
 #include <linux/kexec.h>
 #include <crypto/hash_info.h>
+#include <keys/system_keyring.h>
 struct linux_binprm;
 
 #ifdef CONFIG_IMA
@@ -176,6 +177,21 @@ static inline void ima_post_key_create_or_update(struct key *keyring,
 						 bool create) {}
 #endif  /* CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS */
 
+#ifdef CONFIG_ASYMMETRIC_KEY_TYPE
+#ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
+#define ima_validate_builtin_rot restrict_link_by_rot_builtin_and_secondary_trusted
+#else
+#define ima_validate_builtin_rot restrict_link_by_rot_builtin_trusted
+#endif
+#else
+static inline int ima_validate_builtin_rot(struct key *dest_keyring,
+					   const struct key_type *type,
+					   const union key_payload *payload,
+					   struct key *unused){
+	return -EPERM;
+}
+#endif
+
 #ifdef CONFIG_IMA_APPRAISE
 extern bool is_ima_appraise_enabled(void);
 extern void ima_inode_post_setattr(struct user_namespace *mnt_userns,
diff --git a/include/linux/key-type.h b/include/linux/key-type.h
index ed0aaad3849b..da09e68903e2 100644
--- a/include/linux/key-type.h
+++ b/include/linux/key-type.h
@@ -38,6 +38,7 @@ struct key_preparsed_payload {
 	time64_t	expiry;		/* Expiry time of key */
 	unsigned int	payload_flags;  /* Proposed payload flags */
 #define KEY_ALLOC_ROT	0x0001		/* Proposed Root of Trust (ROT) key */
+#define KEY_MAYBE_ROT	0x0002		/* Proposed possible Root of Trust key */
 } __randomize_layout;
 
 typedef int (*request_key_actor_t)(struct key *auth_key, void *aux);
diff --git a/security/keys/key.c b/security/keys/key.c
index 732bb837fc51..c553040dcc02 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -900,6 +900,11 @@ key_ref_t key_create_or_update(key_ref_t keyring_ref,
 		}
 	}
 
+	/* Previous restriction check passed therefore try to validate root of trust */
+	if ((prep.payload_flags & KEY_MAYBE_ROT) &&
+	   !(ima_validate_builtin_rot(keyring, index_key.type, &prep.payload, NULL)))
+		prep.payload_flags |= KEY_ALLOC_ROT;
+
 	/* if we're going to allocate a new key, we're going to have
 	 * to modify the keyring */
 	ret = key_permission(keyring_ref, KEY_NEED_WRITE);
-- 
2.27.0

[PATCH 7/7] integrity: Use root of trust signature restriction

[Eric Snowberg]

Keys added to the IMA keyring must be vouched for by keys contained
within the builtin or secondary keyrings.  These keys must also be self
signed, have the CA bit set and have the kernCertSign KeyUsage bit set.
Or they could be validated by a properly formed intermediate CA.
Currently these restrictions are not enforced. Use the new
restrict_link_by_rot_builtin_and_secondary_trusted and
restrict_link_by_rot_builtin_trusted to enforce the missing
CA restrictions when adding keys to the IMA keyring. With the
CA restrictions enforced, allow the machine keyring to be
enabled with IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY.

Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
---
 security/integrity/Kconfig  | 1 -
 security/integrity/digsig.c | 4 ++--
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/security/integrity/Kconfig b/security/integrity/Kconfig
index 599429f99f99..14cc3c767270 100644
--- a/security/integrity/Kconfig
+++ b/security/integrity/Kconfig
@@ -68,7 +68,6 @@ config INTEGRITY_MACHINE_KEYRING
 	depends on INTEGRITY_ASYMMETRIC_KEYS
 	depends on SYSTEM_BLACKLIST_KEYRING
 	depends on LOAD_UEFI_KEYS
-	depends on !IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY
 	help
 	 If set, provide a keyring to which Machine Owner Keys (MOK) may
 	 be added. This keyring shall contain just MOK keys.  Unlike keys
diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
index c8c8a4a4e7a0..cfde2ea9c55b 100644
--- a/security/integrity/digsig.c
+++ b/security/integrity/digsig.c
@@ -34,9 +34,9 @@ static const char * const keyring_name[INTEGRITY_KEYRING_MAX] = {
 };
 
 #ifdef CONFIG_IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY
-#define restrict_link_to_ima restrict_link_by_builtin_and_secondary_trusted
+#define restrict_link_to_ima restrict_link_by_rot_builtin_and_secondary_trusted
 #else
-#define restrict_link_to_ima restrict_link_by_builtin_trusted
+#define restrict_link_to_ima restrict_link_by_rot_builtin_trusted
 #endif
 
 static struct key *integrity_keyring_from_id(const unsigned int id)
-- 
2.27.0

Re: [PATCH 5/7] KEYS: Introduce sig restriction that validates root of trust

[kernel test robot]

Hi Eric,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on 3123109284176b1532874591f7c81f3837bbdc17]

url:    https://github.com/intel-lab-lkp/linux/commits/Eric-Snowberg/Add-CA-enforcement-keyring-restrictions/20220407-003209
base:   3123109284176b1532874591f7c81f3837bbdc17
config: riscv-randconfig-r042-20220406 (https://download.01.org/0day-ci/archive/20220407/202204070321.X7bLj3Ce-lkp@intel.com/config)
compiler: riscv64-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/68d98a175d29032d888f3f5700c43cf771ef17d8
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Eric-Snowberg/Add-CA-enforcement-keyring-restrictions/20220407-003209
        git checkout 68d98a175d29032d888f3f5700c43cf771ef17d8
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=riscv SHELL=/bin/bash crypto/asymmetric_keys/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> crypto/asymmetric_keys/restrict.c:111:5: warning: no previous prototype for 'restrict_link_by_rot_and_signature' [-Wmissing-prototypes]
     111 | int restrict_link_by_rot_and_signature(struct key *dest_keyring,
         |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


vim +/restrict_link_by_rot_and_signature +111 crypto/asymmetric_keys/restrict.c

   110	
 > 111	int restrict_link_by_rot_and_signature(struct key *dest_keyring,
   112					       const struct key_type *type,
   113					       const union key_payload *payload,
   114					       struct key *trust_keyring)
   115	{
   116		const struct public_key_signature *sig;
   117		struct key *key;
   118		int ret;
   119	
   120		if (!trust_keyring)
   121			return -ENOKEY;
   122	
   123		if (type != &key_type_asymmetric)
   124			return -EOPNOTSUPP;
   125	
   126		sig = payload->data[asym_auth];
   127		if (!sig)
   128			return -ENOPKG;
   129		if (!sig->auth_ids[0] && !sig->auth_ids[1] && !sig->auth_ids[2])
   130			return -ENOKEY;
   131	
   132		if (ca_keyid && !asymmetric_key_id_partial(sig->auth_ids[1], ca_keyid))
   133			return -EPERM;
   134	
   135		/* See if we have a key that signed this one. */
   136		key = find_asymmetric_key(trust_keyring,
   137					  sig->auth_ids[0], sig->auth_ids[1],
   138					  sig->auth_ids[2], false);
   139		if (IS_ERR(key))
   140			return -ENOKEY;
   141	
   142		if (!test_bit(KEY_FLAG_BUILTIN_ROT, &key->flags))
   143			ret = -ENOKEY;
   144		else if (use_builtin_keys && !test_bit(KEY_FLAG_BUILTIN, &key->flags))
   145			ret = -ENOKEY;
   146		else
   147			ret = verify_signature(key, sig);
   148		key_put(key);
   149		return ret;
   150	}
   151	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH 0/7] Add CA enforcement keyring restrictions

[Mimi Zohar]

Hi Eric,

On Tue, 2022-04-05 at 21:53 -0400, Eric Snowberg wrote:
> A key added to the ima keyring must be signed by a key contained within 
> either the builtin trusted or secondary trusted keyrings. Currently, there are 
> CA restrictions described in IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY,
> but these restrictions are not enforced within code. Therefore, keys within 
> either the builtin or secondary may not be a CA and could be used to
> vouch for an ima key.
> 
> The machine keyring can not be used as another trust anchor for adding keys 
> to the ima keyring, since CA enforcement does not currently exist [1]. This 
> would expand the current integrity gap.
> 
> Introduce a new root of trust key flag to close this integrity gap for
> all keyrings.  The first key type to use this is X.509.  When a X.509 
> certificate is self signed, contains kernCertSign Key Usage and contains 
> the CA bit, the new flag is set.  Introduce new keyring restrictions 
> that not only validates a key is signed by a key contained within the 
> keyring, but also validates the key has the new root of trust key flag 
> set.  Use this new restriction for keys added to the ima keyring.  Now 
> that we have CA enforcement, allow the machine keyring to be used as another 
> trust anchor for the ima keyring.
> 
> To recap, all keys that previously loaded into the builtin, secondary or
> machine keyring will still load after applying this series.  Keys
> contained within these keyrings may carry the root of trust flag. The
> ima keyring will use the new root of trust restriction to validate
> CA enforcement. Other keyrings that require a root of trust could also 
> use this in the future.

Your initial patch set indicated that you were addressing Linus'
request to allow end-users the ability "to add their own keys and sign
modules they trust".  However, from the design of the previous patch
set and now this one, everything indicates a lot more is going on than
just allowing end-users to add their own keys.  There would be no
reason for loading all the MOK keys, rather than just the CA keys, onto
the "machine" keyring.  Please provide the motivation for this design.

Please note that Patch 6/7 permits intermediary CA keys, without any
mention of it in the cover letter.  Please include this in the
motivation for this design.

thanks,

Mimi

Re: [PATCH 0/7] Add CA enforcement keyring restrictions

[Eric Snowberg]

> On Apr 6, 2022, at 2:45 PM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> 
> Hi Eric,
> 
> On Tue, 2022-04-05 at 21:53 -0400, Eric Snowberg wrote:
>> A key added to the ima keyring must be signed by a key contained within 
>> either the builtin trusted or secondary trusted keyrings. Currently, there are 
>> CA restrictions described in IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY,
>> but these restrictions are not enforced within code. Therefore, keys within 
>> either the builtin or secondary may not be a CA and could be used to
>> vouch for an ima key.
>> 
>> The machine keyring can not be used as another trust anchor for adding keys 
>> to the ima keyring, since CA enforcement does not currently exist [1]. This 
>> would expand the current integrity gap.
>> 
>> Introduce a new root of trust key flag to close this integrity gap for
>> all keyrings.  The first key type to use this is X.509.  When a X.509 
>> certificate is self signed, contains kernCertSign Key Usage and contains 
>> the CA bit, the new flag is set.  Introduce new keyring restrictions 
>> that not only validates a key is signed by a key contained within the 
>> keyring, but also validates the key has the new root of trust key flag 
>> set.  Use this new restriction for keys added to the ima keyring.  Now 
>> that we have CA enforcement, allow the machine keyring to be used as another 
>> trust anchor for the ima keyring.
>> 
>> To recap, all keys that previously loaded into the builtin, secondary or
>> machine keyring will still load after applying this series.  Keys
>> contained within these keyrings may carry the root of trust flag. The
>> ima keyring will use the new root of trust restriction to validate
>> CA enforcement. Other keyrings that require a root of trust could also 
>> use this in the future.
> 
> Your initial patch set indicated that you were addressing Linus'
> request to allow end-users the ability "to add their own keys and sign
> modules they trust".  However, from the design of the previous patch
> set and now this one, everything indicates a lot more is going on than
> just allowing end-users to add their own keys.  There would be no
> reason for loading all the MOK keys, rather than just the CA keys, onto
> the "machine" keyring.  Please provide the motivation for this design.

The motivation is to satisfy both Linus and your requests. Linus requested 
the ability to allow users to add their own keys and sign modules they trust.  
A code signing CA certificate does not require kernCertSign in the usage. Adding 
this as a requirement for kernel modules would be a regression (or a bug).

This series addresses your request to only trust validly signed CA certs. 
As you pointed out in the Kconfig help for 
IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY:

help
  Keys may be added to the IMA or IMA blacklist keyrings, if the
  key is validly signed by a CA cert in the system built-in or
  secondary trusted keyrings.

  Intermediate keys between those the kernel has compiled in and the 
  IMA keys to be added may be added to the system secondary keyring,
  provided they are validly signed by a key already resident in the
  built-in or secondary trusted keyrings.

requires keys to be “validly” signed by a CA cert. Later the definition of a 
validly signed CA cert was defined as: self signed, contains kernCertSign 
key usage and contains the CA bit. While this help file states the CA restriction, 
nothing in code enforces it.  One can place any type of self signed cert in either 
keyring and ima will use it.  The motivation is for all keys added to the ima 
keyring to abide by the restriction defined in the Kconfig help.  With this series 
this can be accomplished without introducing a regression on keys placed in 
any of the system keyrings.

> Please note that Patch 6/7 permits intermediary CA keys, without any
> mention of it in the cover letter.  Please include this in the
> motivation for this design.

Ok, I’ll add that in the next round.

Re: [PATCH 6/7] KEYS: X.509: Flag Intermediate CA certs as built in

[kernel test robot]

Hi Eric,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on 3123109284176b1532874591f7c81f3837bbdc17]

url:    https://github.com/intel-lab-lkp/linux/commits/Eric-Snowberg/Add-CA-enforcement-keyring-restrictions/20220407-003209
base:   3123109284176b1532874591f7c81f3837bbdc17
config: um-i386_defconfig (https://download.01.org/0day-ci/archive/20220407/202204070929.nFQNU3B8-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.2.0-19) 11.2.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/b0858df3dd6d627f8fa75cc973f55516372a5c98
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Eric-Snowberg/Add-CA-enforcement-keyring-restrictions/20220407-003209
        git checkout b0858df3dd6d627f8fa75cc973f55516372a5c98
        # save the config file to linux build tree
        mkdir build_dir
        make W=1 O=build_dir ARCH=um SUBARCH=i386 SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from fs/file_table.c:28:
>> include/linux/ima.h:189:56: warning: 'union key_payload' declared inside parameter list will not be visible outside of this definition or declaration
     189 |                                            const union key_payload *payload,
         |                                                        ^~~~~~~~~~~
>> include/linux/ima.h:188:57: warning: 'struct key_type' declared inside parameter list will not be visible outside of this definition or declaration
     188 |                                            const struct key_type *type,
         |                                                         ^~~~~~~~


vim +189 include/linux/ima.h

   179	
   180	#ifdef CONFIG_ASYMMETRIC_KEY_TYPE
   181	#ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
   182	#define ima_validate_builtin_rot restrict_link_by_rot_builtin_and_secondary_trusted
   183	#else
   184	#define ima_validate_builtin_rot restrict_link_by_rot_builtin_trusted
   185	#endif
   186	#else
   187	static inline int ima_validate_builtin_rot(struct key *dest_keyring,
 > 188						   const struct key_type *type,
 > 189						   const union key_payload *payload,
   190						   struct key *unused){
   191		return -EPERM;
   192	}
   193	#endif
   194	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH 2/7] KEYS: X.509: Parse Basic Constraints for CA

[Mimi Zohar]

On Tue, 2022-04-05 at 21:53 -0400, Eric Snowberg wrote:
> Parse the X.509 Basic Constraints.  The basic constraints extension
> identifies whether the subject of the certificate is a CA.
> 
> BasicConstraints ::= SEQUENCE {
>         cA                      BOOLEAN DEFAULT FALSE,
>         pathLenConstraint       INTEGER (0..MAX) OPTIONAL }
> 
> If the CA is true, store it in the x509_certificate.  This will be used
> in a follow on patch that requires knowing if the public key is a CA.
> 
> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
> ---
>  crypto/asymmetric_keys/x509_cert_parser.c | 9 +++++++++
>  crypto/asymmetric_keys/x509_parser.h      | 1 +
>  2 files changed, 10 insertions(+)
> 
> diff --git a/crypto/asymmetric_keys/x509_cert_parser.c b/crypto/asymmetric_keys/x509_cert_parser.c
> index 2899ed80bb18..30f7374ea9c0 100644
> --- a/crypto/asymmetric_keys/x509_cert_parser.c
> +++ b/crypto/asymmetric_keys/x509_cert_parser.c
> @@ -583,6 +583,15 @@ int x509_process_extension(void *context, size_t hdrlen,
>  		return 0;
>  	}
>  
> +	if (ctx->last_oid == OID_basicConstraints) {
> +		if (vlen < 2 || v[0] != (ASN1_CONS_BIT | ASN1_SEQ))
> +			return -EBADMSG;
> +		if (v[1] != vlen - 2)
> +			return -EBADMSG;
> +		if (vlen >= 4 && v[1] != 0 && v[2] == ASN1_BOOL && v[3] == 1)
> +			ctx->cert->is_root_ca = true;
> +	}
> +
>  	return 0;
>  }
>  
> diff --git a/crypto/asymmetric_keys/x509_parser.h b/crypto/asymmetric_keys/x509_parser.h
> index 97a886cbe01c..dc45df9f6594 100644
> --- a/crypto/asymmetric_keys/x509_parser.h
> +++ b/crypto/asymmetric_keys/x509_parser.h
> @@ -38,6 +38,7 @@ struct x509_certificate {
>  	bool		self_signed;		/* T if self-signed (check unsupported_sig too) */
>  	bool		unsupported_sig;	/* T if signature uses unsupported crypto */
>  	bool		blacklisted;
> +	bool		is_root_ca;		/* T if basic constraints CA is set */

There's no need to prefix variables with "is_".  Similar to the
variable "self_signed" simply name this variable "root_ca".

thanks,

Mimi


>  };
>  
>  /*

Re: [PATCH 3/7] KEYS: X.509: Parse Key Usage

[Mimi Zohar]

On Tue, 2022-04-05 at 21:53 -0400, Eric Snowberg wrote:
> Parse the X.509 Key Usage.  The key usage extension defines the purpose of
> the key contained in the certificate.
> 
>    id-ce-keyUsage OBJECT IDENTIFIER ::=  { id-ce 15 }
> 
>       KeyUsage ::= BIT STRING {
>            digitalSignature        (0),
>            contentCommitment       (1),
>            keyEncipherment         (2),
>            dataEncipherment        (3),
>            keyAgreement            (4),
>            keyCertSign             (5),
>            cRLSign                 (6),
>            encipherOnly            (7),
>            decipherOnly            (8) }
> 
> If the keyCertSign is set, store it in the x509_certificate structure.
> This will be used in a follow on patch that requires knowing the
> certificate key usage type.
> 
> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
> ---
>  crypto/asymmetric_keys/x509_cert_parser.c | 20 ++++++++++++++++++++
>  crypto/asymmetric_keys/x509_parser.h      |  1 +
>  2 files changed, 21 insertions(+)
> 
> diff --git a/crypto/asymmetric_keys/x509_cert_parser.c b/crypto/asymmetric_keys/x509_cert_parser.c
> index 30f7374ea9c0..a89f1e0c8a0f 100644
> --- a/crypto/asymmetric_keys/x509_cert_parser.c
> +++ b/crypto/asymmetric_keys/x509_cert_parser.c
> @@ -576,6 +576,26 @@ int x509_process_extension(void *context, size_t hdrlen,
>  		return 0;
>  	}
>  
> +	if (ctx->last_oid == OID_keyUsage) {
> +		/*
> +		 * Get hold of the keyUsage bit string to validate keyCertSign
> +		 * v[1] is the encoding size
> +		 *       (Expect either 0x02 or 0x03, making it 1 or 2 bytes)
> +		 * v[2] is the number of unused bits in the bit string
> +		 *       (If >= 3 keyCertSign is missing)
> +		 * v[3] and possibly v[4] contain the bit string
> +		 * 0x04 is where KeyCertSign lands in this bit string (from
> +		 *      RFC 5280 4.2.1.3)
> +		 */
> +		if (v[0] != ASN1_BTS || vlen < 4)
> +			return -EBADMSG;
> +		if (v[1] == 0x02 && v[2] <= 2 && (v[3] & 0x04))
> +			ctx->cert->is_kcs_set = true;
> +		else if (vlen > 4 && v[1] == 0x03 && (v[3] & 0x04))
> +			ctx->cert->is_kcs_set = true;
> +		return 0;
> +	}
> +
>  	if (ctx->last_oid == OID_authorityKeyIdentifier) {
>  		/* Get hold of the CA key fingerprint */
>  		ctx->raw_akid = v;
> diff --git a/crypto/asymmetric_keys/x509_parser.h b/crypto/asymmetric_keys/x509_parser.h
> index dc45df9f6594..d6ac0985d8a5 100644
> --- a/crypto/asymmetric_keys/x509_parser.h
> +++ b/crypto/asymmetric_keys/x509_parser.h
> @@ -39,6 +39,7 @@ struct x509_certificate {
>  	bool		unsupported_sig;	/* T if signature uses unsupported crypto */
>  	bool		blacklisted;
>  	bool		is_root_ca;		/* T if basic constraints CA is set */
> +	bool		is_kcs_set;		/* T if keyCertSign is set */
>  };

Again there is no need to prefix the variable with "is_" or suffix it
with "set".  Simply naming the variable "cert_signing" or
"keycertsign", like "self_signed", will improve code readability.

thanks,

Mimi

Re: [PATCH 4/7] KEYS: Introduce a builtin root of trust key flag

[Mimi Zohar]

On Tue, 2022-04-05 at 21:53 -0400, Eric Snowberg wrote:
> Some subsystems are interested in knowing if keys within a keyring could
> be used as a foundation of a root of trust.  Introduce a new builtin root
> of trust key flag.

Unfortunately a root of trust is not something that can simply be built
into a certificate.  Roots of trust are normally established based on
HW.  The root of trust for the "builtin_trusted_keys" is established
for systems with secure boot enabled by verifying the signature chain
of trust up to and including the kernel image's signature.  Similarly,
the root of trust for keys on the "secondary_trusted_keys" is based on
all certificates being signed by a key on the "builtin_trusted_keys"
keyring or other keys on the "secondary_trusted_keys" keyring.

Defining a new variable claiming that a root-ca with cert signing usage
on any keyring is a root of trust is just wrong.

> 
> The first type of key to use this is X.509.  When a X.509 certificate
> is self signed, has the kernCertSign Key Usage set and contains the
> CA bit set this new flag is set.
> 
> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>

> 
> diff --git a/include/linux/key.h b/include/linux/key.h
> index 7febc4881363..97f6a1f86a27 100644
> --- a/include/linux/key.h
> +++ b/include/linux/key.h
> @@ -230,6 +230,7 @@ struct key {
>  #define KEY_FLAG_ROOT_CAN_INVAL	7	/* set if key can be invalidated by root without permission */
>  #define KEY_FLAG_KEEP		8	/* set if key should not be removed */
>  #define KEY_FLAG_UID_KEYRING	9	/* set if key is a user or user session keyring */
> +#define KEY_FLAG_BUILTIN_ROT	10	/* set if key is a builtin Root of Trust key */
>  
>  	/* the key type and key description string
>  	 * - the desc is used to match a key against search criteria
> @@ -290,6 +291,7 @@ extern struct key *key_alloc(struct key_type *type,
>  #define KEY_ALLOC_BYPASS_RESTRICTION	0x0008	/* Override the check on restricted keyrings */
>  #define KEY_ALLOC_UID_KEYRING		0x0010	/* allocating a user or user session keyring */
>  #define KEY_ALLOC_SET_KEEP		0x0020	/* Set the KEEP flag on the key/keyring */
> +#define KEY_ALLOC_BUILT_IN_ROT		0x0040  /* Add builtin root of trust key */

Since the concept of root of trust is not generic, but limited to
specific keyrings, the root CA certificate signing keys on the
"machine" keyring need to be identified.  Similar to the
KEY_ALLOC_BUILT_IN/KEY_FLAG_BUILTIN, new flags
KEY_ALLOC_MACHINE/KEY_FLAG_MACHINE should be defined instead.

thanks,

Mimi

Re: [PATCH 0/7] Add CA enforcement keyring restrictions

[Mimi Zohar]

On Wed, 2022-04-06 at 22:53 +0000, Eric Snowberg wrote:
> 
> > On Apr 6, 2022, at 2:45 PM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> > 
> > Hi Eric,
> > 
> > On Tue, 2022-04-05 at 21:53 -0400, Eric Snowberg wrote:
> >> A key added to the ima keyring must be signed by a key contained within 
> >> either the builtin trusted or secondary trusted keyrings. Currently, there are 
> >> CA restrictions described in IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY,
> >> but these restrictions are not enforced within code. Therefore, keys within 
> >> either the builtin or secondary may not be a CA and could be used to
> >> vouch for an ima key.
> >> 
> >> The machine keyring can not be used as another trust anchor for adding keys 
> >> to the ima keyring, since CA enforcement does not currently exist [1]. This 
> >> would expand the current integrity gap.
> >> 
> >> Introduce a new root of trust key flag to close this integrity gap for
> >> all keyrings.  The first key type to use this is X.509.  When a X.509 
> >> certificate is self signed, contains kernCertSign Key Usage and contains 
> >> the CA bit, the new flag is set.  Introduce new keyring restrictions 
> >> that not only validates a key is signed by a key contained within the 
> >> keyring, but also validates the key has the new root of trust key flag 
> >> set.  Use this new restriction for keys added to the ima keyring.  Now 
> >> that we have CA enforcement, allow the machine keyring to be used as another 
> >> trust anchor for the ima keyring.
> >> 
> >> To recap, all keys that previously loaded into the builtin, secondary or
> >> machine keyring will still load after applying this series.  Keys
> >> contained within these keyrings may carry the root of trust flag. The
> >> ima keyring will use the new root of trust restriction to validate
> >> CA enforcement. Other keyrings that require a root of trust could also 
> >> use this in the future.
> > 
> > Your initial patch set indicated that you were addressing Linus'
> > request to allow end-users the ability "to add their own keys and sign
> > modules they trust".  However, from the design of the previous patch
> > set and now this one, everything indicates a lot more is going on than
> > just allowing end-users to add their own keys.  There would be no
> > reason for loading all the MOK keys, rather than just the CA keys, onto
> > the "machine" keyring.  Please provide the motivation for this design.
> 
> The motivation is to satisfy both Linus and your requests. Linus requested 
> the ability to allow users to add their own keys and sign modules they trust.  
> A code signing CA certificate does not require kernCertSign in the usage. Adding 
> this as a requirement for kernel modules would be a regression (or a bug).

Of course a code signing CA certificate should not also be a
certificate signing key (keyCertSign).  Remember the
"builtin_trusted_keys" and "secondary_trusted_keys" keyrings are
special.  Their root of trust is based on a secure boot signature chain
of trust up to and including a signed kernel image.  The "machine"
keyring is totally different in this regard.  Establishing a new root
of trust is really difficult.  Requiring a root-CA to have key
certifcate signing usage is a level of indirection, which I would
consider a small price to pay for being able to establish a, hopefully
safe or at least safer, new root of trust for trusting "end-user" keys.

> 
> This series addresses your request to only trust validly signed CA certs. 
> As you pointed out in the Kconfig help for 
> IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY:
> 
> help
>   Keys may be added to the IMA or IMA blacklist keyrings, if the
>   key is validly signed by a CA cert in the system built-in or
>   secondary trusted keyrings.
> 
>   Intermediate keys between those the kernel has compiled in and the 
>   IMA keys to be added may be added to the system secondary keyring,
>   provided they are validly signed by a key already resident in the
>   built-in or secondary trusted keyrings.
> 
> requires keys to be “validly” signed by a CA cert. Later the definition of a 
> validly signed CA cert was defined as: self signed, contains kernCertSign 
> key usage and contains the CA bit. While this help file states the CA restriction, 
> nothing in code enforces it.  One can place any type of self signed cert in either 
> keyring and ima will use it.  The motivation is for all keys added to the ima 
> keyring to abide by the restriction defined in the Kconfig help.  With this series 
> this can be accomplished without introducing a regression on keys placed in 
> any of the system keyrings.
> 
> > Please note that Patch 6/7 permits intermediary CA keys, without any
> > mention of it in the cover letter.  Please include this in the
> > motivation for this design.
> 
> Ok, I’ll add that in the next round.

Your cover letter should say that this patch series enables
verification of 3rd party modules.

thanks,

Mimi

Re: [PATCH 4/7] KEYS: Introduce a builtin root of trust key flag

[Eric Snowberg]

> On Apr 8, 2022, at 8:40 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> 
> On Tue, 2022-04-05 at 21:53 -0400, Eric Snowberg wrote:
>> 
>> The first type of key to use this is X.509.  When a X.509 certificate
>> is self signed, has the kernCertSign Key Usage set and contains the
>> CA bit set this new flag is set.
>> 
>> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
>> 
>> diff --git a/include/linux/key.h b/include/linux/key.h
>> index 7febc4881363..97f6a1f86a27 100644
>> --- a/include/linux/key.h
>> +++ b/include/linux/key.h
>> @@ -230,6 +230,7 @@ struct key {
>> #define KEY_FLAG_ROOT_CAN_INVAL	7	/* set if key can be invalidated by root without permission */
>> #define KEY_FLAG_KEEP		8	/* set if key should not be removed */
>> #define KEY_FLAG_UID_KEYRING	9	/* set if key is a user or user session keyring */
>> +#define KEY_FLAG_BUILTIN_ROT	10	/* set if key is a builtin Root of Trust key */
>> 
>> 	/* the key type and key description string
>> 	 * - the desc is used to match a key against search criteria
>> @@ -290,6 +291,7 @@ extern struct key *key_alloc(struct key_type *type,
>> #define KEY_ALLOC_BYPASS_RESTRICTION	0x0008	/* Override the check on restricted keyrings */
>> #define KEY_ALLOC_UID_KEYRING		0x0010	/* allocating a user or user session keyring */
>> #define KEY_ALLOC_SET_KEEP		0x0020	/* Set the KEEP flag on the key/keyring */
>> +#define KEY_ALLOC_BUILT_IN_ROT		0x0040  /* Add builtin root of trust key */
> 
> Since the concept of root of trust is not generic, but limited to
> specific keyrings, the root CA certificate signing keys on the
> "machine" keyring need to be identified.  Similar to the
> KEY_ALLOC_BUILT_IN/KEY_FLAG_BUILTIN, new flags
> KEY_ALLOC_MACHINE/KEY_FLAG_MACHINE should be defined instead.

I’m open to renaming these, however this name change seems confusing to me.  
This flag gets set when the X.509 certificate contains the three CA requirements 
identified above.  The remaining keys in the machine keyring can be used for 
anything else.  Plus this flag can be set for keys loaded into the secondary trusted 
keyring (6th patch in the series).  When an intermediate CA gets loaded into the 
secondary, the flag is set as well.

Re: [PATCH 2/7] KEYS: X.509: Parse Basic Constraints for CA

[Eric Snowberg]

> On Apr 8, 2022, at 8:39 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> 
> On Tue, 2022-04-05 at 21:53 -0400, Eric Snowberg wrote:
>> Parse the X.509 Basic Constraints.  The basic constraints extension
>> identifies whether the subject of the certificate is a CA.
>> 
>> BasicConstraints ::= SEQUENCE {
>>        cA                      BOOLEAN DEFAULT FALSE,
>>        pathLenConstraint       INTEGER (0..MAX) OPTIONAL }
>> 
>> If the CA is true, store it in the x509_certificate.  This will be used
>> in a follow on patch that requires knowing if the public key is a CA.
>> 
>> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
>> ---
>> crypto/asymmetric_keys/x509_cert_parser.c | 9 +++++++++
>> crypto/asymmetric_keys/x509_parser.h      | 1 +
>> 2 files changed, 10 insertions(+)
>> 
>> diff --git a/crypto/asymmetric_keys/x509_cert_parser.c b/crypto/asymmetric_keys/x509_cert_parser.c
>> index 2899ed80bb18..30f7374ea9c0 100644
>> --- a/crypto/asymmetric_keys/x509_cert_parser.c
>> +++ b/crypto/asymmetric_keys/x509_cert_parser.c
>> @@ -583,6 +583,15 @@ int x509_process_extension(void *context, size_t hdrlen,
>> 		return 0;
>> 	}
>> 
>> +	if (ctx->last_oid == OID_basicConstraints) {
>> +		if (vlen < 2 || v[0] != (ASN1_CONS_BIT | ASN1_SEQ))
>> +			return -EBADMSG;
>> +		if (v[1] != vlen - 2)
>> +			return -EBADMSG;
>> +		if (vlen >= 4 && v[1] != 0 && v[2] == ASN1_BOOL && v[3] == 1)
>> +			ctx->cert->is_root_ca = true;
>> +	}
>> +
>> 	return 0;
>> }
>> 
>> diff --git a/crypto/asymmetric_keys/x509_parser.h b/crypto/asymmetric_keys/x509_parser.h
>> index 97a886cbe01c..dc45df9f6594 100644
>> --- a/crypto/asymmetric_keys/x509_parser.h
>> +++ b/crypto/asymmetric_keys/x509_parser.h
>> @@ -38,6 +38,7 @@ struct x509_certificate {
>> 	bool		self_signed;		/* T if self-signed (check unsupported_sig too) */
>> 	bool		unsupported_sig;	/* T if signature uses unsupported crypto */
>> 	bool		blacklisted;
>> +	bool		is_root_ca;		/* T if basic constraints CA is set */
> 
> There's no need to prefix variables with "is_".  Similar to the
> variable "self_signed" simply name this variable "root_ca".

I’ll change this name (and also the one you identified in the 3rd patch) in the next
round, thanks.

Re: [PATCH 4/7] KEYS: Introduce a builtin root of trust key flag

[Mimi Zohar]

On Fri, 2022-04-08 at 15:27 +0000, Eric Snowberg wrote:
> 
> > On Apr 8, 2022, at 8:40 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> > 
> > On Tue, 2022-04-05 at 21:53 -0400, Eric Snowberg wrote:
> >> 
> >> The first type of key to use this is X.509.  When a X.509 certificate
> >> is self signed, has the kernCertSign Key Usage set and contains the
> >> CA bit set this new flag is set.
> >> 
> >> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
> >> 
> >> diff --git a/include/linux/key.h b/include/linux/key.h
> >> index 7febc4881363..97f6a1f86a27 100644
> >> --- a/include/linux/key.h
> >> +++ b/include/linux/key.h
> >> @@ -230,6 +230,7 @@ struct key {
> >> #define KEY_FLAG_ROOT_CAN_INVAL	7	/* set if key can be invalidated by root without permission */
> >> #define KEY_FLAG_KEEP		8	/* set if key should not be removed */
> >> #define KEY_FLAG_UID_KEYRING	9	/* set if key is a user or user session keyring */
> >> +#define KEY_FLAG_BUILTIN_ROT	10	/* set if key is a builtin Root of Trust key */
> >> 
> >> 	/* the key type and key description string
> >> 	 * - the desc is used to match a key against search criteria
> >> @@ -290,6 +291,7 @@ extern struct key *key_alloc(struct key_type *type,
> >> #define KEY_ALLOC_BYPASS_RESTRICTION	0x0008	/* Override the check on restricted keyrings */
> >> #define KEY_ALLOC_UID_KEYRING		0x0010	/* allocating a user or user session keyring */
> >> #define KEY_ALLOC_SET_KEEP		0x0020	/* Set the KEEP flag on the key/keyring */
> >> +#define KEY_ALLOC_BUILT_IN_ROT		0x0040  /* Add builtin root of trust key */
> > 
> > Since the concept of root of trust is not generic, but limited to
> > specific keyrings, the root CA certificate signing keys on the
> > "machine" keyring need to be identified.  Similar to the
> > KEY_ALLOC_BUILT_IN/KEY_FLAG_BUILTIN, new flags
> > KEY_ALLOC_MACHINE/KEY_FLAG_MACHINE should be defined instead.
> 
> I’m open to renaming these, however this name change seems confusing to me.  
> This flag gets set when the X.509 certificate contains the three CA requirements 
> identified above.  The remaining keys in the machine keyring can be used for 
> anything else.

Renaming the flag to KEY_ALLOC_MACHINE/KEY_FLAG_MACHINE differentiates
between the "builtin" keys from the "machine" keys.  The trust models
are very different.

> Plus this flag can be set for keys loaded into the secondary trusted 
> keyring (6th patch in the series).  When an intermediate CA gets loaded into the 
> secondary, the flag is set as well.

Please include a full explanation with the motivation in the patch
description as to why support for intermediary CAs is required for the
"end-user" use case.

thanks,

Mimi

Re: [PATCH 4/7] KEYS: Introduce a builtin root of trust key flag

[Eric Snowberg]

> On Apr 8, 2022, at 10:55 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> 
> On Fri, 2022-04-08 at 15:27 +0000, Eric Snowberg wrote:
>> 
>>> On Apr 8, 2022, at 8:40 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
>>> 
>>> On Tue, 2022-04-05 at 21:53 -0400, Eric Snowberg wrote:
>>>> 
>>>> The first type of key to use this is X.509.  When a X.509 certificate
>>>> is self signed, has the kernCertSign Key Usage set and contains the
>>>> CA bit set this new flag is set.
>>>> 
>>>> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
>>>> 
>>>> diff --git a/include/linux/key.h b/include/linux/key.h
>>>> index 7febc4881363..97f6a1f86a27 100644
>>>> --- a/include/linux/key.h
>>>> +++ b/include/linux/key.h
>>>> @@ -230,6 +230,7 @@ struct key {
>>>> #define KEY_FLAG_ROOT_CAN_INVAL	7	/* set if key can be invalidated by root without permission */
>>>> #define KEY_FLAG_KEEP		8	/* set if key should not be removed */
>>>> #define KEY_FLAG_UID_KEYRING	9	/* set if key is a user or user session keyring */
>>>> +#define KEY_FLAG_BUILTIN_ROT	10	/* set if key is a builtin Root of Trust key */
>>>> 
>>>> 	/* the key type and key description string
>>>> 	 * - the desc is used to match a key against search criteria
>>>> @@ -290,6 +291,7 @@ extern struct key *key_alloc(struct key_type *type,
>>>> #define KEY_ALLOC_BYPASS_RESTRICTION	0x0008	/* Override the check on restricted keyrings */
>>>> #define KEY_ALLOC_UID_KEYRING		0x0010	/* allocating a user or user session keyring */
>>>> #define KEY_ALLOC_SET_KEEP		0x0020	/* Set the KEEP flag on the key/keyring */
>>>> +#define KEY_ALLOC_BUILT_IN_ROT		0x0040  /* Add builtin root of trust key */
>>> 
>>> Since the concept of root of trust is not generic, but limited to
>>> specific keyrings, the root CA certificate signing keys on the
>>> "machine" keyring need to be identified.  Similar to the
>>> KEY_ALLOC_BUILT_IN/KEY_FLAG_BUILTIN, new flags
>>> KEY_ALLOC_MACHINE/KEY_FLAG_MACHINE should be defined instead.
>> 
>> I’m open to renaming these, however this name change seems confusing to me.  
>> This flag gets set when the X.509 certificate contains the three CA requirements 
>> identified above.  The remaining keys in the machine keyring can be used for 
>> anything else.
> 
> Renaming the flag to KEY_ALLOC_MACHINE/KEY_FLAG_MACHINE differentiates
> between the "builtin" keys from the "machine" keys.  The trust models
> are very different.

Isn’t the trust model the same for machine and secondary keys?  Both are supplied by 
the end-user. That is why I’m confused by naming something _MACHINE when it applies 
to more than one keyring.

>> Plus this flag can be set for keys loaded into the secondary trusted 
>> keyring (6th patch in the series).  When an intermediate CA gets loaded into the 
>> secondary, the flag is set as well.
> 
> Please include a full explanation with the motivation in the patch
> description as to why support for intermediary CAs is required for the
> "end-user" use case.

Ok, I can add it.  I thought this was an expectation, based on the help section of
IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY:

" Intermediate keys between those the kernel has compiled in and the 
 IMA keys to be added may be added to the system secondary keyring,
 provided they are validly signed by a key already resident in the
 built-in or secondary trusted keyrings."

Re: [PATCH 4/7] KEYS: Introduce a builtin root of trust key flag

[Mimi Zohar]

On Fri, 2022-04-08 at 17:34 +0000, Eric Snowberg wrote:
> 
> > On Apr 8, 2022, at 10:55 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> > 
> > On Fri, 2022-04-08 at 15:27 +0000, Eric Snowberg wrote:
> >> 
> >>> On Apr 8, 2022, at 8:40 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> >>> 
> >>> On Tue, 2022-04-05 at 21:53 -0400, Eric Snowberg wrote:
> >>>> 
> >>>> The first type of key to use this is X.509.  When a X.509 certificate
> >>>> is self signed, has the kernCertSign Key Usage set and contains the
> >>>> CA bit set this new flag is set.
> >>>> 
> >>>> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
> >>>> 
> >>>> diff --git a/include/linux/key.h b/include/linux/key.h
> >>>> index 7febc4881363..97f6a1f86a27 100644
> >>>> --- a/include/linux/key.h
> >>>> +++ b/include/linux/key.h
> >>>> @@ -230,6 +230,7 @@ struct key {
> >>>> #define KEY_FLAG_ROOT_CAN_INVAL	7	/* set if key can be invalidated by root without permission */
> >>>> #define KEY_FLAG_KEEP		8	/* set if key should not be removed */
> >>>> #define KEY_FLAG_UID_KEYRING	9	/* set if key is a user or user session keyring */
> >>>> +#define KEY_FLAG_BUILTIN_ROT	10	/* set if key is a builtin Root of Trust key */
> >>>> 
> >>>> 	/* the key type and key description string
> >>>> 	 * - the desc is used to match a key against search criteria
> >>>> @@ -290,6 +291,7 @@ extern struct key *key_alloc(struct key_type *type,
> >>>> #define KEY_ALLOC_BYPASS_RESTRICTION	0x0008	/* Override the check on restricted keyrings */
> >>>> #define KEY_ALLOC_UID_KEYRING		0x0010	/* allocating a user or user session keyring */
> >>>> #define KEY_ALLOC_SET_KEEP		0x0020	/* Set the KEEP flag on the key/keyring */
> >>>> +#define KEY_ALLOC_BUILT_IN_ROT		0x0040  /* Add builtin root of trust key */
> >>> 
> >>> Since the concept of root of trust is not generic, but limited to
> >>> specific keyrings, the root CA certificate signing keys on the
> >>> "machine" keyring need to be identified.  Similar to the
> >>> KEY_ALLOC_BUILT_IN/KEY_FLAG_BUILTIN, new flags
> >>> KEY_ALLOC_MACHINE/KEY_FLAG_MACHINE should be defined instead.
> >> 
> >> I’m open to renaming these, however this name change seems confusing to me.  
> >> This flag gets set when the X.509 certificate contains the three CA requirements 
> >> identified above.  The remaining keys in the machine keyring can be used for 
> >> anything else.
> > 
> > Renaming the flag to KEY_ALLOC_MACHINE/KEY_FLAG_MACHINE differentiates
> > between the "builtin" keys from the "machine" keys.  The trust models
> > are very different.
> 
> Isn’t the trust model the same for machine and secondary keys?  Both are supplied by 
> the end-user. That is why I’m confused by naming something _MACHINE when it applies 
> to more than one keyring.

True both are supplied by the end-user, but the trust models are
different.  In one case the certificates are coming indirectly from
firmware, while in the other case the certificates would be limited to
certificates signed by the initial firmware certificates.  Loading only
root-CA signing key certificates onto the "machine" keyring highlights
and enforces the different types of trust.

> 
> >> Plus this flag can be set for keys loaded into the secondary trusted 
> >> keyring (6th patch in the series).  When an intermediate CA gets loaded into the 
> >> secondary, the flag is set as well.
> > 
> > Please include a full explanation with the motivation in the patch
> > description as to why support for intermediary CAs is required for the
> > "end-user" use case.
> 
> Ok, I can add it.  I thought this was an expectation, based on the help section of
> IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY:
> 
> " Intermediate keys between those the kernel has compiled in and the 
>  IMA keys to be added may be added to the system secondary keyring,
>  provided they are validly signed by a key already resident in the
>  built-in or secondary trusted keyrings."

This paragraph refers to keys on the "builtin_trusted_keys" keyring. 
The concept would need to be expanded to include keys on the "machine"
keyring.   Since support for intermediary CA keys isn't required for
the simple "end-user" use case, the motivation  needs to be provided.

thanks,

Mimi

Re: [PATCH 4/7] KEYS: Introduce a builtin root of trust key flag

[Eric Snowberg]

> On Apr 8, 2022, at 12:49 PM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> 
> On Fri, 2022-04-08 at 17:34 +0000, Eric Snowberg wrote:
>> 
>>> On Apr 8, 2022, at 10:55 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
>>> 
>>> On Fri, 2022-04-08 at 15:27 +0000, Eric Snowberg wrote:
>>>> 
>>>>> On Apr 8, 2022, at 8:40 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
>>>>> 
>>>>> On Tue, 2022-04-05 at 21:53 -0400, Eric Snowberg wrote:
>>>>>> 
>>>>>> The first type of key to use this is X.509.  When a X.509 certificate
>>>>>> is self signed, has the kernCertSign Key Usage set and contains the
>>>>>> CA bit set this new flag is set.
>>>>>> 
>>>>>> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
>>>>>> 
>>>>>> diff --git a/include/linux/key.h b/include/linux/key.h
>>>>>> index 7febc4881363..97f6a1f86a27 100644
>>>>>> --- a/include/linux/key.h
>>>>>> +++ b/include/linux/key.h
>>>>>> @@ -230,6 +230,7 @@ struct key {
>>>>>> #define KEY_FLAG_ROOT_CAN_INVAL	7	/* set if key can be invalidated by root without permission */
>>>>>> #define KEY_FLAG_KEEP		8	/* set if key should not be removed */
>>>>>> #define KEY_FLAG_UID_KEYRING	9	/* set if key is a user or user session keyring */
>>>>>> +#define KEY_FLAG_BUILTIN_ROT	10	/* set if key is a builtin Root of Trust key */
>>>>>> 
>>>>>> 	/* the key type and key description string
>>>>>> 	 * - the desc is used to match a key against search criteria
>>>>>> @@ -290,6 +291,7 @@ extern struct key *key_alloc(struct key_type *type,
>>>>>> #define KEY_ALLOC_BYPASS_RESTRICTION	0x0008	/* Override the check on restricted keyrings */
>>>>>> #define KEY_ALLOC_UID_KEYRING		0x0010	/* allocating a user or user session keyring */
>>>>>> #define KEY_ALLOC_SET_KEEP		0x0020	/* Set the KEEP flag on the key/keyring */
>>>>>> +#define KEY_ALLOC_BUILT_IN_ROT		0x0040  /* Add builtin root of trust key */
>>>>> 
>>>>> Since the concept of root of trust is not generic, but limited to
>>>>> specific keyrings, the root CA certificate signing keys on the
>>>>> "machine" keyring need to be identified.  Similar to the
>>>>> KEY_ALLOC_BUILT_IN/KEY_FLAG_BUILTIN, new flags
>>>>> KEY_ALLOC_MACHINE/KEY_FLAG_MACHINE should be defined instead.
>>>> 
>>>> I’m open to renaming these, however this name change seems confusing to me.  
>>>> This flag gets set when the X.509 certificate contains the three CA requirements 
>>>> identified above.  The remaining keys in the machine keyring can be used for 
>>>> anything else.
>>> 
>>> Renaming the flag to KEY_ALLOC_MACHINE/KEY_FLAG_MACHINE differentiates
>>> between the "builtin" keys from the "machine" keys.  The trust models
>>> are very different.
>> 
>> Isn’t the trust model the same for machine and secondary keys?  Both are supplied by 
>> the end-user. That is why I’m confused by naming something _MACHINE when it applies 
>> to more than one keyring.
> 
> True both are supplied by the end-user, but the trust models are
> different.

I think I need more information here, I’m not seeing how they are different trust 
models.

>  In one case the certificates are coming indirectly from
> firmware,

Any kernel signed by a cert in the MokList will boot.  The very thing the machine 
keyring contains.

For example, if a user has a cert (CA bit set false, keyCertSign not set, and it isn’t 
self signed), they can use insert-sys-cert to get it into their kernel.  They can then 
sign the kernel with any key in their MokList.  Why would we want to treat this key 
different if it was injected into the kernel verses coming in through the machine 
keyring? 

I can see the desire to have a root of trust all the way back to the root CA.  What 
I can’t see is if we ignore this for certain keyrings.

> while in the other case the certificates would be limited to
> certificates signed by the initial firmware certificates.  Loading only
> root-CA signing key certificates onto the "machine" keyring highlights
> and enforces the different types of trust.

If the root-CA cert must contain keyCertSign, I don’t see the point in loading only 
root-CA certs either. Why would we want to prevent  a code signing cert with the 
CA bit set from loading into the machine keyring?  A code signing cert should be 
allowed to validate a kernel module, but It should not be allowed to validate other
certs.

Re: [PATCH 4/7] KEYS: Introduce a builtin root of trust key flag

[Mimi Zohar]

On Fri, 2022-04-08 at 21:59 +0000, Eric Snowberg wrote:
> > On Apr 8, 2022, at 12:49 PM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> > 
> > On Fri, 2022-04-08 at 17:34 +0000, Eric Snowberg wrote:
> >> 
> >>> On Apr 8, 2022, at 10:55 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> >>> 
> >>> On Fri, 2022-04-08 at 15:27 +0000, Eric Snowberg wrote:
> >>>> 
> >>>>> On Apr 8, 2022, at 8:40 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> >>>>> 
> >>>>> On Tue, 2022-04-05 at 21:53 -0400, Eric Snowberg wrote:
> >>>>>> 
> >>>>>> The first type of key to use this is X.509.  When a X.509 certificate
> >>>>>> is self signed, has the kernCertSign Key Usage set and contains the
> >>>>>> CA bit set this new flag is set.
> >>>>>> 
> >>>>>> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
> >>>>>> 
> >>>>>> diff --git a/include/linux/key.h b/include/linux/key.h
> >>>>>> index 7febc4881363..97f6a1f86a27 100644
> >>>>>> --- a/include/linux/key.h
> >>>>>> +++ b/include/linux/key.h
> >>>>>> @@ -230,6 +230,7 @@ struct key {
> >>>>>> #define KEY_FLAG_ROOT_CAN_INVAL  7       /* set if key can be invalidated by root without permission */
> >>>>>> #define KEY_FLAG_KEEP            8       /* set if key should not be removed */
> >>>>>> #define KEY_FLAG_UID_KEYRING     9       /* set if key is a user or user session keyring */
> >>>>>> +#define KEY_FLAG_BUILTIN_ROT    10      /* set if key is a builtin Root of Trust key */
> >>>>>> 
> >>>>>>  /* the key type and key description string
> >>>>>>   * - the desc is used to match a key against search criteria
> >>>>>> @@ -290,6 +291,7 @@ extern struct key *key_alloc(struct key_type *type,
> >>>>>> #define KEY_ALLOC_BYPASS_RESTRICTION     0x0008  /* Override the check on restricted keyrings */
> >>>>>> #define KEY_ALLOC_UID_KEYRING            0x0010  /* allocating a user or user session keyring */
> >>>>>> #define KEY_ALLOC_SET_KEEP               0x0020  /* Set the KEEP flag on the key/keyring */
> >>>>>> +#define KEY_ALLOC_BUILT_IN_ROT          0x0040  /* Add builtin root of trust key */
> >>>>> 
> >>>>> Since the concept of root of trust is not generic, but limited to
> >>>>> specific keyrings, the root CA certificate signing keys on the
> >>>>> "machine" keyring need to be identified.  Similar to the
> >>>>> KEY_ALLOC_BUILT_IN/KEY_FLAG_BUILTIN, new flags
> >>>>> KEY_ALLOC_MACHINE/KEY_FLAG_MACHINE should be defined instead.
> >>>> 
> >>>> I’m open to renaming these, however this name change seems confusing to me.  
> >>>> This flag gets set when the X.509 certificate contains the three CA requirements 
> >>>> identified above.  The remaining keys in the machine keyring can be used for 
> >>>> anything else.
> >>> 
> >>> Renaming the flag to KEY_ALLOC_MACHINE/KEY_FLAG_MACHINE differentiates
> >>> between the "builtin" keys from the "machine" keys.  The trust models
> >>> are very different.
> >> 
> >> Isn’t the trust model the same for machine and secondary keys?  Both are supplied by 
> >> the end-user. That is why I’m confused by naming something _MACHINE when it applies 
> >> to more than one keyring.
> > 
> > True both are supplied by the end-user, but the trust models are
> > different.
> 
> I think I need more information here, I’m not seeing how they are different trust 
> models.

In order to discuss trust models, we need to understand the different
use-cases that are being discussed here without ever having been
explicitly stated.  Here are a few:
- Allow users to sign their own kernel modules.
- Allow users to selectively authorize 3rd party certificates to verify
kernel modules.
- From an IMA perspective, allow users to sign files within their own
software packages.

Each of the above use-cases needs to be independently configurable,
thoroughly explained, and enforced.

thanks,

Mimi


> 
> >  In one case the certificates are coming indirectly from
> > firmware,

Re: [PATCH 4/7] KEYS: Introduce a builtin root of trust key flag

[Eric Snowberg]

> On Apr 11, 2022, at 9:30 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> 
> On Fri, 2022-04-08 at 21:59 +0000, Eric Snowberg wrote:
>>> On Apr 8, 2022, at 12:49 PM, Mimi Zohar <zohar@linux.ibm.com> wrote:
>>> 
>>> On Fri, 2022-04-08 at 17:34 +0000, Eric Snowberg wrote:
>>>> 
>>>>> On Apr 8, 2022, at 10:55 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
>>>>> 
>>>>> On Fri, 2022-04-08 at 15:27 +0000, Eric Snowberg wrote:
>>>>>> 
>>>>>>> On Apr 8, 2022, at 8:40 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
>>>>>>> 
>>>>>>> On Tue, 2022-04-05 at 21:53 -0400, Eric Snowberg wrote:
>>>>>>>> 
>>>>>>>> The first type of key to use this is X.509.  When a X.509 certificate
>>>>>>>> is self signed, has the kernCertSign Key Usage set and contains the
>>>>>>>> CA bit set this new flag is set.
>>>>>>>> 
>>>>>>>> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
>>>>>>>> 
>>>>>>>> diff --git a/include/linux/key.h b/include/linux/key.h
>>>>>>>> index 7febc4881363..97f6a1f86a27 100644
>>>>>>>> --- a/include/linux/key.h
>>>>>>>> +++ b/include/linux/key.h
>>>>>>>> @@ -230,6 +230,7 @@ struct key {
>>>>>>>> #define KEY_FLAG_ROOT_CAN_INVAL  7       /* set if key can be invalidated by root without permission */
>>>>>>>> #define KEY_FLAG_KEEP            8       /* set if key should not be removed */
>>>>>>>> #define KEY_FLAG_UID_KEYRING     9       /* set if key is a user or user session keyring */
>>>>>>>> +#define KEY_FLAG_BUILTIN_ROT    10      /* set if key is a builtin Root of Trust key */
>>>>>>>> 
>>>>>>>> /* the key type and key description string
>>>>>>>>  * - the desc is used to match a key against search criteria
>>>>>>>> @@ -290,6 +291,7 @@ extern struct key *key_alloc(struct key_type *type,
>>>>>>>> #define KEY_ALLOC_BYPASS_RESTRICTION     0x0008  /* Override the check on restricted keyrings */
>>>>>>>> #define KEY_ALLOC_UID_KEYRING            0x0010  /* allocating a user or user session keyring */
>>>>>>>> #define KEY_ALLOC_SET_KEEP               0x0020  /* Set the KEEP flag on the key/keyring */
>>>>>>>> +#define KEY_ALLOC_BUILT_IN_ROT          0x0040  /* Add builtin root of trust key */
>>>>>>> 
>>>>>>> Since the concept of root of trust is not generic, but limited to
>>>>>>> specific keyrings, the root CA certificate signing keys on the
>>>>>>> "machine" keyring need to be identified.  Similar to the
>>>>>>> KEY_ALLOC_BUILT_IN/KEY_FLAG_BUILTIN, new flags
>>>>>>> KEY_ALLOC_MACHINE/KEY_FLAG_MACHINE should be defined instead.
>>>>>> 
>>>>>> I’m open to renaming these, however this name change seems confusing to me.  
>>>>>> This flag gets set when the X.509 certificate contains the three CA requirements 
>>>>>> identified above.  The remaining keys in the machine keyring can be used for 
>>>>>> anything else.
>>>>> 
>>>>> Renaming the flag to KEY_ALLOC_MACHINE/KEY_FLAG_MACHINE differentiates
>>>>> between the "builtin" keys from the "machine" keys.  The trust models
>>>>> are very different.
>>>> 
>>>> Isn’t the trust model the same for machine and secondary keys?  Both are supplied by 
>>>> the end-user. That is why I’m confused by naming something _MACHINE when it applies 
>>>> to more than one keyring.
>>> 
>>> True both are supplied by the end-user, but the trust models are
>>> different.
>> 
>> I think I need more information here, I’m not seeing how they are different trust 
>> models.
> 
> In order to discuss trust models, we need to understand the different
> use-cases that are being discussed here without ever having been
> explicitly stated.  Here are a few:
> - Allow users to sign their own kernel modules.
> - Allow users to selectively authorize 3rd party certificates to verify
> kernel modules.
> - From an IMA perspective, allow users to sign files within their own
> software packages.
> 
> Each of the above use-cases needs to be independently configurable,
> thoroughly explained, and enforced.

I’m still confused by the request here.  All these use cases can be done 
today with insert-sys-cert.  Take the, " allow user to sign their own kernel 
modules" use case.  Using insert-sys-cert, any type of key can be added 
to the builtin trusted keyring, it doesn’t need to be self signed, there are 
no restrictions on fields in the certificate.  The same approach can be used 
to allow users to ima sign their own files. Any key can be added, it doesn’t 
need to be a CA. The same goes for 3rd party signed modules.

This series doesn’t enable keys to be used for any new purpose than what 
can be done today.  In fact it limits how system keys may be used. It does 
this by adding a new restriction.  The new restriction enforces the CA 
requirements ima expects. This restriction is enforced on all keyrings ima 
references (builtin or secondary).  Since the machine keyring is linked to 
the secondary, it may now be used, since the CA restriction ima expects will 
be enforced.

Re: [PATCH 4/7] KEYS: Introduce a builtin root of trust key flag

[Mimi Zohar]

On Thu, 2022-04-14 at 16:36 +0000, Eric Snowberg wrote:
> 
> > On Apr 11, 2022, at 9:30 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> > 
> > On Fri, 2022-04-08 at 21:59 +0000, Eric Snowberg wrote:
> >>> On Apr 8, 2022, at 12:49 PM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> >>> 
> >>> On Fri, 2022-04-08 at 17:34 +0000, Eric Snowberg wrote:
> >>>> 
> >>>>> On Apr 8, 2022, at 10:55 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> >>>>> 
> >>>>> On Fri, 2022-04-08 at 15:27 +0000, Eric Snowberg wrote:
> >>>>>> 
> >>>>>>> On Apr 8, 2022, at 8:40 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> >>>>>>> 
> >>>>>>> On Tue, 2022-04-05 at 21:53 -0400, Eric Snowberg wrote:
> >>>>>>>> 
> >>>>>>>> The first type of key to use this is X.509.  When a X.509 certificate
> >>>>>>>> is self signed, has the kernCertSign Key Usage set and contains the
> >>>>>>>> CA bit set this new flag is set.
> >>>>>>>> 
> >>>>>>>> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
> >>>>>>>> 
> >>>>>>>> diff --git a/include/linux/key.h b/include/linux/key.h
> >>>>>>>> index 7febc4881363..97f6a1f86a27 100644
> >>>>>>>> --- a/include/linux/key.h
> >>>>>>>> +++ b/include/linux/key.h
> >>>>>>>> @@ -230,6 +230,7 @@ struct key {
> >>>>>>>> #define KEY_FLAG_ROOT_CAN_INVAL  7       /* set if key can be invalidated by root without permission */
> >>>>>>>> #define KEY_FLAG_KEEP            8       /* set if key should not be removed */
> >>>>>>>> #define KEY_FLAG_UID_KEYRING     9       /* set if key is a user or user session keyring */
> >>>>>>>> +#define KEY_FLAG_BUILTIN_ROT    10      /* set if key is a builtin Root of Trust key */
> >>>>>>>> 
> >>>>>>>> /* the key type and key description string
> >>>>>>>>  * - the desc is used to match a key against search criteria
> >>>>>>>> @@ -290,6 +291,7 @@ extern struct key *key_alloc(struct key_type *type,
> >>>>>>>> #define KEY_ALLOC_BYPASS_RESTRICTION     0x0008  /* Override the check on restricted keyrings */
> >>>>>>>> #define KEY_ALLOC_UID_KEYRING            0x0010  /* allocating a user or user session keyring */
> >>>>>>>> #define KEY_ALLOC_SET_KEEP               0x0020  /* Set the KEEP flag on the key/keyring */
> >>>>>>>> +#define KEY_ALLOC_BUILT_IN_ROT          0x0040  /* Add builtin root of trust key */
> >>>>>>> 
> >>>>>>> Since the concept of root of trust is not generic, but limited to
> >>>>>>> specific keyrings, the root CA certificate signing keys on the
> >>>>>>> "machine" keyring need to be identified.  Similar to the
> >>>>>>> KEY_ALLOC_BUILT_IN/KEY_FLAG_BUILTIN, new flags
> >>>>>>> KEY_ALLOC_MACHINE/KEY_FLAG_MACHINE should be defined instead.
> >>>>>> 
> >>>>>> I’m open to renaming these, however this name change seems confusing to me.  
> >>>>>> This flag gets set when the X.509 certificate contains the three CA requirements 
> >>>>>> identified above.  The remaining keys in the machine keyring can be used for 
> >>>>>> anything else.
> >>>>> 
> >>>>> Renaming the flag to KEY_ALLOC_MACHINE/KEY_FLAG_MACHINE differentiates
> >>>>> between the "builtin" keys from the "machine" keys.  The trust models
> >>>>> are very different.
> >>>> 
> >>>> Isn’t the trust model the same for machine and secondary keys?  Both are supplied by 
> >>>> the end-user. That is why I’m confused by naming something _MACHINE when it applies 
> >>>> to more than one keyring.
> >>> 
> >>> True both are supplied by the end-user, but the trust models are
> >>> different.
> >> 
> >> I think I need more information here, I’m not seeing how they are different trust 
> >> models.
> > 
> > In order to discuss trust models, we need to understand the different
> > use-cases that are being discussed here without ever having been
> > explicitly stated.  Here are a few:
> > - Allow users to sign their own kernel modules.
> > - Allow users to selectively authorize 3rd party certificates to verify
> > kernel modules.
> > - From an IMA perspective, allow users to sign files within their own
> > software packages.
> > 
> > Each of the above use-cases needs to be independently configurable,
> > thoroughly explained, and enforced.
> 
> I’m still confused by the request here.  All these use cases can be done 
> today with insert-sys-cert.  Take the, " allow user to sign their own kernel 
> modules" use case.  Using insert-sys-cert, any type of key can be added 
> to the builtin trusted keyring, it doesn’t need to be self signed, there are 
> no restrictions on fields in the certificate.  The same approach can be used 
> to allow users to ima sign their own files. Any key can be added, it doesn’t 
> need to be a CA. The same goes for 3rd party signed modules.

The difference is "where" the key is coming from.  In the builtin use-
case or the post build insert-sys-cert case, the kernel image is
signed, or re-signed, and the kernel image signature is verified.  The
root of trust is straight forward - secure boot with a HW root of trust
up to and including verifying the kernel image signature, then
transition to the builtin keys.

Keys on the "machine" keyring are not part of that signature chain of
trust, requiring them to be handled differently, more carefully.  At
least from an IMA perspective, one way of doing so is by loading a root
CA key, defined as a KeySigning cert, onto the "machine" keyring.  All
other certs would be loaded via userspace either onto the "secondary"
or "ima" keyrings.

This satifies all of the above requirements, even allowing users to
selectively authorize 3rd party certificates to verify kernel modules.
> 
> This series doesn’t enable keys to be used for any new purpose than what 
> can be done today.  In fact it limits how system keys may be used. It does 
> this by adding a new restriction.  The new restriction enforces the CA 
> requirements ima expects. This restriction is enforced on all keyrings ima 
> references (builtin or secondary).  Since the machine keyring is linked to 
> the secondary, it may now be used, since the CA restriction ima expects will 
> be enforced.

Limiting the change to just the IMA keyring is insufficient.  For this
reason, choosing to load all of the MOK keys onto the "machine" keyring
needs to be independently configurable and thoroughly explained.

thanks,

Mimi

Re: [PATCH 4/7] KEYS: Introduce a builtin root of trust key flag

[Eric Snowberg]

> On Apr 14, 2022, at 12:09 PM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> 
> On Thu, 2022-04-14 at 16:36 +0000, Eric Snowberg wrote:
>> 
>>> On Apr 11, 2022, at 9:30 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
>>> 
>>> On Fri, 2022-04-08 at 21:59 +0000, Eric Snowberg wrote:
>>>>> On Apr 8, 2022, at 12:49 PM, Mimi Zohar <zohar@linux.ibm.com> wrote:
>>>>> 
>>>>> On Fri, 2022-04-08 at 17:34 +0000, Eric Snowberg wrote:
>>>>>> 
>>>>>>> On Apr 8, 2022, at 10:55 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
>>>>>>> 
>>>>>>> On Fri, 2022-04-08 at 15:27 +0000, Eric Snowberg wrote:
>>>>>>>> 
>>>>>>>>> On Apr 8, 2022, at 8:40 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
>>>>>>>>> 
>>>>>>>>> On Tue, 2022-04-05 at 21:53 -0400, Eric Snowberg wrote:
>>>>>>>>>> 
>>>>>>>>>> The first type of key to use this is X.509.  When a X.509 certificate
>>>>>>>>>> is self signed, has the kernCertSign Key Usage set and contains the
>>>>>>>>>> CA bit set this new flag is set.
>>>>>>>>>> 
>>>>>>>>>> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
>>>>>>>>>> 
>>>>>>>>>> diff --git a/include/linux/key.h b/include/linux/key.h
>>>>>>>>>> index 7febc4881363..97f6a1f86a27 100644
>>>>>>>>>> --- a/include/linux/key.h
>>>>>>>>>> +++ b/include/linux/key.h
>>>>>>>>>> @@ -230,6 +230,7 @@ struct key {
>>>>>>>>>> #define KEY_FLAG_ROOT_CAN_INVAL  7       /* set if key can be invalidated by root without permission */
>>>>>>>>>> #define KEY_FLAG_KEEP            8       /* set if key should not be removed */
>>>>>>>>>> #define KEY_FLAG_UID_KEYRING     9       /* set if key is a user or user session keyring */
>>>>>>>>>> +#define KEY_FLAG_BUILTIN_ROT    10      /* set if key is a builtin Root of Trust key */
>>>>>>>>>> 
>>>>>>>>>> /* the key type and key description string
>>>>>>>>>> * - the desc is used to match a key against search criteria
>>>>>>>>>> @@ -290,6 +291,7 @@ extern struct key *key_alloc(struct key_type *type,
>>>>>>>>>> #define KEY_ALLOC_BYPASS_RESTRICTION     0x0008  /* Override the check on restricted keyrings */
>>>>>>>>>> #define KEY_ALLOC_UID_KEYRING            0x0010  /* allocating a user or user session keyring */
>>>>>>>>>> #define KEY_ALLOC_SET_KEEP               0x0020  /* Set the KEEP flag on the key/keyring */
>>>>>>>>>> +#define KEY_ALLOC_BUILT_IN_ROT          0x0040  /* Add builtin root of trust key */
>>>>>>>>> 
>>>>>>>>> Since the concept of root of trust is not generic, but limited to
>>>>>>>>> specific keyrings, the root CA certificate signing keys on the
>>>>>>>>> "machine" keyring need to be identified.  Similar to the
>>>>>>>>> KEY_ALLOC_BUILT_IN/KEY_FLAG_BUILTIN, new flags
>>>>>>>>> KEY_ALLOC_MACHINE/KEY_FLAG_MACHINE should be defined instead.
>>>>>>>> 
>>>>>>>> I’m open to renaming these, however this name change seems confusing to me.  
>>>>>>>> This flag gets set when the X.509 certificate contains the three CA requirements 
>>>>>>>> identified above.  The remaining keys in the machine keyring can be used for 
>>>>>>>> anything else.
>>>>>>> 
>>>>>>> Renaming the flag to KEY_ALLOC_MACHINE/KEY_FLAG_MACHINE differentiates
>>>>>>> between the "builtin" keys from the "machine" keys.  The trust models
>>>>>>> are very different.
>>>>>> 
>>>>>> Isn’t the trust model the same for machine and secondary keys?  Both are supplied by 
>>>>>> the end-user. That is why I’m confused by naming something _MACHINE when it applies 
>>>>>> to more than one keyring.
>>>>> 
>>>>> True both are supplied by the end-user, but the trust models are
>>>>> different.
>>>> 
>>>> I think I need more information here, I’m not seeing how they are different trust 
>>>> models.
>>> 
>>> In order to discuss trust models, we need to understand the different
>>> use-cases that are being discussed here without ever having been
>>> explicitly stated.  Here are a few:
>>> - Allow users to sign their own kernel modules.
>>> - Allow users to selectively authorize 3rd party certificates to verify
>>> kernel modules.
>>> - From an IMA perspective, allow users to sign files within their own
>>> software packages.
>>> 
>>> Each of the above use-cases needs to be independently configurable,
>>> thoroughly explained, and enforced.
>> 
>> I’m still confused by the request here.  All these use cases can be done 
>> today with insert-sys-cert.  Take the, " allow user to sign their own kernel 
>> modules" use case.  Using insert-sys-cert, any type of key can be added 
>> to the builtin trusted keyring, it doesn’t need to be self signed, there are 
>> no restrictions on fields in the certificate.  The same approach can be used 
>> to allow users to ima sign their own files. Any key can be added, it doesn’t 
>> need to be a CA. The same goes for 3rd party signed modules.
> 
> The difference is "where" the key is coming from.  In the builtin use-
> case or the post build insert-sys-cert case, the kernel image is
> signed, or re-signed, and the kernel image signature is verified.  The
> root of trust is straight forward - secure boot with a HW root of trust
> up to and including verifying the kernel image signature, then
> transition to the builtin keys.
> 
> Keys on the "machine" keyring are not part of that signature chain of
> trust,

The machine keyring contains all keys in the MokList. On x86 (and other 
architectures that boot with shim) all keys in the MokList are part of the signature 
chain of trust. Shim uses MOKList keys to validate the kernel image signature 
when booting with SecureBoot enabled. Secure Boot DB keys are used to
validate shim, but rarely used to validate the kernel.

Re: [PATCH 4/7] KEYS: Introduce a builtin root of trust key flag

[Mimi Zohar]

On Thu, 2022-04-14 at 21:59 +0000, Eric Snowberg wrote:
> 
> > On Apr 14, 2022, at 12:09 PM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> > 
> > On Thu, 2022-04-14 at 16:36 +0000, Eric Snowberg wrote:
> >> 
> >>> On Apr 11, 2022, at 9:30 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> >>> 
> >>> On Fri, 2022-04-08 at 21:59 +0000, Eric Snowberg wrote:
> >>>>> On Apr 8, 2022, at 12:49 PM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> >>>>> 
> >>>>> On Fri, 2022-04-08 at 17:34 +0000, Eric Snowberg wrote:
> >>>>>> 
> >>>>>>> On Apr 8, 2022, at 10:55 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> >>>>>>> 
> >>>>>>> On Fri, 2022-04-08 at 15:27 +0000, Eric Snowberg wrote:
> >>>>>>>> 
> >>>>>>>>> On Apr 8, 2022, at 8:40 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> >>>>>>>>> 
> >>>>>>>>> On Tue, 2022-04-05 at 21:53 -0400, Eric Snowberg wrote:
> >>>>>>>>>> 
> >>>>>>>>>> The first type of key to use this is X.509.  When a X.509 certificate
> >>>>>>>>>> is self signed, has the kernCertSign Key Usage set and contains the
> >>>>>>>>>> CA bit set this new flag is set.
> >>>>>>>>>> 
> >>>>>>>>>> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
> >>>>>>>>>> 
> >>>>>>>>>> diff --git a/include/linux/key.h b/include/linux/key.h
> >>>>>>>>>> index 7febc4881363..97f6a1f86a27 100644
> >>>>>>>>>> --- a/include/linux/key.h
> >>>>>>>>>> +++ b/include/linux/key.h
> >>>>>>>>>> @@ -230,6 +230,7 @@ struct key {
> >>>>>>>>>> #define KEY_FLAG_ROOT_CAN_INVAL  7       /* set if key can be invalidated by root without permission */
> >>>>>>>>>> #define KEY_FLAG_KEEP            8       /* set if key should not be removed */
> >>>>>>>>>> #define KEY_FLAG_UID_KEYRING     9       /* set if key is a user or user session keyring */
> >>>>>>>>>> +#define KEY_FLAG_BUILTIN_ROT    10      /* set if key is a builtin Root of Trust key */
> >>>>>>>>>> 
> >>>>>>>>>> /* the key type and key description string
> >>>>>>>>>> * - the desc is used to match a key against search criteria
> >>>>>>>>>> @@ -290,6 +291,7 @@ extern struct key *key_alloc(struct key_type *type,
> >>>>>>>>>> #define KEY_ALLOC_BYPASS_RESTRICTION     0x0008  /* Override the check on restricted keyrings */
> >>>>>>>>>> #define KEY_ALLOC_UID_KEYRING            0x0010  /* allocating a user or user session keyring */
> >>>>>>>>>> #define KEY_ALLOC_SET_KEEP               0x0020  /* Set the KEEP flag on the key/keyring */
> >>>>>>>>>> +#define KEY_ALLOC_BUILT_IN_ROT          0x0040  /* Add builtin root of trust key */
> >>>>>>>>> 
> >>>>>>>>> Since the concept of root of trust is not generic, but limited to
> >>>>>>>>> specific keyrings, the root CA certificate signing keys on the
> >>>>>>>>> "machine" keyring need to be identified.  Similar to the
> >>>>>>>>> KEY_ALLOC_BUILT_IN/KEY_FLAG_BUILTIN, new flags
> >>>>>>>>> KEY_ALLOC_MACHINE/KEY_FLAG_MACHINE should be defined instead.
> >>>>>>>> 
> >>>>>>>> I’m open to renaming these, however this name change seems confusing to me.  
> >>>>>>>> This flag gets set when the X.509 certificate contains the three CA requirements 
> >>>>>>>> identified above.  The remaining keys in the machine keyring can be used for 
> >>>>>>>> anything else.
> >>>>>>> 
> >>>>>>> Renaming the flag to KEY_ALLOC_MACHINE/KEY_FLAG_MACHINE differentiates
> >>>>>>> between the "builtin" keys from the "machine" keys.  The trust models
> >>>>>>> are very different.
> >>>>>> 
> >>>>>> Isn’t the trust model the same for machine and secondary keys?  Both are supplied by 
> >>>>>> the end-user. That is why I’m confused by naming something _MACHINE when it applies 
> >>>>>> to more than one keyring.
> >>>>> 
> >>>>> True both are supplied by the end-user, but the trust models are
> >>>>> different.
> >>>> 
> >>>> I think I need more information here, I’m not seeing how they are different trust 
> >>>> models.
> >>> 
> >>> In order to discuss trust models, we need to understand the different
> >>> use-cases that are being discussed here without ever having been
> >>> explicitly stated.  Here are a few:
> >>> - Allow users to sign their own kernel modules.
> >>> - Allow users to selectively authorize 3rd party certificates to verify
> >>> kernel modules.
> >>> - From an IMA perspective, allow users to sign files within their own
> >>> software packages.
> >>> 
> >>> Each of the above use-cases needs to be independently configurable,
> >>> thoroughly explained, and enforced.
> >> 
> >> I’m still confused by the request here.  All these use cases can be done 
> >> today with insert-sys-cert.  Take the, " allow user to sign their own kernel 
> >> modules" use case.  Using insert-sys-cert, any type of key can be added 
> >> to the builtin trusted keyring, it doesn’t need to be self signed, there are 
> >> no restrictions on fields in the certificate.  The same approach can be used 
> >> to allow users to ima sign their own files. Any key can be added, it doesn’t 
> >> need to be a CA. The same goes for 3rd party signed modules.
> > 
> > The difference is "where" the key is coming from.  In the builtin use-
> > case or the post build insert-sys-cert case, the kernel image is
> > signed, or re-signed, and the kernel image signature is verified.  The
> > root of trust is straight forward - secure boot with a HW root of trust
> > up to and including verifying the kernel image signature, then
> > transition to the builtin keys.
> > 
> > Keys on the "machine" keyring are not part of that signature chain of
> > trust,
> 
> The machine keyring contains all keys in the MokList. On x86 (and other 
> architectures that boot with shim) all keys in the MokList are part of the signature 
> chain of trust. Shim uses MOKList keys to validate the kernel image signature 
> when booting with SecureBoot enabled. Secure Boot DB keys are used to
> validate shim, but rarely used to validate the kernel.

Sure, keys on the "machine" keyring can be used to verify the kexec
kernel image signature.

As all of the above requirements is satisfied by loading a root CA, def
ined as a KeySigning cert, without needing to load all of the MOK keys
onto the "machine" keyring, support both trust models.  Please make
loading all MOK keys configurable, with a thorough explanation.

thanks,

Mimi

Re: [PATCH 0/7] Add CA enforcement keyring restrictions

[Coiby Xu]

Hi Eric,

I wonder if there is any update on this work? I would be glad to do
anything that may be helpful including testing a new version of code.

-- 
Best regards,
Coiby

Re: [PATCH 0/7] Add CA enforcement keyring restrictions

[Eric Snowberg]

> On Nov 4, 2022, at 7:20 AM, Coiby Xu <coxu@redhat.com> wrote:
> 
> Hi Eric,
> 
> I wonder if there is any update on this work? I would be glad to do
> anything that may be helpful including testing a new version of code.
> 
> -- 
> Best regards,
> Coiby


The discussion on this topic briefly moved over to this thread [1].  I took 
the lack of response to mean an approach like this would not be considered.  
If it would be considered, I am willing to continue working on a solution 
to this problem.

1. https://lore.kernel.org/linux-integrity/8BB9D406-0394-4E2E-9B84-4A320AFDBDC4@oracle.com/

Re: [PATCH 0/7] Add CA enforcement keyring restrictions

[Elaine Palmer]

On 2022/11/04 9:20 AM, Coiby Xu wrote:
> Hi Eric,
>
> I wonder if there is any update on this work? I would be glad to do
> anything that may be helpful including testing a new version of code.
>
Hi Coiby,

Yes, this discussion got stuck when we couldn't agree on one of the
following options:

(A) Filter which keys from MOK (or a management system) are loaded
    onto the .machine keyring. Specifically, load only keys with
    CA+keyCertSign attributes.

(B) Load all keys from MOK (or a management system) onto the
    .machine keyring. Then, subsequently filter those to restrict
    which ones can be loaded onto the .ima keyring specifically.

The objection to (A) was that distros would have to go through
two steps instead of one to load keys. The one-step method of
loading keys was supported by an out-of-tree patch and then by
the addition of the .machine keyring.

The objection to (B) was that, because the .machine keyring is now
linked to the .secondary keyring, it expands the scope of what the
kernel has trusted in the past. The effect is that keys in MOK
have the same broad scope as keys previously restricted to
.builtin and .secondary. It doesn't affect just IMA, but the rest
of the kernel as well.

I would suggest that we can get unstuck by considering:

(C) Defining a systemd (or dracut module) to load keys onto the
    .secondary keyring

(D) Using a configuration option to specify what types of
    .machine keys should be allowed to pass through to the
    .secondary keyring.
   
    The distro could choose (A) by allowing only
    CA+keyCertSign keys.

    The distro could choose (B) by allowing any kind
    of key.

We all seemed to agree that enforcing key usage should be
implemented and that a useful future effort is to add policies
to keys and keyrings, like, "This key can only be used for
verifying kernel modules."

I hope we can come to an agreement so work can proceed and IMA
can be re-enabled.

-Elaine Palmer

Re: [PATCH 0/7] Add CA enforcement keyring restrictions

[Eric Snowberg]

> On Nov 8, 2022, at 6:24 PM, Elaine Palmer <erpalmerny@gmail.com> wrote:
> 
> 
> 
> On 2022/11/04 9:20 AM, Coiby Xu wrote:
>> Hi Eric,
>> 
>> I wonder if there is any update on this work? I would be glad to do
>> anything that may be helpful including testing a new version of code.
>> 
> Hi Coiby,
> 
> Yes, this discussion got stuck when we couldn't agree on one of the
> following options:
> 
> (A) Filter which keys from MOK (or a management system) are loaded
>     onto the .machine keyring. Specifically, load only keys with
>     CA+keyCertSign attributes.
> 
> (B) Load all keys from MOK (or a management system) onto the
>     .machine keyring. Then, subsequently filter those to restrict
>     which ones can be loaded onto the .ima keyring specifically.
> 
> The objection to (A) was that distros would have to go through
> two steps instead of one to load keys. The one-step method of
> loading keys was supported by an out-of-tree patch and then by
> the addition of the .machine keyring.
> 
> The objection to (B) was that, because the .machine keyring is now
> linked to the .secondary keyring, it expands the scope of what the
> kernel has trusted in the past. The effect is that keys in MOK
> have the same broad scope as keys previously restricted to
> .builtin and .secondary. It doesn't affect just IMA, but the rest
> of the kernel as well.
> 
> I would suggest that we can get unstuck by considering:
> 
> (C) Defining a systemd (or dracut module) to load keys onto the
>     .secondary keyring
> 
> (D) Using a configuration option to specify what types of
>     .machine keys should be allowed to pass through to the
>     .secondary keyring.
>    
>     The distro could choose (A) by allowing only
>     CA+keyCertSign keys.
> 
>     The distro could choose (B) by allowing any kind
>     of key.
> 
> We all seemed to agree that enforcing key usage should be
> implemented and that a useful future effort is to add policies
> to keys and keyrings, like, "This key can only be used for
> verifying kernel modules."
> 
> I hope we can come to an agreement so work can proceed and IMA
> can be re-enabled.

I would be open to making the changes necessary to support both (A and B) 
options.  What type of configuration option would be considered?  Would this 
be a compile time Kconfig, a Linux boot command line parameter, or another 
MOK variable?

Re: [PATCH 0/7] Add CA enforcement keyring restrictions

[Elaine Palmer]

On 2022/11/09 9:25 AM, Eric Snowberg wrote:
>
>> On Nov 8, 2022, at 6:24 PM, Elaine Palmer <erpalmerny@gmail.com> wrote:
>>
>>
>>
>> On 2022/11/04 9:20 AM, Coiby Xu wrote:
>>> Hi Eric,
>>>
>>> I wonder if there is any update on this work? I would be glad to do
>>> anything that may be helpful including testing a new version of code.
>>>
>> Hi Coiby,
>>
>> Yes, this discussion got stuck when we couldn't agree on one of the
>> following options:
>>
>> (A) Filter which keys from MOK (or a management system) are loaded
>>     onto the .machine keyring. Specifically, load only keys with
>>     CA+keyCertSign attributes.
>>
>> (B) Load all keys from MOK (or a management system) onto the
>>     .machine keyring. Then, subsequently filter those to restrict
>>     which ones can be loaded onto the .ima keyring specifically.
>>
>> The objection to (A) was that distros would have to go through
>> two steps instead of one to load keys. The one-step method of
>> loading keys was supported by an out-of-tree patch and then by
>> the addition of the .machine keyring.
>>
>> The objection to (B) was that, because the .machine keyring is now
>> linked to the .secondary keyring, it expands the scope of what the
>> kernel has trusted in the past. The effect is that keys in MOK
>> have the same broad scope as keys previously restricted to
>> .builtin and .secondary. It doesn't affect just IMA, but the rest
>> of the kernel as well.
>>
>> I would suggest that we can get unstuck by considering:
>>
>> (C) Defining a systemd (or dracut module) to load keys onto the
>>     .secondary keyring
>>
>> (D) Using a configuration option to specify what types of
>>     .machine keys should be allowed to pass through to the
>>     .secondary keyring.
>>    
>>     The distro could choose (A) by allowing only
>>     CA+keyCertSign keys.
>>
>>     The distro could choose (B) by allowing any kind
>>     of key.
>>
>> We all seemed to agree that enforcing key usage should be
>> implemented and that a useful future effort is to add policies
>> to keys and keyrings, like, "This key can only be used for
>> verifying kernel modules."
>>
>> I hope we can come to an agreement so work can proceed and IMA
>> can be re-enabled.
> I would be open to making the changes necessary to support both (A and B) 
> options.  What type of configuration option would be considered?  Would this 
> be a compile time Kconfig, a Linux boot command line parameter, or another 
> MOK variable?
>
Thank you, Eric.  A compile time Kconfig would be the most secure, yet
would still support (B) when allowed.