* [PATCH 0/4] Add CA enforcement in the machine keyring
@ 2022-03-01 17:36 Eric Snowberg
2022-03-01 17:36 ` [PATCH 1/4] KEYS: Create static version of public_key_verify_signature Eric Snowberg
` (5 more replies)
0 siblings, 6 replies; 30+ messages in thread
From: Eric Snowberg @ 2022-03-01 17:36 UTC (permalink / raw)
To: zohar, jarkko, dhowells, dwmw2
Cc: herbert, davem, jmorris, serge, eric.snowberg, stefanb, nayna,
mic, konrad.wilk, keyrings, linux-kernel, linux-crypto,
linux-security-module
A key added to the IMA keyring must be signed by a key contained in either the
built-in trusted or secondary trusted keyring. IMA also requires these keys
to be a CA. The only option for an end-user to add their own CA is to compile
it into the kernel themselves or to use the insert-sys-cert. Many end-users
do not want to compile their own kernels. With the insert-sys-cert option,
there are missing upstream changes.
Currently, all Machine Owner Keys (MOK) load into the machine keyring. Add
a new Kconfig option to only allow CA keys into the machine keyring. When
compiled with the new INTEGRITY_MACHINE_KEYRING_CA_ENFORCED Kconfig, non CA
keys will load into the platform keyring instead. This will allow the end-
user to enroll their own CA key into the machine keyring for use with IMA.
These patches are based on Jarkko's linux-tpmdd tree.
git://git.kernel.org/pub/scm/linux/kernel/git/jarkko/linux-tpmdd.git
Eric Snowberg (4):
KEYS: Create static version of public_key_verify_signature
X.509: Parse Basic Constraints for CA
KEYS: CA link restriction
integrity: restrict INTEGRITY_KEYRING_MACHINE to restrict_link_by_ca
certs/system_keyring.c | 9 ++--
crypto/asymmetric_keys/restrict.c | 43 +++++++++++++++++++
crypto/asymmetric_keys/x509_cert_parser.c | 9 ++++
include/crypto/public_key.h | 25 +++++++++++
include/keys/system_keyring.h | 3 +-
security/integrity/Kconfig | 21 +++++++++
security/integrity/Makefile | 1 +
security/integrity/digsig.c | 14 ++++--
security/integrity/integrity.h | 3 +-
.../platform_certs/keyring_handler.c | 4 +-
10 files changed, 123 insertions(+), 9 deletions(-)
base-commit: c9e54f38976a1c0ec69c0a6208b3fd55fceb01d1
--
2.27.0
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 1/4] KEYS: Create static version of public_key_verify_signature
2022-03-01 17:36 [PATCH 0/4] Add CA enforcement in the machine keyring Eric Snowberg
@ 2022-03-01 17:36 ` Eric Snowberg
2022-03-01 17:36 ` [PATCH 2/4] X.509: Parse Basic Constraints for CA Eric Snowberg
` (4 subsequent siblings)
5 siblings, 0 replies; 30+ messages in thread
From: Eric Snowberg @ 2022-03-01 17:36 UTC (permalink / raw)
To: zohar, jarkko, dhowells, dwmw2
Cc: herbert, davem, jmorris, serge, eric.snowberg, stefanb, nayna,
mic, konrad.wilk, keyrings, linux-kernel, linux-crypto,
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.
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
---
include/crypto/public_key.h | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/include/crypto/public_key.h b/include/crypto/public_key.h
index 68f7aa2a7e55..6d61695e1cde 100644
--- a/include/crypto/public_key.h
+++ b/include/crypto/public_key.h
@@ -80,7 +80,16 @@ extern int create_signature(struct kernel_pkey_params *, const void *, void *);
extern int verify_signature(const struct key *,
const struct public_key_signature *);
+#if IS_REACHABLE(CONFIG_ASYMMETRIC_PUBLIC_KEY_SUBTYPE)
int public_key_verify_signature(const struct public_key *pkey,
const struct public_key_signature *sig);
+#else
+static inline
+int public_key_verify_signature(const struct public_key *pkey,
+ const struct public_key_signature *sig)
+{
+ return -EINVAL;
+}
+#endif
#endif /* _LINUX_PUBLIC_KEY_H */
--
2.27.0
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 2/4] X.509: Parse Basic Constraints for CA
2022-03-01 17:36 [PATCH 0/4] Add CA enforcement in the machine keyring Eric Snowberg
2022-03-01 17:36 ` [PATCH 1/4] KEYS: Create static version of public_key_verify_signature Eric Snowberg
@ 2022-03-01 17:36 ` Eric Snowberg
2022-03-04 15:10 ` Stefan Berger
2022-03-01 17:36 ` [PATCH 3/4] KEYS: CA link restriction Eric Snowberg
` (3 subsequent siblings)
5 siblings, 1 reply; 30+ messages in thread
From: Eric Snowberg @ 2022-03-01 17:36 UTC (permalink / raw)
To: zohar, jarkko, dhowells, dwmw2
Cc: herbert, davem, jmorris, serge, eric.snowberg, stefanb, nayna,
mic, konrad.wilk, keyrings, linux-kernel, linux-crypto,
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 a new public_key field call key_is_ca.
This will be used in a follow on patch that requires knowing if the
public key is a CA.
Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
---
crypto/asymmetric_keys/x509_cert_parser.c | 9 +++++++++
include/crypto/public_key.h | 1 +
2 files changed, 10 insertions(+)
diff --git a/crypto/asymmetric_keys/x509_cert_parser.c b/crypto/asymmetric_keys/x509_cert_parser.c
index 2899ed80bb18..38c907f4ce27 100644
--- a/crypto/asymmetric_keys/x509_cert_parser.c
+++ b/crypto/asymmetric_keys/x509_cert_parser.c
@@ -583,6 +583,15 @@ int x509_process_extension(void *context, size_t hdrlen,
return 0;
}
+ if (ctx->last_oid == OID_basicConstraints) {
+ if (v[0] != (ASN1_CONS_BIT | ASN1_SEQ))
+ return -EBADMSG;
+ if (v[1] != vlen - 2)
+ return -EBADMSG;
+ if (v[1] != 0 && v[2] == ASN1_BOOL && v[3] == 1)
+ ctx->cert->pub->key_is_ca = true;
+ }
+
return 0;
}
diff --git a/include/crypto/public_key.h b/include/crypto/public_key.h
index 6d61695e1cde..0521241764b7 100644
--- a/include/crypto/public_key.h
+++ b/include/crypto/public_key.h
@@ -26,6 +26,7 @@ struct public_key {
void *params;
u32 paramlen;
bool key_is_private;
+ bool key_is_ca;
const char *id_type;
const char *pkey_algo;
};
--
2.27.0
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 3/4] KEYS: CA link restriction
2022-03-01 17:36 [PATCH 0/4] Add CA enforcement in the machine keyring Eric Snowberg
2022-03-01 17:36 ` [PATCH 1/4] KEYS: Create static version of public_key_verify_signature Eric Snowberg
2022-03-01 17:36 ` [PATCH 2/4] X.509: Parse Basic Constraints for CA Eric Snowberg
@ 2022-03-01 17:36 ` Eric Snowberg
2022-03-04 15:28 ` Stefan Berger
2022-03-01 17:36 ` [PATCH 4/4] integrity: CA enforcement in machine keyring Eric Snowberg
` (2 subsequent siblings)
5 siblings, 1 reply; 30+ messages in thread
From: Eric Snowberg @ 2022-03-01 17:36 UTC (permalink / raw)
To: zohar, jarkko, dhowells, dwmw2
Cc: herbert, davem, jmorris, serge, eric.snowberg, stefanb, nayna,
mic, konrad.wilk, keyrings, linux-kernel, linux-crypto,
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 | 43 +++++++++++++++++++++++++++++++
include/crypto/public_key.h | 15 +++++++++++
2 files changed, 58 insertions(+)
diff --git a/crypto/asymmetric_keys/restrict.c b/crypto/asymmetric_keys/restrict.c
index 6b1ac5f5896a..49bb2ea7f609 100644
--- a/crypto/asymmetric_keys/restrict.c
+++ b/crypto/asymmetric_keys/restrict.c
@@ -108,6 +108,49 @@ 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_signature *sig;
+ const struct public_key *pkey;
+
+ if (type != &key_type_asymmetric)
+ return -EOPNOTSUPP;
+
+ sig = payload->data[asym_auth];
+ if (!sig)
+ return -ENOPKG;
+
+ if (!sig->auth_ids[0] && !sig->auth_ids[1])
+ return -ENOKEY;
+
+ pkey = payload->data[asym_crypto];
+ if (!pkey)
+ return -ENOPKG;
+
+ if (!pkey->key_is_ca)
+ return -ENOKEY;
+
+ return public_key_verify_signature(pkey, sig);
+}
+
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 0521241764b7..5eadb182a400 100644
--- a/include/crypto/public_key.h
+++ b/include/crypto/public_key.h
@@ -72,6 +72,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] 30+ messages in thread
* [PATCH 4/4] integrity: CA enforcement in machine keyring
2022-03-01 17:36 [PATCH 0/4] Add CA enforcement in the machine keyring Eric Snowberg
` (2 preceding siblings ...)
2022-03-01 17:36 ` [PATCH 3/4] KEYS: CA link restriction Eric Snowberg
@ 2022-03-01 17:36 ` Eric Snowberg
2022-03-04 23:19 ` Stefan Berger
2022-03-06 23:33 ` [PATCH 0/4] Add CA enforcement in the " Mimi Zohar
2022-03-09 18:43 ` Mimi Zohar
5 siblings, 1 reply; 30+ messages in thread
From: Eric Snowberg @ 2022-03-01 17:36 UTC (permalink / raw)
To: zohar, jarkko, dhowells, dwmw2
Cc: herbert, davem, jmorris, serge, eric.snowberg, stefanb, nayna,
mic, konrad.wilk, keyrings, linux-kernel, linux-crypto,
linux-security-module
When INTEGRITY_MACHINE_KEYRING is set, all Machine Owner Keys (MOK)
are loaded into the machine keyring. Add a new
INTEGRITY_MACHINE_KEYRING_CA_ENFORCED option where only MOK CA keys are
added.
Set the restriction check to restrict_link_by_ca. This will only allow
CA keys into the machine keyring. Unlike when INTEGRITY_MACHINE_KEYRING
is enabled, IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY may
also be enabled, allowing IMA to use keys in the machine keyring as
another trust anchor.
Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
---
certs/system_keyring.c | 9 +++++---
include/keys/system_keyring.h | 3 ++-
security/integrity/Kconfig | 21 +++++++++++++++++++
security/integrity/Makefile | 1 +
security/integrity/digsig.c | 14 ++++++++++---
security/integrity/integrity.h | 3 ++-
.../platform_certs/keyring_handler.c | 4 +++-
7 files changed, 46 insertions(+), 9 deletions(-)
diff --git a/certs/system_keyring.c b/certs/system_keyring.c
index 05b66ce9d1c9..0811b44cf3bf 100644
--- a/certs/system_keyring.c
+++ b/certs/system_keyring.c
@@ -22,7 +22,8 @@ static struct key *builtin_trusted_keys;
#ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
static struct key *secondary_trusted_keys;
#endif
-#ifdef CONFIG_INTEGRITY_MACHINE_KEYRING
+#if defined(CONFIG_INTEGRITY_MACHINE_KEYRING) || \
+ defined(CONFIG_INTEGRITY_MACHINE_KEYRING_CA_ENFORCED)
static struct key *machine_trusted_keys;
#endif
#ifdef CONFIG_INTEGRITY_PLATFORM_KEYRING
@@ -89,7 +90,8 @@ static __init struct key_restriction *get_builtin_and_secondary_restriction(void
if (!restriction)
panic("Can't allocate secondary trusted keyring restriction\n");
- if (IS_ENABLED(CONFIG_INTEGRITY_MACHINE_KEYRING))
+ if (IS_ENABLED(CONFIG_INTEGRITY_MACHINE_KEYRING) ||
+ IS_ENABLED(CONFIG_INTEGRITY_MACHINE_KEYRING_CA_ENFORCED))
restriction->check = restrict_link_by_builtin_secondary_and_machine;
else
restriction->check = restrict_link_by_builtin_and_secondary_trusted;
@@ -97,7 +99,8 @@ static __init struct key_restriction *get_builtin_and_secondary_restriction(void
return restriction;
}
#endif
-#ifdef CONFIG_INTEGRITY_MACHINE_KEYRING
+#if defined(CONFIG_INTEGRITY_MACHINE_KEYRING) || \
+ defined(CONFIG_INTEGRITY_MACHINE_KEYRING_CA_ENFORCED)
void __init set_machine_trusted_keys(struct key *keyring)
{
machine_trusted_keys = keyring;
diff --git a/include/keys/system_keyring.h b/include/keys/system_keyring.h
index 91e080efb918..e4a6574bbcb6 100644
--- a/include/keys/system_keyring.h
+++ b/include/keys/system_keyring.h
@@ -45,7 +45,8 @@ extern int restrict_link_by_builtin_and_secondary_trusted(
#define restrict_link_by_builtin_and_secondary_trusted restrict_link_by_builtin_trusted
#endif
-#ifdef CONFIG_INTEGRITY_MACHINE_KEYRING
+#if defined(CONFIG_INTEGRITY_MACHINE_KEYRING) || \
+ defined(CONFIG_INTEGRITY_MACHINE_KEYRING_CA_ENFORCED)
extern int restrict_link_by_builtin_secondary_and_machine(
struct key *dest_keyring,
const struct key_type *type,
diff --git a/security/integrity/Kconfig b/security/integrity/Kconfig
index 599429f99f99..14c927eea5ee 100644
--- a/security/integrity/Kconfig
+++ b/security/integrity/Kconfig
@@ -62,6 +62,14 @@ config INTEGRITY_PLATFORM_KEYRING
provided by the platform for verifying the kexec'ed kerned image
and, possibly, the initramfs signature.
+
+choice
+ prompt "Machine keyring"
+ default INTEGRITY_MACHINE_NONE
+
+config INTEGRITY_MACHINE_NONE
+ bool "Do not enable the Machine Owner Keyring"
+
config INTEGRITY_MACHINE_KEYRING
bool "Provide a keyring to which Machine Owner Keys may be added"
depends on SECONDARY_TRUSTED_KEYRING
@@ -75,6 +83,19 @@ config INTEGRITY_MACHINE_KEYRING
in the platform keyring, keys contained in the .machine keyring will
be trusted within the kernel.
+config INTEGRITY_MACHINE_KEYRING_CA_ENFORCED
+ bool "Provide a keyring to which Machine Owner CA Keys may be added"
+ depends on SECONDARY_TRUSTED_KEYRING
+ depends on INTEGRITY_ASYMMETRIC_KEYS
+ depends on SYSTEM_BLACKLIST_KEYRING
+ depends on LOAD_UEFI_KEYS
+ help
+ If set, provide a keyring to which CA Machine Owner Keys (MOK) may
+ be added. This keyring shall contain just CA MOK keys. Unlike keys
+ in the platform keyring, keys contained in the .machine keyring will
+ be trusted within the kernel.
+endchoice
+
config LOAD_UEFI_KEYS
depends on INTEGRITY_PLATFORM_KEYRING
depends on EFI
diff --git a/security/integrity/Makefile b/security/integrity/Makefile
index d0ffe37dc1d6..370ee63774c3 100644
--- a/security/integrity/Makefile
+++ b/security/integrity/Makefile
@@ -11,6 +11,7 @@ integrity-$(CONFIG_INTEGRITY_SIGNATURE) += digsig.o
integrity-$(CONFIG_INTEGRITY_ASYMMETRIC_KEYS) += digsig_asymmetric.o
integrity-$(CONFIG_INTEGRITY_PLATFORM_KEYRING) += platform_certs/platform_keyring.o
integrity-$(CONFIG_INTEGRITY_MACHINE_KEYRING) += platform_certs/machine_keyring.o
+integrity-$(CONFIG_INTEGRITY_MACHINE_KEYRING_CA_ENFORCED) += platform_certs/machine_keyring.o
integrity-$(CONFIG_LOAD_UEFI_KEYS) += platform_certs/efi_parser.o \
platform_certs/load_uefi.o \
platform_certs/keyring_handler.o
diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
index c8c8a4a4e7a0..041edd9744db 100644
--- a/security/integrity/digsig.c
+++ b/security/integrity/digsig.c
@@ -34,7 +34,11 @@ static const char * const keyring_name[INTEGRITY_KEYRING_MAX] = {
};
#ifdef CONFIG_IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY
+#ifdef CONFIG_INTEGRITY_MACHINE_KEYRING_CA_ENFORCED
+#define restrict_link_to_ima restrict_link_by_builtin_secondary_and_machine
+#else
#define restrict_link_to_ima restrict_link_by_builtin_and_secondary_trusted
+#endif
#else
#define restrict_link_to_ima restrict_link_by_builtin_trusted
#endif
@@ -130,19 +134,23 @@ int __init integrity_init_keyring(const unsigned int id)
| KEY_USR_READ | KEY_USR_SEARCH;
if (id == INTEGRITY_KEYRING_PLATFORM ||
- id == INTEGRITY_KEYRING_MACHINE) {
+ (IS_ENABLED(CONFIG_INTEGRITY_MACHINE_KEYRING))) {
restriction = NULL;
goto out;
}
- if (!IS_ENABLED(CONFIG_INTEGRITY_TRUSTED_KEYRING))
+ if (!IS_ENABLED(CONFIG_INTEGRITY_TRUSTED_KEYRING) &&
+ id != INTEGRITY_KEYRING_MACHINE)
return 0;
restriction = kzalloc(sizeof(struct key_restriction), GFP_KERNEL);
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
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index 2e214c761158..ca4d72fbd045 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -285,7 +285,8 @@ static inline void __init add_to_platform_keyring(const char *source,
}
#endif
-#ifdef CONFIG_INTEGRITY_MACHINE_KEYRING
+#if defined(CONFIG_INTEGRITY_MACHINE_KEYRING) || \
+ defined(CONFIG_INTEGRITY_MACHINE_KEYRING_CA_ENFORCED)
void __init add_to_machine_keyring(const char *source, const void *data, size_t len);
bool __init trust_moklist(void);
#else
diff --git a/security/integrity/platform_certs/keyring_handler.c b/security/integrity/platform_certs/keyring_handler.c
index a2464f3e66cc..9c456ad0ab67 100644
--- a/security/integrity/platform_certs/keyring_handler.c
+++ b/security/integrity/platform_certs/keyring_handler.c
@@ -61,7 +61,9 @@ __init efi_element_handler_t get_handler_for_db(const efi_guid_t *sig_type)
__init efi_element_handler_t get_handler_for_mok(const efi_guid_t *sig_type)
{
if (efi_guidcmp(*sig_type, efi_cert_x509_guid) == 0) {
- if (IS_ENABLED(CONFIG_INTEGRITY_MACHINE_KEYRING) && trust_moklist())
+ if ((IS_ENABLED(CONFIG_INTEGRITY_MACHINE_KEYRING) ||
+ IS_ENABLED(CONFIG_INTEGRITY_MACHINE_KEYRING_CA_ENFORCED)) &&
+ trust_moklist())
return add_to_machine_keyring;
else
return add_to_platform_keyring;
--
2.27.0
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH 2/4] X.509: Parse Basic Constraints for CA
2022-03-01 17:36 ` [PATCH 2/4] X.509: Parse Basic Constraints for CA Eric Snowberg
@ 2022-03-04 15:10 ` Stefan Berger
2022-03-07 18:02 ` Eric Snowberg
0 siblings, 1 reply; 30+ messages in thread
From: Stefan Berger @ 2022-03-04 15:10 UTC (permalink / raw)
To: Eric Snowberg, zohar, jarkko, dhowells, dwmw2
Cc: herbert, davem, jmorris, serge, nayna, mic, konrad.wilk,
keyrings, linux-kernel, linux-crypto, linux-security-module
On 3/1/22 12:36, 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 a new public_key field call key_is_ca.
> This will be used in a follow on patch that requires knowing if the
> public key is a CA.
>
> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
> ---
> crypto/asymmetric_keys/x509_cert_parser.c | 9 +++++++++
> include/crypto/public_key.h | 1 +
> 2 files changed, 10 insertions(+)
>
> diff --git a/crypto/asymmetric_keys/x509_cert_parser.c b/crypto/asymmetric_keys/x509_cert_parser.c
> index 2899ed80bb18..38c907f4ce27 100644
> --- a/crypto/asymmetric_keys/x509_cert_parser.c
> +++ b/crypto/asymmetric_keys/x509_cert_parser.c
> @@ -583,6 +583,15 @@ int x509_process_extension(void *context, size_t hdrlen,
> return 0;
> }
>
> + if (ctx->last_oid == OID_basicConstraints) {
Don't you have to check whether you can access v[0] and v[1]?
if (vlen < 3)
return -EBADMSG;
or should it even be
if (vlen != 3)
return -EBADMSG;
> + if (v[0] != (ASN1_CONS_BIT | ASN1_SEQ))
> + return -EBADMSG;
> + if (v[1] != vlen - 2)
> + return -EBADMSG;
> + if (v[1] != 0 && v[2] == ASN1_BOOL && v[3] == 1)
> + ctx->cert->pub->key_is_ca = true;
> + }
> +
> return 0;
> }
>
> diff --git a/include/crypto/public_key.h b/include/crypto/public_key.h
> index 6d61695e1cde..0521241764b7 100644
> --- a/include/crypto/public_key.h
> +++ b/include/crypto/public_key.h
> @@ -26,6 +26,7 @@ struct public_key {
> void *params;
> u32 paramlen;
> bool key_is_private;
> + bool key_is_ca;
> const char *id_type;
> const char *pkey_algo;
> };
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 3/4] KEYS: CA link restriction
2022-03-01 17:36 ` [PATCH 3/4] KEYS: CA link restriction Eric Snowberg
@ 2022-03-04 15:28 ` Stefan Berger
2022-03-07 18:06 ` Eric Snowberg
0 siblings, 1 reply; 30+ messages in thread
From: Stefan Berger @ 2022-03-04 15:28 UTC (permalink / raw)
To: Eric Snowberg, zohar, jarkko, dhowells, dwmw2
Cc: herbert, davem, jmorris, serge, nayna, mic, konrad.wilk,
keyrings, linux-kernel, linux-crypto, linux-security-module
On 3/1/22 12:36, 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>
> ---
> crypto/asymmetric_keys/restrict.c | 43 +++++++++++++++++++++++++++++++
> include/crypto/public_key.h | 15 +++++++++++
> 2 files changed, 58 insertions(+)
>
> diff --git a/crypto/asymmetric_keys/restrict.c b/crypto/asymmetric_keys/restrict.c
> index 6b1ac5f5896a..49bb2ea7f609 100644
> --- a/crypto/asymmetric_keys/restrict.c
> +++ b/crypto/asymmetric_keys/restrict.c
> @@ -108,6 +108,49 @@ 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.
CA = root CA here, right?
> + *
> + * 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)
This function needs to correspond to the key_restrict_link_func_t and
therefore has 4 parameter. Call the unused 'trust_keyring' 'unused' instead?
> +{
> + const struct public_key_signature *sig;
> + const struct public_key *pkey;
> +
> + if (type != &key_type_asymmetric)
> + return -EOPNOTSUPP;
> +
> + sig = payload->data[asym_auth];
> + if (!sig)
> + return -ENOPKG;
> +
> + if (!sig->auth_ids[0] && !sig->auth_ids[1])
> + return -ENOKEY;
> +
> + pkey = payload->data[asym_crypto];
> + if (!pkey)
> + return -ENOPKG;
> +
> + if (!pkey->key_is_ca)
> + return -ENOKEY;
> +
> + return public_key_verify_signature(pkey, sig);
> +}
> +
Comparing this to 'restrict_link_by_signature'... looks good.
> 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 0521241764b7..5eadb182a400 100644
> --- a/include/crypto/public_key.h
> +++ b/include/crypto/public_key.h
> @@ -72,6 +72,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 *);
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 4/4] integrity: CA enforcement in machine keyring
2022-03-01 17:36 ` [PATCH 4/4] integrity: CA enforcement in machine keyring Eric Snowberg
@ 2022-03-04 23:19 ` Stefan Berger
2022-03-07 18:13 ` Eric Snowberg
0 siblings, 1 reply; 30+ messages in thread
From: Stefan Berger @ 2022-03-04 23:19 UTC (permalink / raw)
To: Eric Snowberg, zohar, jarkko, dhowells, dwmw2
Cc: herbert, davem, jmorris, serge, nayna, mic, konrad.wilk,
keyrings, linux-kernel, linux-crypto, linux-security-module
On 3/1/22 12:36, Eric Snowberg wrote:
> When INTEGRITY_MACHINE_KEYRING is set, all Machine Owner Keys (MOK)
> are loaded into the machine keyring. Add a new
> INTEGRITY_MACHINE_KEYRING_CA_ENFORCED option where only MOK CA keys are
> added.
>
> Set the restriction check to restrict_link_by_ca. This will only allow
> CA keys into the machine keyring. Unlike when INTEGRITY_MACHINE_KEYRING
> is enabled, IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY may
> also be enabled, allowing IMA to use keys in the machine keyring as
> another trust anchor.
I tried to test this but could only do it by disabling the
MokListTrustedRT variable check and then also the check for secure boot.
It did load the expected keys onto the .machine keyring, enforcing the
x509 indicating a self-signed CA if the compile time option
CONFIG_INTEGRITY_MACHINE_KEYRING_CA_ENFORCED=y was set, loading all keys
in the case of CONFIG_INTEGRITY_MACHINE_KEYRING=y.
I tried with this branch here from mokutils
https://github.com/esnowberg/mokutil/tree/trust-mok but this seems to
create an EFI variable with a different name. I guess this is the wrong
branch?
Stefan
> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
> ---
> certs/system_keyring.c | 9 +++++---
> include/keys/system_keyring.h | 3 ++-
> security/integrity/Kconfig | 21 +++++++++++++++++++
> security/integrity/Makefile | 1 +
> security/integrity/digsig.c | 14 ++++++++++---
> security/integrity/integrity.h | 3 ++-
> .../platform_certs/keyring_handler.c | 4 +++-
> 7 files changed, 46 insertions(+), 9 deletions(-)
>
> diff --git a/certs/system_keyring.c b/certs/system_keyring.c
> index 05b66ce9d1c9..0811b44cf3bf 100644
> --- a/certs/system_keyring.c
> +++ b/certs/system_keyring.c
> @@ -22,7 +22,8 @@ static struct key *builtin_trusted_keys;
> #ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
> static struct key *secondary_trusted_keys;
> #endif
> -#ifdef CONFIG_INTEGRITY_MACHINE_KEYRING
> +#if defined(CONFIG_INTEGRITY_MACHINE_KEYRING) || \
> + defined(CONFIG_INTEGRITY_MACHINE_KEYRING_CA_ENFORCED)
> static struct key *machine_trusted_keys;
> #endif
> #ifdef CONFIG_INTEGRITY_PLATFORM_KEYRING
> @@ -89,7 +90,8 @@ static __init struct key_restriction *get_builtin_and_secondary_restriction(void
> if (!restriction)
> panic("Can't allocate secondary trusted keyring restriction\n");
>
> - if (IS_ENABLED(CONFIG_INTEGRITY_MACHINE_KEYRING))
> + if (IS_ENABLED(CONFIG_INTEGRITY_MACHINE_KEYRING) ||
> + IS_ENABLED(CONFIG_INTEGRITY_MACHINE_KEYRING_CA_ENFORCED))
> restriction->check = restrict_link_by_builtin_secondary_and_machine;
> else
> restriction->check = restrict_link_by_builtin_and_secondary_trusted;
> @@ -97,7 +99,8 @@ static __init struct key_restriction *get_builtin_and_secondary_restriction(void
> return restriction;
> }
> #endif
> -#ifdef CONFIG_INTEGRITY_MACHINE_KEYRING
> +#if defined(CONFIG_INTEGRITY_MACHINE_KEYRING) || \
> + defined(CONFIG_INTEGRITY_MACHINE_KEYRING_CA_ENFORCED)
> void __init set_machine_trusted_keys(struct key *keyring)
> {
> machine_trusted_keys = keyring;
> diff --git a/include/keys/system_keyring.h b/include/keys/system_keyring.h
> index 91e080efb918..e4a6574bbcb6 100644
> --- a/include/keys/system_keyring.h
> +++ b/include/keys/system_keyring.h
> @@ -45,7 +45,8 @@ extern int restrict_link_by_builtin_and_secondary_trusted(
> #define restrict_link_by_builtin_and_secondary_trusted restrict_link_by_builtin_trusted
> #endif
>
> -#ifdef CONFIG_INTEGRITY_MACHINE_KEYRING
> +#if defined(CONFIG_INTEGRITY_MACHINE_KEYRING) || \
> + defined(CONFIG_INTEGRITY_MACHINE_KEYRING_CA_ENFORCED)
> extern int restrict_link_by_builtin_secondary_and_machine(
> struct key *dest_keyring,
> const struct key_type *type,
> diff --git a/security/integrity/Kconfig b/security/integrity/Kconfig
> index 599429f99f99..14c927eea5ee 100644
> --- a/security/integrity/Kconfig
> +++ b/security/integrity/Kconfig
> @@ -62,6 +62,14 @@ config INTEGRITY_PLATFORM_KEYRING
> provided by the platform for verifying the kexec'ed kerned image
> and, possibly, the initramfs signature.
>
> +
> +choice
> + prompt "Machine keyring"
> + default INTEGRITY_MACHINE_NONE
> +
> +config INTEGRITY_MACHINE_NONE
> + bool "Do not enable the Machine Owner Keyring"
> +
> config INTEGRITY_MACHINE_KEYRING
> bool "Provide a keyring to which Machine Owner Keys may be added"
> depends on SECONDARY_TRUSTED_KEYRING
> @@ -75,6 +83,19 @@ config INTEGRITY_MACHINE_KEYRING
> in the platform keyring, keys contained in the .machine keyring will
> be trusted within the kernel.
>
> +config INTEGRITY_MACHINE_KEYRING_CA_ENFORCED
> + bool "Provide a keyring to which Machine Owner CA Keys may be added"
> + depends on SECONDARY_TRUSTED_KEYRING
> + depends on INTEGRITY_ASYMMETRIC_KEYS
> + depends on SYSTEM_BLACKLIST_KEYRING
> + depends on LOAD_UEFI_KEYS
> + help
> + If set, provide a keyring to which CA Machine Owner Keys (MOK) may
> + be added. This keyring shall contain just CA MOK keys. Unlike keys
> + in the platform keyring, keys contained in the .machine keyring will
> + be trusted within the kernel.
> +endchoice
> +
> config LOAD_UEFI_KEYS
> depends on INTEGRITY_PLATFORM_KEYRING
> depends on EFI
> diff --git a/security/integrity/Makefile b/security/integrity/Makefile
> index d0ffe37dc1d6..370ee63774c3 100644
> --- a/security/integrity/Makefile
> +++ b/security/integrity/Makefile
> @@ -11,6 +11,7 @@ integrity-$(CONFIG_INTEGRITY_SIGNATURE) += digsig.o
> integrity-$(CONFIG_INTEGRITY_ASYMMETRIC_KEYS) += digsig_asymmetric.o
> integrity-$(CONFIG_INTEGRITY_PLATFORM_KEYRING) += platform_certs/platform_keyring.o
> integrity-$(CONFIG_INTEGRITY_MACHINE_KEYRING) += platform_certs/machine_keyring.o
> +integrity-$(CONFIG_INTEGRITY_MACHINE_KEYRING_CA_ENFORCED) += platform_certs/machine_keyring.o
> integrity-$(CONFIG_LOAD_UEFI_KEYS) += platform_certs/efi_parser.o \
> platform_certs/load_uefi.o \
> platform_certs/keyring_handler.o
> diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
> index c8c8a4a4e7a0..041edd9744db 100644
> --- a/security/integrity/digsig.c
> +++ b/security/integrity/digsig.c
> @@ -34,7 +34,11 @@ static const char * const keyring_name[INTEGRITY_KEYRING_MAX] = {
> };
>
> #ifdef CONFIG_IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY
> +#ifdef CONFIG_INTEGRITY_MACHINE_KEYRING_CA_ENFORCED
> +#define restrict_link_to_ima restrict_link_by_builtin_secondary_and_machine
> +#else
> #define restrict_link_to_ima restrict_link_by_builtin_and_secondary_trusted
> +#endif
> #else
> #define restrict_link_to_ima restrict_link_by_builtin_trusted
> #endif
> @@ -130,19 +134,23 @@ int __init integrity_init_keyring(const unsigned int id)
> | KEY_USR_READ | KEY_USR_SEARCH;
>
> if (id == INTEGRITY_KEYRING_PLATFORM ||
> - id == INTEGRITY_KEYRING_MACHINE) {
> + (IS_ENABLED(CONFIG_INTEGRITY_MACHINE_KEYRING))) {
> restriction = NULL;
> goto out;
> }
>
> - if (!IS_ENABLED(CONFIG_INTEGRITY_TRUSTED_KEYRING))
> + if (!IS_ENABLED(CONFIG_INTEGRITY_TRUSTED_KEYRING) &&
> + id != INTEGRITY_KEYRING_MACHINE)
> return 0;
>
> restriction = kzalloc(sizeof(struct key_restriction), GFP_KERNEL);
> 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
> diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
> index 2e214c761158..ca4d72fbd045 100644
> --- a/security/integrity/integrity.h
> +++ b/security/integrity/integrity.h
> @@ -285,7 +285,8 @@ static inline void __init add_to_platform_keyring(const char *source,
> }
> #endif
>
> -#ifdef CONFIG_INTEGRITY_MACHINE_KEYRING
> +#if defined(CONFIG_INTEGRITY_MACHINE_KEYRING) || \
> + defined(CONFIG_INTEGRITY_MACHINE_KEYRING_CA_ENFORCED)
> void __init add_to_machine_keyring(const char *source, const void *data, size_t len);
> bool __init trust_moklist(void);
> #else
> diff --git a/security/integrity/platform_certs/keyring_handler.c b/security/integrity/platform_certs/keyring_handler.c
> index a2464f3e66cc..9c456ad0ab67 100644
> --- a/security/integrity/platform_certs/keyring_handler.c
> +++ b/security/integrity/platform_certs/keyring_handler.c
> @@ -61,7 +61,9 @@ __init efi_element_handler_t get_handler_for_db(const efi_guid_t *sig_type)
> __init efi_element_handler_t get_handler_for_mok(const efi_guid_t *sig_type)
> {
> if (efi_guidcmp(*sig_type, efi_cert_x509_guid) == 0) {
> - if (IS_ENABLED(CONFIG_INTEGRITY_MACHINE_KEYRING) && trust_moklist())
> + if ((IS_ENABLED(CONFIG_INTEGRITY_MACHINE_KEYRING) ||
> + IS_ENABLED(CONFIG_INTEGRITY_MACHINE_KEYRING_CA_ENFORCED)) &&
> + trust_moklist())
> return add_to_machine_keyring;
> else
> return add_to_platform_keyring;
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 0/4] Add CA enforcement in the machine keyring
2022-03-01 17:36 [PATCH 0/4] Add CA enforcement in the machine keyring Eric Snowberg
` (3 preceding siblings ...)
2022-03-01 17:36 ` [PATCH 4/4] integrity: CA enforcement in machine keyring Eric Snowberg
@ 2022-03-06 23:33 ` Mimi Zohar
2022-03-07 18:55 ` Eric Snowberg
2022-03-09 18:43 ` Mimi Zohar
5 siblings, 1 reply; 30+ messages in thread
From: Mimi Zohar @ 2022-03-06 23:33 UTC (permalink / raw)
To: Eric Snowberg, jarkko, dhowells, dwmw2
Cc: herbert, davem, jmorris, serge, stefanb, nayna, mic, konrad.wilk,
keyrings, linux-kernel, linux-crypto, linux-security-module
Hi Eric,
On Tue, 2022-03-01 at 12:36 -0500, Eric Snowberg wrote:
> A key added to the IMA keyring must be signed by a key contained in either the
> built-in trusted or secondary trusted keyring. IMA also requires these keys
> to be a CA. The only option for an end-user to add their own CA is to compile
> it into the kernel themselves or to use the insert-sys-cert. Many end-users
> do not want to compile their own kernels. With the insert-sys-cert option,
> there are missing upstream changes.
>
> Currently, all Machine Owner Keys (MOK) load into the machine keyring. Add
> a new Kconfig option to only allow CA keys into the machine keyring. When
> compiled with the new INTEGRITY_MACHINE_KEYRING_CA_ENFORCED Kconfig, non CA
> keys will load into the platform keyring instead. This will allow the end-
> user to enroll their own CA key into the machine keyring for use with IMA.
In addition to only loading the MOK CA keys onto the .machine keyring,
the keyUsage should be required and limited to keyCertSign. Certs
with keyUsage of keyCertSign should not be allowed on the IMA keyring.
thanks,
Mimi
>
> These patches are based on Jarkko's linux-tpmdd tree.
> git://git.kernel.org/pub/scm/linux/kernel/git/jarkko/linux-tpmdd.git
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 2/4] X.509: Parse Basic Constraints for CA
2022-03-04 15:10 ` Stefan Berger
@ 2022-03-07 18:02 ` Eric Snowberg
0 siblings, 0 replies; 30+ messages in thread
From: Eric Snowberg @ 2022-03-07 18:02 UTC (permalink / raw)
To: Stefan Berger
Cc: Mimi Zohar, Jarkko Sakkinen, David Howells, dwmw2, herbert,
davem, jmorris, serge, nayna, mic, Konrad Wilk, keyrings,
linux-kernel, linux-crypto, linux-security-module
> On Mar 4, 2022, at 8:10 AM, Stefan Berger <stefanb@linux.ibm.com> wrote:
>
>
> On 3/1/22 12:36, 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 a new public_key field call key_is_ca.
>> This will be used in a follow on patch that requires knowing if the
>> public key is a CA.
>>
>> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
>> ---
>> crypto/asymmetric_keys/x509_cert_parser.c | 9 +++++++++
>> include/crypto/public_key.h | 1 +
>> 2 files changed, 10 insertions(+)
>>
>> diff --git a/crypto/asymmetric_keys/x509_cert_parser.c b/crypto/asymmetric_keys/x509_cert_parser.c
>> index 2899ed80bb18..38c907f4ce27 100644
>> --- a/crypto/asymmetric_keys/x509_cert_parser.c
>> +++ b/crypto/asymmetric_keys/x509_cert_parser.c
>> @@ -583,6 +583,15 @@ int x509_process_extension(void *context, size_t hdrlen,
>> return 0;
>> }
>> + if (ctx->last_oid == OID_basicConstraints) {
>
> Don't you have to check whether you can access v[0] and v[1]?
Good catch, I’ll add the check
> if (vlen < 3)
>
> return -EBADMSG;
I think this would be best
> or should it even be
>
> if (vlen != 3)
>
> return -EBADMSG;
since the length could be larger than 3 if the optional pathLenConstraint
is supplied.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 3/4] KEYS: CA link restriction
2022-03-04 15:28 ` Stefan Berger
@ 2022-03-07 18:06 ` Eric Snowberg
2022-03-07 23:01 ` Mimi Zohar
0 siblings, 1 reply; 30+ messages in thread
From: Eric Snowberg @ 2022-03-07 18:06 UTC (permalink / raw)
To: Stefan Berger
Cc: Mimi Zohar, Jarkko Sakkinen, David Howells, dwmw2, herbert,
davem, jmorris, serge, nayna, mic, Konrad Wilk, keyrings,
linux-kernel, linux-crypto, linux-security-module
> On Mar 4, 2022, at 8:28 AM, Stefan Berger <stefanb@linux.ibm.com> wrote:
>
>
> On 3/1/22 12:36, 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>
>> ---
>> crypto/asymmetric_keys/restrict.c | 43 +++++++++++++++++++++++++++++++
>> include/crypto/public_key.h | 15 +++++++++++
>> 2 files changed, 58 insertions(+)
>>
>> diff --git a/crypto/asymmetric_keys/restrict.c b/crypto/asymmetric_keys/restrict.c
>> index 6b1ac5f5896a..49bb2ea7f609 100644
>> --- a/crypto/asymmetric_keys/restrict.c
>> +++ b/crypto/asymmetric_keys/restrict.c
>> @@ -108,6 +108,49 @@ 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.
>
> CA = root CA here, right?
Yes, I’ll update the comment
>> + *
>> + * 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)
> This function needs to correspond to the key_restrict_link_func_t and therefore has 4 parameter. Call the unused 'trust_keyring' 'unused' instead?
and I’ll change the name in the next round.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 4/4] integrity: CA enforcement in machine keyring
2022-03-04 23:19 ` Stefan Berger
@ 2022-03-07 18:13 ` Eric Snowberg
2022-03-07 18:36 ` Stefan Berger
0 siblings, 1 reply; 30+ messages in thread
From: Eric Snowberg @ 2022-03-07 18:13 UTC (permalink / raw)
To: Stefan Berger
Cc: Mimi Zohar, Jarkko Sakkinen, David Howells, dwmw2, herbert,
davem, jmorris, serge, nayna, mic, Konrad Wilk, keyrings,
linux-kernel, linux-crypto, linux-security-module
> On Mar 4, 2022, at 4:19 PM, Stefan Berger <stefanb@linux.ibm.com> wrote:
>
>
> On 3/1/22 12:36, Eric Snowberg wrote:
>> When INTEGRITY_MACHINE_KEYRING is set, all Machine Owner Keys (MOK)
>> are loaded into the machine keyring. Add a new
>> INTEGRITY_MACHINE_KEYRING_CA_ENFORCED option where only MOK CA keys are
>> added.
>>
>> Set the restriction check to restrict_link_by_ca. This will only allow
>> CA keys into the machine keyring. Unlike when INTEGRITY_MACHINE_KEYRING
>> is enabled, IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY may
>> also be enabled, allowing IMA to use keys in the machine keyring as
>> another trust anchor.
>
> I tried to test this but could only do it by disabling the MokListTrustedRT variable check and then also the check for secure boot. It did load the expected keys onto the .machine keyring, enforcing the x509 indicating a self-signed CA if the compile time option CONFIG_INTEGRITY_MACHINE_KEYRING_CA_ENFORCED=y was set, loading all keys in the case of CONFIG_INTEGRITY_MACHINE_KEYRING=y.
>
> I tried with this branch here from mokutils https://github.com/esnowberg/mokutil/tree/trust-mok but this seems to create an EFI variable with a different name. I guess this is the wrong branch?
Thanks for testing. During the shim review, Peter requested an EFI variable name
change. This did not impact anything in the kernel. However it did impact mokutil.
The necessary mokutil changes are available in this pull request:
https://github.com/lcp/mokutil/pull/49
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 4/4] integrity: CA enforcement in machine keyring
2022-03-07 18:13 ` Eric Snowberg
@ 2022-03-07 18:36 ` Stefan Berger
2022-03-07 18:48 ` Eric Snowberg
0 siblings, 1 reply; 30+ messages in thread
From: Stefan Berger @ 2022-03-07 18:36 UTC (permalink / raw)
To: Eric Snowberg
Cc: Mimi Zohar, Jarkko Sakkinen, David Howells, dwmw2, herbert,
davem, jmorris, serge, nayna, mic, Konrad Wilk, keyrings,
linux-kernel, linux-crypto, linux-security-module
On 3/7/22 13:13, Eric Snowberg wrote:
>
>
>> On Mar 4, 2022, at 4:19 PM, Stefan Berger <stefanb@linux.ibm.com> wrote:
>>
>>
>> On 3/1/22 12:36, Eric Snowberg wrote:
>>> When INTEGRITY_MACHINE_KEYRING is set, all Machine Owner Keys (MOK)
>>> are loaded into the machine keyring. Add a new
>>> INTEGRITY_MACHINE_KEYRING_CA_ENFORCED option where only MOK CA keys are
>>> added.
>>>
>>> Set the restriction check to restrict_link_by_ca. This will only allow
>>> CA keys into the machine keyring. Unlike when INTEGRITY_MACHINE_KEYRING
>>> is enabled, IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY may
>>> also be enabled, allowing IMA to use keys in the machine keyring as
>>> another trust anchor.
>>
>> I tried to test this but could only do it by disabling the MokListTrustedRT variable check and then also the check for secure boot. It did load the expected keys onto the .machine keyring, enforcing the x509 indicating a self-signed CA if the compile time option CONFIG_INTEGRITY_MACHINE_KEYRING_CA_ENFORCED=y was set, loading all keys in the case of CONFIG_INTEGRITY_MACHINE_KEYRING=y.
>>
>> I tried with this branch here from mokutils https://github.com/esnowberg/mokutil/tree/trust-mok but this seems to create an EFI variable with a different name. I guess this is the wrong branch?
>
> Thanks for testing. During the shim review, Peter requested an EFI variable name
> change. This did not impact anything in the kernel. However it did impact mokutil.
> The necessary mokutil changes are available in this pull request:
>
> https://github.com/lcp/mokutil/pull/49
>
The following is in Jarkko's tree:
https://git.kernel.org/pub/scm/linux/kernel/git/jarkko/linux-tpmdd.git/commit/?id=4d83e5144e224b90f6589d11b5fecde33c0dd211
+
+/*
+ * Try to load the MokListTrustedRT MOK variable to see if we should trust
+ * the MOK keys within the kernel. It is not an error if this variable
+ * does not exist. If it does not exist, MOK keys should not be trusted
+ * within the machine keyring.
+ */
+static __init bool uefi_check_trust_mok_keys(void)
+{
+ struct efi_mokvar_table_entry *mokvar_entry;
+
+ mokvar_entry = efi_mokvar_entry_find("MokListTrustedRT");
+
+ if (mokvar_entry)
+ return true;
+
+ return false;
+}
I don't think this works with your mokutil PR:
static int
trust_mok_keys()
{
return set_toggle("MokListTrustedNew", 0);
}
From what I saw, MokListTrustedRT searches for a mok-variable entry in
the MOK-specific directory in sysfs while MokListTrustedNew creates one
in the EFI dir...
Stefan
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 4/4] integrity: CA enforcement in machine keyring
2022-03-07 18:36 ` Stefan Berger
@ 2022-03-07 18:48 ` Eric Snowberg
0 siblings, 0 replies; 30+ messages in thread
From: Eric Snowberg @ 2022-03-07 18:48 UTC (permalink / raw)
To: Stefan Berger
Cc: Mimi Zohar, Jarkko Sakkinen, David Howells, dwmw2, herbert,
davem, jmorris, serge, nayna, mic, Konrad Wilk, keyrings,
linux-kernel, linux-crypto, linux-security-module
> On Mar 7, 2022, at 11:36 AM, Stefan Berger <stefanb@linux.ibm.com> wrote:
>
>
>
> On 3/7/22 13:13, Eric Snowberg wrote:
>>> On Mar 4, 2022, at 4:19 PM, Stefan Berger <stefanb@linux.ibm.com> wrote:
>>>
>>>
>>> On 3/1/22 12:36, Eric Snowberg wrote:
>>>> When INTEGRITY_MACHINE_KEYRING is set, all Machine Owner Keys (MOK)
>>>> are loaded into the machine keyring. Add a new
>>>> INTEGRITY_MACHINE_KEYRING_CA_ENFORCED option where only MOK CA keys are
>>>> added.
>>>>
>>>> Set the restriction check to restrict_link_by_ca. This will only allow
>>>> CA keys into the machine keyring. Unlike when INTEGRITY_MACHINE_KEYRING
>>>> is enabled, IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY may
>>>> also be enabled, allowing IMA to use keys in the machine keyring as
>>>> another trust anchor.
>>>
>>> I tried to test this but could only do it by disabling the MokListTrustedRT variable check and then also the check for secure boot. It did load the expected keys onto the .machine keyring, enforcing the x509 indicating a self-signed CA if the compile time option CONFIG_INTEGRITY_MACHINE_KEYRING_CA_ENFORCED=y was set, loading all keys in the case of CONFIG_INTEGRITY_MACHINE_KEYRING=y.
>>>
>>> I tried with this branch here from mokutils https://github.com/esnowberg/mokutil/tree/trust-mok but this seems to create an EFI variable with a different name. I guess this is the wrong branch?
>> Thanks for testing. During the shim review, Peter requested an EFI variable name
>> change. This did not impact anything in the kernel. However it did impact mokutil.
>> The necessary mokutil changes are available in this pull request:
>> https://github.com/lcp/mokutil/pull/49
>
> The following is in Jarkko's tree:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/jarkko/linux-tpmdd.git/commit/?id=4d83e5144e224b90f6589d11b5fecde33c0dd211
>
>
> +
> +/*
> + * Try to load the MokListTrustedRT MOK variable to see if we should trust
> + * the MOK keys within the kernel. It is not an error if this variable
> + * does not exist. If it does not exist, MOK keys should not be trusted
> + * within the machine keyring.
> + */
> +static __init bool uefi_check_trust_mok_keys(void)
> +{
> + struct efi_mokvar_table_entry *mokvar_entry;
> +
> + mokvar_entry = efi_mokvar_entry_find("MokListTrustedRT");
> +
> + if (mokvar_entry)
> + return true;
> +
> + return false;
> +}
>
> I don't think this works with your mokutil PR:
>
> static int
> trust_mok_keys()
> {
> return set_toggle("MokListTrustedNew", 0);
> }
>
> From what I saw, MokListTrustedRT searches for a mok-variable entry in the MOK-specific directory in sysfs while MokListTrustedNew creates one in the EFI dir…
MokListTrustedNew is set by mokutil. The variable is then used by MokManager.
When shim boots and sees the variable is set, it loads MokManager instead of grub.
The MokManager then asks the user if they want to make the change. If the user
accepts the change, shim stores a new boot services variable and the MokListTrustedNew
variable is deleted. Afterwards the machine is rebooted, shim creates the
MokListTrustedRT based on the boot services variable previously set.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 0/4] Add CA enforcement in the machine keyring
2022-03-06 23:33 ` [PATCH 0/4] Add CA enforcement in the " Mimi Zohar
@ 2022-03-07 18:55 ` Eric Snowberg
0 siblings, 0 replies; 30+ messages in thread
From: Eric Snowberg @ 2022-03-07 18:55 UTC (permalink / raw)
To: Mimi Zohar
Cc: Jarkko Sakkinen, David Howells, dwmw2, herbert, davem, jmorris,
serge, Stefan Berger, nayna, mic, Konrad Wilk, keyrings,
linux-kernel, linux-crypto, linux-security-module
> On Mar 6, 2022, at 4:33 PM, Mimi Zohar <zohar@linux.ibm.com> wrote:
>
> Hi Eric,
>
> On Tue, 2022-03-01 at 12:36 -0500, Eric Snowberg wrote:
>> A key added to the IMA keyring must be signed by a key contained in either the
>> built-in trusted or secondary trusted keyring. IMA also requires these keys
>> to be a CA. The only option for an end-user to add their own CA is to compile
>> it into the kernel themselves or to use the insert-sys-cert. Many end-users
>> do not want to compile their own kernels. With the insert-sys-cert option,
>> there are missing upstream changes.
>>
>> Currently, all Machine Owner Keys (MOK) load into the machine keyring. Add
>> a new Kconfig option to only allow CA keys into the machine keyring. When
>> compiled with the new INTEGRITY_MACHINE_KEYRING_CA_ENFORCED Kconfig, non CA
>> keys will load into the platform keyring instead. This will allow the end-
>> user to enroll their own CA key into the machine keyring for use with IMA.
>
> In addition to only loading the MOK CA keys onto the .machine keyring,
> the keyUsage should be required and limited to keyCertSign.
Ok, I’ll add this in the next round.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 3/4] KEYS: CA link restriction
2022-03-07 18:06 ` Eric Snowberg
@ 2022-03-07 23:01 ` Mimi Zohar
2022-03-07 23:38 ` Eric Snowberg
0 siblings, 1 reply; 30+ messages in thread
From: Mimi Zohar @ 2022-03-07 23:01 UTC (permalink / raw)
To: Eric Snowberg, Stefan Berger
Cc: Jarkko Sakkinen, David Howells, dwmw2, herbert, davem, jmorris,
serge, nayna, mic, Konrad Wilk, keyrings, linux-kernel,
linux-crypto, linux-security-module
On Mon, 2022-03-07 at 18:06 +0000, Eric Snowberg wrote:
>
> >> diff --git a/crypto/asymmetric_keys/restrict.c b/crypto/asymmetric_keys/restrict.c
> >> index 6b1ac5f5896a..49bb2ea7f609 100644
> >> --- a/crypto/asymmetric_keys/restrict.c
> >> +++ b/crypto/asymmetric_keys/restrict.c
> >> @@ -108,6 +108,49 @@ 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.
> >
> > CA = root CA here, right?
>
> Yes, I’ll update the comment
Updating the comment is not enough. There's an existing function named
"x509_check_for_self_signed()" which determines whether the certificate
is self-signed.
thanks,
Mimi
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 3/4] KEYS: CA link restriction
2022-03-07 23:01 ` Mimi Zohar
@ 2022-03-07 23:38 ` Eric Snowberg
2022-03-08 2:31 ` Stefan Berger
0 siblings, 1 reply; 30+ messages in thread
From: Eric Snowberg @ 2022-03-07 23:38 UTC (permalink / raw)
To: Mimi Zohar
Cc: Stefan Berger, Jarkko Sakkinen, David Howells, dwmw2, herbert,
davem, jmorris, serge, nayna, mic, Konrad Wilk, keyrings,
linux-kernel, linux-crypto, linux-security-module
> On Mar 7, 2022, at 4:01 PM, Mimi Zohar <zohar@linux.ibm.com> wrote:
>
> On Mon, 2022-03-07 at 18:06 +0000, Eric Snowberg wrote:
>>
>>>> diff --git a/crypto/asymmetric_keys/restrict.c b/crypto/asymmetric_keys/restrict.c
>>>> index 6b1ac5f5896a..49bb2ea7f609 100644
>>>> --- a/crypto/asymmetric_keys/restrict.c
>>>> +++ b/crypto/asymmetric_keys/restrict.c
>>>> @@ -108,6 +108,49 @@ 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.
>>>
>>> CA = root CA here, right?
>>
>> Yes, I’ll update the comment
>
> Updating the comment is not enough. There's an existing function named
> "x509_check_for_self_signed()" which determines whether the certificate
> is self-signed.
Originally I tried using that function. However when the restrict link code is called,
all the necessary x509 information is no longer available. The code in
restrict_link_by_ca is basically doing the equivalent to x509_check_for_self_signed.
After verifying the cert has the CA flag set, the call to public_key_verify_signature
validates the cert is self signed.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 3/4] KEYS: CA link restriction
2022-03-07 23:38 ` Eric Snowberg
@ 2022-03-08 2:31 ` Stefan Berger
2022-03-08 12:45 ` Mimi Zohar
0 siblings, 1 reply; 30+ messages in thread
From: Stefan Berger @ 2022-03-08 2:31 UTC (permalink / raw)
To: Eric Snowberg, Mimi Zohar
Cc: Jarkko Sakkinen, David Howells, dwmw2, herbert, davem, jmorris,
serge, nayna, mic, Konrad Wilk, keyrings, linux-kernel,
linux-crypto, linux-security-module
On 3/7/22 18:38, Eric Snowberg wrote:
>
>
>> On Mar 7, 2022, at 4:01 PM, Mimi Zohar <zohar@linux.ibm.com> wrote:
>>
>> On Mon, 2022-03-07 at 18:06 +0000, Eric Snowberg wrote:
>>>
>>>>> diff --git a/crypto/asymmetric_keys/restrict.c b/crypto/asymmetric_keys/restrict.c
>>>>> index 6b1ac5f5896a..49bb2ea7f609 100644
>>>>> --- a/crypto/asymmetric_keys/restrict.c
>>>>> +++ b/crypto/asymmetric_keys/restrict.c
>>>>> @@ -108,6 +108,49 @@ 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.
>>>>
>>>> CA = root CA here, right?
>>>
>>> Yes, I’ll update the comment
>>
>> Updating the comment is not enough. There's an existing function named
>> "x509_check_for_self_signed()" which determines whether the certificate
>> is self-signed.
>
> Originally I tried using that function. However when the restrict link code is called,
> all the necessary x509 information is no longer available. The code in
> restrict_link_by_ca is basically doing the equivalent to x509_check_for_self_signed.
> After verifying the cert has the CA flag set, the call to public_key_verify_signature
> validates the cert is self signed.
>
Isn't x509_cert_parse() being called as part of parsing the certificate?
If so, it seems to check for a self-signed certificate every time. You
could add something like the following to x509_check_for_self_signed(cert):
pub->x509_self_signed = cert->self_signed = true;
This could then reduce the function in 3/4 to something like:
return payload->data[asym_crypto]->x509_self_signed;
Stefan
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 3/4] KEYS: CA link restriction
2022-03-08 2:31 ` Stefan Berger
@ 2022-03-08 12:45 ` Mimi Zohar
2022-03-08 13:56 ` Stefan Berger
2022-03-08 18:02 ` Eric Snowberg
0 siblings, 2 replies; 30+ messages in thread
From: Mimi Zohar @ 2022-03-08 12:45 UTC (permalink / raw)
To: Stefan Berger, Eric Snowberg
Cc: Jarkko Sakkinen, David Howells, dwmw2, herbert, davem, jmorris,
serge, nayna, mic, Konrad Wilk, keyrings, linux-kernel,
linux-crypto, linux-security-module
On Mon, 2022-03-07 at 21:31 -0500, Stefan Berger wrote:
>
> On 3/7/22 18:38, Eric Snowberg wrote:
> >
> >
> >> On Mar 7, 2022, at 4:01 PM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> >>
> >> On Mon, 2022-03-07 at 18:06 +0000, Eric Snowberg wrote:
> >>>
> >>>>> diff --git a/crypto/asymmetric_keys/restrict.c b/crypto/asymmetric_keys/restrict.c
> >>>>> index 6b1ac5f5896a..49bb2ea7f609 100644
> >>>>> --- a/crypto/asymmetric_keys/restrict.c
> >>>>> +++ b/crypto/asymmetric_keys/restrict.c
> >>>>> @@ -108,6 +108,49 @@ 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.
> >>>>
> >>>> CA = root CA here, right?
> >>>
> >>> Yes, I’ll update the comment
> >>
> >> Updating the comment is not enough. There's an existing function named
> >> "x509_check_for_self_signed()" which determines whether the certificate
> >> is self-signed.
> >
> > Originally I tried using that function. However when the restrict link code is called,
> > all the necessary x509 information is no longer available. The code in
> > restrict_link_by_ca is basically doing the equivalent to x509_check_for_self_signed.
> > After verifying the cert has the CA flag set, the call to public_key_verify_signature
> > validates the cert is self signed.
> >
> Isn't x509_cert_parse() being called as part of parsing the certificate?
> If so, it seems to check for a self-signed certificate every time. You
> could add something like the following to x509_check_for_self_signed(cert):
> pub->x509_self_signed = cert->self_signed = true;
>
> This could then reduce the function in 3/4 to something like:
>
> return payload->data[asym_crypto]->x509_self_signed;
Agreed, as long as the other two criteria are also met: CA and keyUsage
should be required and limited to keyCertSign.
thanks,
Mimi
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 3/4] KEYS: CA link restriction
2022-03-08 12:45 ` Mimi Zohar
@ 2022-03-08 13:56 ` Stefan Berger
2022-03-08 18:02 ` Eric Snowberg
1 sibling, 0 replies; 30+ messages in thread
From: Stefan Berger @ 2022-03-08 13:56 UTC (permalink / raw)
To: Mimi Zohar, Eric Snowberg
Cc: Jarkko Sakkinen, David Howells, dwmw2, herbert, davem, jmorris,
serge, nayna, mic, Konrad Wilk, keyrings, linux-kernel,
linux-crypto, linux-security-module
On 3/8/22 07:45, Mimi Zohar wrote:
> On Mon, 2022-03-07 at 21:31 -0500, Stefan Berger wrote:
>>
>> On 3/7/22 18:38, Eric Snowberg wrote:
>>>
>>>
>>>> On Mar 7, 2022, at 4:01 PM, Mimi Zohar <zohar@linux.ibm.com> wrote:
>>>>
>>>> On Mon, 2022-03-07 at 18:06 +0000, Eric Snowberg wrote:
>>>>>
>>>>>>> diff --git a/crypto/asymmetric_keys/restrict.c b/crypto/asymmetric_keys/restrict.c
>>>>>>> index 6b1ac5f5896a..49bb2ea7f609 100644
>>>>>>> --- a/crypto/asymmetric_keys/restrict.c
>>>>>>> +++ b/crypto/asymmetric_keys/restrict.c
>>>>>>> @@ -108,6 +108,49 @@ 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.
>>>>>>
>>>>>> CA = root CA here, right?
>>>>>
>>>>> Yes, I’ll update the comment
>>>>
>>>> Updating the comment is not enough. There's an existing function named
>>>> "x509_check_for_self_signed()" which determines whether the certificate
>>>> is self-signed.
>>>
>>> Originally I tried using that function. However when the restrict link code is called,
>>> all the necessary x509 information is no longer available. The code in
>>> restrict_link_by_ca is basically doing the equivalent to x509_check_for_self_signed.
>>> After verifying the cert has the CA flag set, the call to public_key_verify_signature
>>> validates the cert is self signed.
>>>
>> Isn't x509_cert_parse() being called as part of parsing the certificate?
>> If so, it seems to check for a self-signed certificate every time. You
>> could add something like the following to x509_check_for_self_signed(cert):
>> pub->x509_self_signed = cert->self_signed = true;
>>
>> This could then reduce the function in 3/4 to something like:
>>
>> return payload->data[asym_crypto]->x509_self_signed;
>
> Agreed, as long as the other two criteria are also met: CA and keyUsage
> should be required and limited to keyCertSign.
right, it's not as easy as the return statement above...
>
> thanks,
>
> Mimi
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 3/4] KEYS: CA link restriction
2022-03-08 12:45 ` Mimi Zohar
2022-03-08 13:56 ` Stefan Berger
@ 2022-03-08 18:02 ` Eric Snowberg
2022-03-09 17:12 ` Stefan Berger
2022-03-09 17:33 ` Mimi Zohar
1 sibling, 2 replies; 30+ messages in thread
From: Eric Snowberg @ 2022-03-08 18:02 UTC (permalink / raw)
To: Mimi Zohar, Stefan Berger
Cc: Jarkko Sakkinen, David Howells, dwmw2, herbert, davem, jmorris,
serge, nayna, mic, Konrad Wilk, keyrings, linux-kernel,
linux-crypto, linux-security-module
> On Mar 8, 2022, at 5:45 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
>
> On Mon, 2022-03-07 at 21:31 -0500, Stefan Berger wrote:
>>
>> On 3/7/22 18:38, Eric Snowberg wrote:
>>>
>>>
>>>> On Mar 7, 2022, at 4:01 PM, Mimi Zohar <zohar@linux.ibm.com> wrote:
>>>>
>>>> On Mon, 2022-03-07 at 18:06 +0000, Eric Snowberg wrote:
>>>>>
>>>>>>> diff --git a/crypto/asymmetric_keys/restrict.c b/crypto/asymmetric_keys/restrict.c
>>>>>>> index 6b1ac5f5896a..49bb2ea7f609 100644
>>>>>>> --- a/crypto/asymmetric_keys/restrict.c
>>>>>>> +++ b/crypto/asymmetric_keys/restrict.c
>>>>>>> @@ -108,6 +108,49 @@ 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.
>>>>>>
>>>>>> CA = root CA here, right?
>>>>>
>>>>> Yes, I’ll update the comment
>>>>
>>>> Updating the comment is not enough. There's an existing function named
>>>> "x509_check_for_self_signed()" which determines whether the certificate
>>>> is self-signed.
>>>
>>> Originally I tried using that function. However when the restrict link code is called,
>>> all the necessary x509 information is no longer available. The code in
>>> restrict_link_by_ca is basically doing the equivalent to x509_check_for_self_signed.
>>> After verifying the cert has the CA flag set, the call to public_key_verify_signature
>>> validates the cert is self signed.
>>>
>> Isn't x509_cert_parse() being called as part of parsing the certificate?
>> If so, it seems to check for a self-signed certificate every time. You
>> could add something like the following to x509_check_for_self_signed(cert):
>> pub->x509_self_signed = cert->self_signed = true;
>>
>> This could then reduce the function in 3/4 to something like:
>>
>> return payload->data[asym_crypto]->x509_self_signed;
When I was studying the restriction code, before writing this patch, it looked like
it was written from the standpoint to be as generic as possible. All code contained
within it works on either a public_key_signature or a public_key. I had assumed it
was written this way to be used with different asymmetrical key types now and in
the future. I called the public_key_verify_signature function instead of interrogating
the x509 payload to keep in line with what I thought was the original design. Let me
know if I should be carrying x509 code in here to make the change above.
> Agreed, as long as the other two criteria are also met: CA and keyUsage
> should be required and limited to keyCertSign.
I have added the key_is_ca in the public_key header. I can look at adding the usage
too. Before doing this I would like to understand the "limited to" above. Many CA keys
that have keyCertSign set, also have digitalSignature set for key usage. For
example:
http://cacerts.digicert.com/DigiCertEVCodeSigningCA-SHA2.crt
Are you saying we would want to exclude a CA like the one above, since it as the
digitalSignature usage set too?
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 3/4] KEYS: CA link restriction
2022-03-08 18:02 ` Eric Snowberg
@ 2022-03-09 17:12 ` Stefan Berger
2022-03-09 17:17 ` Stefan Berger
2022-03-09 18:13 ` Eric Snowberg
2022-03-09 17:33 ` Mimi Zohar
1 sibling, 2 replies; 30+ messages in thread
From: Stefan Berger @ 2022-03-09 17:12 UTC (permalink / raw)
To: Eric Snowberg, Mimi Zohar
Cc: Jarkko Sakkinen, David Howells, dwmw2, herbert, davem, jmorris,
serge, nayna, mic, Konrad Wilk, keyrings, linux-kernel,
linux-crypto, linux-security-module
On 3/8/22 13:02, Eric Snowberg wrote:
>
>
>> On Mar 8, 2022, at 5:45 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
>>
>> On Mon, 2022-03-07 at 21:31 -0500, Stefan Berger wrote:
>>>
>>> On 3/7/22 18:38, Eric Snowberg wrote:
>>>>
>>>>
>>>>> On Mar 7, 2022, at 4:01 PM, Mimi Zohar <zohar@linux.ibm.com> wrote:
>>>>>
>>>>> On Mon, 2022-03-07 at 18:06 +0000, Eric Snowberg wrote:
>>>>>>
>>>>>>>> diff --git a/crypto/asymmetric_keys/restrict.c b/crypto/asymmetric_keys/restrict.c
>>>>>>>> index 6b1ac5f5896a..49bb2ea7f609 100644
>>>>>>>> --- a/crypto/asymmetric_keys/restrict.c
>>>>>>>> +++ b/crypto/asymmetric_keys/restrict.c
>>>>>>>> @@ -108,6 +108,49 @@ 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.
>>>>>>>
>>>>>>> CA = root CA here, right?
>>>>>>
>>>>>> Yes, I’ll update the comment
>>>>>
>>>>> Updating the comment is not enough. There's an existing function named
>>>>> "x509_check_for_self_signed()" which determines whether the certificate
>>>>> is self-signed.
>>>>
>>>> Originally I tried using that function. However when the restrict link code is called,
>>>> all the necessary x509 information is no longer available. The code in
>>>> restrict_link_by_ca is basically doing the equivalent to x509_check_for_self_signed.
>>>> After verifying the cert has the CA flag set, the call to public_key_verify_signature
>>>> validates the cert is self signed.
>>>>
>>> Isn't x509_cert_parse() being called as part of parsing the certificate?
>>> If so, it seems to check for a self-signed certificate every time. You
>>> could add something like the following to x509_check_for_self_signed(cert):
>>> pub->x509_self_signed = cert->self_signed = true;
>>>
>>> This could then reduce the function in 3/4 to something like:
>>>
>>> return payload->data[asym_crypto]->x509_self_signed;
>
> When I was studying the restriction code, before writing this patch, it looked like
> it was written from the standpoint to be as generic as possible. All code contained
> within it works on either a public_key_signature or a public_key. I had assumed it
> was written this way to be used with different asymmetrical key types now and in
> the future. I called the public_key_verify_signature function instead of interrogating
> the x509 payload to keep in line with what I thought was the original design. Let me
> know if I should be carrying x509 code in here to make the change above.
It does not seem right if there were two functions trying to determine
whether an x509 cert is self-signed. The existing is invoked as part of
loading a key onto the machine keyring from what I can see. It has
access to more data about the cert and therefore can do stronger tests,
yours doesn't have access to the data. So I guess I would remember in a
boolean in the public key structure that the x509 cert it comes from was
self signed following the existing test. Key in your function may be
that that payload->data[] array is guaranteed to be from the x509 cert
as set in x509_key_preparse().
https://elixir.bootlin.com/linux/v5.17-rc7/source/crypto/asymmetric_keys/x509_public_key.c#L236
StefanIt does not seem right if there were two functions trying to
determine whether an x509 cert is self-signed. The existing is invoked
as part of loading a key onto the machine keyring from what I can see.
It has access to more data about the cert and therefore can do stronger
tests, yours doesn't have access to the data. So I guess I would
remember in a boolean in the public key structure that the x509 cert it
comes from was self signed following the existing test. Key in your
function may be that that payload->data[] array is guaranteed to be from
the x509 cert as set in x509_key_preparse().
https://elixir.bootlin.com/linux/v5.17-rc7/source/crypto/asymmetric_keys/x509_public_key.c#L236
Stefan
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 3/4] KEYS: CA link restriction
2022-03-09 17:12 ` Stefan Berger
@ 2022-03-09 17:17 ` Stefan Berger
2022-03-09 18:13 ` Eric Snowberg
1 sibling, 0 replies; 30+ messages in thread
From: Stefan Berger @ 2022-03-09 17:17 UTC (permalink / raw)
To: Eric Snowberg, Mimi Zohar
Cc: Jarkko Sakkinen, David Howells, dwmw2, herbert, davem, jmorris,
serge, nayna, mic, Konrad Wilk, keyrings, linux-kernel,
linux-crypto, linux-security-module
On 3/9/22 12:12, Stefan Berger wrote:
>
>
> On 3/8/22 13:02, Eric Snowberg wrote:
>>
>>
>>> On Mar 8, 2022, at 5:45 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
>>>
>>> On Mon, 2022-03-07 at 21:31 -0500, Stefan Berger wrote:
>>>>
>>>> On 3/7/22 18:38, Eric Snowberg wrote:
>>>>>
>>>>>
>>>>>> On Mar 7, 2022, at 4:01 PM, Mimi Zohar <zohar@linux.ibm.com> wrote:
>>>>>>
>>>>>> On Mon, 2022-03-07 at 18:06 +0000, Eric Snowberg wrote:
>>>>>>>
>>>>>>>>> diff --git a/crypto/asymmetric_keys/restrict.c
>>>>>>>>> b/crypto/asymmetric_keys/restrict.c
>>>>>>>>> index 6b1ac5f5896a..49bb2ea7f609 100644
>>>>>>>>> --- a/crypto/asymmetric_keys/restrict.c
>>>>>>>>> +++ b/crypto/asymmetric_keys/restrict.c
>>>>>>>>> @@ -108,6 +108,49 @@ 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.
>>>>>>>>
>>>>>>>> CA = root CA here, right?
>>>>>>>
>>>>>>> Yes, I’ll update the comment
>>>>>>
>>>>>> Updating the comment is not enough. There's an existing function
>>>>>> named
>>>>>> "x509_check_for_self_signed()" which determines whether the
>>>>>> certificate
>>>>>> is self-signed.
>>>>>
>>>>> Originally I tried using that function. However when the restrict
>>>>> link code is called,
>>>>> all the necessary x509 information is no longer available. The
>>>>> code in
>>>>> restrict_link_by_ca is basically doing the equivalent to
>>>>> x509_check_for_self_signed.
>>>>> After verifying the cert has the CA flag set, the call to
>>>>> public_key_verify_signature
>>>>> validates the cert is self signed.
>>>>>
>>>> Isn't x509_cert_parse() being called as part of parsing the
>>>> certificate?
>>>> If so, it seems to check for a self-signed certificate every time. You
>>>> could add something like the following to
>>>> x509_check_for_self_signed(cert):
>>>> pub->x509_self_signed = cert->self_signed = true;
>>>>
>>>> This could then reduce the function in 3/4 to something like:
>>>>
>>>> return payload->data[asym_crypto]->x509_self_signed;
>>
>> When I was studying the restriction code, before writing this patch,
>> it looked like
>> it was written from the standpoint to be as generic as possible. All
>> code contained
>> within it works on either a public_key_signature or a public_key. I
>> had assumed it
>> was written this way to be used with different asymmetrical key types
>> now and in
>> the future. I called the public_key_verify_signature function instead
>> of interrogating
>> the x509 payload to keep in line with what I thought was the original
>> design. Let me
>> know if I should be carrying x509 code in here to make the change above.
>
> It does not seem right if there were two functions trying to determine
> whether an x509 cert is self-signed. The existing is invoked as part of
> loading a key onto the machine keyring from what I can see. It has
> access to more data about the cert and therefore can do stronger tests,
> yours doesn't have access to the data. So I guess I would remember in a
> boolean in the public key structure that the x509 cert it comes from was
> self signed following the existing test. Key in your function may be
> that that payload->data[] array is guaranteed to be from the x509 cert
> as set in x509_key_preparse().
>
> https://elixir.bootlin.com/linux/v5.17-rc7/source/crypto/asymmetric_keys/x509_public_key.c#L236
>
>
> Stefan
Sorry for the mess in the response. The first version is the good one :-)
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 3/4] KEYS: CA link restriction
2022-03-08 18:02 ` Eric Snowberg
2022-03-09 17:12 ` Stefan Berger
@ 2022-03-09 17:33 ` Mimi Zohar
1 sibling, 0 replies; 30+ messages in thread
From: Mimi Zohar @ 2022-03-09 17:33 UTC (permalink / raw)
To: Eric Snowberg, Stefan Berger
Cc: Jarkko Sakkinen, David Howells, dwmw2, herbert, davem, jmorris,
serge, nayna, mic, Konrad Wilk, keyrings, linux-kernel,
linux-crypto, linux-security-module
On Tue, 2022-03-08 at 18:02 +0000, Eric Snowberg wrote:
> > On Mar 8, 2022, at 5:45 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> > Agreed, as long as the other two criteria are also met: CA and keyUsage
> > should be required and limited to keyCertSign.
>
> I have added the key_is_ca in the public_key header. I can look at adding the usage
> too. Before doing this I would like to understand the "limited to" above. Many CA keys
> that have keyCertSign set, also have digitalSignature set for key usage. For
> example:
>
> http://cacerts.digicert.com/DigiCertEVCodeSigningCA-SHA2.crt
>
> Are you saying we would want to exclude a CA like the one above, since it as the
> digitalSignature usage set too?
Yes, the "machine" keyring is defining a new root of trust to support
allowing end-users the ability "to add their own keys and sign modules
they trust". There should be a clear distinction between keys used
for certificate signing from those used for code signing. Certificate
signing keys should be added to the .machine keyring. Code signing
keys should be added to the IMA keyring.
thanks,
Mimi
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 3/4] KEYS: CA link restriction
2022-03-09 17:12 ` Stefan Berger
2022-03-09 17:17 ` Stefan Berger
@ 2022-03-09 18:13 ` Eric Snowberg
2022-03-09 19:02 ` Stefan Berger
1 sibling, 1 reply; 30+ messages in thread
From: Eric Snowberg @ 2022-03-09 18:13 UTC (permalink / raw)
To: Stefan Berger
Cc: Mimi Zohar, Jarkko Sakkinen, David Howells, dwmw2, herbert,
davem, jmorris, serge, nayna, mic, Konrad Wilk, keyrings,
linux-kernel, linux-crypto, linux-security-module
> On Mar 9, 2022, at 10:12 AM, Stefan Berger <stefanb@linux.ibm.com> wrote:
>
>
>
> On 3/8/22 13:02, Eric Snowberg wrote:
>>> On Mar 8, 2022, at 5:45 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
>>>
>>> On Mon, 2022-03-07 at 21:31 -0500, Stefan Berger wrote:
>>>>
>>>> On 3/7/22 18:38, Eric Snowberg wrote:
>>>>>
>>>>>
>>>>>> On Mar 7, 2022, at 4:01 PM, Mimi Zohar <zohar@linux.ibm.com> wrote:
>>>>>>
>>>>>> On Mon, 2022-03-07 at 18:06 +0000, Eric Snowberg wrote:
>>>>>>>
>>>>>>>>> diff --git a/crypto/asymmetric_keys/restrict.c b/crypto/asymmetric_keys/restrict.c
>>>>>>>>> index 6b1ac5f5896a..49bb2ea7f609 100644
>>>>>>>>> --- a/crypto/asymmetric_keys/restrict.c
>>>>>>>>> +++ b/crypto/asymmetric_keys/restrict.c
>>>>>>>>> @@ -108,6 +108,49 @@ 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.
>>>>>>>>
>>>>>>>> CA = root CA here, right?
>>>>>>>
>>>>>>> Yes, I’ll update the comment
>>>>>>
>>>>>> Updating the comment is not enough. There's an existing function named
>>>>>> "x509_check_for_self_signed()" which determines whether the certificate
>>>>>> is self-signed.
>>>>>
>>>>> Originally I tried using that function. However when the restrict link code is called,
>>>>> all the necessary x509 information is no longer available. The code in
>>>>> restrict_link_by_ca is basically doing the equivalent to x509_check_for_self_signed.
>>>>> After verifying the cert has the CA flag set, the call to public_key_verify_signature
>>>>> validates the cert is self signed.
>>>>>
>>>> Isn't x509_cert_parse() being called as part of parsing the certificate?
>>>> If so, it seems to check for a self-signed certificate every time. You
>>>> could add something like the following to x509_check_for_self_signed(cert):
>>>> pub->x509_self_signed = cert->self_signed = true;
>>>>
>>>> This could then reduce the function in 3/4 to something like:
>>>>
>>>> return payload->data[asym_crypto]->x509_self_signed;
>> When I was studying the restriction code, before writing this patch, it looked like
>> it was written from the standpoint to be as generic as possible. All code contained
>> within it works on either a public_key_signature or a public_key. I had assumed it
>> was written this way to be used with different asymmetrical key types now and in
>> the future. I called the public_key_verify_signature function instead of interrogating
>> the x509 payload to keep in line with what I thought was the original design. Let me
>> know if I should be carrying x509 code in here to make the change above.
>
> It does not seem right if there were two functions trying to determine whether an x509 cert is self-signed. The existing is invoked as part of loading a key onto the machine keyring from what I can see. It has access to more data about the cert and therefore can do stronger tests, yours doesn't have access to the data. So I guess I would remember in a boolean in the public key structure that the x509 cert it comes from was self signed following the existing test. Key in your function may be that that payload->data[] array is guaranteed to be from the x509 cert as set in x509_key_preparse().
>
> https://elixir.bootlin.com/linux/v5.17-rc7/source/crypto/asymmetric_keys/x509_public_key.c#L236
I could add another bool to the public key structure to designate if the key was self signed,
but this seems to go against what the kernel document states. "Asymmetric / Public-key
Cryptography Key Type” [1] states:
"The “asymmetric” key type is designed to be a container for the keys used in public-key
cryptography, without imposing any particular restrictions on the form or mechanism of
the cryptography or form of the key.
The asymmetric key is given a subtype that defines what sort of data is associated with
the key and provides operations to describe and destroy it. However, no requirement is
made that the key data actually be stored in the key."
Now every public key type would need to fill in the information on whether the key is self
signed or not. Instead of going through the public_key_verify_signature function currently
used in this patch.
https://docs.kernel.org/crypto/asymmetric-keys.html
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 0/4] Add CA enforcement in the machine keyring
2022-03-01 17:36 [PATCH 0/4] Add CA enforcement in the machine keyring Eric Snowberg
` (4 preceding siblings ...)
2022-03-06 23:33 ` [PATCH 0/4] Add CA enforcement in the " Mimi Zohar
@ 2022-03-09 18:43 ` Mimi Zohar
5 siblings, 0 replies; 30+ messages in thread
From: Mimi Zohar @ 2022-03-09 18:43 UTC (permalink / raw)
To: Eric Snowberg, jarkko, dhowells, dwmw2
Cc: herbert, davem, jmorris, serge, stefanb, nayna, mic, konrad.wilk,
keyrings, linux-kernel, linux-crypto, linux-security-module
On Tue, 2022-03-01 at 12:36 -0500, Eric Snowberg wrote:
I would begin by saying,
The "Enroll kernel keys thru MOK" patch set introduced a new root of
trust by defining a "machine" keyring, which is linked to the
secondary_trusted_keyring. All Machine Owner Keys (MOK) are loaded
into the machine keyring.
Then proceed with the IMA new root of trust requirements - root CA
(self-signed CA) with keyUsage limited to keyCertSign.
> A key added to the IMA keyring must be signed by a key contained in either the
^A certificate ... must be signed
> built-in trusted or secondary trusted keyring. IMA also requires these keys
> to be a CA. The only option for an end-user to add their own CA is to compile
> it into the kernel themselves or to use the insert-sys-cert. Many end-users
> do not want to compile their own kernels. With the insert-sys-cert option,
> there are missing upstream changes.
>
> Currently, all Machine Owner Keys (MOK) load into the machine keyring.
Moved to the beginning.
> Add
^Define
>
> a new Kconfig option to only allow CA keys into the machine keyring. When
Add the other criteria here as well.
> compiled with the new INTEGRITY_MACHINE_KEYRING_CA_ENFORCED Kconfig, non CA
> keys will load into the platform keyring instead. This will allow the end-
> user to enroll their own CA key into the machine keyring for use with IMA.
>
> These patches are based on Jarkko's linux-tpmdd tree.
> git://git.kernel.org/pub/scm/linux/kernel/git/jarkko/linux-tpmdd.git
thanks,
Mimi
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 3/4] KEYS: CA link restriction
2022-03-09 18:13 ` Eric Snowberg
@ 2022-03-09 19:02 ` Stefan Berger
2022-03-11 18:44 ` Eric Snowberg
0 siblings, 1 reply; 30+ messages in thread
From: Stefan Berger @ 2022-03-09 19:02 UTC (permalink / raw)
To: Eric Snowberg
Cc: Mimi Zohar, Jarkko Sakkinen, David Howells, dwmw2, herbert,
davem, jmorris, serge, nayna, mic, Konrad Wilk, keyrings,
linux-kernel, linux-crypto, linux-security-module
On 3/9/22 13:13, Eric Snowberg wrote:
>
>
>> On Mar 9, 2022, at 10:12 AM, Stefan Berger <stefanb@linux.ibm.com> wrote:
>>
>>
>>
>> On 3/8/22 13:02, Eric Snowberg wrote:
>>>> On Mar 8, 2022, at 5:45 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
>>>>
>>>> On Mon, 2022-03-07 at 21:31 -0500, Stefan Berger wrote:
>>>>>
>>>>> On 3/7/22 18:38, Eric Snowberg wrote:
>>>>>>
>>>>>>
>>>>>>> On Mar 7, 2022, at 4:01 PM, Mimi Zohar <zohar@linux.ibm.com> wrote:
>>>>>>>
>>>>>>> On Mon, 2022-03-07 at 18:06 +0000, Eric Snowberg wrote:
>>>>>>>>
>>>>>>>>>> diff --git a/crypto/asymmetric_keys/restrict.c b/crypto/asymmetric_keys/restrict.c
>>>>>>>>>> index 6b1ac5f5896a..49bb2ea7f609 100644
>>>>>>>>>> --- a/crypto/asymmetric_keys/restrict.c
>>>>>>>>>> +++ b/crypto/asymmetric_keys/restrict.c
>>>>>>>>>> @@ -108,6 +108,49 @@ 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.
>>>>>>>>>
>>>>>>>>> CA = root CA here, right?
>>>>>>>>
>>>>>>>> Yes, I’ll update the comment
>>>>>>>
>>>>>>> Updating the comment is not enough. There's an existing function named
>>>>>>> "x509_check_for_self_signed()" which determines whether the certificate
>>>>>>> is self-signed.
>>>>>>
>>>>>> Originally I tried using that function. However when the restrict link code is called,
>>>>>> all the necessary x509 information is no longer available. The code in
>>>>>> restrict_link_by_ca is basically doing the equivalent to x509_check_for_self_signed.
>>>>>> After verifying the cert has the CA flag set, the call to public_key_verify_signature
>>>>>> validates the cert is self signed.
>>>>>>
>>>>> Isn't x509_cert_parse() being called as part of parsing the certificate?
>>>>> If so, it seems to check for a self-signed certificate every time. You
>>>>> could add something like the following to x509_check_for_self_signed(cert):
>>>>> pub->x509_self_signed = cert->self_signed = true;
>>>>>
>>>>> This could then reduce the function in 3/4 to something like:
>>>>>
>>>>> return payload->data[asym_crypto]->x509_self_signed;
>>> When I was studying the restriction code, before writing this patch, it looked like
>>> it was written from the standpoint to be as generic as possible. All code contained
>>> within it works on either a public_key_signature or a public_key. I had assumed it
>>> was written this way to be used with different asymmetrical key types now and in
>>> the future. I called the public_key_verify_signature function instead of interrogating
>>> the x509 payload to keep in line with what I thought was the original design. Let me
>>> know if I should be carrying x509 code in here to make the change above.
>>
>> It does not seem right if there were two functions trying to determine whether an x509 cert is self-signed. The existing is invoked as part of loading a key onto the machine keyring from what I can see. It has access to more data about the cert and therefore can do stronger tests, yours doesn't have access to the data. So I guess I would remember in a boolean in the public key structure that the x509 cert it comes from was self signed following the existing test. Key in your function may be that that payload->data[] array is guaranteed to be from the x509 cert as set in x509_key_preparse().
>>
>> https://elixir.bootlin.com/linux/v5.17-rc7/source/crypto/asymmetric_keys/x509_public_key.c#L236
>
> I could add another bool to the public key structure to designate if the key was self signed,
> but this seems to go against what the kernel document states. "Asymmetric / Public-key
> Cryptography Key Type” [1] states:
>
> "The “asymmetric” key type is designed to be a container for the keys used in public-key
> cryptography, without imposing any particular restrictions on the form or mechanism of
> the cryptography or form of the key.
>
> The asymmetric key is given a subtype that defines what sort of data is associated with
> the key and provides operations to describe and destroy it. However, no requirement is
> made that the key data actually be stored in the key."
>
> Now every public key type would need to fill in the information on whether the key is self
> signed or not. Instead of going through the public_key_verify_signature function currently
> used in this patch.
Every public key extracted from a x509 certificate would have to set
this field to true if the public key originates from a self-signed x509
cert. Is this different from this code here where now every public key
would have to set the key_is_ca field?
+ if (v[1] != 0 && v[2] == ASN1_BOOL && v[3] == 1)
+ ctx->cert->pub->key_is_ca = true;
The extension I would have suggested looked similar:
cert->pub->x509_self_sign = cert->self_signed = true
[ to be put here:
https://elixir.bootlin.com/linux/v5.17-rc7/source/crypto/asymmetric_keys/x509_public_key.c#L147
]
>
> https://docs.kernel.org/crypto/asymmetric-keys.html
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 3/4] KEYS: CA link restriction
2022-03-09 19:02 ` Stefan Berger
@ 2022-03-11 18:44 ` Eric Snowberg
2022-03-11 20:23 ` Stefan Berger
0 siblings, 1 reply; 30+ messages in thread
From: Eric Snowberg @ 2022-03-11 18:44 UTC (permalink / raw)
To: Stefan Berger
Cc: Mimi Zohar, Jarkko Sakkinen, David Howells, dwmw2, herbert,
davem, jmorris, serge, nayna, mic, Konrad Wilk, keyrings,
linux-kernel, linux-crypto, linux-security-module
> On Mar 9, 2022, at 12:02 PM, Stefan Berger <stefanb@linux.ibm.com> wrote:
>
>
>
> On 3/9/22 13:13, Eric Snowberg wrote:
>>> On Mar 9, 2022, at 10:12 AM, Stefan Berger <stefanb@linux.ibm.com> wrote:
>>>
>>>
>>>
>>> On 3/8/22 13:02, Eric Snowberg wrote:
>>>>> On Mar 8, 2022, at 5:45 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
>>>>>
>>>>> On Mon, 2022-03-07 at 21:31 -0500, Stefan Berger wrote:
>>>>>>
>>>>>> On 3/7/22 18:38, Eric Snowberg wrote:
>>>>>>>
>>>>>>>
>>>>>>>> On Mar 7, 2022, at 4:01 PM, Mimi Zohar <zohar@linux.ibm.com> wrote:
>>>>>>>>
>>>>>>>> On Mon, 2022-03-07 at 18:06 +0000, Eric Snowberg wrote:
>>>>>>>>>
>>>>>>>>>>> diff --git a/crypto/asymmetric_keys/restrict.c b/crypto/asymmetric_keys/restrict.c
>>>>>>>>>>> index 6b1ac5f5896a..49bb2ea7f609 100644
>>>>>>>>>>> --- a/crypto/asymmetric_keys/restrict.c
>>>>>>>>>>> +++ b/crypto/asymmetric_keys/restrict.c
>>>>>>>>>>> @@ -108,6 +108,49 @@ 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.
>>>>>>>>>>
>>>>>>>>>> CA = root CA here, right?
>>>>>>>>>
>>>>>>>>> Yes, I’ll update the comment
>>>>>>>>
>>>>>>>> Updating the comment is not enough. There's an existing function named
>>>>>>>> "x509_check_for_self_signed()" which determines whether the certificate
>>>>>>>> is self-signed.
>>>>>>>
>>>>>>> Originally I tried using that function. However when the restrict link code is called,
>>>>>>> all the necessary x509 information is no longer available. The code in
>>>>>>> restrict_link_by_ca is basically doing the equivalent to x509_check_for_self_signed.
>>>>>>> After verifying the cert has the CA flag set, the call to public_key_verify_signature
>>>>>>> validates the cert is self signed.
>>>>>>>
>>>>>> Isn't x509_cert_parse() being called as part of parsing the certificate?
>>>>>> If so, it seems to check for a self-signed certificate every time. You
>>>>>> could add something like the following to x509_check_for_self_signed(cert):
>>>>>> pub->x509_self_signed = cert->self_signed = true;
>>>>>>
>>>>>> This could then reduce the function in 3/4 to something like:
>>>>>>
>>>>>> return payload->data[asym_crypto]->x509_self_signed;
>>>> When I was studying the restriction code, before writing this patch, it looked like
>>>> it was written from the standpoint to be as generic as possible. All code contained
>>>> within it works on either a public_key_signature or a public_key. I had assumed it
>>>> was written this way to be used with different asymmetrical key types now and in
>>>> the future. I called the public_key_verify_signature function instead of interrogating
>>>> the x509 payload to keep in line with what I thought was the original design. Let me
>>>> know if I should be carrying x509 code in here to make the change above.
>>>
>>> It does not seem right if there were two functions trying to determine whether an x509 cert is self-signed. The existing is invoked as part of loading a key onto the machine keyring from what I can see. It has access to more data about the cert and therefore can do stronger tests, yours doesn't have access to the data. So I guess I would remember in a boolean in the public key structure that the x509 cert it comes from was self signed following the existing test. Key in your function may be that that payload->data[] array is guaranteed to be from the x509 cert as set in x509_key_preparse().
>>>
>>> https://elixir.bootlin.com/linux/v5.17-rc7/source/crypto/asymmetric_keys/x509_public_key.c#L236
>> I could add another bool to the public key structure to designate if the key was self signed,
>> but this seems to go against what the kernel document states. "Asymmetric / Public-key
>> Cryptography Key Type” [1] states:
>> "The “asymmetric” key type is designed to be a container for the keys used in public-key
>> cryptography, without imposing any particular restrictions on the form or mechanism of
>> the cryptography or form of the key.
>> The asymmetric key is given a subtype that defines what sort of data is associated with
>> the key and provides operations to describe and destroy it. However, no requirement is
>> made that the key data actually be stored in the key."
>> Now every public key type would need to fill in the information on whether the key is self
>> signed or not. Instead of going through the public_key_verify_signature function currently
>> used in this patch.
>
> Every public key extracted from a x509 certificate would have to set this field to true if the public key originates from a self-signed x509 cert. Is this different from this code here where now every public key would have to set the key_is_ca field?
The information to determine if the key is a CA can not be derived without help from
the specific key type. Up to this point, no one has needed it.
>
> + if (v[1] != 0 && v[2] == ASN1_BOOL && v[3] == 1)
> + ctx->cert->pub->key_is_ca = true;
>
> The extension I would have suggested looked similar:
>
> cert->pub->x509_self_sign = cert->self_signed = true
>
> [ to be put here: https://elixir.bootlin.com/linux/v5.17-rc7/source/crypto/asymmetric_keys/x509_public_key.c#L147 ]
The information to determine if a key is self signed can be derived without help
from the specific key type. This can be achieved without modification to a generic
public header file. Adding a field to the public header would need to either be
x509 specific or generic for all key types. Adding a x509 specific field seems to
go against the goal outlined in the kernel documentation. Adding a generic
self_signed field impacts all key types, now each needs to be modified to fill in
the new field.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 3/4] KEYS: CA link restriction
2022-03-11 18:44 ` Eric Snowberg
@ 2022-03-11 20:23 ` Stefan Berger
2022-03-14 12:00 ` Stefan Berger
0 siblings, 1 reply; 30+ messages in thread
From: Stefan Berger @ 2022-03-11 20:23 UTC (permalink / raw)
To: Eric Snowberg
Cc: Mimi Zohar, Jarkko Sakkinen, David Howells, dwmw2, herbert,
davem, jmorris, serge, nayna, mic, Konrad Wilk, keyrings,
linux-kernel, linux-crypto, linux-security-module
On 3/11/22 13:44, Eric Snowberg wrote:
>
>
>> On Mar 9, 2022, at 12:02 PM, Stefan Berger <stefanb@linux.ibm.com> wrote:
>>
>>
>>
>> On 3/9/22 13:13, Eric Snowberg wrote:
>>>> On Mar 9, 2022, at 10:12 AM, Stefan Berger <stefanb@linux.ibm.com> wrote:
>>>>
>>>>
>>>>
>>>> On 3/8/22 13:02, Eric Snowberg wrote:
>>>>>> On Mar 8, 2022, at 5:45 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
>>>>>>
>>>>>> On Mon, 2022-03-07 at 21:31 -0500, Stefan Berger wrote:
>>>>>>>
>>>>>>> On 3/7/22 18:38, Eric Snowberg wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>> On Mar 7, 2022, at 4:01 PM, Mimi Zohar <zohar@linux.ibm.com> wrote:
>>>>>>>>>
>>>>>>>>> On Mon, 2022-03-07 at 18:06 +0000, Eric Snowberg wrote:
>>>>>>>>>>
>>>>>>>>>>>> diff --git a/crypto/asymmetric_keys/restrict.c b/crypto/asymmetric_keys/restrict.c
>>>>>>>>>>>> index 6b1ac5f5896a..49bb2ea7f609 100644
>>>>>>>>>>>> --- a/crypto/asymmetric_keys/restrict.c
>>>>>>>>>>>> +++ b/crypto/asymmetric_keys/restrict.c
>>>>>>>>>>>> @@ -108,6 +108,49 @@ 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.
>>>>>>>>>>>
>>>>>>>>>>> CA = root CA here, right?
>>>>>>>>>>
>>>>>>>>>> Yes, I’ll update the comment
>>>>>>>>>
>>>>>>>>> Updating the comment is not enough. There's an existing function named
>>>>>>>>> "x509_check_for_self_signed()" which determines whether the certificate
>>>>>>>>> is self-signed.
>>>>>>>>
>>>>>>>> Originally I tried using that function. However when the restrict link code is called,
>>>>>>>> all the necessary x509 information is no longer available. The code in
>>>>>>>> restrict_link_by_ca is basically doing the equivalent to x509_check_for_self_signed.
>>>>>>>> After verifying the cert has the CA flag set, the call to public_key_verify_signature
>>>>>>>> validates the cert is self signed.
>>>>>>>>
>>>>>>> Isn't x509_cert_parse() being called as part of parsing the certificate?
>>>>>>> If so, it seems to check for a self-signed certificate every time. You
>>>>>>> could add something like the following to x509_check_for_self_signed(cert):
>>>>>>> pub->x509_self_signed = cert->self_signed = true;
>>>>>>>
>>>>>>> This could then reduce the function in 3/4 to something like:
>>>>>>>
>>>>>>> return payload->data[asym_crypto]->x509_self_signed;
>>>>> When I was studying the restriction code, before writing this patch, it looked like
>>>>> it was written from the standpoint to be as generic as possible. All code contained
>>>>> within it works on either a public_key_signature or a public_key. I had assumed it
>>>>> was written this way to be used with different asymmetrical key types now and in
>>>>> the future. I called the public_key_verify_signature function instead of interrogating
>>>>> the x509 payload to keep in line with what I thought was the original design. Let me
>>>>> know if I should be carrying x509 code in here to make the change above.
>>>>
>>>> It does not seem right if there were two functions trying to determine whether an x509 cert is self-signed. The existing is invoked as part of loading a key onto the machine keyring from what I can see. It has access to more data about the cert and therefore can do stronger tests, yours doesn't have access to the data. So I guess I would remember in a boolean in the public key structure that the x509 cert it comes from was self signed following the existing test. Key in your function may be that that payload->data[] array is guaranteed to be from the x509 cert as set in x509_key_preparse().
>>>>
>>>> https://elixir.bootlin.com/linux/v5.17-rc7/source/crypto/asymmetric_keys/x509_public_key.c#L236
>>> I could add another bool to the public key structure to designate if the key was self signed,
>>> but this seems to go against what the kernel document states. "Asymmetric / Public-key
>>> Cryptography Key Type” [1] states:
>>> "The “asymmetric” key type is designed to be a container for the keys used in public-key
>>> cryptography, without imposing any particular restrictions on the form or mechanism of
>>> the cryptography or form of the key.
>>> The asymmetric key is given a subtype that defines what sort of data is associated with
>>> the key and provides operations to describe and destroy it. However, no requirement is
>>> made that the key data actually be stored in the key."
>>> Now every public key type would need to fill in the information on whether the key is self
>>> signed or not. Instead of going through the public_key_verify_signature function currently
>>> used in this patch.
>>
>> Every public key extracted from a x509 certificate would have to set this field to true if the public key originates from a self-signed x509 cert. Is this different from this code here where now every public key would have to set the key_is_ca field?
>
> The information to determine if the key is a CA can not be derived without help from
> the specific key type. Up to this point, no one has needed it.
>
>>
>> + if (v[1] != 0 && v[2] == ASN1_BOOL && v[3] == 1)
>> + ctx->cert->pub->key_is_ca = true;
>>
>> The extension I would have suggested looked similar:
>>
>> cert->pub->x509_self_sign = cert->self_signed = true
>>
>> [ to be put here: https://elixir.bootlin.com/linux/v5.17-rc7/source/crypto/asymmetric_keys/x509_public_key.c#L147 ]
>
> The information to determine if a key is self signed can be derived without help
> from the specific key type. This can be achieved without modification to a generic
> public header file. Adding a field to the public header would need to either be
> x509 specific or generic for all key types. Adding a x509 specific field seems to
> go against the goal outlined in the kernel documentation. Adding a generic
> self_signed field impacts all key types, now each needs to be modified to fill in
> the new field.
>
If we now called the generic field cert_self_signed we could let it
indicate whether the certificate the key was extracted from was
self-self signed. The next question then is how many different types of
certificates does the key subsystem support besides x509 so we know
where to set this field if necessary? I don't know of any other... x509
seems to be the only type of certificate associated with struct public_key.
What seems to be the case is that pkcs7 also runs the x509 cert parser
to extract an x509 certificate, thus the flag will be set down this call
path as well.
https://elixir.bootlin.com/linux/latest/source/crypto/asymmetric_keys/pkcs7_parser.c#L408
Further, the public_key struct is only used in a few places and only in
the crypto/asymmetric_keys directory filled in. Its usage in pkcs8 seems
not relevant for certs, so leaving cert_self_signed there uninitialized
seems just right. The code in public_key.c seems to not deal with
certificates. So what's left is the x509_cert_parser.c and the function
x509_cert_parse() allocates it and then calls
x509_check_for_self_signed(cert), which can set the flag.
It looks to me giving it a generic name and only ever setting it to true
iin x509_check_for_self_sign(cert) should work.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 3/4] KEYS: CA link restriction
2022-03-11 20:23 ` Stefan Berger
@ 2022-03-14 12:00 ` Stefan Berger
0 siblings, 0 replies; 30+ messages in thread
From: Stefan Berger @ 2022-03-14 12:00 UTC (permalink / raw)
To: Eric Snowberg
Cc: Mimi Zohar, Jarkko Sakkinen, David Howells, dwmw2, herbert,
davem, jmorris, serge, nayna, mic, Konrad Wilk, keyrings,
linux-kernel, linux-crypto, linux-security-module
On 3/11/22 15:23, Stefan Berger wrote:
>
>
> On 3/11/22 13:44, Eric Snowberg wrote:
>>
>>
>>> On Mar 9, 2022, at 12:02 PM, Stefan Berger <stefanb@linux.ibm.com>
>>> wrote:
>>>
>>>
>>>
>>> On 3/9/22 13:13, Eric Snowberg wrote:
>>>>> On Mar 9, 2022, at 10:12 AM, Stefan Berger <stefanb@linux.ibm.com>
>>>>> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 3/8/22 13:02, Eric Snowberg wrote:
>>>>>>> On Mar 8, 2022, at 5:45 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
>>>>>>>
>>>>>>> On Mon, 2022-03-07 at 21:31 -0500, Stefan Berger wrote:
>>>>>>>>
>>>>>>>> On 3/7/22 18:38, Eric Snowberg wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> On Mar 7, 2022, at 4:01 PM, Mimi Zohar <zohar@linux.ibm.com>
>>>>>>>>>> wrote:
>>>>>>>>>>
>>>>>>>>>> On Mon, 2022-03-07 at 18:06 +0000, Eric Snowberg wrote:
>>>>>>>>>>>
>>>>>>>>>>>>> diff --git a/crypto/asymmetric_keys/restrict.c
>>>>>>>>>>>>> b/crypto/asymmetric_keys/restrict.c
>>>>>>>>>>>>> index 6b1ac5f5896a..49bb2ea7f609 100644
>>>>>>>>>>>>> --- a/crypto/asymmetric_keys/restrict.c
>>>>>>>>>>>>> +++ b/crypto/asymmetric_keys/restrict.c
>>>>>>>>>>>>> @@ -108,6 +108,49 @@ 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.
>>>>>>>>>>>>
>>>>>>>>>>>> CA = root CA here, right?
>>>>>>>>>>>
>>>>>>>>>>> Yes, I’ll update the comment
>>>>>>>>>>
>>>>>>>>>> Updating the comment is not enough. There's an existing
>>>>>>>>>> function named
>>>>>>>>>> "x509_check_for_self_signed()" which determines whether the
>>>>>>>>>> certificate
>>>>>>>>>> is self-signed.
>>>>>>>>>
>>>>>>>>> Originally I tried using that function. However when the
>>>>>>>>> restrict link code is called,
>>>>>>>>> all the necessary x509 information is no longer available.
>>>>>>>>> The code in
>>>>>>>>> restrict_link_by_ca is basically doing the equivalent to
>>>>>>>>> x509_check_for_self_signed.
>>>>>>>>> After verifying the cert has the CA flag set, the call to
>>>>>>>>> public_key_verify_signature
>>>>>>>>> validates the cert is self signed.
>>>>>>>>>
>>>>>>>> Isn't x509_cert_parse() being called as part of parsing the
>>>>>>>> certificate?
>>>>>>>> If so, it seems to check for a self-signed certificate every
>>>>>>>> time. You
>>>>>>>> could add something like the following to
>>>>>>>> x509_check_for_self_signed(cert):
>>>>>>>> pub->x509_self_signed = cert->self_signed = true;
>>>>>>>>
>>>>>>>> This could then reduce the function in 3/4 to something like:
>>>>>>>>
>>>>>>>> return payload->data[asym_crypto]->x509_self_signed;
>>>>>> When I was studying the restriction code, before writing this
>>>>>> patch, it looked like
>>>>>> it was written from the standpoint to be as generic as possible.
>>>>>> All code contained
>>>>>> within it works on either a public_key_signature or a public_key.
>>>>>> I had assumed it
>>>>>> was written this way to be used with different asymmetrical key
>>>>>> types now and in
>>>>>> the future. I called the public_key_verify_signature function
>>>>>> instead of interrogating
>>>>>> the x509 payload to keep in line with what I thought was the
>>>>>> original design. Let me
>>>>>> know if I should be carrying x509 code in here to make the change
>>>>>> above.
>>>>>
>>>>> It does not seem right if there were two functions trying to
>>>>> determine whether an x509 cert is self-signed. The existing is
>>>>> invoked as part of loading a key onto the machine keyring from what
>>>>> I can see. It has access to more data about the cert and therefore
>>>>> can do stronger tests, yours doesn't have access to the data. So I
>>>>> guess I would remember in a boolean in the public key structure
>>>>> that the x509 cert it comes from was self signed following the
>>>>> existing test. Key in your function may be that that
>>>>> payload->data[] array is guaranteed to be from the x509 cert as set
>>>>> in x509_key_preparse().
>>>>>
>>>>> https://elixir.bootlin.com/linux/v5.17-rc7/source/crypto/asymmetric_keys/x509_public_key.c#L236
>>>>>
>>>> I could add another bool to the public key structure to designate if
>>>> the key was self signed,
>>>> but this seems to go against what the kernel document states.
>>>> "Asymmetric / Public-key
>>>> Cryptography Key Type” [1] states:
>>>> "The “asymmetric” key type is designed to be a container for the
>>>> keys used in public-key
>>>> cryptography, without imposing any particular restrictions on the
>>>> form or mechanism of
>>>> the cryptography or form of the key.
>>>> The asymmetric key is given a subtype that defines what sort of data
>>>> is associated with
>>>> the key and provides operations to describe and destroy it. However,
>>>> no requirement is
>>>> made that the key data actually be stored in the key."
>>>> Now every public key type would need to fill in the information on
>>>> whether the key is self
>>>> signed or not. Instead of going through the
>>>> public_key_verify_signature function currently
>>>> used in this patch.
>>>
>>> Every public key extracted from a x509 certificate would have to set
>>> this field to true if the public key originates from a self-signed
>>> x509 cert. Is this different from this code here where now every
>>> public key would have to set the key_is_ca field?
>>
>> The information to determine if the key is a CA can not be derived
>> without help from
>> the specific key type. Up to this point, no one has needed it.
>>
>>>
>>> + if (v[1] != 0 && v[2] == ASN1_BOOL && v[3] == 1)
>>> + ctx->cert->pub->key_is_ca = true;
>>>
>>> The extension I would have suggested looked similar:
>>>
>>> cert->pub->x509_self_sign = cert->self_signed = true
>>>
>>> [ to be put here:
>>> https://elixir.bootlin.com/linux/v5.17-rc7/source/crypto/asymmetric_keys/x509_public_key.c#L147
>>> ]
>>
>> The information to determine if a key is self signed can be derived
>> without help
>> from the specific key type. This can be achieved without modification
>> to a generic
>> public header file. Adding a field to the public header would need to
>> either be
>> x509 specific or generic for all key types. Adding a x509 specific
>> field seems to
>> go against the goal outlined in the kernel documentation. Adding a
>> generic
>> self_signed field impacts all key types, now each needs to be modified
>> to fill in
>> the new field.
>>
>
> If we now called the generic field cert_self_signed we could let it
> indicate whether the certificate the key was extracted from was
> self-self signed. The next question then is how many different types of
> certificates does the key subsystem support besides x509 so we know
> where to set this field if necessary? I don't know of any other... x509
> seems to be the only type of certificate associated with struct public_key.
> What seems to be the case is that pkcs7 also runs the x509 cert parser
> to extract an x509 certificate, thus the flag will be set down this call
> path as well.
>
> https://elixir.bootlin.com/linux/latest/source/crypto/asymmetric_keys/pkcs7_parser.c#L408
>
>
> Further, the public_key struct is only used in a few places and only in
> the crypto/asymmetric_keys directory filled in. Its usage in pkcs8 seems
> not relevant for certs, so leaving cert_self_signed there uninitialized
> seems just right. The code in public_key.c seems to not deal with
> certificates. So what's left is the x509_cert_parser.c and the function
> x509_cert_parse() allocates it and then calls
> x509_check_for_self_signed(cert), which can set the flag.
>
> It looks to me giving it a generic name and only ever setting it to true
> iin x509_check_for_self_sign(cert) should work.
Otherwise maybe we could introduce
struct cert_info {
bool is_ca;
bool self_sign;
}
And use it like this:
+ if (v[1] != 0 && v[2] == ASN1_BOOL && v[3] == 1)
+ ctx->cert->cert_info->is_ca = true;
New index in the data array:
prep->payload.data[asym_subtype] = &public_key_subtype;
prep->payload.data[asym_key_ids] = kids;
prep->payload.data[asym_crypto] = cert->pub;
prep->payload.data[asym_auth] = cert->sig;
prep->payload.data[asym_cert_info] = cert->cert_info;
There are a few more places where this new array index would need to be
set to NULL.
^ permalink raw reply [flat|nested] 30+ messages in thread
end of thread, other threads:[~2022-03-14 13:34 UTC | newest]
Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-01 17:36 [PATCH 0/4] Add CA enforcement in the machine keyring Eric Snowberg
2022-03-01 17:36 ` [PATCH 1/4] KEYS: Create static version of public_key_verify_signature Eric Snowberg
2022-03-01 17:36 ` [PATCH 2/4] X.509: Parse Basic Constraints for CA Eric Snowberg
2022-03-04 15:10 ` Stefan Berger
2022-03-07 18:02 ` Eric Snowberg
2022-03-01 17:36 ` [PATCH 3/4] KEYS: CA link restriction Eric Snowberg
2022-03-04 15:28 ` Stefan Berger
2022-03-07 18:06 ` Eric Snowberg
2022-03-07 23:01 ` Mimi Zohar
2022-03-07 23:38 ` Eric Snowberg
2022-03-08 2:31 ` Stefan Berger
2022-03-08 12:45 ` Mimi Zohar
2022-03-08 13:56 ` Stefan Berger
2022-03-08 18:02 ` Eric Snowberg
2022-03-09 17:12 ` Stefan Berger
2022-03-09 17:17 ` Stefan Berger
2022-03-09 18:13 ` Eric Snowberg
2022-03-09 19:02 ` Stefan Berger
2022-03-11 18:44 ` Eric Snowberg
2022-03-11 20:23 ` Stefan Berger
2022-03-14 12:00 ` Stefan Berger
2022-03-09 17:33 ` Mimi Zohar
2022-03-01 17:36 ` [PATCH 4/4] integrity: CA enforcement in machine keyring Eric Snowberg
2022-03-04 23:19 ` Stefan Berger
2022-03-07 18:13 ` Eric Snowberg
2022-03-07 18:36 ` Stefan Berger
2022-03-07 18:48 ` Eric Snowberg
2022-03-06 23:33 ` [PATCH 0/4] Add CA enforcement in the " Mimi Zohar
2022-03-07 18:55 ` Eric Snowberg
2022-03-09 18:43 ` Mimi Zohar
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.