keyrings.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v4 0/6] Add CA enforcement keyring restrictions
@ 2023-02-07  2:59 Eric Snowberg
  2023-02-07  2:59 ` [PATCH v4 1/6] KEYS: Create static version of public_key_verify_signature Eric Snowberg
                   ` (6 more replies)
  0 siblings, 7 replies; 24+ messages in thread
From: Eric Snowberg @ 2023-02-07  2:59 UTC (permalink / raw)
  To: jarkko, zohar, dhowells, dwmw2
  Cc: herbert, davem, dmitry.kasatkin, paul, jmorris, serge, pvorel,
	tadeusz.struk, eric.snowberg, kanth.ghatraju, konrad.wilk,
	erpalmer, coxu, keyrings, linux-kernel, linux-crypto,
	linux-integrity, linux-security-module

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.

This series introduces a way to do CA enforement with the machine
keyring. It introduces three different ways to configure the machine
keyring. A new menu option is added to control the type of keys that may
be added to it.  The options include none, min, and max restrictions. The
default is CONFIG_INTEGRITY_CA_MACHINE_KEYRING_NONE. This allows all MOK
keys into the machine keyring.  When CONFIG_INTEGRITY_CA_MACHINE_KEYRING_MIN
is selected, the X.509 CA bit must be true.  Also, the key usage must
contain keyCertSign, any other usage field may also be set. When 
CONFIG_INTEGRITY_CA_MACHINE_KEYRING_MAX is selected, the X.509 CA bit
must be true.  Also, the key usage must contain keyCertSign and the
digitialSignature usage may not be set. If a key doesn't pass the CA
restriction check, instead of going into the machine keyring, it is
added to the platform keyring. With the ability to configure the machine
keyring with CA restrictions, code that prevented the machine keyring
from being enabled with IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY
has been removed.

Changelog:
v4:
- Removed all code that validated the certificate chain back to the root
  CA. Now the only restriction is what is initially placed in the
  machine keyring.
- Check and store if the X.509 usage contains digitalSignature
- New Kconfig menu item with none, min and max CA restriction on the 
  machine keyring

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 (6):
  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: CA link restriction
  integrity: machine keyring CA configuration

 certs/system_keyring.c                    | 14 +++++--
 crypto/asymmetric_keys/restrict.c         | 40 ++++++++++++++++++
 crypto/asymmetric_keys/x509_cert_parser.c | 50 +++++++++++++++++++++++
 include/crypto/public_key.h               | 28 +++++++++++++
 security/integrity/Kconfig                | 39 +++++++++++++++++-
 security/integrity/digsig.c               |  8 +++-
 6 files changed, 173 insertions(+), 6 deletions(-)


base-commit: 4ec5183ec48656cec489c49f989c508b68b518e3
-- 
2.27.0


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

* [PATCH v4 1/6] KEYS: Create static version of public_key_verify_signature
  2023-02-07  2:59 [PATCH v4 0/6] Add CA enforcement keyring restrictions Eric Snowberg
@ 2023-02-07  2:59 ` Eric Snowberg
  2023-02-10  3:39   ` Jarkko Sakkinen
  2023-02-07  2:59 ` [PATCH v4 2/6] KEYS: Add missing function documentation Eric Snowberg
                   ` (5 subsequent siblings)
  6 siblings, 1 reply; 24+ messages in thread
From: Eric Snowberg @ 2023-02-07  2:59 UTC (permalink / raw)
  To: jarkko, zohar, dhowells, dwmw2
  Cc: herbert, davem, dmitry.kasatkin, paul, jmorris, serge, pvorel,
	tadeusz.struk, eric.snowberg, kanth.ghatraju, konrad.wilk,
	erpalmer, coxu, keyrings, linux-kernel, linux-crypto,
	linux-integrity, linux-security-module

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.

Fixes: db6c43bd2132 ("crypto: KEYS: convert public key and digsig asym to the akcipher api")
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


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

* [PATCH v4 2/6] KEYS: Add missing function documentation
  2023-02-07  2:59 [PATCH v4 0/6] Add CA enforcement keyring restrictions Eric Snowberg
  2023-02-07  2:59 ` [PATCH v4 1/6] KEYS: Create static version of public_key_verify_signature Eric Snowberg
@ 2023-02-07  2:59 ` Eric Snowberg
  2023-02-08 21:48   ` Mimi Zohar
  2023-02-10  3:40   ` Jarkko Sakkinen
  2023-02-07  2:59 ` [PATCH v4 3/6] KEYS: X.509: Parse Basic Constraints for CA Eric Snowberg
                   ` (4 subsequent siblings)
  6 siblings, 2 replies; 24+ messages in thread
From: Eric Snowberg @ 2023-02-07  2:59 UTC (permalink / raw)
  To: jarkko, zohar, dhowells, dwmw2
  Cc: herbert, davem, dmitry.kasatkin, paul, jmorris, serge, pvorel,
	tadeusz.struk, eric.snowberg, kanth.ghatraju, konrad.wilk,
	erpalmer, coxu, keyrings, linux-kernel, linux-crypto,
	linux-integrity, linux-security-module

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.

Fixes: 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


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

* [PATCH v4 3/6] KEYS: X.509: Parse Basic Constraints for CA
  2023-02-07  2:59 [PATCH v4 0/6] Add CA enforcement keyring restrictions Eric Snowberg
  2023-02-07  2:59 ` [PATCH v4 1/6] KEYS: Create static version of public_key_verify_signature Eric Snowberg
  2023-02-07  2:59 ` [PATCH v4 2/6] KEYS: Add missing function documentation Eric Snowberg
@ 2023-02-07  2:59 ` Eric Snowberg
  2023-02-08 21:01   ` Mimi Zohar
  2023-02-10  3:46   ` Jarkko Sakkinen
  2023-02-07  2:59 ` [PATCH v4 4/6] KEYS: X.509: Parse Key Usage Eric Snowberg
                   ` (3 subsequent siblings)
  6 siblings, 2 replies; 24+ messages in thread
From: Eric Snowberg @ 2023-02-07  2:59 UTC (permalink / raw)
  To: jarkko, zohar, dhowells, dwmw2
  Cc: herbert, davem, dmitry.kasatkin, paul, jmorris, serge, pvorel,
	tadeusz.struk, eric.snowberg, kanth.ghatraju, konrad.wilk,
	erpalmer, coxu, keyrings, linux-kernel, linux-crypto,
	linux-integrity, linux-security-module

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 public_key.  This will be used
in a follow on patch that requires knowing if the public key is a CA.

Link: https://www.rfc-editor.org/rfc/rfc5280#section-4.2.1.9
Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
---
 crypto/asymmetric_keys/x509_cert_parser.c | 22 ++++++++++++++++++++++
 include/crypto/public_key.h               |  2 ++
 2 files changed, 24 insertions(+)

diff --git a/crypto/asymmetric_keys/x509_cert_parser.c b/crypto/asymmetric_keys/x509_cert_parser.c
index 7a9b084e2043..77547d4bd94d 100644
--- a/crypto/asymmetric_keys/x509_cert_parser.c
+++ b/crypto/asymmetric_keys/x509_cert_parser.c
@@ -586,6 +586,28 @@ int x509_process_extension(void *context, size_t hdrlen,
 		return 0;
 	}
 
+	if (ctx->last_oid == OID_basicConstraints) {
+		/*
+		 * Get hold of the basicConstraints
+		 * v[1] is the encoding size
+		 *	(Expect 0x2 or greater, making it 1 or more bytes)
+		 * v[2] is the encoding type
+		 *	(Expect an ASN1_BOOL for the CA)
+		 * v[3] is the contents of the ASN1_BOOL
+		 *      (Expect 1 if the CA is TRUE)
+		 * vlen should match the entire extension size
+		 */
+		if (v[0] != (ASN1_CONS_BIT | ASN1_SEQ))
+			return -EBADMSG;
+		if (vlen < 2)
+			return -EBADMSG;
+		if (v[1] != vlen - 2)
+			return -EBADMSG;
+		if (vlen >= 4 && v[1] != 0 && v[2] == ASN1_BOOL && v[3] == 1)
+			ctx->cert->pub->key_eflags |= 1 << KEY_EFLAG_CA;
+		return 0;
+	}
+
 	return 0;
 }
 
diff --git a/include/crypto/public_key.h b/include/crypto/public_key.h
index 6d61695e1cde..c401762850f2 100644
--- a/include/crypto/public_key.h
+++ b/include/crypto/public_key.h
@@ -28,6 +28,8 @@ struct public_key {
 	bool key_is_private;
 	const char *id_type;
 	const char *pkey_algo;
+	unsigned long key_eflags;	/* key extension flags */
+#define KEY_EFLAG_CA		0	/* set if the CA basic constraints is set */
 };
 
 extern void public_key_free(struct public_key *key);
-- 
2.27.0


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

* [PATCH v4 4/6] KEYS: X.509: Parse Key Usage
  2023-02-07  2:59 [PATCH v4 0/6] Add CA enforcement keyring restrictions Eric Snowberg
                   ` (2 preceding siblings ...)
  2023-02-07  2:59 ` [PATCH v4 3/6] KEYS: X.509: Parse Basic Constraints for CA Eric Snowberg
@ 2023-02-07  2:59 ` Eric Snowberg
  2023-02-08 21:02   ` Mimi Zohar
  2023-02-10  3:48   ` Jarkko Sakkinen
  2023-02-07  2:59 ` [PATCH v4 5/6] KEYS: CA link restriction Eric Snowberg
                   ` (2 subsequent siblings)
  6 siblings, 2 replies; 24+ messages in thread
From: Eric Snowberg @ 2023-02-07  2:59 UTC (permalink / raw)
  To: jarkko, zohar, dhowells, dwmw2
  Cc: herbert, davem, dmitry.kasatkin, paul, jmorris, serge, pvorel,
	tadeusz.struk, eric.snowberg, kanth.ghatraju, konrad.wilk,
	erpalmer, coxu, keyrings, linux-kernel, linux-crypto,
	linux-integrity, linux-security-module

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 or digitalSignature is set, store it in the
public_key structure.  This will be used in a follow on patch that
requires knowing the certificate key usage type.

Link: https://www.rfc-editor.org/rfc/rfc5280#section-4.2.1.3
Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
---
 crypto/asymmetric_keys/x509_cert_parser.c | 28 +++++++++++++++++++++++
 include/crypto/public_key.h               |  2 ++
 2 files changed, 30 insertions(+)

diff --git a/crypto/asymmetric_keys/x509_cert_parser.c b/crypto/asymmetric_keys/x509_cert_parser.c
index 77547d4bd94d..0a7049b470c1 100644
--- a/crypto/asymmetric_keys/x509_cert_parser.c
+++ b/crypto/asymmetric_keys/x509_cert_parser.c
@@ -579,6 +579,34 @@ int x509_process_extension(void *context, size_t hdrlen,
 		return 0;
 	}
 
+	if (ctx->last_oid == OID_keyUsage) {
+		/*
+		 * Get hold of the keyUsage bit string
+		 * 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 when v[1] = 0x02)
+		 * v[3] and possibly v[4] contain the bit string
+		 *
+		 * From RFC 5280 4.2.1.3:
+		 *   0x04 is where keyCertSign lands in this bit string
+		 *   0x80 is where digitalSignature lands in this bit string
+		 */
+		if (v[0] != ASN1_BTS)
+			return -EBADMSG;
+		if (vlen < 4)
+			return -EBADMSG;
+		if (v[2] >= 8)
+			return -EBADMSG;
+		if (v[3] & 0x80)
+			ctx->cert->pub->key_eflags |= 1 << KEY_EFLAG_DIGITALSIG;
+		if (v[1] == 0x02 && v[2] <= 2 && (v[3] & 0x04))
+			ctx->cert->pub->key_eflags |= 1 << KEY_EFLAG_KEYCERTSIGN;
+		else if (vlen > 4 && v[1] == 0x03 && (v[3] & 0x04))
+			ctx->cert->pub->key_eflags |= 1 << KEY_EFLAG_KEYCERTSIGN;
+		return 0;
+	}
+
 	if (ctx->last_oid == OID_authorityKeyIdentifier) {
 		/* Get hold of the CA key fingerprint */
 		ctx->raw_akid = v;
diff --git a/include/crypto/public_key.h b/include/crypto/public_key.h
index c401762850f2..03c3fb990d59 100644
--- a/include/crypto/public_key.h
+++ b/include/crypto/public_key.h
@@ -30,6 +30,8 @@ struct public_key {
 	const char *pkey_algo;
 	unsigned long key_eflags;	/* key extension flags */
 #define KEY_EFLAG_CA		0	/* set if the CA basic constraints is set */
+#define KEY_EFLAG_DIGITALSIG	1	/* set if the digitalSignature usage is set */
+#define KEY_EFLAG_KEYCERTSIGN	2	/* set if the keyCertSign usage is set */
 };
 
 extern void public_key_free(struct public_key *key);
-- 
2.27.0


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

* [PATCH v4 5/6] KEYS: CA link restriction
  2023-02-07  2:59 [PATCH v4 0/6] Add CA enforcement keyring restrictions Eric Snowberg
                   ` (3 preceding siblings ...)
  2023-02-07  2:59 ` [PATCH v4 4/6] KEYS: X.509: Parse Key Usage Eric Snowberg
@ 2023-02-07  2:59 ` Eric Snowberg
  2023-02-09  2:55   ` Mimi Zohar
  2023-02-07  2:59 ` [PATCH v4 6/6] integrity: machine keyring CA configuration Eric Snowberg
  2023-02-08 12:38 ` [PATCH v4 0/6] Add CA enforcement keyring restrictions Mimi Zohar
  6 siblings, 1 reply; 24+ messages in thread
From: Eric Snowberg @ 2023-02-07  2:59 UTC (permalink / raw)
  To: jarkko, zohar, dhowells, dwmw2
  Cc: herbert, davem, dmitry.kasatkin, paul, jmorris, serge, pvorel,
	tadeusz.struk, eric.snowberg, kanth.ghatraju, konrad.wilk,
	erpalmer, coxu, keyrings, linux-kernel, linux-crypto,
	linux-integrity, linux-security-module

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 | 38 +++++++++++++++++++++++++++++++
 include/crypto/public_key.h       | 15 ++++++++++++
 2 files changed, 53 insertions(+)

diff --git a/crypto/asymmetric_keys/restrict.c b/crypto/asymmetric_keys/restrict.c
index 6b1ac5f5896a..48457c6f33f9 100644
--- a/crypto/asymmetric_keys/restrict.c
+++ b/crypto/asymmetric_keys/restrict.c
@@ -108,6 +108,44 @@ 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 (!test_bit(KEY_EFLAG_CA, &pkey->key_eflags))
+		return -ENOKEY;
+	if (!test_bit(KEY_EFLAG_KEYCERTSIGN, &pkey->key_eflags))
+		return -ENOKEY;
+	if (test_bit(KEY_EFLAG_DIGITALSIG, &pkey->key_eflags))
+		return -ENOKEY;
+
+	return 0;
+}
+
 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 03c3fb990d59..653992a6e941 100644
--- a/include/crypto/public_key.h
+++ b/include/crypto/public_key.h
@@ -75,6 +75,21 @@ extern int restrict_link_by_key_or_keyring_chain(struct key *trust_keyring,
 						 const union key_payload *payload,
 						 struct key *trusted);
 
+#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


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

* [PATCH v4 6/6] integrity: machine keyring CA configuration
  2023-02-07  2:59 [PATCH v4 0/6] Add CA enforcement keyring restrictions Eric Snowberg
                   ` (4 preceding siblings ...)
  2023-02-07  2:59 ` [PATCH v4 5/6] KEYS: CA link restriction Eric Snowberg
@ 2023-02-07  2:59 ` Eric Snowberg
  2023-02-10 13:05   ` Mimi Zohar
  2023-02-08 12:38 ` [PATCH v4 0/6] Add CA enforcement keyring restrictions Mimi Zohar
  6 siblings, 1 reply; 24+ messages in thread
From: Eric Snowberg @ 2023-02-07  2:59 UTC (permalink / raw)
  To: jarkko, zohar, dhowells, dwmw2
  Cc: herbert, davem, dmitry.kasatkin, paul, jmorris, serge, pvorel,
	tadeusz.struk, eric.snowberg, kanth.ghatraju, konrad.wilk,
	erpalmer, coxu, keyrings, linux-kernel, linux-crypto,
	linux-integrity, linux-security-module

Add a machine keyring CA restriction menu option to control the type of
keys that may be added to it. The options include none, min and max
restrictions.

When no restrictions are selected, all Machine Owner Keys (MOK) are added
to the machine keyring.  When CONFIG_INTEGRITY_CA_MACHINE_KEYRING_MIN is
selected, the CA bit must be true.  Also the key usage must contain
keyCertSign, any other usage field may be set as well.

When CONFIG_INTEGRITY_CA_MACHINE_KEYRING_MAX is selected, the CA bit must
be true. Also the key usage must contain keyCertSign and the
digitialSignature usage may not be set.

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

diff --git a/crypto/asymmetric_keys/restrict.c b/crypto/asymmetric_keys/restrict.c
index 48457c6f33f9..633021ea7901 100644
--- a/crypto/asymmetric_keys/restrict.c
+++ b/crypto/asymmetric_keys/restrict.c
@@ -140,6 +140,8 @@ int restrict_link_by_ca(struct key *dest_keyring,
 		return -ENOKEY;
 	if (!test_bit(KEY_EFLAG_KEYCERTSIGN, &pkey->key_eflags))
 		return -ENOKEY;
+	if (IS_ENABLED(CONFIG_INTEGRITY_CA_MACHINE_KEYRING_MIN))
+		return 0;
 	if (test_bit(KEY_EFLAG_DIGITALSIG, &pkey->key_eflags))
 		return -ENOKEY;
 
diff --git a/security/integrity/Kconfig b/security/integrity/Kconfig
index 599429f99f99..eba6fd59fd16 100644
--- a/security/integrity/Kconfig
+++ b/security/integrity/Kconfig
@@ -68,13 +68,50 @@ 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
 	 in the platform keyring, keys contained in the .machine keyring will
 	 be trusted within the kernel.
 
+choice
+	prompt "Enforce Machine Keyring CA Restrictions"
+	default INTEGRITY_CA_MACHINE_KEYRING_NONE
+	depends on INTEGRITY_MACHINE_KEYRING
+	help
+	  The .machine keyring can be configured to enforce CA restriction
+	  on any key added to it. The options include none, min and max
+	  restrictions. By default no restrictions are in place and all
+	  Machine Owner Keys (MOK) are added to the machine keyring.
+
+config INTEGRITY_CA_MACHINE_KEYRING_NONE
+	bool "No restrictions"
+	help
+	  When no restrictions are selected, all Machine Owner Keys (MOK)
+	  are added to the machine keyring. MOK keys do not require the
+	  CA bit to be set. The key usage field is ignored. This is the
+	  default setting.
+
+config INTEGRITY_CA_MACHINE_KEYRING_MIN
+	bool "Only CA keys (with or without DigitialSignature usage set)"
+	help
+	  When min is selected, only load CA keys into the machine keyring.
+	  The CA bit must be set along with the keyCertSign Usage field.
+	  Keys containing the digitialSignature Usage field will also be
+	  loaded. The remaining MOK keys are loaded into the .platform
+	  keyring.
+
+config INTEGRITY_CA_MACHINE_KEYRING_MAX
+	bool "Only CA keys"
+	help
+	  When max is selected, only load CA keys into the machine keyring.
+	  The CA bit must be set along with the keyCertSign Usage field.
+	  Keys containing the digitialSignature Usage field will not be
+	  loaded. The remaining MOK keys are loaded into the .platform
+	  keyring.
+
+endchoice
+
 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 f2193c531f4a..3385f534f1da 100644
--- a/security/integrity/digsig.c
+++ b/security/integrity/digsig.c
@@ -132,7 +132,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_NONE))) {
 		restriction = NULL;
 		goto out;
 	}
@@ -144,7 +145,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


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

* Re: [PATCH v4 0/6] Add CA enforcement keyring restrictions
  2023-02-07  2:59 [PATCH v4 0/6] Add CA enforcement keyring restrictions Eric Snowberg
                   ` (5 preceding siblings ...)
  2023-02-07  2:59 ` [PATCH v4 6/6] integrity: machine keyring CA configuration Eric Snowberg
@ 2023-02-08 12:38 ` Mimi Zohar
  2023-02-08 23:26   ` Eric Snowberg
  6 siblings, 1 reply; 24+ messages in thread
From: Mimi Zohar @ 2023-02-08 12:38 UTC (permalink / raw)
  To: Eric Snowberg, jarkko, dhowells, dwmw2
  Cc: herbert, davem, dmitry.kasatkin, paul, jmorris, serge, pvorel,
	tadeusz.struk, kanth.ghatraju, konrad.wilk, erpalmer, coxu,
	keyrings, linux-kernel, linux-crypto, linux-integrity,
	linux-security-module, Lee, Chun-Yi

[CC'ing: Lee, Chun-Yi]

On Mon, 2023-02-06 at 21:59 -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.
> 
> This series introduces a way to do CA enforement with the machine
> keyring. It introduces three different ways to configure the machine
> keyring. A new menu option is added to control the type of keys that may
> be added to it.  The options include none, min, and max restrictions. The
> default is CONFIG_INTEGRITY_CA_MACHINE_KEYRING_NONE. This allows all MOK
> keys into the machine keyring.  When CONFIG_INTEGRITY_CA_MACHINE_KEYRING_MIN
> is selected, the X.509 CA bit must be true.  Also, the key usage must
> contain keyCertSign, any other usage field may also be set. When 
> CONFIG_INTEGRITY_CA_MACHINE_KEYRING_MAX is selected, the X.509 CA bit
> must be true.  Also, the key usage must contain keyCertSign and the
> digitialSignature usage may not be set. If a key doesn't pass the CA
> restriction check, instead of going into the machine keyring, it is
> added to the platform keyring. With the ability to configure the machine
> keyring with CA restrictions, code that prevented the machine keyring
> from being enabled with IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY
> has been removed.
> 
> Changelog:
> v4:
> - Removed all code that validated the certificate chain back to the root
>   CA. Now the only restriction is what is initially placed in the
>   machine keyring.
> - Check and store if the X.509 usage contains digitalSignature
> - New Kconfig menu item with none, min and max CA restriction on the 
>   machine keyring

Thank you, Eric.

For complete separation of certificate usage, at least in the "max" CA
restriction case, the next step would be to limit certificates being
loaded onto the IMA keyring to those with key usage of
"digitalSignature".

Perhaps also require a "codeSigning" extendedKeyUsage, though that
might break existing usages.  The "codeSigning" checking could
piggyback on Joey's proposed "Check codeSigning extended key usage
extension" patch set.

What do you think?  Do you have any concerns with limiting the type of
certificate being loaded onto the IMA keyring to those with
"digitalSignature"?

-- 
thanks,

Mimi


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

* Re: [PATCH v4 3/6] KEYS: X.509: Parse Basic Constraints for CA
  2023-02-07  2:59 ` [PATCH v4 3/6] KEYS: X.509: Parse Basic Constraints for CA Eric Snowberg
@ 2023-02-08 21:01   ` Mimi Zohar
  2023-02-10  3:46   ` Jarkko Sakkinen
  1 sibling, 0 replies; 24+ messages in thread
From: Mimi Zohar @ 2023-02-08 21:01 UTC (permalink / raw)
  To: Eric Snowberg, jarkko, dhowells, dwmw2
  Cc: herbert, davem, dmitry.kasatkin, paul, jmorris, serge, pvorel,
	tadeusz.struk, kanth.ghatraju, konrad.wilk, erpalmer, coxu,
	keyrings, linux-kernel, linux-crypto, linux-integrity,
	linux-security-module

On Mon, 2023-02-06 at 21:59 -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 public_key.  This will be used
> in a follow on patch that requires knowing if the public key is a CA.
> 
> Link: https://www.rfc-editor.org/rfc/rfc5280#section-4.2.1.9
> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>

Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>


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

* Re: [PATCH v4 4/6] KEYS: X.509: Parse Key Usage
  2023-02-07  2:59 ` [PATCH v4 4/6] KEYS: X.509: Parse Key Usage Eric Snowberg
@ 2023-02-08 21:02   ` Mimi Zohar
  2023-02-10  3:48   ` Jarkko Sakkinen
  1 sibling, 0 replies; 24+ messages in thread
From: Mimi Zohar @ 2023-02-08 21:02 UTC (permalink / raw)
  To: Eric Snowberg, jarkko, dhowells, dwmw2
  Cc: herbert, davem, dmitry.kasatkin, paul, jmorris, serge, pvorel,
	tadeusz.struk, kanth.ghatraju, konrad.wilk, erpalmer, coxu,
	keyrings, linux-kernel, linux-crypto, linux-integrity,
	linux-security-module

On Mon, 2023-02-06 at 21:59 -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 or digitalSignature is set, store it in the
> public_key structure.  This will be used in a follow on patch that
> requires knowing the certificate key usage type.
> 
> Link: https://www.rfc-editor.org/rfc/rfc5280#section-4.2.1.3
> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>

Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>


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

* Re: [PATCH v4 2/6] KEYS: Add missing function documentation
  2023-02-07  2:59 ` [PATCH v4 2/6] KEYS: Add missing function documentation Eric Snowberg
@ 2023-02-08 21:48   ` Mimi Zohar
  2023-02-10  3:40   ` Jarkko Sakkinen
  1 sibling, 0 replies; 24+ messages in thread
From: Mimi Zohar @ 2023-02-08 21:48 UTC (permalink / raw)
  To: Eric Snowberg, jarkko, dhowells, dwmw2
  Cc: herbert, davem, dmitry.kasatkin, paul, jmorris, serge, pvorel,
	tadeusz.struk, kanth.ghatraju, konrad.wilk, erpalmer, coxu,
	keyrings, linux-kernel, linux-crypto, linux-integrity,
	linux-security-module

On Mon, 2023-02-06 at 21:59 -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.
> 
> Fixes: 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

While fixing the kernel doc, might as well update "built in" to be
consistent.

> + * @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.

and here 

> + * @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

and here

> @@ -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().
>   */

Reviewed-by:  Mimi Zohar <zohar@linux.ibm.com>


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

* Re: [PATCH v4 0/6] Add CA enforcement keyring restrictions
  2023-02-08 12:38 ` [PATCH v4 0/6] Add CA enforcement keyring restrictions Mimi Zohar
@ 2023-02-08 23:26   ` Eric Snowberg
  2023-02-09  2:54     ` Mimi Zohar
  0 siblings, 1 reply; 24+ messages in thread
From: Eric Snowberg @ 2023-02-08 23:26 UTC (permalink / raw)
  To: Mimi Zohar
  Cc: Jarkko Sakkinen, David Howells, David Woodhouse, herbert, davem,
	dmitry.kasatkin, paul, jmorris, serge, pvorel, tadeusz.struk,
	Kanth Ghatraju, Konrad Wilk, Elaine Palmer, Coiby Xu, keyrings,
	linux-kernel, linux-crypto, linux-integrity,
	linux-security-module, Lee, Chun-Yi



> On Feb 8, 2023, at 5:38 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> 
> [CC'ing: Lee, Chun-Yi]
> 
> On Mon, 2023-02-06 at 21:59 -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.
>> 
>> This series introduces a way to do CA enforement with the machine
>> keyring. It introduces three different ways to configure the machine
>> keyring. A new menu option is added to control the type of keys that may
>> be added to it.  The options include none, min, and max restrictions. The
>> default is CONFIG_INTEGRITY_CA_MACHINE_KEYRING_NONE. This allows all MOK
>> keys into the machine keyring.  When CONFIG_INTEGRITY_CA_MACHINE_KEYRING_MIN
>> is selected, the X.509 CA bit must be true.  Also, the key usage must
>> contain keyCertSign, any other usage field may also be set. When 
>> CONFIG_INTEGRITY_CA_MACHINE_KEYRING_MAX is selected, the X.509 CA bit
>> must be true.  Also, the key usage must contain keyCertSign and the
>> digitialSignature usage may not be set. If a key doesn't pass the CA
>> restriction check, instead of going into the machine keyring, it is
>> added to the platform keyring. With the ability to configure the machine
>> keyring with CA restrictions, code that prevented the machine keyring
>> from being enabled with IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY
>> has been removed.
>> 
>> Changelog:
>> v4:
>> - Removed all code that validated the certificate chain back to the root
>>  CA. Now the only restriction is what is initially placed in the
>>  machine keyring.
>> - Check and store if the X.509 usage contains digitalSignature
>> - New Kconfig menu item with none, min and max CA restriction on the 
>>  machine keyring
> 
> Thank you, Eric.
> 
> For complete separation of certificate usage, at least in the "max" CA
> restriction case, the next step would be to limit certificates being
> loaded onto the IMA keyring to those with key usage of
> "digitalSignature".
> 
> Perhaps also require a "codeSigning" extendedKeyUsage, though that
> might break existing usages.  The "codeSigning" checking could
> piggyback on Joey's proposed "Check codeSigning extended key usage
> extension" patch set.
> 
> What do you think?  Do you have any concerns with limiting the type of
> certificate being loaded onto the IMA keyring to those with
> "digitalSignature"?

In the MAX setting I would not have a concern.  Instead of restrict_link_to_ima 
being a macro, a new restriction similar to restrict_link_by_ca could be created.  
The new restriction would simply verify digitialSignature is set and the key can be 
vouched for by either the built-in or secondary keyrings. Joey’s work to parse 
the extended key usage extension could also be included in this restriction.

I’m assuming this would be follow on work?


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

* Re: [PATCH v4 0/6] Add CA enforcement keyring restrictions
  2023-02-08 23:26   ` Eric Snowberg
@ 2023-02-09  2:54     ` Mimi Zohar
  0 siblings, 0 replies; 24+ messages in thread
From: Mimi Zohar @ 2023-02-09  2:54 UTC (permalink / raw)
  To: Eric Snowberg
  Cc: Jarkko Sakkinen, David Howells, David Woodhouse, herbert, davem,
	dmitry.kasatkin, paul, jmorris, serge, pvorel, tadeusz.struk,
	Kanth Ghatraju, Konrad Wilk, Elaine Palmer, Coiby Xu, keyrings,
	linux-kernel, linux-crypto, linux-integrity,
	linux-security-module, Lee, Chun-Yi

On Wed, 2023-02-08 at 23:26 +0000, Eric Snowberg wrote:
> 
> > On Feb 8, 2023, at 5:38 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> > 
> > [CC'ing: Lee, Chun-Yi]
> > 
> > On Mon, 2023-02-06 at 21:59 -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.
> >> 
> >> This series introduces a way to do CA enforement with the machine
> >> keyring. It introduces three different ways to configure the machine
> >> keyring. A new menu option is added to control the type of keys that may
> >> be added to it.  The options include none, min, and max restrictions. The
> >> default is CONFIG_INTEGRITY_CA_MACHINE_KEYRING_NONE. This allows all MOK
> >> keys into the machine keyring.  When CONFIG_INTEGRITY_CA_MACHINE_KEYRING_MIN
> >> is selected, the X.509 CA bit must be true.  Also, the key usage must
> >> contain keyCertSign, any other usage field may also be set. When 
> >> CONFIG_INTEGRITY_CA_MACHINE_KEYRING_MAX is selected, the X.509 CA bit
> >> must be true.  Also, the key usage must contain keyCertSign and the
> >> digitialSignature usage may not be set. If a key doesn't pass the CA
> >> restriction check, instead of going into the machine keyring, it is
> >> added to the platform keyring. With the ability to configure the machine
> >> keyring with CA restrictions, code that prevented the machine keyring
> >> from being enabled with IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY
> >> has been removed.
> >> 
> >> Changelog:
> >> v4:
> >> - Removed all code that validated the certificate chain back to the root
> >>  CA. Now the only restriction is what is initially placed in the
> >>  machine keyring.
> >> - Check and store if the X.509 usage contains digitalSignature
> >> - New Kconfig menu item with none, min and max CA restriction on the 
> >>  machine keyring
> > 
> > Thank you, Eric.
> > 
> > For complete separation of certificate usage, at least in the "max" CA
> > restriction case, the next step would be to limit certificates being
> > loaded onto the IMA keyring to those with key usage of
> > "digitalSignature".
> > 
> > Perhaps also require a "codeSigning" extendedKeyUsage, though that
> > might break existing usages.  The "codeSigning" checking could
> > piggyback on Joey's proposed "Check codeSigning extended key usage
> > extension" patch set.
> > 
> > What do you think?  Do you have any concerns with limiting the type of
> > certificate being loaded onto the IMA keyring to those with
> > "digitalSignature"?
> 
> In the MAX setting I would not have a concern.  Instead of restrict_link_to_ima 
> being a macro, a new restriction similar to restrict_link_by_ca could be created.  
> The new restriction would simply verify digitialSignature is set and the key can be 
> vouched for by either the built-in or secondary keyrings. Joey’s work to parse 
> the extended key usage extension could also be included in this restriction.

Sounds good.

> I’m assuming this would be follow on work?

Yes, that probably makes the most sense.

-- 
thanks,

Mimi


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

* Re: [PATCH v4 5/6] KEYS: CA link restriction
  2023-02-07  2:59 ` [PATCH v4 5/6] KEYS: CA link restriction Eric Snowberg
@ 2023-02-09  2:55   ` Mimi Zohar
  0 siblings, 0 replies; 24+ messages in thread
From: Mimi Zohar @ 2023-02-09  2:55 UTC (permalink / raw)
  To: Eric Snowberg, jarkko, dhowells, dwmw2
  Cc: herbert, davem, dmitry.kasatkin, paul, jmorris, serge, pvorel,
	tadeusz.struk, kanth.ghatraju, konrad.wilk, erpalmer, coxu,
	keyrings, linux-kernel, linux-crypto, linux-integrity,
	linux-security-module

On Mon, 2023-02-06 at 21:59 -0500, Eric Snowberg wrote:
> Add a new link restriction.  Restrict the addition of keys in a keyring
> based on the key to be added being a CA.
> 
> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>

Other than expanding the patch description to mention the keyUsage,

Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>


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

* Re: [PATCH v4 1/6] KEYS: Create static version of public_key_verify_signature
  2023-02-07  2:59 ` [PATCH v4 1/6] KEYS: Create static version of public_key_verify_signature Eric Snowberg
@ 2023-02-10  3:39   ` Jarkko Sakkinen
  2023-02-10 22:38     ` Eric Snowberg
  0 siblings, 1 reply; 24+ messages in thread
From: Jarkko Sakkinen @ 2023-02-10  3:39 UTC (permalink / raw)
  To: Eric Snowberg
  Cc: zohar, dhowells, dwmw2, herbert, davem, dmitry.kasatkin, paul,
	jmorris, serge, pvorel, tadeusz.struk, kanth.ghatraju,
	konrad.wilk, erpalmer, coxu, keyrings, linux-kernel,
	linux-crypto, linux-integrity, linux-security-module

On Mon, Feb 06, 2023 at 09:59:53PM -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.
> 
> Fixes: db6c43bd2132 ("crypto: KEYS: convert public key and digsig asym to the akcipher api")
> Reported-by: kernel test robot <lkp@intel.com>

What is this reported-by is good for?

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

BR, Jarkko

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

* Re: [PATCH v4 2/6] KEYS: Add missing function documentation
  2023-02-07  2:59 ` [PATCH v4 2/6] KEYS: Add missing function documentation Eric Snowberg
  2023-02-08 21:48   ` Mimi Zohar
@ 2023-02-10  3:40   ` Jarkko Sakkinen
  1 sibling, 0 replies; 24+ messages in thread
From: Jarkko Sakkinen @ 2023-02-10  3:40 UTC (permalink / raw)
  To: Eric Snowberg
  Cc: zohar, dhowells, dwmw2, herbert, davem, dmitry.kasatkin, paul,
	jmorris, serge, pvorel, tadeusz.struk, kanth.ghatraju,
	konrad.wilk, erpalmer, coxu, keyrings, linux-kernel,
	linux-crypto, linux-integrity, linux-security-module

On Mon, Feb 06, 2023 at 09:59:54PM -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.
> 
> Fixes: 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
> 

Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>

BR, Jarkko

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

* Re: [PATCH v4 3/6] KEYS: X.509: Parse Basic Constraints for CA
  2023-02-07  2:59 ` [PATCH v4 3/6] KEYS: X.509: Parse Basic Constraints for CA Eric Snowberg
  2023-02-08 21:01   ` Mimi Zohar
@ 2023-02-10  3:46   ` Jarkko Sakkinen
  1 sibling, 0 replies; 24+ messages in thread
From: Jarkko Sakkinen @ 2023-02-10  3:46 UTC (permalink / raw)
  To: Eric Snowberg
  Cc: zohar, dhowells, dwmw2, herbert, davem, dmitry.kasatkin, paul,
	jmorris, serge, pvorel, tadeusz.struk, kanth.ghatraju,
	konrad.wilk, erpalmer, coxu, keyrings, linux-kernel,
	linux-crypto, linux-integrity, linux-security-module

On Mon, Feb 06, 2023 at 09:59:55PM -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 public_key.  This will be used
> in a follow on patch that requires knowing if the public key is a CA.
> 
> Link: https://www.rfc-editor.org/rfc/rfc5280#section-4.2.1.9
> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
> ---
>  crypto/asymmetric_keys/x509_cert_parser.c | 22 ++++++++++++++++++++++
>  include/crypto/public_key.h               |  2 ++
>  2 files changed, 24 insertions(+)
> 
> diff --git a/crypto/asymmetric_keys/x509_cert_parser.c b/crypto/asymmetric_keys/x509_cert_parser.c
> index 7a9b084e2043..77547d4bd94d 100644
> --- a/crypto/asymmetric_keys/x509_cert_parser.c
> +++ b/crypto/asymmetric_keys/x509_cert_parser.c
> @@ -586,6 +586,28 @@ int x509_process_extension(void *context, size_t hdrlen,
>  		return 0;
>  	}
>  
> +	if (ctx->last_oid == OID_basicConstraints) {
> +		/*
> +		 * Get hold of the basicConstraints
> +		 * v[1] is the encoding size
> +		 *	(Expect 0x2 or greater, making it 1 or more bytes)
> +		 * v[2] is the encoding type
> +		 *	(Expect an ASN1_BOOL for the CA)
> +		 * v[3] is the contents of the ASN1_BOOL
> +		 *      (Expect 1 if the CA is TRUE)
> +		 * vlen should match the entire extension size
> +		 */
> +		if (v[0] != (ASN1_CONS_BIT | ASN1_SEQ))
> +			return -EBADMSG;
> +		if (vlen < 2)
> +			return -EBADMSG;
> +		if (v[1] != vlen - 2)
> +			return -EBADMSG;
> +		if (vlen >= 4 && v[1] != 0 && v[2] == ASN1_BOOL && v[3] == 1)
> +			ctx->cert->pub->key_eflags |= 1 << KEY_EFLAG_CA;
> +		return 0;
> +	}
> +
>  	return 0;
>  }
>  
> diff --git a/include/crypto/public_key.h b/include/crypto/public_key.h
> index 6d61695e1cde..c401762850f2 100644
> --- a/include/crypto/public_key.h
> +++ b/include/crypto/public_key.h
> @@ -28,6 +28,8 @@ struct public_key {
>  	bool key_is_private;
>  	const char *id_type;
>  	const char *pkey_algo;
> +	unsigned long key_eflags;	/* key extension flags */
> +#define KEY_EFLAG_CA		0	/* set if the CA basic constraints is set */
>  };
>  
>  extern void public_key_free(struct public_key *key);
> -- 
> 2.27.0
> 

Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>

BR, Jarkko

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

* Re: [PATCH v4 4/6] KEYS: X.509: Parse Key Usage
  2023-02-07  2:59 ` [PATCH v4 4/6] KEYS: X.509: Parse Key Usage Eric Snowberg
  2023-02-08 21:02   ` Mimi Zohar
@ 2023-02-10  3:48   ` Jarkko Sakkinen
  2023-02-10 22:39     ` Eric Snowberg
  1 sibling, 1 reply; 24+ messages in thread
From: Jarkko Sakkinen @ 2023-02-10  3:48 UTC (permalink / raw)
  To: Eric Snowberg
  Cc: zohar, dhowells, dwmw2, herbert, davem, dmitry.kasatkin, paul,
	jmorris, serge, pvorel, tadeusz.struk, kanth.ghatraju,
	konrad.wilk, erpalmer, coxu, keyrings, linux-kernel,
	linux-crypto, linux-integrity, linux-security-module

On Mon, Feb 06, 2023 at 09:59:56PM -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 or digitalSignature is set, store it in the
> public_key structure.  This will be used in a follow on patch that
> requires knowing the certificate key usage type.

It would be better to shortly explain why we want to know key usage
type, rather than give zero information with "follow on patch".

> 
> Link: https://www.rfc-editor.org/rfc/rfc5280#section-4.2.1.3
> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
> ---
>  crypto/asymmetric_keys/x509_cert_parser.c | 28 +++++++++++++++++++++++
>  include/crypto/public_key.h               |  2 ++
>  2 files changed, 30 insertions(+)
> 
> diff --git a/crypto/asymmetric_keys/x509_cert_parser.c b/crypto/asymmetric_keys/x509_cert_parser.c
> index 77547d4bd94d..0a7049b470c1 100644
> --- a/crypto/asymmetric_keys/x509_cert_parser.c
> +++ b/crypto/asymmetric_keys/x509_cert_parser.c
> @@ -579,6 +579,34 @@ int x509_process_extension(void *context, size_t hdrlen,
>  		return 0;
>  	}
>  
> +	if (ctx->last_oid == OID_keyUsage) {
> +		/*
> +		 * Get hold of the keyUsage bit string
> +		 * 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 when v[1] = 0x02)
> +		 * v[3] and possibly v[4] contain the bit string
> +		 *
> +		 * From RFC 5280 4.2.1.3:
> +		 *   0x04 is where keyCertSign lands in this bit string
> +		 *   0x80 is where digitalSignature lands in this bit string
> +		 */
> +		if (v[0] != ASN1_BTS)
> +			return -EBADMSG;
> +		if (vlen < 4)
> +			return -EBADMSG;
> +		if (v[2] >= 8)
> +			return -EBADMSG;
> +		if (v[3] & 0x80)
> +			ctx->cert->pub->key_eflags |= 1 << KEY_EFLAG_DIGITALSIG;
> +		if (v[1] == 0x02 && v[2] <= 2 && (v[3] & 0x04))
> +			ctx->cert->pub->key_eflags |= 1 << KEY_EFLAG_KEYCERTSIGN;
> +		else if (vlen > 4 && v[1] == 0x03 && (v[3] & 0x04))
> +			ctx->cert->pub->key_eflags |= 1 << KEY_EFLAG_KEYCERTSIGN;
> +		return 0;
> +	}
> +
>  	if (ctx->last_oid == OID_authorityKeyIdentifier) {
>  		/* Get hold of the CA key fingerprint */
>  		ctx->raw_akid = v;
> diff --git a/include/crypto/public_key.h b/include/crypto/public_key.h
> index c401762850f2..03c3fb990d59 100644
> --- a/include/crypto/public_key.h
> +++ b/include/crypto/public_key.h
> @@ -30,6 +30,8 @@ struct public_key {
>  	const char *pkey_algo;
>  	unsigned long key_eflags;	/* key extension flags */
>  #define KEY_EFLAG_CA		0	/* set if the CA basic constraints is set */
> +#define KEY_EFLAG_DIGITALSIG	1	/* set if the digitalSignature usage is set */
> +#define KEY_EFLAG_KEYCERTSIGN	2	/* set if the keyCertSign usage is set */
>  };
>  
>  extern void public_key_free(struct public_key *key);
> -- 
> 2.27.0
> 

BR, Jarkko

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

* Re: [PATCH v4 6/6] integrity: machine keyring CA configuration
  2023-02-07  2:59 ` [PATCH v4 6/6] integrity: machine keyring CA configuration Eric Snowberg
@ 2023-02-10 13:05   ` Mimi Zohar
  2023-02-10 22:42     ` Eric Snowberg
  2023-02-13  7:54     ` Jarkko Sakkinen
  0 siblings, 2 replies; 24+ messages in thread
From: Mimi Zohar @ 2023-02-10 13:05 UTC (permalink / raw)
  To: Eric Snowberg, jarkko, dhowells, dwmw2
  Cc: herbert, davem, dmitry.kasatkin, paul, jmorris, serge, pvorel,
	tadeusz.struk, kanth.ghatraju, konrad.wilk, erpalmer, coxu,
	keyrings, linux-kernel, linux-crypto, linux-integrity,
	linux-security-module

Hi Eric,

On Mon, 2023-02-06 at 21:59 -0500, Eric Snowberg wrote:
> Add a machine keyring CA restriction menu option to control the type of
> keys that may be added to it. The options include none, min and max
> restrictions.
> 
> When no restrictions are selected, all Machine Owner Keys (MOK) are added
> to the machine keyring.  When CONFIG_INTEGRITY_CA_MACHINE_KEYRING_MIN is
> selected, the CA bit must be true.  Also the key usage must contain
> keyCertSign, any other usage field may be set as well.
> 
> When CONFIG_INTEGRITY_CA_MACHINE_KEYRING_MAX is selected, the CA bit must
> be true. Also the key usage must contain keyCertSign and the
> digitialSignature usage may not be set.
> 
> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>

Missing from the patch description is the motivation for this change.  
The choices none, min, max implies a progression, which is good, and
the technical differences between the choices, but not the reason.

The motivation, at least from my perspective, is separation of
certificate signing from code signing keys, where "none" is no
separation and "max" being total separation of keys based on usage.

Subsequent work, as discussed in the cover letter thread, will limit
certificates being loaded onto the IMA keyring to code signing keys
used for signature verification.

thanks,

Mimi
> ---
>  crypto/asymmetric_keys/restrict.c |  2 ++
>  security/integrity/Kconfig        | 39 ++++++++++++++++++++++++++++++-
>  security/integrity/digsig.c       |  8 +++++--
>  3 files changed, 46 insertions(+), 3 deletions(-)
> 
> diff --git a/crypto/asymmetric_keys/restrict.c b/crypto/asymmetric_keys/restrict.c
> index 48457c6f33f9..633021ea7901 100644
> --- a/crypto/asymmetric_keys/restrict.c
> +++ b/crypto/asymmetric_keys/restrict.c
> @@ -140,6 +140,8 @@ int restrict_link_by_ca(struct key *dest_keyring,
>  		return -ENOKEY;
>  	if (!test_bit(KEY_EFLAG_KEYCERTSIGN, &pkey->key_eflags))
>  		return -ENOKEY;
> +	if (IS_ENABLED(CONFIG_INTEGRITY_CA_MACHINE_KEYRING_MIN))
> +		return 0;
>  	if (test_bit(KEY_EFLAG_DIGITALSIG, &pkey->key_eflags))
>  		return -ENOKEY;
>  
> diff --git a/security/integrity/Kconfig b/security/integrity/Kconfig
> index 599429f99f99..eba6fd59fd16 100644
> --- a/security/integrity/Kconfig
> +++ b/security/integrity/Kconfig
> @@ -68,13 +68,50 @@ 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
>  	 in the platform keyring, keys contained in the .machine keyring will
>  	 be trusted within the kernel.
>  
> +choice
> +	prompt "Enforce Machine Keyring CA Restrictions"
> +	default INTEGRITY_CA_MACHINE_KEYRING_NONE
> +	depends on INTEGRITY_MACHINE_KEYRING
> +	help
> +	  The .machine keyring can be configured to enforce CA restriction
> +	  on any key added to it. The options include none, min and max
> +	  restrictions. By default no restrictions are in place and all
> +	  Machine Owner Keys (MOK) are added to the machine keyring.
> +
> +config INTEGRITY_CA_MACHINE_KEYRING_NONE
> +	bool "No restrictions"
> +	help
> +	  When no restrictions are selected, all Machine Owner Keys (MOK)
> +	  are added to the machine keyring. MOK keys do not require the
> +	  CA bit to be set. The key usage field is ignored. This is the
> +	  default setting.
> +
> +config INTEGRITY_CA_MACHINE_KEYRING_MIN
> +	bool "Only CA keys (with or without DigitialSignature usage set)"
> +	help
> +	  When min is selected, only load CA keys into the machine keyring.
> +	  The CA bit must be set along with the keyCertSign Usage field.
> +	  Keys containing the digitialSignature Usage field will also be
> +	  loaded. The remaining MOK keys are loaded into the .platform
> +	  keyring.
> +
> +config INTEGRITY_CA_MACHINE_KEYRING_MAX
> +	bool "Only CA keys"
> +	help
> +	  When max is selected, only load CA keys into the machine keyring.
> +	  The CA bit must be set along with the keyCertSign Usage field.
> +	  Keys containing the digitialSignature Usage field will not be
> +	  loaded. The remaining MOK keys are loaded into the .platform
> +	  keyring.
> +
> +endchoice
> +
>  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 f2193c531f4a..3385f534f1da 100644
> --- a/security/integrity/digsig.c
> +++ b/security/integrity/digsig.c
> @@ -132,7 +132,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_NONE))) {
>  		restriction = NULL;
>  		goto out;
>  	}
> @@ -144,7 +145,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



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

* Re: [PATCH v4 1/6] KEYS: Create static version of public_key_verify_signature
  2023-02-10  3:39   ` Jarkko Sakkinen
@ 2023-02-10 22:38     ` Eric Snowberg
  0 siblings, 0 replies; 24+ messages in thread
From: Eric Snowberg @ 2023-02-10 22:38 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: Mimi Zohar, David Howells, David Woodhouse, herbert, davem,
	dmitry.kasatkin, paul, jmorris, serge, pvorel, tadeusz.struk,
	Kanth Ghatraju, Konrad Wilk, Elaine Palmer, Coiby Xu, keyrings,
	linux-kernel, linux-crypto, linux-integrity,
	linux-security-module



> On Feb 9, 2023, at 8:39 PM, Jarkko Sakkinen <jarkko@kernel.org> wrote:
> 
> On Mon, Feb 06, 2023 at 09:59:53PM -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.
>> 
>> Fixes: db6c43bd2132 ("crypto: KEYS: convert public key and digsig asym to the akcipher api")
>> Reported-by: kernel test robot <lkp@intel.com>
> 
> What is this reported-by is good for?

Back in September of 2021, I received the following message from the test robot [1].  
Within the message it asks that I add the Reported-by tag.  To prevent the test robot 
from generating this error again,  I have included this patch at the beginning of the 
series for the last year and a half.  It would be great if someone would apply the first 
two patches.

1. https://lore.kernel.org/all/202109161353.wpNkiygQ-lkp@intel.com/


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

* Re: [PATCH v4 4/6] KEYS: X.509: Parse Key Usage
  2023-02-10  3:48   ` Jarkko Sakkinen
@ 2023-02-10 22:39     ` Eric Snowberg
  0 siblings, 0 replies; 24+ messages in thread
From: Eric Snowberg @ 2023-02-10 22:39 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: Mimi Zohar, David Howells, David Woodhouse, herbert, davem,
	dmitry.kasatkin, paul, jmorris, serge, pvorel, tadeusz.struk,
	Kanth Ghatraju, Konrad Wilk, erpalmer, coxu, keyrings,
	linux-kernel, linux-crypto, linux-integrity,
	linux-security-module



> On Feb 9, 2023, at 8:48 PM, Jarkko Sakkinen <jarkko@kernel.org> wrote:
> 
> On Mon, Feb 06, 2023 at 09:59:56PM -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 or digitalSignature is set, store it in the
>> public_key structure.  This will be used in a follow on patch that
>> requires knowing the certificate key usage type.
> 
> It would be better to shortly explain why we want to know key usage
> type, rather than give zero information with "follow on patch”.

Ok, I will add this in the next round, thanks.



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

* Re: [PATCH v4 6/6] integrity: machine keyring CA configuration
  2023-02-10 13:05   ` Mimi Zohar
@ 2023-02-10 22:42     ` Eric Snowberg
  2023-02-13  7:54     ` Jarkko Sakkinen
  1 sibling, 0 replies; 24+ messages in thread
From: Eric Snowberg @ 2023-02-10 22:42 UTC (permalink / raw)
  To: Mimi Zohar
  Cc: Jarkko Sakkinen, David Howells, dwmw2, herbert, davem,
	dmitry.kasatkin, paul, jmorris, serge, pvorel, tadeusz.struk,
	Kanth Ghatraju, Konrad Wilk, erpalmer, coxu, keyrings,
	linux-kernel, linux-crypto, linux-integrity,
	linux-security-module



> On Feb 10, 2023, at 6:05 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> 
> Hi Eric,
> 
> On Mon, 2023-02-06 at 21:59 -0500, Eric Snowberg wrote:
>> Add a machine keyring CA restriction menu option to control the type of
>> keys that may be added to it. The options include none, min and max
>> restrictions.
>> 
>> When no restrictions are selected, all Machine Owner Keys (MOK) are added
>> to the machine keyring.  When CONFIG_INTEGRITY_CA_MACHINE_KEYRING_MIN is
>> selected, the CA bit must be true.  Also the key usage must contain
>> keyCertSign, any other usage field may be set as well.
>> 
>> When CONFIG_INTEGRITY_CA_MACHINE_KEYRING_MAX is selected, the CA bit must
>> be true. Also the key usage must contain keyCertSign and the
>> digitialSignature usage may not be set.
>> 
>> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
> 
> Missing from the patch description is the motivation for this change.  
> The choices none, min, max implies a progression, which is good, and
> the technical differences between the choices, but not the reason.
> 
> The motivation, at least from my perspective, is separation of
> certificate signing from code signing keys, where "none" is no
> separation and "max" being total separation of keys based on usage.
> 
> Subsequent work, as discussed in the cover letter thread, will limit
> certificates being loaded onto the IMA keyring to code signing keys
> used for signature verification.

I will include this in the next round too, thanks.


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

* Re: [PATCH v4 6/6] integrity: machine keyring CA configuration
  2023-02-10 13:05   ` Mimi Zohar
  2023-02-10 22:42     ` Eric Snowberg
@ 2023-02-13  7:54     ` Jarkko Sakkinen
  2023-02-14 21:24       ` Eric Snowberg
  1 sibling, 1 reply; 24+ messages in thread
From: Jarkko Sakkinen @ 2023-02-13  7:54 UTC (permalink / raw)
  To: Mimi Zohar
  Cc: Eric Snowberg, dhowells, dwmw2, herbert, davem, dmitry.kasatkin,
	paul, jmorris, serge, pvorel, tadeusz.struk, kanth.ghatraju,
	konrad.wilk, erpalmer, coxu, keyrings, linux-kernel,
	linux-crypto, linux-integrity, linux-security-module

On Fri, Feb 10, 2023 at 08:05:22AM -0500, Mimi Zohar wrote:
> Hi Eric,
> 
> On Mon, 2023-02-06 at 21:59 -0500, Eric Snowberg wrote:
> > Add a machine keyring CA restriction menu option to control the type of
> > keys that may be added to it. The options include none, min and max
> > restrictions.
> > 
> > When no restrictions are selected, all Machine Owner Keys (MOK) are added
> > to the machine keyring.  When CONFIG_INTEGRITY_CA_MACHINE_KEYRING_MIN is
> > selected, the CA bit must be true.  Also the key usage must contain
> > keyCertSign, any other usage field may be set as well.
> > 
> > When CONFIG_INTEGRITY_CA_MACHINE_KEYRING_MAX is selected, the CA bit must
> > be true. Also the key usage must contain keyCertSign and the
> > digitialSignature usage may not be set.
> > 
> > Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
> 
> Missing from the patch description is the motivation for this change.  
> The choices none, min, max implies a progression, which is good, and
> the technical differences between the choices, but not the reason.
> 
> The motivation, at least from my perspective, is separation of
> certificate signing from code signing keys, where "none" is no
> separation and "max" being total separation of keys based on usage.
> 
> Subsequent work, as discussed in the cover letter thread, will limit
> certificates being loaded onto the IMA keyring to code signing keys
> used for signature verification.


It would be more robust just to have two binary options for CA bit and
keyCertSign. You can use "select" for setting keyCertSign, when CA bit
option is selected.

BR, Jarkko

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

* Re: [PATCH v4 6/6] integrity: machine keyring CA configuration
  2023-02-13  7:54     ` Jarkko Sakkinen
@ 2023-02-14 21:24       ` Eric Snowberg
  0 siblings, 0 replies; 24+ messages in thread
From: Eric Snowberg @ 2023-02-14 21:24 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: Mimi Zohar, David Howells, David Woodhouse, herbert, davem,
	dmitry.kasatkin, paul, jmorris, serge, pvorel, tadeusz.struk,
	Kanth Ghatraju, Konrad Wilk, Elaine Palmer, Coiby Xu, keyrings,
	linux-kernel, linux-crypto, linux-integrity,
	linux-security-module



> On Feb 13, 2023, at 12:54 AM, Jarkko Sakkinen <jarkko@kernel.org> wrote:
> 
> On Fri, Feb 10, 2023 at 08:05:22AM -0500, Mimi Zohar wrote:
>> Hi Eric,
>> 
>> On Mon, 2023-02-06 at 21:59 -0500, Eric Snowberg wrote:
>>> Add a machine keyring CA restriction menu option to control the type of
>>> keys that may be added to it. The options include none, min and max
>>> restrictions.
>>> 
>>> When no restrictions are selected, all Machine Owner Keys (MOK) are added
>>> to the machine keyring.  When CONFIG_INTEGRITY_CA_MACHINE_KEYRING_MIN is
>>> selected, the CA bit must be true.  Also the key usage must contain
>>> keyCertSign, any other usage field may be set as well.
>>> 
>>> When CONFIG_INTEGRITY_CA_MACHINE_KEYRING_MAX is selected, the CA bit must
>>> be true. Also the key usage must contain keyCertSign and the
>>> digitialSignature usage may not be set.
>>> 
>>> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
>> 
>> Missing from the patch description is the motivation for this change.  
>> The choices none, min, max implies a progression, which is good, and
>> the technical differences between the choices, but not the reason.
>> 
>> The motivation, at least from my perspective, is separation of
>> certificate signing from code signing keys, where "none" is no
>> separation and "max" being total separation of keys based on usage.
>> 
>> Subsequent work, as discussed in the cover letter thread, will limit
>> certificates being loaded onto the IMA keyring to code signing keys
>> used for signature verification.
> 
> 
> It would be more robust just to have two binary options for CA bit and
> keyCertSign. You can use "select" for setting keyCertSign, when CA bit
> option is selected.

Ok, I will make that change in the next round, thanks.


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

end of thread, other threads:[~2023-02-14 21:26 UTC | newest]

Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-02-07  2:59 [PATCH v4 0/6] Add CA enforcement keyring restrictions Eric Snowberg
2023-02-07  2:59 ` [PATCH v4 1/6] KEYS: Create static version of public_key_verify_signature Eric Snowberg
2023-02-10  3:39   ` Jarkko Sakkinen
2023-02-10 22:38     ` Eric Snowberg
2023-02-07  2:59 ` [PATCH v4 2/6] KEYS: Add missing function documentation Eric Snowberg
2023-02-08 21:48   ` Mimi Zohar
2023-02-10  3:40   ` Jarkko Sakkinen
2023-02-07  2:59 ` [PATCH v4 3/6] KEYS: X.509: Parse Basic Constraints for CA Eric Snowberg
2023-02-08 21:01   ` Mimi Zohar
2023-02-10  3:46   ` Jarkko Sakkinen
2023-02-07  2:59 ` [PATCH v4 4/6] KEYS: X.509: Parse Key Usage Eric Snowberg
2023-02-08 21:02   ` Mimi Zohar
2023-02-10  3:48   ` Jarkko Sakkinen
2023-02-10 22:39     ` Eric Snowberg
2023-02-07  2:59 ` [PATCH v4 5/6] KEYS: CA link restriction Eric Snowberg
2023-02-09  2:55   ` Mimi Zohar
2023-02-07  2:59 ` [PATCH v4 6/6] integrity: machine keyring CA configuration Eric Snowberg
2023-02-10 13:05   ` Mimi Zohar
2023-02-10 22:42     ` Eric Snowberg
2023-02-13  7:54     ` Jarkko Sakkinen
2023-02-14 21:24       ` Eric Snowberg
2023-02-08 12:38 ` [PATCH v4 0/6] Add CA enforcement keyring restrictions Mimi Zohar
2023-02-08 23:26   ` Eric Snowberg
2023-02-09  2:54     ` Mimi Zohar

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).