2023-03-22 16:23:37

by Eric Snowberg

[permalink] [raw]
Subject: [PATCH v6 0/6] Add CA enforcement keyring restrictions

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 enforcement with the machine
keyring. It introduces three different ways to configure the machine
keyring. New Kconfig options are added to control the types of keys
that may be added to it. The default option allows all MOK keys into the
machine keyring. When CONFIG_INTEGRITY_CA_MACHINE_KEYRING is selected,
the X.509 CA bit must be true and the key usage must contain keyCertSign;
any other usage field may also be set. When
CONFIG_INTEGRITY_CA_MACHINE_KEYRING_MAX is also selected, the X.509 CA
bit must be true and the key usage must contain keyCertSign. With this
option 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:
v6:
- No new code changes
- Added Reviewed-by and ACKs
- Formatting change requested by Jarkko

v5:
- Removed the Kconfig _MIN Kconfig option and split it into different
entries.
- Added requested commit message changes

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 | 45 ++++++++++++++++++++
crypto/asymmetric_keys/x509_cert_parser.c | 50 +++++++++++++++++++++++
include/crypto/public_key.h | 28 +++++++++++++
security/integrity/Kconfig | 23 ++++++++++-
security/integrity/digsig.c | 8 +++-
6 files changed, 162 insertions(+), 6 deletions(-)


base-commit: e8d018dd0257f744ca50a729e3d042cf2ec9da65
--
2.27.0


2023-03-22 16:23:37

by Eric Snowberg

[permalink] [raw]
Subject: [PATCH v6 3/6] KEYS: X.509: Parse Basic Constraints for CA

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 <[email protected]>
Reviewed-by: Mimi Zohar <[email protected]>
Reviewed-by: Jarkko Sakkinen <[email protected]>
---
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

2023-03-22 16:23:55

by Eric Snowberg

[permalink] [raw]
Subject: [PATCH v6 6/6] integrity: machine keyring CA configuration

Add machine keyring CA restriction options to control the type of
keys that may be added to it. The motivation is separation of
certificate signing from code signing keys. Subsquent work will
limit certificates being loaded into the IMA keyring to code
signing keys used for signature verification.

When no restrictions are selected, all Machine Owner Keys (MOK) are added
to the machine keyring. When CONFIG_INTEGRITY_CA_MACHINE_KEYRING 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 <[email protected]>
Acked-by: Mimi Zohar <[email protected]>
---
crypto/asymmetric_keys/restrict.c | 3 +++
security/integrity/Kconfig | 23 ++++++++++++++++++++++-
security/integrity/digsig.c | 8 ++++++--
3 files changed, 31 insertions(+), 3 deletions(-)

diff --git a/crypto/asymmetric_keys/restrict.c b/crypto/asymmetric_keys/restrict.c
index dd9ced32c8a1..d6cd1dc2bec8 100644
--- a/crypto/asymmetric_keys/restrict.c
+++ b/crypto/asymmetric_keys/restrict.c
@@ -144,6 +144,9 @@ int restrict_link_by_ca(struct key *dest_keyring,
if (!test_bit(KEY_EFLAG_KEYCERTSIGN, &pkey->key_eflags))
return -ENOKEY;

+ if (!IS_ENABLED(CONFIG_INTEGRITY_CA_MACHINE_KEYRING_MAX))
+ 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..ec6e0d789da1 100644
--- a/security/integrity/Kconfig
+++ b/security/integrity/Kconfig
@@ -68,13 +68,34 @@ 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.

+config INTEGRITY_CA_MACHINE_KEYRING
+ bool "Enforce Machine Keyring CA Restrictions"
+ depends on INTEGRITY_MACHINE_KEYRING
+ default n
+ help
+ The .machine keyring can be configured to enforce CA restriction
+ on any key added to it. By default no restrictions are in place
+ and all Machine Owner Keys (MOK) are added to the machine keyring.
+ If enabled only CA keys are added to the machine keyring, all
+ other MOK keys load into the platform keyring.
+
+config INTEGRITY_CA_MACHINE_KEYRING_MAX
+ bool "Only CA keys without DigitialSignature usage set"
+ depends on INTEGRITY_CA_MACHINE_KEYRING
+ default n
+ help
+ When selected, only load CA keys are loaded into the machine
+ keyring that contain the CA bit 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.
+
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..6f31ffe23c48 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))) {
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

2023-03-22 16:24:10

by Eric Snowberg

[permalink] [raw]
Subject: [PATCH v6 5/6] KEYS: CA link restriction

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 <[email protected]>
Reviewed-by: Mimi Zohar <[email protected]>
---
crypto/asymmetric_keys/restrict.c | 42 +++++++++++++++++++++++++++++++
include/crypto/public_key.h | 15 +++++++++++
2 files changed, 57 insertions(+)

diff --git a/crypto/asymmetric_keys/restrict.c b/crypto/asymmetric_keys/restrict.c
index 6b1ac5f5896a..dd9ced32c8a1 100644
--- a/crypto/asymmetric_keys/restrict.c
+++ b/crypto/asymmetric_keys/restrict.c
@@ -108,6 +108,48 @@ int restrict_link_by_signature(struct key *dest_keyring,
return ret;
}

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

2023-03-22 16:28:25

by Eric Snowberg

[permalink] [raw]
Subject: [PATCH v6 1/6] KEYS: Create static version of public_key_verify_signature

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

Fixes: db6c43bd2132 ("crypto: KEYS: convert public key and digsig asym to the akcipher api")
Reported-by: kernel test robot <[email protected]>
Signed-off-by: Eric Snowberg <[email protected]>
Reviewed-by: Mimi Zohar <[email protected]>
Reviewed-by: Petr Vorel <[email protected]>
Reviewed-by: Jarkko Sakkinen <[email protected]>
---
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

2023-03-22 16:28:48

by Eric Snowberg

[permalink] [raw]
Subject: [PATCH v6 2/6] KEYS: Add missing function documentation

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 <[email protected]>
Reviewed-by: Petr Vorel <[email protected]>
Reviewed-by: Mimi Zohar <[email protected]>
Reviewed-by: Jarkko Sakkinen <[email protected]>
---
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..a7a49b17ceb1 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 built-in 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

2023-03-29 22:08:55

by Jarkko Sakkinen

[permalink] [raw]
Subject: Re: [PATCH v6 0/6] Add CA enforcement keyring restrictions

On Wed, Mar 22, 2023 at 12:16:28PM -0400, 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 enforcement with the machine
> keyring. It introduces three different ways to configure the machine
> keyring. New Kconfig options are added to control the types of keys
> that may be added to it. The default option allows all MOK keys into the
> machine keyring. When CONFIG_INTEGRITY_CA_MACHINE_KEYRING is selected,
> the X.509 CA bit must be true and the key usage must contain keyCertSign;
> any other usage field may also be set. When
> CONFIG_INTEGRITY_CA_MACHINE_KEYRING_MAX is also selected, the X.509 CA
> bit must be true and the key usage must contain keyCertSign. With this
> option 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:
> v6:
> - No new code changes
> - Added Reviewed-by and ACKs
> - Formatting change requested by Jarkko
>
> v5:
> - Removed the Kconfig _MIN Kconfig option and split it into different
> entries.
> - Added requested commit message changes
>
> 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 | 45 ++++++++++++++++++++
> crypto/asymmetric_keys/x509_cert_parser.c | 50 +++++++++++++++++++++++
> include/crypto/public_key.h | 28 +++++++++++++
> security/integrity/Kconfig | 23 ++++++++++-
> security/integrity/digsig.c | 8 +++-
> 6 files changed, 162 insertions(+), 6 deletions(-)
>
>
> base-commit: e8d018dd0257f744ca50a729e3d042cf2ec9da65
> --
> 2.27.0
>

I can pick this, and I guess I can add Mimi's tested-by's to all of the
patches?

BR, Jarkko