[PATCH v3 00/10] Add CA enforcement keyring restrictions

[PATCH v3 00/10] Add CA enforcement keyring restrictions

[Eric Snowberg]

Prior to the introduction of the machine keyring, most distros simply 
allowed all keys contained within the platform keyring to be used
for both kernel and module verification.  This was done by an out of
tree patch.  Some distros took it even further and loaded all these keys
into the secondary trusted keyring.  This also allowed the system owner 
to add their own key for IMA usage.

Each distro contains similar documentation on how to sign kernel modules
and enroll the key into the MOK.  The process is fairly straightforward.
With the introduction of the machine keyring, the process remains
basically the same, without the need for any out of tree patches.

The machine keyring allowed distros to eliminate the out of tree patches
for kernel module signing.  However, it falls short in allowing the end 
user to add their own keys for IMA. Currently 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.  This would expand the current 
integrity gap. The IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY 
Kconfig states that keys may be added to the ima keyrings if the key is 
validly signed by a CA cert in the system built-in or secondary trusted 
keyring.  Currently there is not code that enforces the contents of a
CA cert. Any key in the builtin or secondary keyring can be used. 

To allow IMA to be enabled with the machine keyring, this series introduces
enforcement of key usage in the certificate. This series also applies
this enforcement across all kernel keyrings.

The machine keyring shares  similarities with both the builtin and
secondary keyrings.  Similar to the builtin, no keys may be added to the
machine keyring following boot. The secondary keyring allows user
provided keys to be added following boot; however, a previously enrolled
kernel key must vouch for the key before it may be included. The system
owner may include their own keys into the machine keyring prior to boot.
If the end-user is not the system owner, they may not add their own keys
to the machine keyring.  

The machine keyring is only populated when Secure Boot is enabled.  A
system owner has the ability to control the entire Secure Boot keychain
(PK, KEK, DB, and DBX).  The system owner can also turn Secure Boot off.
With this control, they may use insert-sys-cert to include their own key 
and re-sign their kernel and have it boot.  The system owner also has 
control to include or exclude MOK keys. This series does not try to 
interpret how a system owner has configured their machine.  If the system 
owner has taken the steps to add their own MOK keys, they will be 
included in the machine keyring and used for verification, exactly 
the same way as keys contained in the builtin and secondary keyrings.
Since the system owner has the ability to add keys before booting to
either the machine or builtin keyrings, it is viewed as inconsequential 
if the key originated from one or the other.

This series introduces two different ways to configure the machine keyring.
By default, nothing changes and all MOK keys are loaded into it.  Whenever
a CA cert is found within the machine, builtin, or secondary, a flag 
indicating this is stored in the public key struct.  The other option is 
if the new Kconfig INTEGRITY_CA_MACHINE_KEYRING is enabled, only CA certs 
will be loaded into the machine keyring. All remaining MOK keys will be 
loaded into the platform keyring.

A CA cert shall be defined as any X509 certificate that contains the 
keyCertSign key usage and has the CA bit set to true.

With this series applied, CA enforcement is in place whenever 
IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY is enabled.  Meaning, 
before any key can be included into the ima keyring, it must be
vouched for by a CA key contained within the builtin, secondary, or 
machine keyrings.

IMA allows userspace applications to be signed. The enduser may sign
their own application, however they may also want to use an application
provided by a 3rd party.  The entity building the kernel, may not be the
same entity building the userspace program.  The system owner may also
be a third entity.  If the system owner trusts the entity building the
userspace program, they will include their public key within the MOK.
This key would be used to sign the key added to the ima keyring. Not all
3rd party userspace providers have the capability to properly manage a
root CA.  Some may outsource to a different code signing provider.  Many
code signing providers use Intermediate CA certificates. Therefore, this
series also includes support for Intermediate CA certificates.

This series could be broken up into 3 different parts.  The first two
patches could be taken now.  They solve current issues that will be
triggered by the build robots.  Patches 3-8 add CA enforcement for the
ima keyring.  Patches 9-10 restrict the machine keyring to only load CA
certs into it.  Patches 9-10 require all the previous patches. 

Changelog:

v3:
- Allow Intermediate CA certs to be enrolled through the MOK. The
  Intermediate CA cert must contain keyCertSign key usage and have the 
  CA bit set to true. This was done by removing the self signed
  requirement.


Eric Snowberg (10):
  KEYS: Create static version of public_key_verify_signature
  KEYS: Add missing function documentation
  KEYS: X.509: Parse Basic Constraints for CA
  KEYS: X.509: Parse Key Usage
  KEYS: Introduce a CA endorsed flag
  KEYS: Introduce keyring restriction that validates ca trust
  KEYS: X.509: Flag Intermediate CA certs as endorsed
  integrity: Use root of trust signature restriction
  KEYS: CA link restriction
  integrity: restrict INTEGRITY_KEYRING_MACHINE to restrict_link_by_ca

 certs/system_keyring.c                    | 32 +++++++++-
 crypto/asymmetric_keys/restrict.c         | 76 +++++++++++++++++++++++
 crypto/asymmetric_keys/x509_cert_parser.c | 31 +++++++++
 crypto/asymmetric_keys/x509_parser.h      |  2 +
 crypto/asymmetric_keys/x509_public_key.c  | 16 +++++
 include/crypto/public_key.h               | 30 +++++++++
 include/keys/system_keyring.h             | 12 +++-
 include/linux/ima.h                       | 11 ++++
 include/linux/key-type.h                  |  3 +
 include/linux/key.h                       |  2 +
 security/integrity/Kconfig                | 11 +++-
 security/integrity/digsig.c               | 12 ++--
 security/integrity/ima/Kconfig            |  6 +-
 security/keys/key.c                       | 13 ++++
 14 files changed, 245 insertions(+), 12 deletions(-)


base-commit: 830b3c68c1fb1e9176028d02ef86f3cf76aa2476
-- 
2.27.0

[PATCH v3 01/10] 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>
Reviewed-by: Petr Vorel <pvorel@suse.cz>
---
 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 v3 02/10] KEYS: Add missing function documentation

[Eric Snowberg]

Compiling with 'W=1' results in warnings that 'Function parameter or member
not described'

Add the missing parameters for
restrict_link_by_builtin_and_secondary_trusted and
restrict_link_to_builtin_trusted.

Use /* instead of /** for get_builtin_and_secondary_restriction, since
it is a static function.

Fix wrong function name restrict_link_to_builtin_trusted brought by:
commit d3bfe84129f6 ("certs: Add a secondary system keyring that can be added to dynamically")

Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
Reviewed-by: Petr Vorel <pvorel@suse.cz>
---
 certs/system_keyring.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/certs/system_keyring.c b/certs/system_keyring.c
index 5042cc54fa5e..e531b88bc570 100644
--- a/certs/system_keyring.c
+++ b/certs/system_keyring.c
@@ -33,7 +33,11 @@ extern __initconst const unsigned long system_certificate_list_size;
 extern __initconst const unsigned long module_cert_size;
 
 /**
- * restrict_link_to_builtin_trusted - Restrict keyring addition by built in CA
+ * restrict_link_by_builtin_trusted - Restrict keyring addition by built in CA
+ * @dest_keyring: Keyring being linked to.
+ * @type: The type of key being added.
+ * @payload: The payload of the new key.
+ * @restriction_key: A ring of keys that can be used to vouch for the new cert.
  *
  * Restrict the addition of keys into a keyring based on the key-to-be-added
  * being vouched for by a key in the built in system keyring.
@@ -50,7 +54,11 @@ int restrict_link_by_builtin_trusted(struct key *dest_keyring,
 #ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
 /**
  * restrict_link_by_builtin_and_secondary_trusted - Restrict keyring
- *   addition by both builtin and secondary keyrings
+ *   addition by both builtin and secondary keyrings.
+ * @dest_keyring: Keyring being linked to.
+ * @type: The type of key being added.
+ * @payload: The payload of the new key.
+ * @restrict_key: A ring of keys that can be used to vouch for the new cert.
  *
  * Restrict the addition of keys into a keyring based on the key-to-be-added
  * being vouched for by a key in either the built-in or the secondary system
@@ -75,7 +83,7 @@ int restrict_link_by_builtin_and_secondary_trusted(
 					  secondary_trusted_keys);
 }
 
-/**
+/*
  * Allocate a struct key_restriction for the "builtin and secondary trust"
  * keyring. Only for use in system_trusted_keyring_init().
  */
-- 
2.27.0

[PATCH v3 03/10] 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 7a9b084e2043..b4443e507153 100644
--- a/crypto/asymmetric_keys/x509_cert_parser.c
+++ b/crypto/asymmetric_keys/x509_cert_parser.c
@@ -586,6 +586,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->root_ca = true;
+	}
+
 	return 0;
 }
 
diff --git a/crypto/asymmetric_keys/x509_parser.h b/crypto/asymmetric_keys/x509_parser.h
index a299c9c56f40..7c5c0ad1c22e 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		root_ca;		/* T if basic constraints CA is set */
 };
 
 /*
-- 
2.27.0

[PATCH v3 04/10] 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 | 22 ++++++++++++++++++++++
 crypto/asymmetric_keys/x509_parser.h      |  1 +
 2 files changed, 23 insertions(+)

diff --git a/crypto/asymmetric_keys/x509_cert_parser.c b/crypto/asymmetric_keys/x509_cert_parser.c
index b4443e507153..edb22cf04eed 100644
--- a/crypto/asymmetric_keys/x509_cert_parser.c
+++ b/crypto/asymmetric_keys/x509_cert_parser.c
@@ -579,6 +579,28 @@ 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)
+			return -EBADMSG;
+		if (vlen < 4)
+			return -EBADMSG;
+		if (v[1] == 0x02 && v[2] <= 2 && (v[3] & 0x04))
+			ctx->cert->kcs_set = true;
+		else if (vlen > 4 && v[1] == 0x03 && (v[3] & 0x04))
+			ctx->cert->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 7c5c0ad1c22e..74a9f929e400 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		root_ca;		/* T if basic constraints CA is set */
+	bool		kcs_set;		/* T if keyCertSign is set */
 };
 
 /*
-- 
2.27.0

[PATCH v3 05/10] KEYS: Introduce a CA endorsed flag

[Eric Snowberg]

Some subsystems are interested in knowing if a key has been endorsed
as or by a Certificate Authority (CA). From the data contained in struct
key, it is not possible to make this determination after the key
parsing is complete.  Introduce a new Endorsed Certificate Authority
flag called KEY_FLAG_ECA.

The first type of key to use this is X.509.  When a X.509 certificate
has the keyCertSign Key Usage set and contains the CA bit set, this new
flag is set. In the future, other usage fields could be added as flags,
i.e. digitialSignature.

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

diff --git a/crypto/asymmetric_keys/x509_public_key.c b/crypto/asymmetric_keys/x509_public_key.c
index 0b4943a4592b..fd1d7d6e68e7 100644
--- a/crypto/asymmetric_keys/x509_public_key.c
+++ b/crypto/asymmetric_keys/x509_public_key.c
@@ -208,6 +208,9 @@ static int x509_key_preparse(struct key_preparsed_payload *prep)
 		goto error_free_kids;
 	}
 
+	if (cert->kcs_set && cert->root_ca)
+		prep->payload_flags |= KEY_ALLOC_PECA;
+
 	/* We're pinning the module by being linked against it */
 	__module_get(public_key_subtype.owner);
 	prep->payload.data[asym_subtype] = &public_key_subtype;
diff --git a/include/linux/key-type.h b/include/linux/key-type.h
index 7d985a1dfe4a..0b500578441c 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_PECA	0x0001		/* Proposed Endorsed CA (ECA) 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 d27477faf00d..21d5a13ee4a9 100644
--- a/include/linux/key.h
+++ b/include/linux/key.h
@@ -236,6 +236,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_ECA		10	/* set if key is an Endorsed CA key */
 
 	/* the key type and key description string
 	 * - the desc is used to match a key against search criteria
@@ -296,6 +297,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_ECA			0x0040	/* Add Endorsed CA 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..e6b4946aca70 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_ECA)
+		key->flags |= 1 << KEY_FLAG_ECA;
 
 #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_ECA flag to be set by preparser contents */
+	if (prep.payload_flags & KEY_ALLOC_PECA)
+		flags |= KEY_ALLOC_ECA;
+	else
+		flags &= ~KEY_ALLOC_ECA;
+
 	/* 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 v3 06/10] KEYS: Introduce keyring restriction that validates ca 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_ca_and_signature that both vouches for the new key and
validates the vouching key is an endorsed certificate authority.

Two new system keyring restrictions are added to use
restrict_link_by_ca_and_signature.  The first restriction called
restrict_link_by_ca_builtin_trusted  uses the builtin_trusted_keys as
the restricted keyring.  The second system keyring restriction called
restrict_link_by_ca_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 | 41 +++++++++++++++++++++++++++++++
 include/crypto/public_key.h       |  5 ++++
 include/keys/system_keyring.h     | 12 ++++++++-
 4 files changed, 75 insertions(+), 1 deletion(-)

diff --git a/certs/system_keyring.c b/certs/system_keyring.c
index e531b88bc570..0d219b6895aa 100644
--- a/certs/system_keyring.c
+++ b/certs/system_keyring.c
@@ -51,6 +51,14 @@ int restrict_link_by_builtin_trusted(struct key *dest_keyring,
 					  builtin_trusted_keys);
 }
 
+int restrict_link_by_ca_builtin_trusted(struct key *dest_keyring,
+					 const struct key_type *type,
+					 const union key_payload *payload,
+					 struct key *unused)
+{
+	return restrict_link_by_ca_and_signature(dest_keyring, type, payload,
+						  builtin_trusted_keys);
+}
 #ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
 /**
  * restrict_link_by_builtin_and_secondary_trusted - Restrict keyring
@@ -83,6 +91,16 @@ int restrict_link_by_builtin_and_secondary_trusted(
 					  secondary_trusted_keys);
 }
 
+int restrict_link_by_ca_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_ca_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..005cb28969e4 100644
--- a/crypto/asymmetric_keys/restrict.c
+++ b/crypto/asymmetric_keys/restrict.c
@@ -108,6 +108,47 @@ int restrict_link_by_signature(struct key *dest_keyring,
 	return ret;
 }
 
+int restrict_link_by_ca_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_ECA, &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/crypto/public_key.h b/include/crypto/public_key.h
index 6d61695e1cde..e51bbc5ffe17 100644
--- a/include/crypto/public_key.h
+++ b/include/crypto/public_key.h
@@ -71,6 +71,11 @@ extern int restrict_link_by_key_or_keyring_chain(struct key *trust_keyring,
 						 const union key_payload *payload,
 						 struct key *trusted);
 
+extern int restrict_link_by_ca_and_signature(struct key *dest_keyring,
+					      const struct key_type *type,
+					      const union key_payload *payload,
+					      struct key *unused);
+
 extern int query_asymmetric_key(const struct kernel_pkey_params *,
 				struct kernel_pkey_query *);
 
diff --git a/include/keys/system_keyring.h b/include/keys/system_keyring.h
index 91e080efb918..4e94bf72b998 100644
--- a/include/keys/system_keyring.h
+++ b/include/keys/system_keyring.h
@@ -24,9 +24,13 @@ 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_ca_builtin_trusted(struct key *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_ca_builtin_trusted restrict_link_reject
 
 static inline __init int load_module_cert(struct key *keyring)
 {
@@ -41,8 +45,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_ca_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_ca_builtin_and_secondary_trusted restrict_link_by_builtin_trusted
 #endif
 
 #ifdef CONFIG_INTEGRITY_MACHINE_KEYRING
-- 
2.27.0

[PATCH v3 07/10] KEYS: X.509: Flag Intermediate CA certs as endorsed

[Eric Snowberg]

Currently X.509 intermediate certs with the CA flag set to false do not
have the endorsed CA (KEY_FLAG_ECA) set. Allow these intermediate certs to
be added.  Requirements for an intermediate include: Usage extension
defined as keyCertSign, Basic Constrains for CA is false, and the
intermediate cert is signed by a current endorsed CA.

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

diff --git a/crypto/asymmetric_keys/x509_public_key.c b/crypto/asymmetric_keys/x509_public_key.c
index fd1d7d6e68e7..75699987a6b1 100644
--- a/crypto/asymmetric_keys/x509_public_key.c
+++ b/crypto/asymmetric_keys/x509_public_key.c
@@ -208,8 +208,18 @@ static int x509_key_preparse(struct key_preparsed_payload *prep)
 		goto error_free_kids;
 	}
 
-	if (cert->kcs_set && cert->root_ca)
-		prep->payload_flags |= KEY_ALLOC_PECA;
+	if (cert->kcs_set) {
+		if (cert->root_ca)
+			prep->payload_flags |= KEY_ALLOC_PECA;
+		/*
+		 * In this case it could be an Intermediate CA.  Set
+		 * KEY_MAYBE_PECA for now.  If the restriction check
+		 * passes later, the key will be allocated with the
+		 * correct CA flag
+		 */
+		else
+			prep->payload_flags |= KEY_MAYBE_PECA;
+	}
 
 	/* We're pinning the module by being linked against it */
 	__module_get(public_key_subtype.owner);
diff --git a/include/linux/ima.h b/include/linux/ima.h
index 81708ca0ebc7..6597081b6b1a 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
@@ -181,6 +182,16 @@ 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_ca restrict_link_by_ca_builtin_and_secondary_trusted
+#else
+#define ima_validate_builtin_ca restrict_link_by_ca_builtin_trusted
+#endif
+#else
+#define ima_validate_builtin_ca restrict_link_reject
+#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 0b500578441c..0d2f95f6b8a1 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_PECA	0x0001		/* Proposed Endorsed CA (ECA) key */
+#define KEY_MAYBE_PECA	0x0002		/* Proposed possible ECA 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 e6b4946aca70..69d5f143683f 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 endorsed ca */
+	if ((prep.payload_flags & KEY_MAYBE_PECA) &&
+	   !(ima_validate_builtin_ca(keyring, index_key.type, &prep.payload, NULL)))
+		prep.payload_flags |= KEY_ALLOC_PECA;
+
 	/* 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 v3 08/10] 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
endorsed as or by a CA. The CA qualifications include having the CA bit
and the keyCertSign KeyUsage bit set. Or they could be validated by a
properly formed intermediate certificate as long as it was signed by a
qualifying CA.  Currently these restrictions are not enforced. Use the
new restrict_link_by_ca_builtin_and_secondary_trusted and
restrict_link_by_ca_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 ++--
 security/integrity/ima/Kconfig | 6 +++---
 3 files changed, 5 insertions(+), 6 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 8a82a6c7f48a..1fe8d1ed6e0b 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_ca_builtin_and_secondary_trusted
 #else
-#define restrict_link_to_ima restrict_link_by_builtin_trusted
+#define restrict_link_to_ima restrict_link_by_ca_builtin_trusted
 #endif
 
 static struct key *integrity_keyring_from_id(const unsigned int id)
diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
index 7249f16257c7..6fe3bd0e5c82 100644
--- a/security/integrity/ima/Kconfig
+++ b/security/integrity/ima/Kconfig
@@ -269,13 +269,13 @@ config IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY
 	default n
 	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.
+	  key is validly signed by a CA cert in the system built-in,
+	  secondary trusted, or machine 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.
+	  built-in, secondary trusted or machine keyrings.
 
 config IMA_BLACKLIST_KEYRING
 	bool "Create IMA machine owner blacklist keyrings (EXPERIMENTAL)"
-- 
2.27.0

[PATCH v3 09/10] KEYS: CA link restriction

[Eric Snowberg]

Add a new link restriction.  Restrict the addition of keys in a keyring
based on the key to be added being a CA.

Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
---
 crypto/asymmetric_keys/restrict.c        | 35 ++++++++++++++++++++++++
 crypto/asymmetric_keys/x509_public_key.c |  5 +++-
 include/crypto/public_key.h              | 16 +++++++++++
 3 files changed, 55 insertions(+), 1 deletion(-)

diff --git a/crypto/asymmetric_keys/restrict.c b/crypto/asymmetric_keys/restrict.c
index 005cb28969e4..ca305ba1c0b5 100644
--- a/crypto/asymmetric_keys/restrict.c
+++ b/crypto/asymmetric_keys/restrict.c
@@ -108,6 +108,41 @@ int restrict_link_by_signature(struct key *dest_keyring,
 	return ret;
 }
 
+/**
+ * restrict_link_by_ca - Restrict additions to a ring of CA keys
+ * @dest_keyring: Keyring being linked to.
+ * @type: The type of key being added.
+ * @payload: The payload of the new key.
+ * @trust_keyring: Unused.
+ *
+ * Check if the new certificate is a CA. If it is a CA, then mark the new
+ * certificate as being ok to link.
+ *
+ * Returns 0 if the new certificate was accepted, -ENOKEY if the
+ * certificate is not a CA. -ENOPKG if the signature uses unsupported
+ * crypto, or some other error if there is a matching certificate but
+ * the signature check cannot be performed.
+ */
+int restrict_link_by_ca(struct key *dest_keyring,
+			const struct key_type *type,
+			const union key_payload *payload,
+			struct key *trust_keyring)
+{
+	const struct public_key *pkey;
+
+	if (type != &key_type_asymmetric)
+		return -EOPNOTSUPP;
+
+	pkey = payload->data[asym_crypto];
+	if (!pkey)
+		return -ENOPKG;
+
+	if (!pkey->key_is_ca)
+		return -ENOKEY;
+
+	return 0;
+}
+
 int restrict_link_by_ca_and_signature(struct key *dest_keyring,
 				       const struct key_type *type,
 				       const union key_payload *payload,
diff --git a/crypto/asymmetric_keys/x509_public_key.c b/crypto/asymmetric_keys/x509_public_key.c
index 75699987a6b1..88c6e9829224 100644
--- a/crypto/asymmetric_keys/x509_public_key.c
+++ b/crypto/asymmetric_keys/x509_public_key.c
@@ -209,8 +209,11 @@ static int x509_key_preparse(struct key_preparsed_payload *prep)
 	}
 
 	if (cert->kcs_set) {
-		if (cert->root_ca)
+		if (cert->root_ca) {
 			prep->payload_flags |= KEY_ALLOC_PECA;
+			cert->pub->key_is_ca = true;
+		}
+
 		/*
 		 * In this case it could be an Intermediate CA.  Set
 		 * KEY_MAYBE_PECA for now.  If the restriction check
diff --git a/include/crypto/public_key.h b/include/crypto/public_key.h
index e51bbc5ffe17..3de0f8a68914 100644
--- a/include/crypto/public_key.h
+++ b/include/crypto/public_key.h
@@ -26,6 +26,7 @@ struct public_key {
 	void *params;
 	u32 paramlen;
 	bool key_is_private;
+	bool key_is_ca;
 	const char *id_type;
 	const char *pkey_algo;
 };
@@ -76,6 +77,21 @@ extern int restrict_link_by_ca_and_signature(struct key *dest_keyring,
 					      const union key_payload *payload,
 					      struct key *unused);
 
+#if IS_REACHABLE(CONFIG_ASYMMETRIC_KEY_TYPE)
+extern int restrict_link_by_ca(struct key *dest_keyring,
+			       const struct key_type *type,
+			       const union key_payload *payload,
+			       struct key *trust_keyring);
+#else
+static inline int restrict_link_by_ca(struct key *dest_keyring,
+				      const struct key_type *type,
+				      const union key_payload *payload,
+				      struct key *trust_keyring)
+{
+	return 0;
+}
+#endif
+
 extern int query_asymmetric_key(const struct kernel_pkey_params *,
 				struct kernel_pkey_query *);
 
-- 
2.27.0

[PATCH v3 10/10] integrity: restrict INTEGRITY_KEYRING_MACHINE to restrict_link_by_ca

[Eric Snowberg]

Set the restriction check for INTEGRITY_KEYRING_MACHINE keys to
restrict_link_by_ca.  This will only allow CA keys into the machine
keyring.

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

diff --git a/security/integrity/Kconfig b/security/integrity/Kconfig
index 14cc3c767270..3357883fa5a8 100644
--- a/security/integrity/Kconfig
+++ b/security/integrity/Kconfig
@@ -74,6 +74,16 @@ config INTEGRITY_MACHINE_KEYRING
 	 in the platform keyring, keys contained in the .machine keyring will
 	 be trusted within the kernel.
 
+config INTEGRITY_CA_MACHINE_KEYRING
+	bool "Only allow CA keys into the Machine Keyring"
+	depends on INTEGRITY_MACHINE_KEYRING
+	help
+	 If set, only Machine Owner Keys (MOK) that are Certificate
+	 Authority (CA) keys will be added to the .machine keyring. All
+	 other MOK keys will be added to the .platform keyring.  After
+	 booting, any other key signed by the CA key can be added to the
+	 secondary_trusted_keys keyring.
+
 config LOAD_UEFI_KEYS
        depends on INTEGRITY_PLATFORM_KEYRING
        depends on EFI
diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
index 1fe8d1ed6e0b..b0ec615745e3 100644
--- a/security/integrity/digsig.c
+++ b/security/integrity/digsig.c
@@ -131,7 +131,8 @@ int __init integrity_init_keyring(const unsigned int id)
 		| KEY_USR_READ | KEY_USR_SEARCH;
 
 	if (id == INTEGRITY_KEYRING_PLATFORM ||
-	    id == INTEGRITY_KEYRING_MACHINE) {
+	    (id == INTEGRITY_KEYRING_MACHINE &&
+	    !IS_ENABLED(CONFIG_INTEGRITY_CA_MACHINE_KEYRING))) {
 		restriction = NULL;
 		goto out;
 	}
@@ -143,7 +144,10 @@ int __init integrity_init_keyring(const unsigned int id)
 	if (!restriction)
 		return -ENOMEM;
 
-	restriction->check = restrict_link_to_ima;
+	if (id == INTEGRITY_KEYRING_MACHINE)
+		restriction->check = restrict_link_by_ca;
+	else
+		restriction->check = restrict_link_to_ima;
 
 	/*
 	 * MOK keys can only be added through a read-only runtime services
-- 
2.27.0

Re: [PATCH v3 00/10] Add CA enforcement keyring restrictions

[Mimi Zohar]

On Tue, 2022-12-13 at 19:33 -0500, Eric Snowberg wrote:
> Prior to the introduction of the machine keyring, most distros simply 
> allowed all keys contained within the platform keyring to be used
> for both kernel and module verification.  This was done by an out of
> tree patch.  Some distros took it even further and loaded all these keys
> into the secondary trusted keyring.  This also allowed the system owner 
> to add their own key for IMA usage.
> 
> Each distro contains similar documentation on how to sign kernel modules
> and enroll the key into the MOK.  The process is fairly straightforward.
> With the introduction of the machine keyring, the process remains
> basically the same, without the need for any out of tree patches.
> 
> The machine keyring allowed distros to eliminate the out of tree patches
> for kernel module signing.  However, it falls short in allowing the end 
> user to add their own keys for IMA. Currently 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.  This would expand the current 
> integrity gap. The IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY 
> Kconfig states that keys may be added to the ima keyrings if the key is 
> validly signed by a CA cert in the system built-in or secondary trusted 
> keyring.  Currently there is not code that enforces the contents of a
> CA cert. Any key in the builtin or secondary keyring can be used. 
> 
> To allow IMA to be enabled with the machine keyring, this series introduces
> enforcement of key usage in the certificate. This series also applies
> this enforcement across all kernel keyrings.
> 
> The machine keyring shares  similarities with both the builtin and
> secondary keyrings.  Similar to the builtin, no keys may be added to the
> machine keyring following boot. The secondary keyring allows user
> provided keys to be added following boot; however, a previously enrolled
> kernel key must vouch for the key before it may be included. The system
> owner may include their own keys into the machine keyring prior to boot.
> If the end-user is not the system owner, they may not add their own keys
> to the machine keyring.  
> 
> The machine keyring is only populated when Secure Boot is enabled.  A
> system owner has the ability to control the entire Secure Boot keychain
> (PK, KEK, DB, and DBX).  The system owner can also turn Secure Boot off.
> With this control, they may use insert-sys-cert to include their own key 
> and re-sign their kernel and have it boot.  The system owner also has 
> control to include or exclude MOK keys. This series does not try to 
> interpret how a system owner has configured their machine.  If the system 
> owner has taken the steps to add their own MOK keys, they will be 
> included in the machine keyring and used for verification, exactly 
> the same way as keys contained in the builtin and secondary keyrings.
> Since the system owner has the ability to add keys before booting to
> either the machine or builtin keyrings, it is viewed as inconsequential 
> if the key originated from one or the other.
> 
> This series introduces two different ways to configure the machine keyring.
> By default, nothing changes and all MOK keys are loaded into it.  Whenever
> a CA cert is found within the machine, builtin, or secondary, a flag 
> indicating this is stored in the public key struct.  The other option is 
> if the new Kconfig INTEGRITY_CA_MACHINE_KEYRING is enabled, only CA certs 
> will be loaded into the machine keyring. All remaining MOK keys will be 
> loaded into the platform keyring.
> 
> A CA cert shall be defined as any X509 certificate that contains the 
> keyCertSign key usage and has the CA bit set to true.

Hi Eric,

Allowing CA certificates with the digitalSignature key usage flag
enabled defeats the purpose of the new Kconfig.  Please update the
above definition to exclude the digitalSignature key usage flag and
modify the code accordingly.

thanks,

Mimi

> With this series applied, CA enforcement is in place whenever 
> IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY is enabled.  Meaning, 
> before any key can be included into the ima keyring, it must be
> vouched for by a CA key contained within the builtin, secondary, or 
> machine keyrings.
> 
> IMA allows userspace applications to be signed. The enduser may sign
> their own application, however they may also want to use an application
> provided by a 3rd party.  The entity building the kernel, may not be the
> same entity building the userspace program.  The system owner may also
> be a third entity.  If the system owner trusts the entity building the
> userspace program, they will include their public key within the MOK.
> This key would be used to sign the key added to the ima keyring. Not all
> 3rd party userspace providers have the capability to properly manage a
> root CA.  Some may outsource to a different code signing provider.  Many
> code signing providers use Intermediate CA certificates. Therefore, this
> series also includes support for Intermediate CA certificates.
> 
> This series could be broken up into 3 different parts.  The first two
> patches could be taken now.  They solve current issues that will be
> triggered by the build robots.  Patches 3-8 add CA enforcement for the
> ima keyring.  Patches 9-10 restrict the machine keyring to only load CA
> certs into it.  Patches 9-10 require all the previous patches. 
> 
> Changelog:
> 
> v3:
> - Allow Intermediate CA certs to be enrolled through the MOK. The
>   Intermediate CA cert must contain keyCertSign key usage and have the 
>   CA bit set to true. This was done by removing the self signed
>   requirement.
> 
> 
> Eric Snowberg (10):
>   KEYS: Create static version of public_key_verify_signature
>   KEYS: Add missing function documentation
>   KEYS: X.509: Parse Basic Constraints for CA
>   KEYS: X.509: Parse Key Usage
>   KEYS: Introduce a CA endorsed flag
>   KEYS: Introduce keyring restriction that validates ca trust
>   KEYS: X.509: Flag Intermediate CA certs as endorsed
>   integrity: Use root of trust signature restriction
>   KEYS: CA link restriction
>   integrity: restrict INTEGRITY_KEYRING_MACHINE to restrict_link_by_ca
> 
>  certs/system_keyring.c                    | 32 +++++++++-
>  crypto/asymmetric_keys/restrict.c         | 76 +++++++++++++++++++++++
>  crypto/asymmetric_keys/x509_cert_parser.c | 31 +++++++++
>  crypto/asymmetric_keys/x509_parser.h      |  2 +
>  crypto/asymmetric_keys/x509_public_key.c  | 16 +++++
>  include/crypto/public_key.h               | 30 +++++++++
>  include/keys/system_keyring.h             | 12 +++-
>  include/linux/ima.h                       | 11 ++++
>  include/linux/key-type.h                  |  3 +
>  include/linux/key.h                       |  2 +
>  security/integrity/Kconfig                | 11 +++-
>  security/integrity/digsig.c               | 12 ++--
>  security/integrity/ima/Kconfig            |  6 +-
>  security/keys/key.c                       | 13 ++++
>  14 files changed, 245 insertions(+), 12 deletions(-)
> 
> 
> base-commit: 830b3c68c1fb1e9176028d02ef86f3cf76aa2476

Re: [PATCH v3 07/10] KEYS: X.509: Flag Intermediate CA certs as endorsed

[Mimi Zohar]

Hi Eric,

On Tue, 2022-12-13 at 19:33 -0500, Eric Snowberg wrote:
> Currently X.509 intermediate certs with the CA flag set to false do not
> have the endorsed CA (KEY_FLAG_ECA) set. Allow these intermediate certs to
> be added.  Requirements for an intermediate include: Usage extension
> defined as keyCertSign, Basic Constrains for CA is false, and the
> intermediate cert is signed by a current endorsed CA.

Intermediary keys should have the CA flag enabled as well.   Why is
this needed?    At least for the new Kconfig, please keep it simple as
to which certificates may be added to the machine keyring.

thanks,

Mimi

Re: [PATCH v3 03/10] KEYS: X.509: Parse Basic Constraints for CA

[Mimi Zohar]

> diff --git a/crypto/asymmetric_keys/x509_parser.h b/crypto/asymmetric_keys/x509_parser.h
> index a299c9c56f40..7c5c0ad1c22e 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		root_ca;		/* T if basic constraints CA is set */
>  }; 

The variable "root_ca" should probably be renamed to just "ca", right?

-- 

thanks,

Mimi

Re: [PATCH v3 04/10] KEYS: X.509: Parse Key Usage

[Mimi Zohar]

On Tue, 2022-12-13 at 19:33 -0500, 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.

Either in this patch or separately, the "digitalSignature" key usage
flag needs to be saved.

> 
> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
> ---
>  crypto/asymmetric_keys/x509_cert_parser.c | 22 ++++++++++++++++++++++
>  crypto/asymmetric_keys/x509_parser.h      |  1 +
>  2 files changed, 23 insertions(+)
> 
> diff --git a/crypto/asymmetric_keys/x509_cert_parser.c b/crypto/asymmetric_keys/x509_cert_parser.c
> index b4443e507153..edb22cf04eed 100644
> --- a/crypto/asymmetric_keys/x509_cert_parser.c
> +++ b/crypto/asymmetric_keys/x509_cert_parser.c
> @@ -579,6 +579,28 @@ 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)
> +			return -EBADMSG;
> +		if (vlen < 4)
> +			return -EBADMSG;
> +		if (v[1] == 0x02 && v[2] <= 2 && (v[3] & 0x04))
> +			ctx->cert->kcs_set = true;
> +		else if (vlen > 4 && v[1] == 0x03 && (v[3] & 0x04))
> +			ctx->cert->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 7c5c0ad1c22e..74a9f929e400 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		root_ca;		/* T if basic constraints CA is set */
> +	bool		kcs_set;		/* T if keyCertSign is set */

Using acronyms as variable names makes reviewing code more difficult. 
Perhaps rename "kcs_set" to either "key_cert_sign" or "keycertsign".

>  };
>  
>  /*

-- 
thanks,

Mimi

Re: [PATCH v3 00/10] Add CA enforcement keyring restrictions

[Eric Snowberg]

> On Dec 15, 2022, at 3:21 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> 
> On Tue, 2022-12-13 at 19:33 -0500, Eric Snowberg wrote:
>> Prior to the introduction of the machine keyring, most distros simply 
>> allowed all keys contained within the platform keyring to be used
>> for both kernel and module verification.  This was done by an out of
>> tree patch.  Some distros took it even further and loaded all these keys
>> into the secondary trusted keyring.  This also allowed the system owner 
>> to add their own key for IMA usage.
>> 
>> Each distro contains similar documentation on how to sign kernel modules
>> and enroll the key into the MOK.  The process is fairly straightforward.
>> With the introduction of the machine keyring, the process remains
>> basically the same, without the need for any out of tree patches.
>> 
>> The machine keyring allowed distros to eliminate the out of tree patches
>> for kernel module signing.  However, it falls short in allowing the end 
>> user to add their own keys for IMA. Currently 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.  This would expand the current 
>> integrity gap. The IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY 
>> Kconfig states that keys may be added to the ima keyrings if the key is 
>> validly signed by a CA cert in the system built-in or secondary trusted 
>> keyring.  Currently there is not code that enforces the contents of a
>> CA cert. Any key in the builtin or secondary keyring can be used. 
>> 
>> To allow IMA to be enabled with the machine keyring, this series introduces
>> enforcement of key usage in the certificate. This series also applies
>> this enforcement across all kernel keyrings.
>> 
>> The machine keyring shares  similarities with both the builtin and
>> secondary keyrings.  Similar to the builtin, no keys may be added to the
>> machine keyring following boot. The secondary keyring allows user
>> provided keys to be added following boot; however, a previously enrolled
>> kernel key must vouch for the key before it may be included. The system
>> owner may include their own keys into the machine keyring prior to boot.
>> If the end-user is not the system owner, they may not add their own keys
>> to the machine keyring.  
>> 
>> The machine keyring is only populated when Secure Boot is enabled.  A
>> system owner has the ability to control the entire Secure Boot keychain
>> (PK, KEK, DB, and DBX).  The system owner can also turn Secure Boot off.
>> With this control, they may use insert-sys-cert to include their own key 
>> and re-sign their kernel and have it boot.  The system owner also has 
>> control to include or exclude MOK keys. This series does not try to 
>> interpret how a system owner has configured their machine.  If the system 
>> owner has taken the steps to add their own MOK keys, they will be 
>> included in the machine keyring and used for verification, exactly 
>> the same way as keys contained in the builtin and secondary keyrings.
>> Since the system owner has the ability to add keys before booting to
>> either the machine or builtin keyrings, it is viewed as inconsequential 
>> if the key originated from one or the other.
>> 
>> This series introduces two different ways to configure the machine keyring.
>> By default, nothing changes and all MOK keys are loaded into it.  Whenever
>> a CA cert is found within the machine, builtin, or secondary, a flag 
>> indicating this is stored in the public key struct.  The other option is 
>> if the new Kconfig INTEGRITY_CA_MACHINE_KEYRING is enabled, only CA certs 
>> will be loaded into the machine keyring. All remaining MOK keys will be 
>> loaded into the platform keyring.
>> 
>> A CA cert shall be defined as any X509 certificate that contains the 
>> keyCertSign key usage and has the CA bit set to true.
> 
> Hi Eric,
> 
> Allowing CA certificates with the digitalSignature key usage flag
> enabled defeats the purpose of the new Kconfig.  Please update the
> above definition to exclude the digitalSignature key usage flag and
> modify the code accordingly.

Within v2, the request was made to allow Intermediate CA certificates to be 
loaded directly.  The Intermediate CA referenced was the one used by kernel.org.  
This Intermediate CA contains both digitalSignature and keyCertSign.  If the code 
is changed to exclude this certificate, now the root CA has to be loaded again.  Is that 
the intent?

Re: [PATCH v3 00/10] Add CA enforcement keyring restrictions

[Mimi Zohar]

On Thu, 2022-12-15 at 16:26 +0000, Eric Snowberg wrote:
> 
> > On Dec 15, 2022, at 3:21 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> > 
> > On Tue, 2022-12-13 at 19:33 -0500, Eric Snowberg wrote:
> >> Prior to the introduction of the machine keyring, most distros simply 
> >> allowed all keys contained within the platform keyring to be used
> >> for both kernel and module verification.  This was done by an out of
> >> tree patch.  Some distros took it even further and loaded all these keys
> >> into the secondary trusted keyring.  This also allowed the system owner 
> >> to add their own key for IMA usage.
> >> 
> >> Each distro contains similar documentation on how to sign kernel modules
> >> and enroll the key into the MOK.  The process is fairly straightforward.
> >> With the introduction of the machine keyring, the process remains
> >> basically the same, without the need for any out of tree patches.
> >> 
> >> The machine keyring allowed distros to eliminate the out of tree patches
> >> for kernel module signing.  However, it falls short in allowing the end 
> >> user to add their own keys for IMA. Currently 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.  This would expand the current 
> >> integrity gap. The IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY 
> >> Kconfig states that keys may be added to the ima keyrings if the key is 
> >> validly signed by a CA cert in the system built-in or secondary trusted 
> >> keyring.  Currently there is not code that enforces the contents of a
> >> CA cert. Any key in the builtin or secondary keyring can be used. 
> >> 
> >> To allow IMA to be enabled with the machine keyring, this series introduces
> >> enforcement of key usage in the certificate. This series also applies
> >> this enforcement across all kernel keyrings.
> >> 
> >> The machine keyring shares  similarities with both the builtin and
> >> secondary keyrings.  Similar to the builtin, no keys may be added to the
> >> machine keyring following boot. The secondary keyring allows user
> >> provided keys to be added following boot; however, a previously enrolled
> >> kernel key must vouch for the key before it may be included. The system
> >> owner may include their own keys into the machine keyring prior to boot.
> >> If the end-user is not the system owner, they may not add their own keys
> >> to the machine keyring.  
> >> 
> >> The machine keyring is only populated when Secure Boot is enabled.  A
> >> system owner has the ability to control the entire Secure Boot keychain
> >> (PK, KEK, DB, and DBX).  The system owner can also turn Secure Boot off.
> >> With this control, they may use insert-sys-cert to include their own key 
> >> and re-sign their kernel and have it boot.  The system owner also has 
> >> control to include or exclude MOK keys. This series does not try to 
> >> interpret how a system owner has configured their machine.  If the system 
> >> owner has taken the steps to add their own MOK keys, they will be 
> >> included in the machine keyring and used for verification, exactly 
> >> the same way as keys contained in the builtin and secondary keyrings.
> >> Since the system owner has the ability to add keys before booting to
> >> either the machine or builtin keyrings, it is viewed as inconsequential 
> >> if the key originated from one or the other.
> >> 
> >> This series introduces two different ways to configure the machine keyring.
> >> By default, nothing changes and all MOK keys are loaded into it.  Whenever
> >> a CA cert is found within the machine, builtin, or secondary, a flag 
> >> indicating this is stored in the public key struct.  The other option is 
> >> if the new Kconfig INTEGRITY_CA_MACHINE_KEYRING is enabled, only CA certs 
> >> will be loaded into the machine keyring. All remaining MOK keys will be 
> >> loaded into the platform keyring.
> >> 
> >> A CA cert shall be defined as any X509 certificate that contains the 
> >> keyCertSign key usage and has the CA bit set to true.
> > 
> > Hi Eric,
> > 
> > Allowing CA certificates with the digitalSignature key usage flag
> > enabled defeats the purpose of the new Kconfig.  Please update the
> > above definition to exclude the digitalSignature key usage flag and
> > modify the code accordingly.
> 
> Within v2, the request was made to allow Intermediate CA certificates to be 
> loaded directly.  The Intermediate CA referenced was the one used by kernel.org.  
> This Intermediate CA contains both digitalSignature and keyCertSign.  If the code 
> is changed to exclude this certificate, now the root CA has to be loaded again.  Is that 
> the intent?

That definitely was not the intent.  Nor would it address the issue of
a particular intermediate CA certificate having both keyCertSign and
digitalSignature.

thanks,

Mimi

Re: [PATCH v3 00/10] Add CA enforcement keyring restrictions

[Eric Snowberg]

> On Dec 15, 2022, at 12:58 PM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> 
> On Thu, 2022-12-15 at 16:26 +0000, Eric Snowberg wrote:
>> 
>>> On Dec 15, 2022, at 3:21 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
>>> 
>>> On Tue, 2022-12-13 at 19:33 -0500, Eric Snowberg wrote:
>>>> Prior to the introduction of the machine keyring, most distros simply 
>>>> allowed all keys contained within the platform keyring to be used
>>>> for both kernel and module verification.  This was done by an out of
>>>> tree patch.  Some distros took it even further and loaded all these keys
>>>> into the secondary trusted keyring.  This also allowed the system owner 
>>>> to add their own key for IMA usage.
>>>> 
>>>> Each distro contains similar documentation on how to sign kernel modules
>>>> and enroll the key into the MOK.  The process is fairly straightforward.
>>>> With the introduction of the machine keyring, the process remains
>>>> basically the same, without the need for any out of tree patches.
>>>> 
>>>> The machine keyring allowed distros to eliminate the out of tree patches
>>>> for kernel module signing.  However, it falls short in allowing the end 
>>>> user to add their own keys for IMA. Currently 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.  This would expand the current 
>>>> integrity gap. The IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY 
>>>> Kconfig states that keys may be added to the ima keyrings if the key is 
>>>> validly signed by a CA cert in the system built-in or secondary trusted 
>>>> keyring.  Currently there is not code that enforces the contents of a
>>>> CA cert. Any key in the builtin or secondary keyring can be used. 
>>>> 
>>>> To allow IMA to be enabled with the machine keyring, this series introduces
>>>> enforcement of key usage in the certificate. This series also applies
>>>> this enforcement across all kernel keyrings.
>>>> 
>>>> The machine keyring shares  similarities with both the builtin and
>>>> secondary keyrings.  Similar to the builtin, no keys may be added to the
>>>> machine keyring following boot. The secondary keyring allows user
>>>> provided keys to be added following boot; however, a previously enrolled
>>>> kernel key must vouch for the key before it may be included. The system
>>>> owner may include their own keys into the machine keyring prior to boot.
>>>> If the end-user is not the system owner, they may not add their own keys
>>>> to the machine keyring.  
>>>> 
>>>> The machine keyring is only populated when Secure Boot is enabled.  A
>>>> system owner has the ability to control the entire Secure Boot keychain
>>>> (PK, KEK, DB, and DBX).  The system owner can also turn Secure Boot off.
>>>> With this control, they may use insert-sys-cert to include their own key 
>>>> and re-sign their kernel and have it boot.  The system owner also has 
>>>> control to include or exclude MOK keys. This series does not try to 
>>>> interpret how a system owner has configured their machine.  If the system 
>>>> owner has taken the steps to add their own MOK keys, they will be 
>>>> included in the machine keyring and used for verification, exactly 
>>>> the same way as keys contained in the builtin and secondary keyrings.
>>>> Since the system owner has the ability to add keys before booting to
>>>> either the machine or builtin keyrings, it is viewed as inconsequential 
>>>> if the key originated from one or the other.
>>>> 
>>>> This series introduces two different ways to configure the machine keyring.
>>>> By default, nothing changes and all MOK keys are loaded into it.  Whenever
>>>> a CA cert is found within the machine, builtin, or secondary, a flag 
>>>> indicating this is stored in the public key struct.  The other option is 
>>>> if the new Kconfig INTEGRITY_CA_MACHINE_KEYRING is enabled, only CA certs 
>>>> will be loaded into the machine keyring. All remaining MOK keys will be 
>>>> loaded into the platform keyring.
>>>> 
>>>> A CA cert shall be defined as any X509 certificate that contains the 
>>>> keyCertSign key usage and has the CA bit set to true.
>>> 
>>> Hi Eric,
>>> 
>>> Allowing CA certificates with the digitalSignature key usage flag
>>> enabled defeats the purpose of the new Kconfig.  Please update the
>>> above definition to exclude the digitalSignature key usage flag and
>>> modify the code accordingly.
>> 
>> Within v2, the request was made to allow Intermediate CA certificates to be 
>> loaded directly.  The Intermediate CA referenced was the one used by kernel.org.  
>> This Intermediate CA contains both digitalSignature and keyCertSign.  If the code 
>> is changed to exclude this certificate, now the root CA has to be loaded again.  Is that 
>> the intent?
> 
> That definitely was not the intent.  Nor would it address the issue of
> a particular intermediate CA certificate having both keyCertSign and
> digitalSignature.

Sorry, I’m not following.  Why is it an issue that an intermediate CA certificate contains 
both keyCertSign and digitalSignature? Why would we want to exclude an Intermediate
CA cert like the one used on kernel.org?

Re: [PATCH v3 00/10] Add CA enforcement keyring restrictions

[Mimi Zohar]

On Thu, 2022-12-15 at 20:28 +0000, Eric Snowberg wrote:
> 
> > On Dec 15, 2022, at 12:58 PM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> > 
> > On Thu, 2022-12-15 at 16:26 +0000, Eric Snowberg wrote:
> >> 
> >>> On Dec 15, 2022, at 3:21 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> >>> 
> >>> On Tue, 2022-12-13 at 19:33 -0500, Eric Snowberg wrote:
> >>>> Prior to the introduction of the machine keyring, most distros simply 
> >>>> allowed all keys contained within the platform keyring to be used
> >>>> for both kernel and module verification.  This was done by an out of
> >>>> tree patch.  Some distros took it even further and loaded all these keys
> >>>> into the secondary trusted keyring.  This also allowed the system owner 
> >>>> to add their own key for IMA usage.
> >>>> 
> >>>> Each distro contains similar documentation on how to sign kernel modules
> >>>> and enroll the key into the MOK.  The process is fairly straightforward.
> >>>> With the introduction of the machine keyring, the process remains
> >>>> basically the same, without the need for any out of tree patches.
> >>>> 
> >>>> The machine keyring allowed distros to eliminate the out of tree patches
> >>>> for kernel module signing.  However, it falls short in allowing the end 
> >>>> user to add their own keys for IMA. Currently 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.  This would expand the current 
> >>>> integrity gap. The IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY 
> >>>> Kconfig states that keys may be added to the ima keyrings if the key is 
> >>>> validly signed by a CA cert in the system built-in or secondary trusted 
> >>>> keyring.  Currently there is not code that enforces the contents of a
> >>>> CA cert. Any key in the builtin or secondary keyring can be used. 
> >>>> 
> >>>> To allow IMA to be enabled with the machine keyring, this series introduces
> >>>> enforcement of key usage in the certificate. This series also applies
> >>>> this enforcement across all kernel keyrings.
> >>>> 
> >>>> The machine keyring shares  similarities with both the builtin and
> >>>> secondary keyrings.  Similar to the builtin, no keys may be added to the
> >>>> machine keyring following boot. The secondary keyring allows user
> >>>> provided keys to be added following boot; however, a previously enrolled
> >>>> kernel key must vouch for the key before it may be included. The system
> >>>> owner may include their own keys into the machine keyring prior to boot.
> >>>> If the end-user is not the system owner, they may not add their own keys
> >>>> to the machine keyring.  
> >>>> 
> >>>> The machine keyring is only populated when Secure Boot is enabled.  A
> >>>> system owner has the ability to control the entire Secure Boot keychain
> >>>> (PK, KEK, DB, and DBX).  The system owner can also turn Secure Boot off.
> >>>> With this control, they may use insert-sys-cert to include their own key 
> >>>> and re-sign their kernel and have it boot.  The system owner also has 
> >>>> control to include or exclude MOK keys. This series does not try to 
> >>>> interpret how a system owner has configured their machine.  If the system 
> >>>> owner has taken the steps to add their own MOK keys, they will be 
> >>>> included in the machine keyring and used for verification, exactly 
> >>>> the same way as keys contained in the builtin and secondary keyrings.
> >>>> Since the system owner has the ability to add keys before booting to
> >>>> either the machine or builtin keyrings, it is viewed as inconsequential 
> >>>> if the key originated from one or the other.
> >>>> 
> >>>> This series introduces two different ways to configure the machine keyring.
> >>>> By default, nothing changes and all MOK keys are loaded into it.  Whenever
> >>>> a CA cert is found within the machine, builtin, or secondary, a flag 
> >>>> indicating this is stored in the public key struct.  The other option is 
> >>>> if the new Kconfig INTEGRITY_CA_MACHINE_KEYRING is enabled, only CA certs 
> >>>> will be loaded into the machine keyring. All remaining MOK keys will be 
> >>>> loaded into the platform keyring.
> >>>> 
> >>>> A CA cert shall be defined as any X509 certificate that contains the 
> >>>> keyCertSign key usage and has the CA bit set to true.
> >>> 
> >>> Hi Eric,
> >>> 
> >>> Allowing CA certificates with the digitalSignature key usage flag
> >>> enabled defeats the purpose of the new Kconfig.  Please update the
> >>> above definition to exclude the digitalSignature key usage flag and
> >>> modify the code accordingly.
> >> 
> >> Within v2, the request was made to allow Intermediate CA certificates to be 
> >> loaded directly.  The Intermediate CA referenced was the one used by kernel.org.  
> >> This Intermediate CA contains both digitalSignature and keyCertSign.  If the code 
> >> is changed to exclude this certificate, now the root CA has to be loaded again.  Is that 
> >> the intent?
> > 
> > That definitely was not the intent.  Nor would it address the issue of
> > a particular intermediate CA certificate having both keyCertSign and
> > digitalSignature.
> 
> Sorry, I’m not following.  Why is it an issue that an intermediate CA certificate contains 
> both keyCertSign and digitalSignature? Why would we want to exclude an Intermediate
> CA cert like the one used on kernel.org?

I must be missing something.  Isn't the purpose of "keyUsage" to
minimize how a certificate may be used?   Why would we want the same
certificate to be used for both certificate signing and code signing?

thanks,

Mimi

Re: [PATCH v3 00/10] Add CA enforcement keyring restrictions

[Eric Snowberg]

> On Dec 15, 2022, at 2:03 PM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> 
> On Thu, 2022-12-15 at 20:28 +0000, Eric Snowberg wrote:
>> 
>>> On Dec 15, 2022, at 12:58 PM, Mimi Zohar <zohar@linux.ibm.com> wrote:
>>> 
>>> On Thu, 2022-12-15 at 16:26 +0000, Eric Snowberg wrote:
>>>> 
>>>>> On Dec 15, 2022, at 3:21 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
>>>>> 
>>>>> On Tue, 2022-12-13 at 19:33 -0500, Eric Snowberg wrote:
>>>>>> Prior to the introduction of the machine keyring, most distros simply 
>>>>>> allowed all keys contained within the platform keyring to be used
>>>>>> for both kernel and module verification.  This was done by an out of
>>>>>> tree patch.  Some distros took it even further and loaded all these keys
>>>>>> into the secondary trusted keyring.  This also allowed the system owner 
>>>>>> to add their own key for IMA usage.
>>>>>> 
>>>>>> Each distro contains similar documentation on how to sign kernel modules
>>>>>> and enroll the key into the MOK.  The process is fairly straightforward.
>>>>>> With the introduction of the machine keyring, the process remains
>>>>>> basically the same, without the need for any out of tree patches.
>>>>>> 
>>>>>> The machine keyring allowed distros to eliminate the out of tree patches
>>>>>> for kernel module signing.  However, it falls short in allowing the end 
>>>>>> user to add their own keys for IMA. Currently 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.  This would expand the current 
>>>>>> integrity gap. The IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY 
>>>>>> Kconfig states that keys may be added to the ima keyrings if the key is 
>>>>>> validly signed by a CA cert in the system built-in or secondary trusted 
>>>>>> keyring.  Currently there is not code that enforces the contents of a
>>>>>> CA cert. Any key in the builtin or secondary keyring can be used. 
>>>>>> 
>>>>>> To allow IMA to be enabled with the machine keyring, this series introduces
>>>>>> enforcement of key usage in the certificate. This series also applies
>>>>>> this enforcement across all kernel keyrings.
>>>>>> 
>>>>>> The machine keyring shares  similarities with both the builtin and
>>>>>> secondary keyrings.  Similar to the builtin, no keys may be added to the
>>>>>> machine keyring following boot. The secondary keyring allows user
>>>>>> provided keys to be added following boot; however, a previously enrolled
>>>>>> kernel key must vouch for the key before it may be included. The system
>>>>>> owner may include their own keys into the machine keyring prior to boot.
>>>>>> If the end-user is not the system owner, they may not add their own keys
>>>>>> to the machine keyring.  
>>>>>> 
>>>>>> The machine keyring is only populated when Secure Boot is enabled.  A
>>>>>> system owner has the ability to control the entire Secure Boot keychain
>>>>>> (PK, KEK, DB, and DBX).  The system owner can also turn Secure Boot off.
>>>>>> With this control, they may use insert-sys-cert to include their own key 
>>>>>> and re-sign their kernel and have it boot.  The system owner also has 
>>>>>> control to include or exclude MOK keys. This series does not try to 
>>>>>> interpret how a system owner has configured their machine.  If the system 
>>>>>> owner has taken the steps to add their own MOK keys, they will be 
>>>>>> included in the machine keyring and used for verification, exactly 
>>>>>> the same way as keys contained in the builtin and secondary keyrings.
>>>>>> Since the system owner has the ability to add keys before booting to
>>>>>> either the machine or builtin keyrings, it is viewed as inconsequential 
>>>>>> if the key originated from one or the other.
>>>>>> 
>>>>>> This series introduces two different ways to configure the machine keyring.
>>>>>> By default, nothing changes and all MOK keys are loaded into it.  Whenever
>>>>>> a CA cert is found within the machine, builtin, or secondary, a flag 
>>>>>> indicating this is stored in the public key struct.  The other option is 
>>>>>> if the new Kconfig INTEGRITY_CA_MACHINE_KEYRING is enabled, only CA certs 
>>>>>> will be loaded into the machine keyring. All remaining MOK keys will be 
>>>>>> loaded into the platform keyring.
>>>>>> 
>>>>>> A CA cert shall be defined as any X509 certificate that contains the 
>>>>>> keyCertSign key usage and has the CA bit set to true.
>>>>> 
>>>>> Hi Eric,
>>>>> 
>>>>> Allowing CA certificates with the digitalSignature key usage flag
>>>>> enabled defeats the purpose of the new Kconfig.  Please update the
>>>>> above definition to exclude the digitalSignature key usage flag and
>>>>> modify the code accordingly.
>>>> 
>>>> Within v2, the request was made to allow Intermediate CA certificates to be 
>>>> loaded directly.  The Intermediate CA referenced was the one used by kernel.org.  
>>>> This Intermediate CA contains both digitalSignature and keyCertSign.  If the code 
>>>> is changed to exclude this certificate, now the root CA has to be loaded again.  Is that 
>>>> the intent?
>>> 
>>> That definitely was not the intent.  Nor would it address the issue of
>>> a particular intermediate CA certificate having both keyCertSign and
>>> digitalSignature.
>> 
>> Sorry, I’m not following.  Why is it an issue that an intermediate CA certificate contains 
>> both keyCertSign and digitalSignature? Why would we want to exclude an Intermediate
>> CA cert like the one used on kernel.org?
> 
> I must be missing something.  Isn't the purpose of "keyUsage" to
> minimize how a certificate may be used?   Why would we want the same
> certificate to be used for both certificate signing and code signing?

Every 3rd party intermediate CA I have looked at so far contains both set. Most have CRLSign set.  
Typically the root CA contains keyCertSign and CRLSign, but some also have digitalSignature 
set.  Finding a 3rd party Intermediate CA without digitalSignature set is probably going to be 
challenging and will severely limit usage.

Re: [PATCH v3 00/10] Add CA enforcement keyring restrictions

[Coiby Xu]

Hi Eric and Mimi,

On Thu, Dec 15, 2022 at 09:45:37PM +0000, Eric Snowberg wrote:
>
>
>>>>>>> A CA cert shall be defined as any X509 certificate that contains the
>>>>>>> keyCertSign key usage and has the CA bit set to true.
>>>>>>
>>>>>> Hi Eric,
>>>>>>
>>>>>> Allowing CA certificates with the digitalSignature key usage flag
>>>>>> enabled defeats the purpose of the new Kconfig.  Please update the
>>>>>> above definition to exclude the digitalSignature key usage flag and
>>>>>> modify the code accordingly.
>>>>>
>>>>> Within v2, the request was made to allow Intermediate CA certificates to be
>>>>> loaded directly.  The Intermediate CA referenced was the one used by kernel.org.
>>>>> This Intermediate CA contains both digitalSignature and keyCertSign.  If the code
>>>>> is changed to exclude this certificate, now the root CA has to be loaded again.  Is that
>>>>> the intent?
>>>>
>>>> That definitely was not the intent.  Nor would it address the issue of
>>>> a particular intermediate CA certificate having both keyCertSign and
>>>> digitalSignature.
>>>
>>> Sorry, I’m not following.  Why is it an issue that an intermediate CA certificate contains
>>> both keyCertSign and digitalSignature? Why would we want to exclude an Intermediate
>>> CA cert like the one used on kernel.org?
>>
>> I must be missing something.  Isn't the purpose of "keyUsage" to
>> minimize how a certificate may be used?   Why would we want the same
>> certificate to be used for both certificate signing and code signing?
>
>Every 3rd party intermediate CA I have looked at so far contains both set. Most have CRLSign set.
>Typically the root CA contains keyCertSign and CRLSign, but some also have digitalSignature
>set.  Finding a 3rd party Intermediate CA without digitalSignature set is probably going to be
>challenging and will severely limit usage.

How about allowing both keyCertSign and digitalSignature asserted but
issuing a warning for this case?

Here's my rationale for this proposal.

I assume we should conform to some X.509 specifications. So I checked
"RFC 5280: Internet X.509 Public Key Infrastructure Certificate and
Certificate Revocation List (CRL) Profile" [1] and ITU-T X.509 (2012-10)
[2].

[1] states in 4.2.1.3. Key Usage,
    "If the keyUsage extension is present, then the subject public key
    MUST NOT be used to verify signatures on certificates or CRLs unless
    the corresponding keyCertSign or cRLSign bit is set.  If the subject
    public key is only to be used for verifying signatures on
    certificates and/or CRLs, then the digitalSignature and
    nonRepudiation bits SHOULD NOT be set.  However, the digitalSignature
    and/or nonRepudiation bits MAY be set in addition to the keyCertSign
    and/or cRLSign bits if the subject public key is to be used to verify
    signatures on certificates and/or CRLs as well as other objects."

and [2] states in 8.2.2.3 Key usage extension that,
   "More than one bit may be set in an instance of the keyUsage extension.
   The setting of multiple bits shall not change the meaning of each
   individual bit but shall indicate that the certificate may be used for
   all of the purposes indicated by the set bits. There may be risks
   incurred when setting multiple bits. A review of those risks is
   documented in Annex I."

I interpret the above texts as we should allow both keyCertSign and
digitalSignature. However [2] warns about the risks of setting multiple
bits. Quoting Annex I,

   "Combining the contentCommitment bit in the keyUsage certificate
   extension with other keyUsage bits may have security implications
   depending on the security environment in which the certificate is to be
   used. If the subject's environment can be fully controlled and trusted,
   then there are no specific security implications. For example, in cases
   where the subject is fully confident about exactly which data is signed
   or cases where the subject is fully confident about the security
   characteristics of the authentication protocol being used. If the
   subject's environment is not fully controlled or not fully trusted, then
   unintentional signing of commitments is possible. Examples include the
   use of badly formed authentication exchanges and the use of a rogue
   software component. If untrusted environments are used by a subject,
   these security implications can be limited through use of the following
   measures:   
    – to not combine the contentCommitment key usage setting in
      certificates with any other key usage setting and to use the
      corresponding private key only with this certificate;   
      
    – to limit the use of private keys associated with certificates that
      have the contentCommitment key usage bit set, to environments which
      are considered adequately controlled and trustworthy"

So maybe it's useful to add a warning if both keyCertSign and
digitalSignature are asserted.


-- 
Best regards,
Coiby

Re: [PATCH v3 00/10] Add CA enforcement keyring restrictions

[Mimi Zohar]

On Fri, 2022-12-16 at 22:06 +0800, Coiby Xu wrote:
> Hi Eric and Mimi,
> 
> On Thu, Dec 15, 2022 at 09:45:37PM +0000, Eric Snowberg wrote:
> >
> >
> >>>>>>> A CA cert shall be defined as any X509 certificate that contains the
> >>>>>>> keyCertSign key usage and has the CA bit set to true.
> >>>>>>
> >>>>>> Hi Eric,
> >>>>>>
> >>>>>> Allowing CA certificates with the digitalSignature key usage flag
> >>>>>> enabled defeats the purpose of the new Kconfig.  Please update the
> >>>>>> above definition to exclude the digitalSignature key usage flag and
> >>>>>> modify the code accordingly.
> >>>>>
> >>>>> Within v2, the request was made to allow Intermediate CA certificates to be
> >>>>> loaded directly.  The Intermediate CA referenced was the one used by kernel.org.
> >>>>> This Intermediate CA contains both digitalSignature and keyCertSign.  If the code
> >>>>> is changed to exclude this certificate, now the root CA has to be loaded again.  Is that
> >>>>> the intent?
> >>>>
> >>>> That definitely was not the intent.  Nor would it address the issue of
> >>>> a particular intermediate CA certificate having both keyCertSign and
> >>>> digitalSignature.
> >>>
> >>> Sorry, I’m not following.  Why is it an issue that an intermediate CA certificate contains
> >>> both keyCertSign and digitalSignature? Why would we want to exclude an Intermediate
> >>> CA cert like the one used on kernel.org?
> >>
> >> I must be missing something.  Isn't the purpose of "keyUsage" to
> >> minimize how a certificate may be used?   Why would we want the same
> >> certificate to be used for both certificate signing and code signing?
> >
> >Every 3rd party intermediate CA I have looked at so far contains both set. Most have CRLSign set.
> >Typically the root CA contains keyCertSign and CRLSign, but some also have digitalSignature
> >set.  Finding a 3rd party Intermediate CA without digitalSignature set is probably going to be
> >challenging and will severely limit usage.
> 
> How about allowing both keyCertSign and digitalSignature asserted but
> issuing a warning for this case?
> 
> Here's my rationale for this proposal.
> 
> I assume we should conform to some X.509 specifications. So I checked
> "RFC 5280: Internet X.509 Public Key Infrastructure Certificate and
> Certificate Revocation List (CRL) Profile" [1] and ITU-T X.509 (2012-10)
> [2].
> 
> [1] states in 4.2.1.3. Key Usage,
>     "If the keyUsage extension is present, then the subject public key
>     MUST NOT be used to verify signatures on certificates or CRLs unless
>     the corresponding keyCertSign or cRLSign bit is set.  If the subject
>     public key is only to be used for verifying signatures on
>     certificates and/or CRLs, then the digitalSignature and
>     nonRepudiation bits SHOULD NOT be set.  However, the digitalSignature
>     and/or nonRepudiation bits MAY be set in addition to the keyCertSign
>     and/or cRLSign bits if the subject public key is to be used to verify
>     signatures on certificates and/or CRLs as well as other objects."
> 
> and [2] states in 8.2.2.3 Key usage extension that,
>    "More than one bit may be set in an instance of the keyUsage extension.
>    The setting of multiple bits shall not change the meaning of each
>    individual bit but shall indicate that the certificate may be used for
>    all of the purposes indicated by the set bits. There may be risks
>    incurred when setting multiple bits. A review of those risks is
>    documented in Annex I."
> 
> I interpret the above texts as we should allow both keyCertSign and
> digitalSignature. However [2] warns about the risks of setting multiple
> bits. Quoting Annex I,
> 
>    "Combining the contentCommitment bit in the keyUsage certificate
>    extension with other keyUsage bits may have security implications
>    depending on the security environment in which the certificate is to be
>    used. If the subject's environment can be fully controlled and trusted,
>    then there are no specific security implications. For example, in cases
>    where the subject is fully confident about exactly which data is signed
>    or cases where the subject is fully confident about the security
>    characteristics of the authentication protocol being used. If the
>    subject's environment is not fully controlled or not fully trusted, then
>    unintentional signing of commitments is possible. Examples include the
>    use of badly formed authentication exchanges and the use of a rogue
>    software component. If untrusted environments are used by a subject,
>    these security implications can be limited through use of the following
>    measures:   
>     – to not combine the contentCommitment key usage setting in
>       certificates with any other key usage setting and to use the
>       corresponding private key only with this certificate;   
>       
>     – to limit the use of private keys associated with certificates that
>       have the contentCommitment key usage bit set, to environments which
>       are considered adequately controlled and trustworthy"
> 
> So maybe it's useful to add a warning if both keyCertSign and
> digitalSignature are asserted.

Coiby, thank you for adding these details.  I was hoping others would
chime in as well.  I agree at minimum there should be a warning.

Perhaps instead of making INTEGRITY_CA_MACHINE_KEYRING dependent on
INTEGRITY_MACHINE_KEYRING, make them a Kconfig "choice" to support the
more restrictive certificate use case requirements:  all certificates,
CA certificate signing and digital signature, only CA certificate
signing.

-- 
thanks,

Mimi

Re: [PATCH v3 00/10] Add CA enforcement keyring restrictions

[Eric Snowberg]

> On Dec 18, 2022, at 5:21 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> 
> On Fri, 2022-12-16 at 22:06 +0800, Coiby Xu wrote:
>> Hi Eric and Mimi,
>> 
>> On Thu, Dec 15, 2022 at 09:45:37PM +0000, Eric Snowberg wrote:
>>> 
>>> 
>>>>>>>>> A CA cert shall be defined as any X509 certificate that contains the
>>>>>>>>> keyCertSign key usage and has the CA bit set to true.
>>>>>>>> 
>>>>>>>> Hi Eric,
>>>>>>>> 
>>>>>>>> Allowing CA certificates with the digitalSignature key usage flag
>>>>>>>> enabled defeats the purpose of the new Kconfig.  Please update the
>>>>>>>> above definition to exclude the digitalSignature key usage flag and
>>>>>>>> modify the code accordingly.
>>>>>>> 
>>>>>>> Within v2, the request was made to allow Intermediate CA certificates to be
>>>>>>> loaded directly.  The Intermediate CA referenced was the one used by kernel.org.
>>>>>>> This Intermediate CA contains both digitalSignature and keyCertSign.  If the code
>>>>>>> is changed to exclude this certificate, now the root CA has to be loaded again.  Is that
>>>>>>> the intent?
>>>>>> 
>>>>>> That definitely was not the intent.  Nor would it address the issue of
>>>>>> a particular intermediate CA certificate having both keyCertSign and
>>>>>> digitalSignature.
>>>>> 
>>>>> Sorry, I’m not following.  Why is it an issue that an intermediate CA certificate contains
>>>>> both keyCertSign and digitalSignature? Why would we want to exclude an Intermediate
>>>>> CA cert like the one used on kernel.org?
>>>> 
>>>> I must be missing something.  Isn't the purpose of "keyUsage" to
>>>> minimize how a certificate may be used?   Why would we want the same
>>>> certificate to be used for both certificate signing and code signing?
>>> 
>>> Every 3rd party intermediate CA I have looked at so far contains both set. Most have CRLSign set.
>>> Typically the root CA contains keyCertSign and CRLSign, but some also have digitalSignature
>>> set.  Finding a 3rd party Intermediate CA without digitalSignature set is probably going to be
>>> challenging and will severely limit usage.
>> 
>> How about allowing both keyCertSign and digitalSignature asserted but
>> issuing a warning for this case?
>> 
>> Here's my rationale for this proposal.
>> 
>> I assume we should conform to some X.509 specifications. So I checked
>> "RFC 5280: Internet X.509 Public Key Infrastructure Certificate and
>> Certificate Revocation List (CRL) Profile" [1] and ITU-T X.509 (2012-10)
>> [2].
>> 
>> [1] states in 4.2.1.3. Key Usage,
>>    "If the keyUsage extension is present, then the subject public key
>>    MUST NOT be used to verify signatures on certificates or CRLs unless
>>    the corresponding keyCertSign or cRLSign bit is set.  If the subject
>>    public key is only to be used for verifying signatures on
>>    certificates and/or CRLs, then the digitalSignature and
>>    nonRepudiation bits SHOULD NOT be set.  However, the digitalSignature
>>    and/or nonRepudiation bits MAY be set in addition to the keyCertSign
>>    and/or cRLSign bits if the subject public key is to be used to verify
>>    signatures on certificates and/or CRLs as well as other objects."
>> 
>> and [2] states in 8.2.2.3 Key usage extension that,
>>   "More than one bit may be set in an instance of the keyUsage extension.
>>   The setting of multiple bits shall not change the meaning of each
>>   individual bit but shall indicate that the certificate may be used for
>>   all of the purposes indicated by the set bits. There may be risks
>>   incurred when setting multiple bits. A review of those risks is
>>   documented in Annex I."
>> 
>> I interpret the above texts as we should allow both keyCertSign and
>> digitalSignature. However [2] warns about the risks of setting multiple
>> bits. Quoting Annex I,
>> 
>>   "Combining the contentCommitment bit in the keyUsage certificate
>>   extension with other keyUsage bits may have security implications
>>   depending on the security environment in which the certificate is to be
>>   used. If the subject's environment can be fully controlled and trusted,
>>   then there are no specific security implications. For example, in cases
>>   where the subject is fully confident about exactly which data is signed
>>   or cases where the subject is fully confident about the security
>>   characteristics of the authentication protocol being used. If the
>>   subject's environment is not fully controlled or not fully trusted, then
>>   unintentional signing of commitments is possible. Examples include the
>>   use of badly formed authentication exchanges and the use of a rogue
>>   software component. If untrusted environments are used by a subject,
>>   these security implications can be limited through use of the following
>>   measures:   
>>    – to not combine the contentCommitment key usage setting in
>>      certificates with any other key usage setting and to use the
>>      corresponding private key only with this certificate;   
>> 
>>    – to limit the use of private keys associated with certificates that
>>      have the contentCommitment key usage bit set, to environments which
>>      are considered adequately controlled and trustworthy"
>> 
>> So maybe it's useful to add a warning if both keyCertSign and
>> digitalSignature are asserted.
> 
> Coiby, thank you for adding these details.  I was hoping others would
> chime in as well.  I agree at minimum there should be a warning.

A warning could be added.

> Perhaps instead of making INTEGRITY_CA_MACHINE_KEYRING dependent on
> INTEGRITY_MACHINE_KEYRING, make them a Kconfig "choice" to support the
> more restrictive certificate use case requirements:  all certificates,
> CA certificate signing and digital signature, only CA certificate
> signing.

As could support for additional restrictions.

Would these additions be required within this series? What is missing from this 
discussion is why would these additions be necessary?  Why should the kernel 
enforce a restriction that is beyond the scope of the X.509 spec?  If a warning was 
to be added, what would be the justification for adding this additional code?  From 
my research every single 3rd party code signing intermediate CA would be flagged 
with the warning.  Isn’t this just going to cause confusion?  Or is there a benefit that 
I am missing that needs to be stated?

Re: [PATCH v3 00/10] Add CA enforcement keyring restrictions

[Mimi Zohar]

On Wed, 2022-12-21 at 18:27 +0000, Eric Snowberg wrote:
> 
> > On Dec 18, 2022, at 5:21 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> > 
> > On Fri, 2022-12-16 at 22:06 +0800, Coiby Xu wrote:
> >> Hi Eric and Mimi,
> >> 
> >> On Thu, Dec 15, 2022 at 09:45:37PM +0000, Eric Snowberg wrote:
> >>> 
> >>> 
> >>>>>>>>> A CA cert shall be defined as any X509 certificate that contains the
> >>>>>>>>> keyCertSign key usage and has the CA bit set to true.
> >>>>>>>> 
> >>>>>>>> Hi Eric,
> >>>>>>>> 
> >>>>>>>> Allowing CA certificates with the digitalSignature key usage flag
> >>>>>>>> enabled defeats the purpose of the new Kconfig.  Please update the
> >>>>>>>> above definition to exclude the digitalSignature key usage flag and
> >>>>>>>> modify the code accordingly.
> >>>>>>> 
> >>>>>>> Within v2, the request was made to allow Intermediate CA certificates to be
> >>>>>>> loaded directly.  The Intermediate CA referenced was the one used by kernel.org.
> >>>>>>> This Intermediate CA contains both digitalSignature and keyCertSign.  If the code
> >>>>>>> is changed to exclude this certificate, now the root CA has to be loaded again.  Is that
> >>>>>>> the intent?
> >>>>>> 
> >>>>>> That definitely was not the intent.  Nor would it address the issue of
> >>>>>> a particular intermediate CA certificate having both keyCertSign and
> >>>>>> digitalSignature.
> >>>>> 
> >>>>> Sorry, I’m not following.  Why is it an issue that an intermediate CA certificate contains
> >>>>> both keyCertSign and digitalSignature? Why would we want to exclude an Intermediate
> >>>>> CA cert like the one used on kernel.org?
> >>>> 
> >>>> I must be missing something.  Isn't the purpose of "keyUsage" to
> >>>> minimize how a certificate may be used?   Why would we want the same
> >>>> certificate to be used for both certificate signing and code signing?
> >>> 
> >>> Every 3rd party intermediate CA I have looked at so far contains both set. Most have CRLSign set.
> >>> Typically the root CA contains keyCertSign and CRLSign, but some also have digitalSignature
> >>> set.  Finding a 3rd party Intermediate CA without digitalSignature set is probably going to be
> >>> challenging and will severely limit usage.
> >> 
> >> How about allowing both keyCertSign and digitalSignature asserted but
> >> issuing a warning for this case?
> >> 
> >> Here's my rationale for this proposal.
> >> 
> >> I assume we should conform to some X.509 specifications. So I checked
> >> "RFC 5280: Internet X.509 Public Key Infrastructure Certificate and
> >> Certificate Revocation List (CRL) Profile" [1] and ITU-T X.509 (2012-10)
> >> [2].
> >> 
> >> [1] states in 4.2.1.3. Key Usage,
> >>    "If the keyUsage extension is present, then the subject public key
> >>    MUST NOT be used to verify signatures on certificates or CRLs unless
> >>    the corresponding keyCertSign or cRLSign bit is set.  If the subject
> >>    public key is only to be used for verifying signatures on
> >>    certificates and/or CRLs, then the digitalSignature and
> >>    nonRepudiation bits SHOULD NOT be set.  However, the digitalSignature
> >>    and/or nonRepudiation bits MAY be set in addition to the keyCertSign
> >>    and/or cRLSign bits if the subject public key is to be used to verify
> >>    signatures on certificates and/or CRLs as well as other objects."
> >> 
> >> and [2] states in 8.2.2.3 Key usage extension that,
> >>   "More than one bit may be set in an instance of the keyUsage extension.
> >>   The setting of multiple bits shall not change the meaning of each
> >>   individual bit but shall indicate that the certificate may be used for
> >>   all of the purposes indicated by the set bits. There may be risks
> >>   incurred when setting multiple bits. A review of those risks is
> >>   documented in Annex I."
> >> 
> >> I interpret the above texts as we should allow both keyCertSign and
> >> digitalSignature. However [2] warns about the risks of setting multiple
> >> bits. Quoting Annex I,
> >> 
> >>   "Combining the contentCommitment bit in the keyUsage certificate
> >>   extension with other keyUsage bits may have security implications
> >>   depending on the security environment in which the certificate is to be
> >>   used. If the subject's environment can be fully controlled and trusted,
> >>   then there are no specific security implications. For example, in cases
> >>   where the subject is fully confident about exactly which data is signed
> >>   or cases where the subject is fully confident about the security
> >>   characteristics of the authentication protocol being used. If the
> >>   subject's environment is not fully controlled or not fully trusted, then
> >>   unintentional signing of commitments is possible. Examples include the
> >>   use of badly formed authentication exchanges and the use of a rogue
> >>   software component. If untrusted environments are used by a subject,
> >>   these security implications can be limited through use of the following
> >>   measures:   
> >>    – to not combine the contentCommitment key usage setting in
> >>      certificates with any other key usage setting and to use the
> >>      corresponding private key only with this certificate;   
> >> 
> >>    – to limit the use of private keys associated with certificates that
> >>      have the contentCommitment key usage bit set, to environments which
> >>      are considered adequately controlled and trustworthy"
> >> 
> >> So maybe it's useful to add a warning if both keyCertSign and
> >> digitalSignature are asserted.
> > 
> > Coiby, thank you for adding these details.  I was hoping others would
> > chime in as well.  I agree at minimum there should be a warning.
> 
> A warning could be added.
> 
> > Perhaps instead of making INTEGRITY_CA_MACHINE_KEYRING dependent on
> > INTEGRITY_MACHINE_KEYRING, make them a Kconfig "choice" to support the
> > more restrictive certificate use case requirements:  all certificates,
> > CA certificate signing and digital signature, only CA certificate
> > signing.
> 
> As could support for additional restrictions.
> 
> Would these additions be required within this series? What is missing from this 
> discussion is why would these additions be necessary?  Why should the kernel 
> enforce a restriction that is beyond the scope of the X.509 spec?  If a warning was 
> to be added, what would be the justification for adding this additional code?  From 
> my research every single 3rd party code signing intermediate CA would be flagged 
> with the warning.  Isn’t this just going to cause confusion?  Or is there a benefit that 
> I am missing that needs to be stated?

You're focusing on third party kernel modules and forgetting about the
simple use case of allowing an end user (or business) to sign their own
code.  True they could use the less restrictive CA certificates, but it
is unnecessary.

-- 
thanks,

Mimi

Re: [PATCH v3 00/10] Add CA enforcement keyring restrictions

[Eric Snowberg]

> On Dec 21, 2022, at 12:01 PM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> 
> On Wed, 2022-12-21 at 18:27 +0000, Eric Snowberg wrote:
>> 
>>> On Dec 18, 2022, at 5:21 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
>>> 
>>> On Fri, 2022-12-16 at 22:06 +0800, Coiby Xu wrote:
>>>> Hi Eric and Mimi,
>>>> 
>>>> On Thu, Dec 15, 2022 at 09:45:37PM +0000, Eric Snowberg wrote:
>>>>> 
>>>>> 
>>>>>>>>>>> A CA cert shall be defined as any X509 certificate that contains the
>>>>>>>>>>> keyCertSign key usage and has the CA bit set to true.
>>>>>>>>>> 
>>>>>>>>>> Hi Eric,
>>>>>>>>>> 
>>>>>>>>>> Allowing CA certificates with the digitalSignature key usage flag
>>>>>>>>>> enabled defeats the purpose of the new Kconfig.  Please update the
>>>>>>>>>> above definition to exclude the digitalSignature key usage flag and
>>>>>>>>>> modify the code accordingly.
>>>>>>>>> 
>>>>>>>>> Within v2, the request was made to allow Intermediate CA certificates to be
>>>>>>>>> loaded directly.  The Intermediate CA referenced was the one used by kernel.org.
>>>>>>>>> This Intermediate CA contains both digitalSignature and keyCertSign.  If the code
>>>>>>>>> is changed to exclude this certificate, now the root CA has to be loaded again.  Is that
>>>>>>>>> the intent?
>>>>>>>> 
>>>>>>>> That definitely was not the intent.  Nor would it address the issue of
>>>>>>>> a particular intermediate CA certificate having both keyCertSign and
>>>>>>>> digitalSignature.
>>>>>>> 
>>>>>>> Sorry, I’m not following.  Why is it an issue that an intermediate CA certificate contains
>>>>>>> both keyCertSign and digitalSignature? Why would we want to exclude an Intermediate
>>>>>>> CA cert like the one used on kernel.org?
>>>>>> 
>>>>>> I must be missing something.  Isn't the purpose of "keyUsage" to
>>>>>> minimize how a certificate may be used?   Why would we want the same
>>>>>> certificate to be used for both certificate signing and code signing?
>>>>> 
>>>>> Every 3rd party intermediate CA I have looked at so far contains both set. Most have CRLSign set.
>>>>> Typically the root CA contains keyCertSign and CRLSign, but some also have digitalSignature
>>>>> set.  Finding a 3rd party Intermediate CA without digitalSignature set is probably going to be
>>>>> challenging and will severely limit usage.
>>>> 
>>>> How about allowing both keyCertSign and digitalSignature asserted but
>>>> issuing a warning for this case?
>>>> 
>>>> Here's my rationale for this proposal.
>>>> 
>>>> I assume we should conform to some X.509 specifications. So I checked
>>>> "RFC 5280: Internet X.509 Public Key Infrastructure Certificate and
>>>> Certificate Revocation List (CRL) Profile" [1] and ITU-T X.509 (2012-10)
>>>> [2].
>>>> 
>>>> [1] states in 4.2.1.3. Key Usage,
>>>>   "If the keyUsage extension is present, then the subject public key
>>>>   MUST NOT be used to verify signatures on certificates or CRLs unless
>>>>   the corresponding keyCertSign or cRLSign bit is set.  If the subject
>>>>   public key is only to be used for verifying signatures on
>>>>   certificates and/or CRLs, then the digitalSignature and
>>>>   nonRepudiation bits SHOULD NOT be set.  However, the digitalSignature
>>>>   and/or nonRepudiation bits MAY be set in addition to the keyCertSign
>>>>   and/or cRLSign bits if the subject public key is to be used to verify
>>>>   signatures on certificates and/or CRLs as well as other objects."
>>>> 
>>>> and [2] states in 8.2.2.3 Key usage extension that,
>>>>  "More than one bit may be set in an instance of the keyUsage extension.
>>>>  The setting of multiple bits shall not change the meaning of each
>>>>  individual bit but shall indicate that the certificate may be used for
>>>>  all of the purposes indicated by the set bits. There may be risks
>>>>  incurred when setting multiple bits. A review of those risks is
>>>>  documented in Annex I."
>>>> 
>>>> I interpret the above texts as we should allow both keyCertSign and
>>>> digitalSignature. However [2] warns about the risks of setting multiple
>>>> bits. Quoting Annex I,
>>>> 
>>>>  "Combining the contentCommitment bit in the keyUsage certificate
>>>>  extension with other keyUsage bits may have security implications
>>>>  depending on the security environment in which the certificate is to be
>>>>  used. If the subject's environment can be fully controlled and trusted,
>>>>  then there are no specific security implications. For example, in cases
>>>>  where the subject is fully confident about exactly which data is signed
>>>>  or cases where the subject is fully confident about the security
>>>>  characteristics of the authentication protocol being used. If the
>>>>  subject's environment is not fully controlled or not fully trusted, then
>>>>  unintentional signing of commitments is possible. Examples include the
>>>>  use of badly formed authentication exchanges and the use of a rogue
>>>>  software component. If untrusted environments are used by a subject,
>>>>  these security implications can be limited through use of the following
>>>>  measures:   
>>>>   – to not combine the contentCommitment key usage setting in
>>>>     certificates with any other key usage setting and to use the
>>>>     corresponding private key only with this certificate;   
>>>> 
>>>>   – to limit the use of private keys associated with certificates that
>>>>     have the contentCommitment key usage bit set, to environments which
>>>>     are considered adequately controlled and trustworthy"
>>>> 
>>>> So maybe it's useful to add a warning if both keyCertSign and
>>>> digitalSignature are asserted.
>>> 
>>> Coiby, thank you for adding these details.  I was hoping others would
>>> chime in as well.  I agree at minimum there should be a warning.
>> 
>> A warning could be added.
>> 
>>> Perhaps instead of making INTEGRITY_CA_MACHINE_KEYRING dependent on
>>> INTEGRITY_MACHINE_KEYRING, make them a Kconfig "choice" to support the
>>> more restrictive certificate use case requirements:  all certificates,
>>> CA certificate signing and digital signature, only CA certificate
>>> signing.
>> 
>> As could support for additional restrictions.
>> 
>> Would these additions be required within this series? What is missing from this 
>> discussion is why would these additions be necessary?  Why should the kernel 
>> enforce a restriction that is beyond the scope of the X.509 spec?  If a warning was 
>> to be added, what would be the justification for adding this additional code?  From 
>> my research every single 3rd party code signing intermediate CA would be flagged 
>> with the warning.  Isn’t this just going to cause confusion?  Or is there a benefit that 
>> I am missing that needs to be stated?
> 
> You're focusing on third party kernel modules and forgetting about the
> simple use case of allowing an end user (or business) to sign their own
> code.  True they could use the less restrictive CA certificates, but it
> is unnecessary.

My focus is on signing user-space applications, as outlined in the cover letter.  This 
series has nothing to do with kernel modules.  Most end-users and businesses rely on 
a third party to deal with code signing.  All third party code signing services I have 
found use an intermediate CA containing more than just the keyCertSign usage set.  
It seems to be an industry accepted practice that does not violate the spec. Before writing
new code to either warn or exclude a third party intermediate CA,  I would like to understand 
the motivation behind this request.

Re: [PATCH v3 00/10] Add CA enforcement keyring restrictions

[Mimi Zohar]

On Thu, 2022-12-22 at 15:15 +0000, Eric Snowberg wrote:
> 
> > On Dec 21, 2022, at 12:01 PM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> > 
> > On Wed, 2022-12-21 at 18:27 +0000, Eric Snowberg wrote:
> >> 
> >>> On Dec 18, 2022, at 5:21 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> >>> 
> >>> On Fri, 2022-12-16 at 22:06 +0800, Coiby Xu wrote:
> >>>> Hi Eric and Mimi,
> >>>> 
> >>>> On Thu, Dec 15, 2022 at 09:45:37PM +0000, Eric Snowberg wrote:
> >>>>> 
> >>>>> 
> >>>>>>>>>>> A CA cert shall be defined as any X509 certificate that contains the
> >>>>>>>>>>> keyCertSign key usage and has the CA bit set to true.
> >>>>>>>>>> 
> >>>>>>>>>> Hi Eric,
> >>>>>>>>>> 
> >>>>>>>>>> Allowing CA certificates with the digitalSignature key usage flag
> >>>>>>>>>> enabled defeats the purpose of the new Kconfig.  Please update the
> >>>>>>>>>> above definition to exclude the digitalSignature key usage flag and
> >>>>>>>>>> modify the code accordingly.
> >>>>>>>>> 
> >>>>>>>>> Within v2, the request was made to allow Intermediate CA certificates to be
> >>>>>>>>> loaded directly.  The Intermediate CA referenced was the one used by kernel.org.
> >>>>>>>>> This Intermediate CA contains both digitalSignature and keyCertSign.  If the code
> >>>>>>>>> is changed to exclude this certificate, now the root CA has to be loaded again.  Is that
> >>>>>>>>> the intent?
> >>>>>>>> 
> >>>>>>>> That definitely was not the intent.  Nor would it address the issue of
> >>>>>>>> a particular intermediate CA certificate having both keyCertSign and
> >>>>>>>> digitalSignature.
> >>>>>>> 
> >>>>>>> Sorry, I’m not following.  Why is it an issue that an intermediate CA certificate contains
> >>>>>>> both keyCertSign and digitalSignature? Why would we want to exclude an Intermediate
> >>>>>>> CA cert like the one used on kernel.org?
> >>>>>> 
> >>>>>> I must be missing something.  Isn't the purpose of "keyUsage" to
> >>>>>> minimize how a certificate may be used?   Why would we want the same
> >>>>>> certificate to be used for both certificate signing and code signing?
> >>>>> 
> >>>>> Every 3rd party intermediate CA I have looked at so far contains both set. Most have CRLSign set.
> >>>>> Typically the root CA contains keyCertSign and CRLSign, but some also have digitalSignature
> >>>>> set.  Finding a 3rd party Intermediate CA without digitalSignature set is probably going to be
> >>>>> challenging and will severely limit usage.
> >>>> 
> >>>> How about allowing both keyCertSign and digitalSignature asserted but
> >>>> issuing a warning for this case?
> >>>> 
> >>>> Here's my rationale for this proposal.
> >>>> 
> >>>> I assume we should conform to some X.509 specifications. So I checked
> >>>> "RFC 5280: Internet X.509 Public Key Infrastructure Certificate and
> >>>> Certificate Revocation List (CRL) Profile" [1] and ITU-T X.509 (2012-10)
> >>>> [2].
> >>>> 
> >>>> [1] states in 4.2.1.3. Key Usage,
> >>>>   "If the keyUsage extension is present, then the subject public key
> >>>>   MUST NOT be used to verify signatures on certificates or CRLs unless
> >>>>   the corresponding keyCertSign or cRLSign bit is set.  If the subject
> >>>>   public key is only to be used for verifying signatures on
> >>>>   certificates and/or CRLs, then the digitalSignature and
> >>>>   nonRepudiation bits SHOULD NOT be set.  However, the digitalSignature
> >>>>   and/or nonRepudiation bits MAY be set in addition to the keyCertSign
> >>>>   and/or cRLSign bits if the subject public key is to be used to verify
> >>>>   signatures on certificates and/or CRLs as well as other objects."
> >>>> 
> >>>> and [2] states in 8.2.2.3 Key usage extension that,
> >>>>  "More than one bit may be set in an instance of the keyUsage extension.
> >>>>  The setting of multiple bits shall not change the meaning of each
> >>>>  individual bit but shall indicate that the certificate may be used for
> >>>>  all of the purposes indicated by the set bits. There may be risks
> >>>>  incurred when setting multiple bits. A review of those risks is
> >>>>  documented in Annex I."
> >>>> 
> >>>> I interpret the above texts as we should allow both keyCertSign and
> >>>> digitalSignature. However [2] warns about the risks of setting multiple
> >>>> bits. Quoting Annex I,
> >>>> 
> >>>>  "Combining the contentCommitment bit in the keyUsage certificate
> >>>>  extension with other keyUsage bits may have security implications
> >>>>  depending on the security environment in which the certificate is to be
> >>>>  used. If the subject's environment can be fully controlled and trusted,
> >>>>  then there are no specific security implications. For example, in cases
> >>>>  where the subject is fully confident about exactly which data is signed
> >>>>  or cases where the subject is fully confident about the security
> >>>>  characteristics of the authentication protocol being used. If the
> >>>>  subject's environment is not fully controlled or not fully trusted, then
> >>>>  unintentional signing of commitments is possible. Examples include the
> >>>>  use of badly formed authentication exchanges and the use of a rogue
> >>>>  software component. If untrusted environments are used by a subject,
> >>>>  these security implications can be limited through use of the following
> >>>>  measures:   
> >>>>   – to not combine the contentCommitment key usage setting in
> >>>>     certificates with any other key usage setting and to use the
> >>>>     corresponding private key only with this certificate;   
> >>>> 
> >>>>   – to limit the use of private keys associated with certificates that
> >>>>     have the contentCommitment key usage bit set, to environments which
> >>>>     are considered adequately controlled and trustworthy"
> >>>> 
> >>>> So maybe it's useful to add a warning if both keyCertSign and
> >>>> digitalSignature are asserted.
> >>> 
> >>> Coiby, thank you for adding these details.  I was hoping others would
> >>> chime in as well.  I agree at minimum there should be a warning.
> >> 
> >> A warning could be added.
> >> 
> >>> Perhaps instead of making INTEGRITY_CA_MACHINE_KEYRING dependent on
> >>> INTEGRITY_MACHINE_KEYRING, make them a Kconfig "choice" to support the
> >>> more restrictive certificate use case requirements:  all certificates,
> >>> CA certificate signing and digital signature, only CA certificate
> >>> signing.
> >> 
> >> As could support for additional restrictions.
> >> 
> >> Would these additions be required within this series? What is missing from this 
> >> discussion is why would these additions be necessary?  Why should the kernel 
> >> enforce a restriction that is beyond the scope of the X.509 spec?  If a warning was 
> >> to be added, what would be the justification for adding this additional code?  From 
> >> my research every single 3rd party code signing intermediate CA would be flagged 
> >> with the warning.  Isn’t this just going to cause confusion?  Or is there a benefit that 
> >> I am missing that needs to be stated?
> > 
> > You're focusing on third party kernel modules and forgetting about the
> > simple use case of allowing an end user (or business) to sign their own
> > code.  True they could use the less restrictive CA certificates, but it
> > is unnecessary.
> 
> My focus is on signing user-space applications, as outlined in the cover letter.  This 
> series has nothing to do with kernel modules.  Most end-users and businesses rely on 
> a third party to deal with code signing.  All third party code signing services I have 
> found use an intermediate CA containing more than just the keyCertSign usage set.  
> It seems to be an industry accepted practice that does not violate the spec. Before writing
> new code to either warn or exclude a third party intermediate CA,  I would like to understand 
> the motivation behind this request.

In older discussions there are comments like, "Any CA certificate, no
matter if it's a root or an intermediate, must have the keyCertSign
extension. If you want to sign a revocation list (CRL) with the CA
certificate as well (you usually do want that), than you have to add
cRLSign as well. Any other keyUsages can and should be avoided for CA
certificates."

The question as to "why" this changed to include "digitalSignature" was
posed here [2] with the response being for "OCSP".   It also includes a
link to Entrusts root and intermediate CAs with just keyCertSign and
cRLSign keyUsages.

The matchine keyring is a means of establishing a new root of trust. 
The motivation for further restricting CA certificates to just
keyCertSign and cRLSign keyUsages is to limit how the CA certificates
may be used.  They should not be used for code signing.

thanks,

Mimi

[1] https://superuser.com/questions/738612/openssl-ca-keyusage-extension
[2] https://security.stackexchange.com/questions/231133/keyusage-extensions-on-a-certificate-authority

Re: [PATCH v3 00/10] Add CA enforcement keyring restrictions

[Eric Snowberg]

> On Dec 22, 2022, at 8:41 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> 
> On Thu, 2022-12-22 at 15:15 +0000, Eric Snowberg wrote:
>> 
>>> On Dec 21, 2022, at 12:01 PM, Mimi Zohar <zohar@linux.ibm.com> wrote:
>>> 
>>> On Wed, 2022-12-21 at 18:27 +0000, Eric Snowberg wrote:
>>>> 
>>>>> On Dec 18, 2022, at 5:21 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
>>>>> 
>>>>> On Fri, 2022-12-16 at 22:06 +0800, Coiby Xu wrote:
>>>>>> Hi Eric and Mimi,
>>>>>> 
>>>>>> On Thu, Dec 15, 2022 at 09:45:37PM +0000, Eric Snowberg wrote:
>>>>>>> 
>>>>>>> 
>>>>>>>>>>>>> A CA cert shall be defined as any X509 certificate that contains the
>>>>>>>>>>>>> keyCertSign key usage and has the CA bit set to true.
>>>>>>>>>>>> 
>>>>>>>>>>>> Hi Eric,
>>>>>>>>>>>> 
>>>>>>>>>>>> Allowing CA certificates with the digitalSignature key usage flag
>>>>>>>>>>>> enabled defeats the purpose of the new Kconfig.  Please update the
>>>>>>>>>>>> above definition to exclude the digitalSignature key usage flag and
>>>>>>>>>>>> modify the code accordingly.
>>>>>>>>>>> 
>>>>>>>>>>> Within v2, the request was made to allow Intermediate CA certificates to be
>>>>>>>>>>> loaded directly.  The Intermediate CA referenced was the one used by kernel.org.
>>>>>>>>>>> This Intermediate CA contains both digitalSignature and keyCertSign.  If the code
>>>>>>>>>>> is changed to exclude this certificate, now the root CA has to be loaded again.  Is that
>>>>>>>>>>> the intent?
>>>>>>>>>> 
>>>>>>>>>> That definitely was not the intent.  Nor would it address the issue of
>>>>>>>>>> a particular intermediate CA certificate having both keyCertSign and
>>>>>>>>>> digitalSignature.
>>>>>>>>> 
>>>>>>>>> Sorry, I’m not following.  Why is it an issue that an intermediate CA certificate contains
>>>>>>>>> both keyCertSign and digitalSignature? Why would we want to exclude an Intermediate
>>>>>>>>> CA cert like the one used on kernel.org?
>>>>>>>> 
>>>>>>>> I must be missing something.  Isn't the purpose of "keyUsage" to
>>>>>>>> minimize how a certificate may be used?   Why would we want the same
>>>>>>>> certificate to be used for both certificate signing and code signing?
>>>>>>> 
>>>>>>> Every 3rd party intermediate CA I have looked at so far contains both set. Most have CRLSign set.
>>>>>>> Typically the root CA contains keyCertSign and CRLSign, but some also have digitalSignature
>>>>>>> set.  Finding a 3rd party Intermediate CA without digitalSignature set is probably going to be
>>>>>>> challenging and will severely limit usage.
>>>>>> 
>>>>>> How about allowing both keyCertSign and digitalSignature asserted but
>>>>>> issuing a warning for this case?
>>>>>> 
>>>>>> Here's my rationale for this proposal.
>>>>>> 
>>>>>> I assume we should conform to some X.509 specifications. So I checked
>>>>>> "RFC 5280: Internet X.509 Public Key Infrastructure Certificate and
>>>>>> Certificate Revocation List (CRL) Profile" [1] and ITU-T X.509 (2012-10)
>>>>>> [2].
>>>>>> 
>>>>>> [1] states in 4.2.1.3. Key Usage,
>>>>>>  "If the keyUsage extension is present, then the subject public key
>>>>>>  MUST NOT be used to verify signatures on certificates or CRLs unless
>>>>>>  the corresponding keyCertSign or cRLSign bit is set.  If the subject
>>>>>>  public key is only to be used for verifying signatures on
>>>>>>  certificates and/or CRLs, then the digitalSignature and
>>>>>>  nonRepudiation bits SHOULD NOT be set.  However, the digitalSignature
>>>>>>  and/or nonRepudiation bits MAY be set in addition to the keyCertSign
>>>>>>  and/or cRLSign bits if the subject public key is to be used to verify
>>>>>>  signatures on certificates and/or CRLs as well as other objects."
>>>>>> 
>>>>>> and [2] states in 8.2.2.3 Key usage extension that,
>>>>>> "More than one bit may be set in an instance of the keyUsage extension.
>>>>>> The setting of multiple bits shall not change the meaning of each
>>>>>> individual bit but shall indicate that the certificate may be used for
>>>>>> all of the purposes indicated by the set bits. There may be risks
>>>>>> incurred when setting multiple bits. A review of those risks is
>>>>>> documented in Annex I."
>>>>>> 
>>>>>> I interpret the above texts as we should allow both keyCertSign and
>>>>>> digitalSignature. However [2] warns about the risks of setting multiple
>>>>>> bits. Quoting Annex I,
>>>>>> 
>>>>>> "Combining the contentCommitment bit in the keyUsage certificate
>>>>>> extension with other keyUsage bits may have security implications
>>>>>> depending on the security environment in which the certificate is to be
>>>>>> used. If the subject's environment can be fully controlled and trusted,
>>>>>> then there are no specific security implications. For example, in cases
>>>>>> where the subject is fully confident about exactly which data is signed
>>>>>> or cases where the subject is fully confident about the security
>>>>>> characteristics of the authentication protocol being used. If the
>>>>>> subject's environment is not fully controlled or not fully trusted, then
>>>>>> unintentional signing of commitments is possible. Examples include the
>>>>>> use of badly formed authentication exchanges and the use of a rogue
>>>>>> software component. If untrusted environments are used by a subject,
>>>>>> these security implications can be limited through use of the following
>>>>>> measures:   
>>>>>>  – to not combine the contentCommitment key usage setting in
>>>>>>    certificates with any other key usage setting and to use the
>>>>>>    corresponding private key only with this certificate;   
>>>>>> 
>>>>>>  – to limit the use of private keys associated with certificates that
>>>>>>    have the contentCommitment key usage bit set, to environments which
>>>>>>    are considered adequately controlled and trustworthy"
>>>>>> 
>>>>>> So maybe it's useful to add a warning if both keyCertSign and
>>>>>> digitalSignature are asserted.
>>>>> 
>>>>> Coiby, thank you for adding these details.  I was hoping others would
>>>>> chime in as well.  I agree at minimum there should be a warning.
>>>> 
>>>> A warning could be added.
>>>> 
>>>>> Perhaps instead of making INTEGRITY_CA_MACHINE_KEYRING dependent on
>>>>> INTEGRITY_MACHINE_KEYRING, make them a Kconfig "choice" to support the
>>>>> more restrictive certificate use case requirements:  all certificates,
>>>>> CA certificate signing and digital signature, only CA certificate
>>>>> signing.
>>>> 
>>>> As could support for additional restrictions.
>>>> 
>>>> Would these additions be required within this series? What is missing from this 
>>>> discussion is why would these additions be necessary?  Why should the kernel 
>>>> enforce a restriction that is beyond the scope of the X.509 spec?  If a warning was 
>>>> to be added, what would be the justification for adding this additional code?  From 
>>>> my research every single 3rd party code signing intermediate CA would be flagged 
>>>> with the warning.  Isn’t this just going to cause confusion?  Or is there a benefit that 
>>>> I am missing that needs to be stated?
>>> 
>>> You're focusing on third party kernel modules and forgetting about the
>>> simple use case of allowing an end user (or business) to sign their own
>>> code.  True they could use the less restrictive CA certificates, but it
>>> is unnecessary.
>> 
>> My focus is on signing user-space applications, as outlined in the cover letter.  This 
>> series has nothing to do with kernel modules.  Most end-users and businesses rely on 
>> a third party to deal with code signing.  All third party code signing services I have 
>> found use an intermediate CA containing more than just the keyCertSign usage set.  
>> It seems to be an industry accepted practice that does not violate the spec. Before writing
>> new code to either warn or exclude a third party intermediate CA,  I would like to understand 
>> the motivation behind this request.
> 
> In older discussions there are comments like, "Any CA certificate, no
> matter if it's a root or an intermediate, must have the keyCertSign
> extension. If you want to sign a revocation list (CRL) with the CA
> certificate as well (you usually do want that), than you have to add
> cRLSign as well. Any other keyUsages can and should be avoided for CA
> certificates."
> 
> The question as to "why" this changed to include "digitalSignature" was
> posed here [2] with the response being for "OCSP".   It also includes a
> link to Entrusts root and intermediate CAs with just keyCertSign and
> cRLSign keyUsages.
> 
> The matchine keyring is a means of establishing a new root of trust. 
> The motivation for further restricting CA certificates to just
> keyCertSign and cRLSign keyUsages is to limit how the CA certificates
> may be used.  They should not be used for code signing.

Fair enough.  If this will be viewed as justification for adding the additional 
code, I can work on adding it.  Above you mentioned a warning would be needed 
at a minimum and a restriction could be placed behind a Kconfig.  How about for 
the default case I add the warning and when compiling with 
INTEGRITY_CA_MACHINE_KEYRING the restriction will be enforced.

Re: [PATCH v3 00/10] Add CA enforcement keyring restrictions

[Mimi Zohar]

On Fri, 2022-12-23 at 16:13 +0000, Eric Snowberg wrote:
> 
> > On Dec 22, 2022, at 8:41 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> > 
> > On Thu, 2022-12-22 at 15:15 +0000, Eric Snowberg wrote:
> >> 
> >>> On Dec 21, 2022, at 12:01 PM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> >>> 
> >>> On Wed, 2022-12-21 at 18:27 +0000, Eric Snowberg wrote:
> >>>> 
> >>>>> On Dec 18, 2022, at 5:21 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> >>>>> 
> >>>>> On Fri, 2022-12-16 at 22:06 +0800, Coiby Xu wrote:
> >>>>>> Hi Eric and Mimi,
> >>>>>> 
> >>>>>> On Thu, Dec 15, 2022 at 09:45:37PM +0000, Eric Snowberg wrote:
> >>>>>>> 
> >>>>>>> 
> >>>>>>>>>>>>> A CA cert shall be defined as any X509 certificate that contains the
> >>>>>>>>>>>>> keyCertSign key usage and has the CA bit set to true.
> >>>>>>>>>>>> 
> >>>>>>>>>>>> Hi Eric,
> >>>>>>>>>>>> 
> >>>>>>>>>>>> Allowing CA certificates with the digitalSignature key usage flag
> >>>>>>>>>>>> enabled defeats the purpose of the new Kconfig.  Please update the
> >>>>>>>>>>>> above definition to exclude the digitalSignature key usage flag and
> >>>>>>>>>>>> modify the code accordingly.
> >>>>>>>>>>> 
> >>>>>>>>>>> Within v2, the request was made to allow Intermediate CA certificates to be
> >>>>>>>>>>> loaded directly.  The Intermediate CA referenced was the one used by kernel.org.
> >>>>>>>>>>> This Intermediate CA contains both digitalSignature and keyCertSign.  If the code
> >>>>>>>>>>> is changed to exclude this certificate, now the root CA has to be loaded again.  Is that
> >>>>>>>>>>> the intent?
> >>>>>>>>>> 
> >>>>>>>>>> That definitely was not the intent.  Nor would it address the issue of
> >>>>>>>>>> a particular intermediate CA certificate having both keyCertSign and
> >>>>>>>>>> digitalSignature.
> >>>>>>>>> 
> >>>>>>>>> Sorry, I’m not following.  Why is it an issue that an intermediate CA certificate contains
> >>>>>>>>> both keyCertSign and digitalSignature? Why would we want to exclude an Intermediate
> >>>>>>>>> CA cert like the one used on kernel.org?
> >>>>>>>> 
> >>>>>>>> I must be missing something.  Isn't the purpose of "keyUsage" to
> >>>>>>>> minimize how a certificate may be used?   Why would we want the same
> >>>>>>>> certificate to be used for both certificate signing and code signing?
> >>>>>>> 
> >>>>>>> Every 3rd party intermediate CA I have looked at so far contains both set. Most have CRLSign set.
> >>>>>>> Typically the root CA contains keyCertSign and CRLSign, but some also have digitalSignature
> >>>>>>> set.  Finding a 3rd party Intermediate CA without digitalSignature set is probably going to be
> >>>>>>> challenging and will severely limit usage.
> >>>>>> 
> >>>>>> How about allowing both keyCertSign and digitalSignature asserted but
> >>>>>> issuing a warning for this case?
> >>>>>> 
> >>>>>> Here's my rationale for this proposal.
> >>>>>> 
> >>>>>> I assume we should conform to some X.509 specifications. So I checked
> >>>>>> "RFC 5280: Internet X.509 Public Key Infrastructure Certificate and
> >>>>>> Certificate Revocation List (CRL) Profile" [1] and ITU-T X.509 (2012-10)
> >>>>>> [2].
> >>>>>> 
> >>>>>> [1] states in 4.2.1.3. Key Usage,
> >>>>>>  "If the keyUsage extension is present, then the subject public key
> >>>>>>  MUST NOT be used to verify signatures on certificates or CRLs unless
> >>>>>>  the corresponding keyCertSign or cRLSign bit is set.  If the subject
> >>>>>>  public key is only to be used for verifying signatures on
> >>>>>>  certificates and/or CRLs, then the digitalSignature and
> >>>>>>  nonRepudiation bits SHOULD NOT be set.  However, the digitalSignature
> >>>>>>  and/or nonRepudiation bits MAY be set in addition to the keyCertSign
> >>>>>>  and/or cRLSign bits if the subject public key is to be used to verify
> >>>>>>  signatures on certificates and/or CRLs as well as other objects."
> >>>>>> 
> >>>>>> and [2] states in 8.2.2.3 Key usage extension that,
> >>>>>> "More than one bit may be set in an instance of the keyUsage extension.
> >>>>>> The setting of multiple bits shall not change the meaning of each
> >>>>>> individual bit but shall indicate that the certificate may be used for
> >>>>>> all of the purposes indicated by the set bits. There may be risks
> >>>>>> incurred when setting multiple bits. A review of those risks is
> >>>>>> documented in Annex I."
> >>>>>> 
> >>>>>> I interpret the above texts as we should allow both keyCertSign and
> >>>>>> digitalSignature. However [2] warns about the risks of setting multiple
> >>>>>> bits. Quoting Annex I,
> >>>>>> 
> >>>>>> "Combining the contentCommitment bit in the keyUsage certificate
> >>>>>> extension with other keyUsage bits may have security implications
> >>>>>> depending on the security environment in which the certificate is to be
> >>>>>> used. If the subject's environment can be fully controlled and trusted,
> >>>>>> then there are no specific security implications. For example, in cases
> >>>>>> where the subject is fully confident about exactly which data is signed
> >>>>>> or cases where the subject is fully confident about the security
> >>>>>> characteristics of the authentication protocol being used. If the
> >>>>>> subject's environment is not fully controlled or not fully trusted, then
> >>>>>> unintentional signing of commitments is possible. Examples include the
> >>>>>> use of badly formed authentication exchanges and the use of a rogue
> >>>>>> software component. If untrusted environments are used by a subject,
> >>>>>> these security implications can be limited through use of the following
> >>>>>> measures:   
> >>>>>>  – to not combine the contentCommitment key usage setting in
> >>>>>>    certificates with any other key usage setting and to use the
> >>>>>>    corresponding private key only with this certificate;   
> >>>>>> 
> >>>>>>  – to limit the use of private keys associated with certificates that
> >>>>>>    have the contentCommitment key usage bit set, to environments which
> >>>>>>    are considered adequately controlled and trustworthy"
> >>>>>> 
> >>>>>> So maybe it's useful to add a warning if both keyCertSign and
> >>>>>> digitalSignature are asserted.
> >>>>> 
> >>>>> Coiby, thank you for adding these details.  I was hoping others would
> >>>>> chime in as well.  I agree at minimum there should be a warning.
> >>>> 
> >>>> A warning could be added.
> >>>> 
> >>>>> Perhaps instead of making INTEGRITY_CA_MACHINE_KEYRING dependent on
> >>>>> INTEGRITY_MACHINE_KEYRING, make them a Kconfig "choice" to support the
> >>>>> more restrictive certificate use case requirements:  all certificates,
> >>>>> CA certificate signing and digital signature, only CA certificate
> >>>>> signing.
> >>>> 
> >>>> As could support for additional restrictions.
> >>>> 
> >>>> Would these additions be required within this series? What is missing from this 
> >>>> discussion is why would these additions be necessary?  Why should the kernel 
> >>>> enforce a restriction that is beyond the scope of the X.509 spec?  If a warning was 
> >>>> to be added, what would be the justification for adding this additional code?  From 
> >>>> my research every single 3rd party code signing intermediate CA would be flagged 
> >>>> with the warning.  Isn’t this just going to cause confusion?  Or is there a benefit that 
> >>>> I am missing that needs to be stated?
> >>> 
> >>> You're focusing on third party kernel modules and forgetting about the
> >>> simple use case of allowing an end user (or business) to sign their own
> >>> code.  True they could use the less restrictive CA certificates, but it
> >>> is unnecessary.
> >> 
> >> My focus is on signing user-space applications, as outlined in the cover letter.  This 
> >> series has nothing to do with kernel modules.  Most end-users and businesses rely on 
> >> a third party to deal with code signing.  All third party code signing services I have 
> >> found use an intermediate CA containing more than just the keyCertSign usage set.  
> >> It seems to be an industry accepted practice that does not violate the spec. Before writing
> >> new code to either warn or exclude a third party intermediate CA,  I would like to understand 
> >> the motivation behind this request.
> > 
> > In older discussions there are comments like, "Any CA certificate, no
> > matter if it's a root or an intermediate, must have the keyCertSign
> > extension. If you want to sign a revocation list (CRL) with the CA
> > certificate as well (you usually do want that), than you have to add
> > cRLSign as well. Any other keyUsages can and should be avoided for CA
> > certificates."
> > 
> > The question as to "why" this changed to include "digitalSignature" was
> > posed here [2] with the response being for "OCSP".   It also includes a
> > link to Entrusts root and intermediate CAs with just keyCertSign and
> > cRLSign keyUsages.
> > 
> > The matchine keyring is a means of establishing a new root of trust. 
> > The motivation for further restricting CA certificates to just
> > keyCertSign and cRLSign keyUsages is to limit how the CA certificates
> > may be used.  They should not be used for code signing.
> 
> Fair enough.  If this will be viewed as justification for adding the additional 
> code, I can work on adding it.  Above you mentioned a warning would be needed 
> at a minimum and a restriction could be placed behind a Kconfig.  How about for 
> the default case I add the warning and when compiling with 
> INTEGRITY_CA_MACHINE_KEYRING the restriction will be enforced.

Sounds good to me.  To avoid misunderstandings, will there be a Kconfig
menu with 3 options?   There were a couple of other comments having to
do with variable names.  Will you address them as well?

-- 
thanks,

Mimi

Re: [PATCH v3 00/10] Add CA enforcement keyring restrictions

[Eric Snowberg]

> On Dec 23, 2022, at 9:34 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> 
> On Fri, 2022-12-23 at 16:13 +0000, Eric Snowberg wrote:
>> 
>>> On Dec 22, 2022, at 8:41 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
>>> 
>>> On Thu, 2022-12-22 at 15:15 +0000, Eric Snowberg wrote:
>>>> 
>>>>> On Dec 21, 2022, at 12:01 PM, Mimi Zohar <zohar@linux.ibm.com> wrote:
>>>>> 
>>>>> On Wed, 2022-12-21 at 18:27 +0000, Eric Snowberg wrote:
>>>>>> 
>>>>>>> On Dec 18, 2022, at 5:21 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
>>>>>>> 
>>>>>>> On Fri, 2022-12-16 at 22:06 +0800, Coiby Xu wrote:
>>>>>>>> Hi Eric and Mimi,
>>>>>>>> 
>>>>>>>> On Thu, Dec 15, 2022 at 09:45:37PM +0000, Eric Snowberg wrote:
>>>>>>>>> 
>>>>>>>>> 
>>>>>>>>>>>>>>> A CA cert shall be defined as any X509 certificate that contains the
>>>>>>>>>>>>>>> keyCertSign key usage and has the CA bit set to true.
>>>>>>>>>>>>>> 
>>>>>>>>>>>>>> Hi Eric,
>>>>>>>>>>>>>> 
>>>>>>>>>>>>>> Allowing CA certificates with the digitalSignature key usage flag
>>>>>>>>>>>>>> enabled defeats the purpose of the new Kconfig.  Please update the
>>>>>>>>>>>>>> above definition to exclude the digitalSignature key usage flag and
>>>>>>>>>>>>>> modify the code accordingly.
>>>>>>>>>>>>> 
>>>>>>>>>>>>> Within v2, the request was made to allow Intermediate CA certificates to be
>>>>>>>>>>>>> loaded directly.  The Intermediate CA referenced was the one used by kernel.org.
>>>>>>>>>>>>> This Intermediate CA contains both digitalSignature and keyCertSign.  If the code
>>>>>>>>>>>>> is changed to exclude this certificate, now the root CA has to be loaded again.  Is that
>>>>>>>>>>>>> the intent?
>>>>>>>>>>>> 
>>>>>>>>>>>> That definitely was not the intent.  Nor would it address the issue of
>>>>>>>>>>>> a particular intermediate CA certificate having both keyCertSign and
>>>>>>>>>>>> digitalSignature.
>>>>>>>>>>> 
>>>>>>>>>>> Sorry, I’m not following.  Why is it an issue that an intermediate CA certificate contains
>>>>>>>>>>> both keyCertSign and digitalSignature? Why would we want to exclude an Intermediate
>>>>>>>>>>> CA cert like the one used on kernel.org?
>>>>>>>>>> 
>>>>>>>>>> I must be missing something.  Isn't the purpose of "keyUsage" to
>>>>>>>>>> minimize how a certificate may be used?   Why would we want the same
>>>>>>>>>> certificate to be used for both certificate signing and code signing?
>>>>>>>>> 
>>>>>>>>> Every 3rd party intermediate CA I have looked at so far contains both set. Most have CRLSign set.
>>>>>>>>> Typically the root CA contains keyCertSign and CRLSign, but some also have digitalSignature
>>>>>>>>> set.  Finding a 3rd party Intermediate CA without digitalSignature set is probably going to be
>>>>>>>>> challenging and will severely limit usage.
>>>>>>>> 
>>>>>>>> How about allowing both keyCertSign and digitalSignature asserted but
>>>>>>>> issuing a warning for this case?
>>>>>>>> 
>>>>>>>> Here's my rationale for this proposal.
>>>>>>>> 
>>>>>>>> I assume we should conform to some X.509 specifications. So I checked
>>>>>>>> "RFC 5280: Internet X.509 Public Key Infrastructure Certificate and
>>>>>>>> Certificate Revocation List (CRL) Profile" [1] and ITU-T X.509 (2012-10)
>>>>>>>> [2].
>>>>>>>> 
>>>>>>>> [1] states in 4.2.1.3. Key Usage,
>>>>>>>> "If the keyUsage extension is present, then the subject public key
>>>>>>>> MUST NOT be used to verify signatures on certificates or CRLs unless
>>>>>>>> the corresponding keyCertSign or cRLSign bit is set.  If the subject
>>>>>>>> public key is only to be used for verifying signatures on
>>>>>>>> certificates and/or CRLs, then the digitalSignature and
>>>>>>>> nonRepudiation bits SHOULD NOT be set.  However, the digitalSignature
>>>>>>>> and/or nonRepudiation bits MAY be set in addition to the keyCertSign
>>>>>>>> and/or cRLSign bits if the subject public key is to be used to verify
>>>>>>>> signatures on certificates and/or CRLs as well as other objects."
>>>>>>>> 
>>>>>>>> and [2] states in 8.2.2.3 Key usage extension that,
>>>>>>>> "More than one bit may be set in an instance of the keyUsage extension.
>>>>>>>> The setting of multiple bits shall not change the meaning of each
>>>>>>>> individual bit but shall indicate that the certificate may be used for
>>>>>>>> all of the purposes indicated by the set bits. There may be risks
>>>>>>>> incurred when setting multiple bits. A review of those risks is
>>>>>>>> documented in Annex I."
>>>>>>>> 
>>>>>>>> I interpret the above texts as we should allow both keyCertSign and
>>>>>>>> digitalSignature. However [2] warns about the risks of setting multiple
>>>>>>>> bits. Quoting Annex I,
>>>>>>>> 
>>>>>>>> "Combining the contentCommitment bit in the keyUsage certificate
>>>>>>>> extension with other keyUsage bits may have security implications
>>>>>>>> depending on the security environment in which the certificate is to be
>>>>>>>> used. If the subject's environment can be fully controlled and trusted,
>>>>>>>> then there are no specific security implications. For example, in cases
>>>>>>>> where the subject is fully confident about exactly which data is signed
>>>>>>>> or cases where the subject is fully confident about the security
>>>>>>>> characteristics of the authentication protocol being used. If the
>>>>>>>> subject's environment is not fully controlled or not fully trusted, then
>>>>>>>> unintentional signing of commitments is possible. Examples include the
>>>>>>>> use of badly formed authentication exchanges and the use of a rogue
>>>>>>>> software component. If untrusted environments are used by a subject,
>>>>>>>> these security implications can be limited through use of the following
>>>>>>>> measures:   
>>>>>>>> – to not combine the contentCommitment key usage setting in
>>>>>>>>   certificates with any other key usage setting and to use the
>>>>>>>>   corresponding private key only with this certificate;   
>>>>>>>> 
>>>>>>>> – to limit the use of private keys associated with certificates that
>>>>>>>>   have the contentCommitment key usage bit set, to environments which
>>>>>>>>   are considered adequately controlled and trustworthy"
>>>>>>>> 
>>>>>>>> So maybe it's useful to add a warning if both keyCertSign and
>>>>>>>> digitalSignature are asserted.
>>>>>>> 
>>>>>>> Coiby, thank you for adding these details.  I was hoping others would
>>>>>>> chime in as well.  I agree at minimum there should be a warning.
>>>>>> 
>>>>>> A warning could be added.
>>>>>> 
>>>>>>> Perhaps instead of making INTEGRITY_CA_MACHINE_KEYRING dependent on
>>>>>>> INTEGRITY_MACHINE_KEYRING, make them a Kconfig "choice" to support the
>>>>>>> more restrictive certificate use case requirements:  all certificates,
>>>>>>> CA certificate signing and digital signature, only CA certificate
>>>>>>> signing.
>>>>>> 
>>>>>> As could support for additional restrictions.
>>>>>> 
>>>>>> Would these additions be required within this series? What is missing from this 
>>>>>> discussion is why would these additions be necessary?  Why should the kernel 
>>>>>> enforce a restriction that is beyond the scope of the X.509 spec?  If a warning was 
>>>>>> to be added, what would be the justification for adding this additional code?  From 
>>>>>> my research every single 3rd party code signing intermediate CA would be flagged 
>>>>>> with the warning.  Isn’t this just going to cause confusion?  Or is there a benefit that 
>>>>>> I am missing that needs to be stated?
>>>>> 
>>>>> You're focusing on third party kernel modules and forgetting about the
>>>>> simple use case of allowing an end user (or business) to sign their own
>>>>> code.  True they could use the less restrictive CA certificates, but it
>>>>> is unnecessary.
>>>> 
>>>> My focus is on signing user-space applications, as outlined in the cover letter.  This 
>>>> series has nothing to do with kernel modules.  Most end-users and businesses rely on 
>>>> a third party to deal with code signing.  All third party code signing services I have 
>>>> found use an intermediate CA containing more than just the keyCertSign usage set.  
>>>> It seems to be an industry accepted practice that does not violate the spec. Before writing
>>>> new code to either warn or exclude a third party intermediate CA,  I would like to understand 
>>>> the motivation behind this request.
>>> 
>>> In older discussions there are comments like, "Any CA certificate, no
>>> matter if it's a root or an intermediate, must have the keyCertSign
>>> extension. If you want to sign a revocation list (CRL) with the CA
>>> certificate as well (you usually do want that), than you have to add
>>> cRLSign as well. Any other keyUsages can and should be avoided for CA
>>> certificates."
>>> 
>>> The question as to "why" this changed to include "digitalSignature" was
>>> posed here [2] with the response being for "OCSP".   It also includes a
>>> link to Entrusts root and intermediate CAs with just keyCertSign and
>>> cRLSign keyUsages.
>>> 
>>> The matchine keyring is a means of establishing a new root of trust. 
>>> The motivation for further restricting CA certificates to just
>>> keyCertSign and cRLSign keyUsages is to limit how the CA certificates
>>> may be used.  They should not be used for code signing.
>> 
>> Fair enough.  If this will be viewed as justification for adding the additional 
>> code, I can work on adding it.  Above you mentioned a warning would be needed 
>> at a minimum and a restriction could be placed behind a Kconfig.  How about for 
>> the default case I add the warning and when compiling with 
>> INTEGRITY_CA_MACHINE_KEYRING the restriction will be enforced.
> 
> Sounds good to me.  To avoid misunderstandings, will there be a Kconfig
> menu with 3 options?  

I will add the three options in the next round.

> There were a couple of other comments having to
> do with variable names.  Will you address them as well?

And take care of the variable name changes.  I won’t get back to this until January.

Re: [PATCH v3 00/10] Add CA enforcement keyring restrictions

[Mimi Zohar]

On Fri, 2022-12-23 at 18:17 +0000, Eric Snowberg wrote:
> >> Fair enough.  If this will be viewed as justification for adding the additional 
> >> code, I can work on adding it.  Above you mentioned a warning would be needed 
> >> at a minimum and a restriction could be placed behind a Kconfig.  How about for 
> >> the default case I add the warning and when compiling with 
> >> INTEGRITY_CA_MACHINE_KEYRING the restriction will be enforced.
> > 
> > Sounds good to me.  To avoid misunderstandings, will there be a Kconfig
> > menu with 3 options?  
> 
> I will add the three options in the next round.
> 
> > There were a couple of other comments having to
> > do with variable names.  Will you address them as well?
> 
> And take care of the variable name changes.  I won’t get back to this until January.

Enjoy your vacation and the holidays.  Looking forward to the next
version.

-- 
thanks,

Mimi

Re: [PATCH v3 01/10] KEYS: Create static version of public_key_verify_signature

[Jarkko Sakkinen]

On Tue, Dec 13, 2022 at 07:33:52PM -0500, Eric Snowberg wrote:
> The kernel test robot reports undefined reference to
> public_key_verify_signature when CONFIG_ASYMMETRIC_PUBLIC_KEY_SUBTYPE is
> not defined. Create a static version in this case and return -EINVAL.
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
> Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
> Reviewed-by: Petr Vorel <pvorel@suse.cz>
> ---
>  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
> 

Should this have fixes tag?

BR, Jarkko

Re: [PATCH v3 02/10] KEYS: Add missing function documentation

[Jarkko Sakkinen]

On Tue, Dec 13, 2022 at 07:33:53PM -0500, Eric Snowberg wrote:
> Compiling with 'W=1' results in warnings that 'Function parameter or member
> not described'
> 
> Add the missing parameters for
> restrict_link_by_builtin_and_secondary_trusted and
> restrict_link_to_builtin_trusted.
> 
> Use /* instead of /** for get_builtin_and_secondary_restriction, since
> it is a static function.
> 
> Fix wrong function name restrict_link_to_builtin_trusted brought by:
> commit d3bfe84129f6 ("certs: Add a secondary system keyring that can be added to dynamically")
> 
> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
> Reviewed-by: Petr Vorel <pvorel@suse.cz>
> ---
>  certs/system_keyring.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/certs/system_keyring.c b/certs/system_keyring.c
> index 5042cc54fa5e..e531b88bc570 100644
> --- a/certs/system_keyring.c
> +++ b/certs/system_keyring.c
> @@ -33,7 +33,11 @@ extern __initconst const unsigned long system_certificate_list_size;
>  extern __initconst const unsigned long module_cert_size;
>  
>  /**
> - * restrict_link_to_builtin_trusted - Restrict keyring addition by built in CA
> + * restrict_link_by_builtin_trusted - Restrict keyring addition by built in CA
> + * @dest_keyring: Keyring being linked to.
> + * @type: The type of key being added.
> + * @payload: The payload of the new key.
> + * @restriction_key: A ring of keys that can be used to vouch for the new cert.
>   *
>   * Restrict the addition of keys into a keyring based on the key-to-be-added
>   * being vouched for by a key in the built in system keyring.
> @@ -50,7 +54,11 @@ int restrict_link_by_builtin_trusted(struct key *dest_keyring,
>  #ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
>  /**
>   * restrict_link_by_builtin_and_secondary_trusted - Restrict keyring
> - *   addition by both builtin and secondary keyrings
> + *   addition by both builtin and secondary keyrings.
> + * @dest_keyring: Keyring being linked to.
> + * @type: The type of key being added.
> + * @payload: The payload of the new key.
> + * @restrict_key: A ring of keys that can be used to vouch for the new cert.
>   *
>   * Restrict the addition of keys into a keyring based on the key-to-be-added
>   * being vouched for by a key in either the built-in or the secondary system
> @@ -75,7 +83,7 @@ int restrict_link_by_builtin_and_secondary_trusted(
>  					  secondary_trusted_keys);
>  }
>  
> -/**
> +/*
>   * Allocate a struct key_restriction for the "builtin and secondary trust"
>   * keyring. Only for use in system_trusted_keyring_init().
>   */
> -- 
> 2.27.0
> 

Should this have a fixes tag?

BR, Jarkko

Re: [PATCH v3 03/10] KEYS: X.509: Parse Basic Constraints for CA

[Jarkko Sakkinen]

On Tue, Dec 13, 2022 at 07:33:54PM -0500, 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.

Please add:

Link: https://www.rfc-editor.org/rfc/rfc5280 # 4.2.1.9. Basic Constraints
 
> 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 7a9b084e2043..b4443e507153 100644
> --- a/crypto/asymmetric_keys/x509_cert_parser.c
> +++ b/crypto/asymmetric_keys/x509_cert_parser.c
> @@ -586,6 +586,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;

Why this instead of either:

1. Each check in separate if-statement.
2. All in a single statement:
   vlen < 2 || v[0] != (ASN1_CONS_BIT | ASN1_SEQ) || v[1] != vlen - 2

It would be also nice to have some sort of explanation in a comment, given
the cryptic statement and the amount of magic numbers in it. I.e. in plain
English what does the check actually means.


> +		if (vlen >= 4 && v[1] != 0 && v[2] == ASN1_BOOL && v[3] == 1)
> +			ctx->cert->root_ca = true;

Ditto for the explanation part. I have really hard time deciphering this.

> +	}
> +
>  	return 0;
>  }
>  
> diff --git a/crypto/asymmetric_keys/x509_parser.h b/crypto/asymmetric_keys/x509_parser.h
> index a299c9c56f40..7c5c0ad1c22e 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		root_ca;		/* T if basic constraints CA is set */
>  };
>  
>  /*
> -- 
> 2.27.0
> 

BR, Jarkko

Re: [PATCH v3 04/10] KEYS: X.509: Parse Key Usage

[Jarkko Sakkinen]

On Tue, Dec 13, 2022 at 07:33:55PM -0500, 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 | 22 ++++++++++++++++++++++
>  crypto/asymmetric_keys/x509_parser.h      |  1 +
>  2 files changed, 23 insertions(+)
> 
> diff --git a/crypto/asymmetric_keys/x509_cert_parser.c b/crypto/asymmetric_keys/x509_cert_parser.c
> index b4443e507153..edb22cf04eed 100644
> --- a/crypto/asymmetric_keys/x509_cert_parser.c
> +++ b/crypto/asymmetric_keys/x509_cert_parser.c
> @@ -579,6 +579,28 @@ 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)
> +			return -EBADMSG;
> +		if (vlen < 4)
> +			return -EBADMSG;
> +		if (v[1] == 0x02 && v[2] <= 2 && (v[3] & 0x04))
> +			ctx->cert->kcs_set = true;
> +		else if (vlen > 4 && v[1] == 0x03 && (v[3] & 0x04))
> +			ctx->cert->kcs_set = true;
> +		return 0;

This is much more easier to follow thanks to explanation.

> +	}
> +
>  	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 7c5c0ad1c22e..74a9f929e400 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		root_ca;		/* T if basic constraints CA is set */
> +	bool		kcs_set;		/* T if keyCertSign is set */
>  };
>  
>  /*
> -- 
> 2.27.0
> 

LGTM but I'll hold with reviewed-by's up until the patch set overally
looks good to me and I have actually tested it.

BR, Jarkko

Re: [PATCH v3 05/10] KEYS: Introduce a CA endorsed flag

[Jarkko Sakkinen]

On Tue, Dec 13, 2022 at 07:33:56PM -0500, Eric Snowberg wrote:
> Some subsystems are interested in knowing if a key has been endorsed
> as or by a Certificate Authority (CA). From the data contained in struct
> key, it is not possible to make this determination after the key
> parsing is complete.  Introduce a new Endorsed Certificate Authority
> flag called KEY_FLAG_ECA.
> 
> The first type of key to use this is X.509.  When a X.509 certificate
> has the keyCertSign Key Usage set and contains the CA bit set, this new
> flag is set. In the future, other usage fields could be added as flags,
> i.e. digitialSignature.
> 
> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
> ---
>  crypto/asymmetric_keys/x509_public_key.c | 3 +++
>  include/linux/key-type.h                 | 2 ++
>  include/linux/key.h                      | 2 ++
>  security/keys/key.c                      | 8 ++++++++
>  4 files changed, 15 insertions(+)
> 
> diff --git a/crypto/asymmetric_keys/x509_public_key.c b/crypto/asymmetric_keys/x509_public_key.c
> index 0b4943a4592b..fd1d7d6e68e7 100644
> --- a/crypto/asymmetric_keys/x509_public_key.c
> +++ b/crypto/asymmetric_keys/x509_public_key.c
> @@ -208,6 +208,9 @@ static int x509_key_preparse(struct key_preparsed_payload *prep)
>  		goto error_free_kids;
>  	}
>  

A comment here?

> +	if (cert->kcs_set && cert->root_ca)
> +		prep->payload_flags |= KEY_ALLOC_PECA;
> +
>  	/* We're pinning the module by being linked against it */
>  	__module_get(public_key_subtype.owner);
>  	prep->payload.data[asym_subtype] = &public_key_subtype;
> diff --git a/include/linux/key-type.h b/include/linux/key-type.h
> index 7d985a1dfe4a..0b500578441c 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_PECA	0x0001		/* Proposed Endorsed CA (ECA) 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 d27477faf00d..21d5a13ee4a9 100644
> --- a/include/linux/key.h
> +++ b/include/linux/key.h
> @@ -236,6 +236,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_ECA		10	/* set if key is an Endorsed CA key */
>  
>  	/* the key type and key description string
>  	 * - the desc is used to match a key against search criteria
> @@ -296,6 +297,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_ECA			0x0040	/* Add Endorsed CA 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..e6b4946aca70 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_ECA)
> +		key->flags |= 1 << KEY_FLAG_ECA;
>  
>  #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_ECA flag to be set by preparser contents */
> +	if (prep.payload_flags & KEY_ALLOC_PECA)
> +		flags |= KEY_ALLOC_ECA;
> +	else
> +		flags &= ~KEY_ALLOC_ECA;
> +
>  	/* allocate a new key */
>  	key = key_alloc(index_key.type, index_key.description,
>  			cred->fsuid, cred->fsgid, cred, perm, flags, NULL);
> -- 
> 2.27.0
> 

BR, Jarkko

Re: [PATCH v3 09/10] KEYS: CA link restriction

[Jarkko Sakkinen]

On Tue, Dec 13, 2022 at 07:34:00PM -0500, Eric Snowberg wrote:
> +/**
> + * restrict_link_by_ca - Restrict additions to a ring of CA keys
> + * @dest_keyring: Keyring being linked to.
> + * @type: The type of key being added.
> + * @payload: The payload of the new key.
> + * @trust_keyring: Unused.
> + *
> + * Check if the new certificate is a CA. If it is a CA, then mark the new
> + * certificate as being ok to link.
> + *
> + * Returns 0 if the new certificate was accepted, -ENOKEY if the
> + * certificate is not a CA. -ENOPKG if the signature uses unsupported
> + * crypto, or some other error if there is a matching certificate but
> + * the signature check cannot be performed.
> + */
> +int restrict_link_by_ca(struct key *dest_keyring,
> +			const struct key_type *type,
> +			const union key_payload *payload,
> +			struct key *trust_keyring)

Why you want to define trust_keyring, other than matching the parameter
list in restrict_link_by_signature()?

Also if it is unused, it should be then just "struct key *)", right?

BR, Jarkko

Re: [PATCH v3 09/10] KEYS: CA link restriction

[Jarkko Sakkinen]

On Wed, Jan 04, 2023 at 11:51:52AM +0000, Jarkko Sakkinen wrote:
> On Tue, Dec 13, 2022 at 07:34:00PM -0500, Eric Snowberg wrote:
> > +/**
> > + * restrict_link_by_ca - Restrict additions to a ring of CA keys
> > + * @dest_keyring: Keyring being linked to.
> > + * @type: The type of key being added.
> > + * @payload: The payload of the new key.
> > + * @trust_keyring: Unused.
> > + *
> > + * Check if the new certificate is a CA. If it is a CA, then mark the new
> > + * certificate as being ok to link.
> > + *
> > + * Returns 0 if the new certificate was accepted, -ENOKEY if the
> > + * certificate is not a CA. -ENOPKG if the signature uses unsupported
> > + * crypto, or some other error if there is a matching certificate but
> > + * the signature check cannot be performed.
> > + */
> > +int restrict_link_by_ca(struct key *dest_keyring,
> > +			const struct key_type *type,
> > +			const union key_payload *payload,
> > +			struct key *trust_keyring)
> 
> Why you want to define trust_keyring, other than matching the parameter
> list in restrict_link_by_signature()?
> 
> Also if it is unused, it should be then just "struct key *)", right?

Please ignore, I forgot how this worked, i.e. "restriction" is set to
the correct function so this looks correct to me :-) So it's good.
Sorry for the confusion.

BR, Jarkko

Re: [PATCH v3 03/10] KEYS: X.509: Parse Basic Constraints for CA

[Jarkko Sakkinen]

On Thu, Dec 15, 2022 at 06:10:04AM -0500, Mimi Zohar wrote:
> > diff --git a/crypto/asymmetric_keys/x509_parser.h b/crypto/asymmetric_keys/x509_parser.h
> > index a299c9c56f40..7c5c0ad1c22e 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		root_ca;		/* T if basic constraints CA is set */
> >  }; 
> 
> The variable "root_ca" should probably be renamed to just "ca", right?

Perhaps is_ca?

BR, Jarkko

Re: [PATCH v3 03/10] KEYS: X.509: Parse Basic Constraints for CA

[Eric Snowberg]

> On Jan 4, 2023, at 5:29 AM, Jarkko Sakkinen <jarkko@kernel.org> wrote:
> 
> On Thu, Dec 15, 2022 at 06:10:04AM -0500, Mimi Zohar wrote:
>>> diff --git a/crypto/asymmetric_keys/x509_parser.h b/crypto/asymmetric_keys/x509_parser.h
>>> index a299c9c56f40..7c5c0ad1c22e 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		root_ca;		/* T if basic constraints CA is set */
>>> }; 
>> 
>> The variable "root_ca" should probably be renamed to just "ca", right?
> 
> Perhaps is_ca?

I am open to renaming this, but need an agreement on whether the “is_” should be used or not:

https://lore.kernel.org/lkml/b28ea211d88e968a5487b20477236e9b507755f4.camel@linux.ibm.com/

Re: [PATCH v3 04/10] KEYS: X.509: Parse Key Usage

[Eric Snowberg]

> On Jan 4, 2023, at 4:43 AM, Jarkko Sakkinen <jarkko@kernel.org> wrote:
> 
> On Tue, Dec 13, 2022 at 07:33:55PM -0500, 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 | 22 ++++++++++++++++++++++
>> crypto/asymmetric_keys/x509_parser.h      |  1 +
>> 2 files changed, 23 insertions(+)
>> 
>> diff --git a/crypto/asymmetric_keys/x509_cert_parser.c b/crypto/asymmetric_keys/x509_cert_parser.c
>> index b4443e507153..edb22cf04eed 100644
>> --- a/crypto/asymmetric_keys/x509_cert_parser.c
>> +++ b/crypto/asymmetric_keys/x509_cert_parser.c
>> @@ -579,6 +579,28 @@ 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)
>> +			return -EBADMSG;
>> +		if (vlen < 4)
>> +			return -EBADMSG;
>> +		if (v[1] == 0x02 && v[2] <= 2 && (v[3] & 0x04))
>> +			ctx->cert->kcs_set = true;
>> +		else if (vlen > 4 && v[1] == 0x03 && (v[3] & 0x04))
>> +			ctx->cert->kcs_set = true;
>> +		return 0;
> 
> This is much more easier to follow thanks to explanation.
> 
>> +	}
>> +
>> 	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 7c5c0ad1c22e..74a9f929e400 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		root_ca;		/* T if basic constraints CA is set */
>> +	bool		kcs_set;		/* T if keyCertSign is set */
>> };
>> 
>> /*
>> -- 
>> 2.27.0
>> 
> 
> LGTM but I'll hold with reviewed-by's up until the patch set overally
> looks good to me and I have actually tested it.

Thanks for your review.  I will make all the other changes you brought up with
the other patches in the next round.

Re: [PATCH v3 03/10] KEYS: X.509: Parse Basic Constraints for CA

[Mimi Zohar]

On Wed, 2023-01-04 at 20:14 +0000, Eric Snowberg wrote:
> 
> > On Jan 4, 2023, at 5:29 AM, Jarkko Sakkinen <jarkko@kernel.org> wrote:
> > 
> > On Thu, Dec 15, 2022 at 06:10:04AM -0500, Mimi Zohar wrote:
> >>> diff --git a/crypto/asymmetric_keys/x509_parser.h b/crypto/asymmetric_keys/x509_parser.h
> >>> index a299c9c56f40..7c5c0ad1c22e 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		root_ca;		/* T if basic constraints CA is set */
> >>> }; 
> >> 
> >> The variable "root_ca" should probably be renamed to just "ca", right?
> > 
> > Perhaps is_ca?
> 
> I am open to renaming this, but need an agreement on whether the “is_” should be used or not:
> 
> https://lore.kernel.org/lkml/b28ea211d88e968a5487b20477236e9b507755f4.camel@linux.ibm.com/

Examples of both functions and variables exist that are prefixed with
"is_".   One is a question; the other a statement.   Naming the
variable "is_ca" and using it like "if (cert->is_ca)" does make sense.

-- 
thanks,

Mimi