| Message ID | 20241017155516.2582369-1-eric.snowberg@oracle.com |
|---|---|
| Headers | show |
| Series | Clavis LSM | expand |
On Thu, 2024-10-17 at 09:55 -0600, Eric Snowberg wrote: > Remove the CONFIG_INTEGRITY_PLATFORM_KEYRING ifdef check so this > pattern does not need to be repeated with new code. > > Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com> > --- > certs/system_keyring.c | 6 ------ > 1 file changed, 6 deletions(-) > > diff --git a/certs/system_keyring.c b/certs/system_keyring.c > index 9de610bf1f4b..e344cee10d28 100644 > --- a/certs/system_keyring.c > +++ b/certs/system_keyring.c > @@ -24,9 +24,7 @@ static struct key *secondary_trusted_keys; > #ifdef CONFIG_INTEGRITY_MACHINE_KEYRING > static struct key *machine_trusted_keys; > #endif > -#ifdef CONFIG_INTEGRITY_PLATFORM_KEYRING > static struct key *platform_trusted_keys; > -#endif > > extern __initconst const u8 system_certificate_list[]; > extern __initconst const unsigned long system_certificate_list_size; > @@ -345,11 +343,7 @@ int verify_pkcs7_message_sig(const void *data, > size_t len, > trusted_keys = builtin_trusted_keys; > #endif > } else if (trusted_keys == VERIFY_USE_PLATFORM_KEYRING) { > -#ifdef CONFIG_INTEGRITY_PLATFORM_KEYRING > trusted_keys = platform_trusted_keys; > -#else > - trusted_keys = NULL; > -#endif > if (!trusted_keys) { > ret = -ENOKEY; > pr_devel("PKCS#7 platform keyring is not > available\n"); Just to check with the argument that any commit should bring the Git tree to another "good state". Why this was flagged? What would be the collateral damage if only this commit was picked and put to a pull request? No intentions to do that, this more like forming a better understanding what is at stake here. I.e. I get that you need this for subsequent commits but I think the commit message should also have like explanation why this is a legit change otherwise. I mean, less flagging better if it does not cause harm is already great without higher level goals. BR, Jarkko
> On Oct 17, 2024, at 10:13 AM, Jarkko Sakkinen <jarkko@kernel.org> wrote: > > On Thu, 2024-10-17 at 09:55 -0600, Eric Snowberg wrote: >> Remove the CONFIG_INTEGRITY_PLATFORM_KEYRING ifdef check so this >> pattern does not need to be repeated with new code. >> >> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com> >> --- >> certs/system_keyring.c | 6 ------ >> 1 file changed, 6 deletions(-) >> >> diff --git a/certs/system_keyring.c b/certs/system_keyring.c >> index 9de610bf1f4b..e344cee10d28 100644 >> --- a/certs/system_keyring.c >> +++ b/certs/system_keyring.c >> @@ -24,9 +24,7 @@ static struct key *secondary_trusted_keys; >> #ifdef CONFIG_INTEGRITY_MACHINE_KEYRING >> static struct key *machine_trusted_keys; >> #endif >> -#ifdef CONFIG_INTEGRITY_PLATFORM_KEYRING >> static struct key *platform_trusted_keys; >> -#endif >> >> extern __initconst const u8 system_certificate_list[]; >> extern __initconst const unsigned long system_certificate_list_size; >> @@ -345,11 +343,7 @@ int verify_pkcs7_message_sig(const void *data, >> size_t len, >> trusted_keys = builtin_trusted_keys; >> #endif >> } else if (trusted_keys == VERIFY_USE_PLATFORM_KEYRING) { >> -#ifdef CONFIG_INTEGRITY_PLATFORM_KEYRING >> trusted_keys = platform_trusted_keys; >> -#else >> - trusted_keys = NULL; >> -#endif >> if (!trusted_keys) { >> ret = -ENOKEY; >> pr_devel("PKCS#7 platform keyring is not >> available\n"); > > Just to check with the argument that any commit should bring the Git > tree to another "good state". Why this was flagged? What would be the > collateral damage if only this commit was picked and put to a pull > request? No intentions to do that, this more like forming a better > understanding what is at stake here. > > I.e. I get that you need this for subsequent commits but I think the > commit message should also have like explanation why this is a legit > change otherwise. Thanks for taking a look at this, I will add a better explanation in the next round.
On Thu, 2024-10-17 at 09:55 -0600, Eric Snowberg wrote: > +static struct asymmetric_key_id *clavis_parse_boot_param(char *kid, > struct asymmetric_key_id *akid, > + int > akid_max_len) > +{ > + int error, hex_len; > + > + if (!kid) > + return 0; > + > + hex_len = strlen(kid) / 2; Hmmm... I'd consider sanity checking this: size_t len; /* ... */ len = strlen(kid); if (len % 2) { pr_err("Clavis key id has invalid length %lu\n", len); return 0; } hex_len = len / 2; BR, Jarkko
On Thu, 2024-10-17 at 09:55 -0600, Eric Snowberg wrote: > Add a new verification type called VERIFYING_CLAVIS_SIGNATURE. This > new > usage will be used for validating keys added to the new clavis LSM > keyring. > This will be introduced in a follow-on patch. > > Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com> > --- > crypto/asymmetric_keys/asymmetric_type.c | 1 + > crypto/asymmetric_keys/pkcs7_verify.c | 1 + > include/linux/verification.h | 2 ++ > 3 files changed, 4 insertions(+) > > diff --git a/crypto/asymmetric_keys/asymmetric_type.c > b/crypto/asymmetric_keys/asymmetric_type.c > index 43af5fa510c0..d7bf95c77f4a 100644 > --- a/crypto/asymmetric_keys/asymmetric_type.c > +++ b/crypto/asymmetric_keys/asymmetric_type.c > @@ -25,6 +25,7 @@ const char *const > key_being_used_for[NR__KEY_BEING_USED_FOR] = { > [VERIFYING_KEY_SIGNATURE] = "key sig", > [VERIFYING_KEY_SELF_SIGNATURE] = "key self sig", > [VERIFYING_UNSPECIFIED_SIGNATURE] = "unspec sig", > + [VERIFYING_CLAVIS_SIGNATURE] = "clavis sig", > }; > EXPORT_SYMBOL_GPL(key_being_used_for); > > diff --git a/crypto/asymmetric_keys/pkcs7_verify.c > b/crypto/asymmetric_keys/pkcs7_verify.c > index f0d4ff3c20a8..1dc80e68ce96 100644 > --- a/crypto/asymmetric_keys/pkcs7_verify.c > +++ b/crypto/asymmetric_keys/pkcs7_verify.c > @@ -428,6 +428,7 @@ int pkcs7_verify(struct pkcs7_message *pkcs7, > } > /* Authattr presence checked in parser */ > break; > + case VERIFYING_CLAVIS_SIGNATURE: > case VERIFYING_UNSPECIFIED_SIGNATURE: > if (pkcs7->data_type != OID_data) { > pr_warn("Invalid unspecified sig (not pkcs7- > data)\n"); > diff --git a/include/linux/verification.h > b/include/linux/verification.h > index cb2d47f28091..02d2d70e2324 100644 > --- a/include/linux/verification.h > +++ b/include/linux/verification.h > @@ -36,6 +36,8 @@ enum key_being_used_for { > VERIFYING_KEY_SIGNATURE, > VERIFYING_KEY_SELF_SIGNATURE, > VERIFYING_UNSPECIFIED_SIGNATURE, > + /* Add new entries above, keep VERIFYING_CLAVIS_SIGNATURE at > the end. */ > + VERIFYING_CLAVIS_SIGNATURE, > NR__KEY_BEING_USED_FOR > }; > extern const char *const key_being_used_for[NR__KEY_BEING_USED_FOR]; This looks as good as it can get. Just wondering that does this Clavis thing connect with the TPM2 asymmetric keys that I've been working on? I.e. can they be used in tandem. I should really update that patch set (latest from April). BR, Jarkko
On Thu, 2024-10-17 at 09:55 -0600, Eric Snowberg wrote: > If the kernel is built with CONFIG_MODULE_SIG_KEY, get the subject > key identifier and add an ACL for it within the .clavis keyring. > > Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com> Super sound splits! Nice to review, have to give credit on this :-) > --- > certs/.gitignore | 1 + > certs/Makefile | 20 ++++++++++++++++++++ > certs/clavis_module_acl.c | 7 +++++++ > security/clavis/clavis.h | 9 +++++++++ > security/clavis/clavis_keyring.c | 27 +++++++++++++++++++++++++++ > 5 files changed, 64 insertions(+) > create mode 100644 certs/clavis_module_acl.c > > diff --git a/certs/.gitignore b/certs/.gitignore > index cec5465f31c1..dc99ae5a2585 100644 > --- a/certs/.gitignore > +++ b/certs/.gitignore > @@ -3,3 +3,4 @@ > /extract-cert > /x509_certificate_list > /x509_revocation_list > +/module_acl > diff --git a/certs/Makefile b/certs/Makefile > index f6fa4d8d75e0..f2555e5296f5 100644 > --- a/certs/Makefile > +++ b/certs/Makefile > @@ -6,6 +6,7 @@ > obj-$(CONFIG_SYSTEM_TRUSTED_KEYRING) += system_keyring.o > system_certificates.o > obj-$(CONFIG_SYSTEM_BLACKLIST_KEYRING) += blacklist.o > blacklist_hashes.o > obj-$(CONFIG_SYSTEM_REVOCATION_LIST) += revocation_certificates.o > +obj-$(CONFIG_SECURITY_CLAVIS) += clavis_module_acl.o > > $(obj)/blacklist_hashes.o: $(obj)/blacklist_hash_list > CFLAGS_blacklist_hashes.o := -I $(obj) > @@ -75,6 +76,25 @@ $(obj)/signing_key.x509: $(filter-out > $(PKCS11_URI),$(CONFIG_MODULE_SIG_KEY)) $( > > targets += signing_key.x509 > > +ifeq ($(CONFIG_MODULE_SIG_KEY),) > +quiet_cmd_make_module_acl = GEN $@ > + cmd_make_module_acl = \ > + echo > $@ > +else > +quiet_cmd_make_module_acl = GEN $@ > + cmd_make_module_acl = \ > + openssl x509 -in $< -inform der -ext subjectKeyIdentifier - > nocert | \ > + tail -n +2 | cut -f2 -d '='| tr -d ':' | tr '[:upper:]' > '[:lower:]' | \ > + sed 's/^[ \t]*//; s/.*/"00:&",/' > $@ > +endif > + > +$(obj)/module_acl: $(obj)/signing_key.x509 FORCE > + $(call if_changed,make_module_acl) > + > +$(obj)/clavis_module_acl.o: $(obj)/module_acl > + > +targets += module_acl > + > $(obj)/revocation_certificates.o: $(obj)/x509_revocation_list > > $(obj)/x509_revocation_list: $(CONFIG_SYSTEM_REVOCATION_KEYS) > $(obj)/extract-cert FORCE > diff --git a/certs/clavis_module_acl.c b/certs/clavis_module_acl.c > new file mode 100644 > index 000000000000..fc2f694c48f9 > --- /dev/null > +++ b/certs/clavis_module_acl.c > @@ -0,0 +1,7 @@ > +// SPDX-License-Identifier: GPL-2.0 > +#include <linux/kernel.h> > + > +const char __initconst *const clavis_module_acl[] = { > +#include "module_acl" > + NULL > +}; > diff --git a/security/clavis/clavis.h b/security/clavis/clavis.h > index 7b55a6050440..92f77a1939ad 100644 > --- a/security/clavis/clavis.h > +++ b/security/clavis/clavis.h > @@ -11,4 +11,13 @@ struct asymmetric_setup_kid { > struct asymmetric_key_id id; > unsigned char data[CLAVIS_BIN_KID_MAX]; > }; > + > +#ifndef CONFIG_SYSTEM_TRUSTED_KEYRING > +const char __initconst *const clavis_module_acl[] = { > + NULL > +}; > +#else > +extern const char __initconst *const clavis_module_acl[]; > +#endif > + > #endif /* _SECURITY_CLAVIS_H_ */ > diff --git a/security/clavis/clavis_keyring.c > b/security/clavis/clavis_keyring.c > index 00163e7f0fe9..2a18d0e77189 100644 > --- a/security/clavis/clavis_keyring.c > +++ b/security/clavis/clavis_keyring.c > @@ -259,6 +259,31 @@ static struct key_restriction > *clavis_restriction_alloc(key_restrict_link_func_t > return restriction; > } > > +static void clavis_add_acl(const char *const *skid_list, struct key > *keyring) > +{ > + const char *const *acl; > + key_ref_t key; > + > + for (acl = skid_list; *acl; acl++) { > + key = key_create(make_key_ref(keyring, true), > + "clavis_key_acl", > + *acl, > + NULL, > + 0, > + KEY_POS_SEARCH | KEY_POS_VIEW | > KEY_USR_SEARCH | KEY_USR_VIEW, > + KEY_ALLOC_NOT_IN_QUOTA | > KEY_ALLOC_BUILT_IN | > + KEY_ALLOC_BYPASS_RESTRICTION); > + if (IS_ERR(key)) { > + if (PTR_ERR(key) == -EEXIST) > + pr_info("Duplicate clavis_key_acl > %s\n", *acl); > + else > + pr_info("Problem with clavis_key_acl > %s: %pe\n", *acl, key); > + } else { > + pr_info("Added clavis_key_acl %s\n", *acl); > + } > + } > +} > + > static int __init clavis_keyring_init(void) > { > struct key_restriction *restriction; > @@ -274,6 +299,8 @@ static int __init clavis_keyring_init(void) > if (IS_ERR(clavis_keyring)) > panic("Can't allocate clavis keyring\n"); > > + clavis_add_acl(clavis_module_acl, clavis_keyring); > + > return 0; > } > Not yet tagging, but neither anything to complain. LGTM BR, Jarkko
> On Oct 17, 2024, at 10:50 AM, Jarkko Sakkinen <jarkko@kernel.org> wrote: > > On Thu, 2024-10-17 at 09:55 -0600, Eric Snowberg wrote: >> +static struct asymmetric_key_id *clavis_parse_boot_param(char *kid, >> struct asymmetric_key_id *akid, >> + int >> akid_max_len) >> +{ >> + int error, hex_len; >> + >> + if (!kid) >> + return 0; >> + >> + hex_len = strlen(kid) / 2; > > Hmmm... I'd consider sanity checking this: > > size_t len; > > /* ... */ > > len = strlen(kid); > if (len % 2) { > pr_err("Clavis key id has invalid length %lu\n", len); > return 0; > } > > hex_len = len / 2; > Good catch, I will include this in the next round. I have also added a kunit test for this as well. Thanks.
On Thu, 2024-10-17 at 20:34 +0000, Eric Snowberg wrote: > > > > On Oct 17, 2024, at 10:50 AM, Jarkko Sakkinen <jarkko@kernel.org> > > wrote: > > > > On Thu, 2024-10-17 at 09:55 -0600, Eric Snowberg wrote: > > > +static struct asymmetric_key_id *clavis_parse_boot_param(char > > > *kid, > > > struct asymmetric_key_id *akid, > > > + int > > > akid_max_len) > > > +{ > > > + int error, hex_len; > > > + > > > + if (!kid) > > > + return 0; > > > + > > > + hex_len = strlen(kid) / 2; > > > > Hmmm... I'd consider sanity checking this: > > > > size_t len; > > > > /* ... */ > > > > len = strlen(kid); > > if (len % 2) { > > pr_err("Clavis key id has invalid length %lu\n", len); > > return 0; > > } > > > > hex_len = len / 2; > > > > Good catch, I will include this in the next round. I have also added > a kunit test > for this as well. Thanks. I guess hex2bin() would eventually catch this issue, i.e. not an actual bug, but I still think that you are better off doing also check here and get an appropriate message to klog if that ever happens :-) BR, Jarkko
> On Oct 17, 2024, at 1:20 PM, Jarkko Sakkinen <jarkko@kernel.org> wrote: > > On Thu, 2024-10-17 at 09:55 -0600, Eric Snowberg wrote: >> Add a new verification type called VERIFYING_CLAVIS_SIGNATURE. This >> new >> usage will be used for validating keys added to the new clavis LSM >> keyring. >> This will be introduced in a follow-on patch. >> >> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com> >> --- >> crypto/asymmetric_keys/asymmetric_type.c | 1 + >> crypto/asymmetric_keys/pkcs7_verify.c | 1 + >> include/linux/verification.h | 2 ++ >> 3 files changed, 4 insertions(+) >> >> diff --git a/crypto/asymmetric_keys/asymmetric_type.c >> b/crypto/asymmetric_keys/asymmetric_type.c >> index 43af5fa510c0..d7bf95c77f4a 100644 >> --- a/crypto/asymmetric_keys/asymmetric_type.c >> +++ b/crypto/asymmetric_keys/asymmetric_type.c >> @@ -25,6 +25,7 @@ const char *const >> key_being_used_for[NR__KEY_BEING_USED_FOR] = { >> [VERIFYING_KEY_SIGNATURE] = "key sig", >> [VERIFYING_KEY_SELF_SIGNATURE] = "key self sig", >> [VERIFYING_UNSPECIFIED_SIGNATURE] = "unspec sig", >> + [VERIFYING_CLAVIS_SIGNATURE] = "clavis sig", >> }; >> EXPORT_SYMBOL_GPL(key_being_used_for); >> >> diff --git a/crypto/asymmetric_keys/pkcs7_verify.c >> b/crypto/asymmetric_keys/pkcs7_verify.c >> index f0d4ff3c20a8..1dc80e68ce96 100644 >> --- a/crypto/asymmetric_keys/pkcs7_verify.c >> +++ b/crypto/asymmetric_keys/pkcs7_verify.c >> @@ -428,6 +428,7 @@ int pkcs7_verify(struct pkcs7_message *pkcs7, >> } >> /* Authattr presence checked in parser */ >> break; >> + case VERIFYING_CLAVIS_SIGNATURE: >> case VERIFYING_UNSPECIFIED_SIGNATURE: >> if (pkcs7->data_type != OID_data) { >> pr_warn("Invalid unspecified sig (not pkcs7- >> data)\n"); >> diff --git a/include/linux/verification.h >> b/include/linux/verification.h >> index cb2d47f28091..02d2d70e2324 100644 >> --- a/include/linux/verification.h >> +++ b/include/linux/verification.h >> @@ -36,6 +36,8 @@ enum key_being_used_for { >> VERIFYING_KEY_SIGNATURE, >> VERIFYING_KEY_SELF_SIGNATURE, >> VERIFYING_UNSPECIFIED_SIGNATURE, >> + /* Add new entries above, keep VERIFYING_CLAVIS_SIGNATURE at >> the end. */ >> + VERIFYING_CLAVIS_SIGNATURE, >> NR__KEY_BEING_USED_FOR >> }; >> extern const char *const key_being_used_for[NR__KEY_BEING_USED_FOR]; > > This looks as good as it can get. Just wondering that does this Clavis > thing connect with the TPM2 asymmetric keys that I've been working on? > I.e. can they be used in tandem. I should really update that patch set > (latest from April). With some changes, I think they could be used in tandem. If I'm looking at the correct series, tpm2_key_rsa_describe would need to be changed to return a unique identifier, instead of "TPM2/RSA". This identifier could be used instead of the skid when creating a Clavis ACL. There would probably also need to be a new system kernel keyring containing these TPM keys. Similar to the builtin, secondary, machine, etc.
On Thu, 2024-10-17 at 21:42 +0000, Eric Snowberg wrote: > > > > On Oct 17, 2024, at 1:20 PM, Jarkko Sakkinen <jarkko@kernel.org> > > wrote: > > > > On Thu, 2024-10-17 at 09:55 -0600, Eric Snowberg wrote: > > > Add a new verification type called VERIFYING_CLAVIS_SIGNATURE. > > > This > > > new > > > usage will be used for validating keys added to the new clavis > > > LSM > > > keyring. > > > This will be introduced in a follow-on patch. > > > > > > Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com> > > > --- > > > crypto/asymmetric_keys/asymmetric_type.c | 1 + > > > crypto/asymmetric_keys/pkcs7_verify.c | 1 + > > > include/linux/verification.h | 2 ++ > > > 3 files changed, 4 insertions(+) > > > > > > diff --git a/crypto/asymmetric_keys/asymmetric_type.c > > > b/crypto/asymmetric_keys/asymmetric_type.c > > > index 43af5fa510c0..d7bf95c77f4a 100644 > > > --- a/crypto/asymmetric_keys/asymmetric_type.c > > > +++ b/crypto/asymmetric_keys/asymmetric_type.c > > > @@ -25,6 +25,7 @@ const char *const > > > key_being_used_for[NR__KEY_BEING_USED_FOR] = { > > > [VERIFYING_KEY_SIGNATURE] = "key sig", > > > [VERIFYING_KEY_SELF_SIGNATURE] = "key self sig", > > > [VERIFYING_UNSPECIFIED_SIGNATURE] = "unspec sig", > > > + [VERIFYING_CLAVIS_SIGNATURE] = "clavis sig", > > > }; > > > EXPORT_SYMBOL_GPL(key_being_used_for); > > > > > > diff --git a/crypto/asymmetric_keys/pkcs7_verify.c > > > b/crypto/asymmetric_keys/pkcs7_verify.c > > > index f0d4ff3c20a8..1dc80e68ce96 100644 > > > --- a/crypto/asymmetric_keys/pkcs7_verify.c > > > +++ b/crypto/asymmetric_keys/pkcs7_verify.c > > > @@ -428,6 +428,7 @@ int pkcs7_verify(struct pkcs7_message *pkcs7, > > > } > > > /* Authattr presence checked in parser */ > > > break; > > > + case VERIFYING_CLAVIS_SIGNATURE: > > > case VERIFYING_UNSPECIFIED_SIGNATURE: > > > if (pkcs7->data_type != OID_data) { > > > pr_warn("Invalid unspecified sig (not pkcs7- > > > data)\n"); > > > diff --git a/include/linux/verification.h > > > b/include/linux/verification.h > > > index cb2d47f28091..02d2d70e2324 100644 > > > --- a/include/linux/verification.h > > > +++ b/include/linux/verification.h > > > @@ -36,6 +36,8 @@ enum key_being_used_for { > > > VERIFYING_KEY_SIGNATURE, > > > VERIFYING_KEY_SELF_SIGNATURE, > > > VERIFYING_UNSPECIFIED_SIGNATURE, > > > + /* Add new entries above, keep VERIFYING_CLAVIS_SIGNATURE at > > > the end. */ > > > + VERIFYING_CLAVIS_SIGNATURE, > > > NR__KEY_BEING_USED_FOR > > > }; > > > extern const char *const > > > key_being_used_for[NR__KEY_BEING_USED_FOR]; > > > > This looks as good as it can get. Just wondering that does this > > Clavis > > thing connect with the TPM2 asymmetric keys that I've been working > > on? > > I.e. can they be used in tandem. I should really update that patch > > set > > (latest from April). > > With some changes, I think they could be used in tandem. If I'm > looking at > the correct series, tpm2_key_rsa_describe would need to be changed to > return a unique identifier, instead of "TPM2/RSA". This identifier > could be > used instead of the skid when creating a Clavis ACL. There would > probably > also need to be a new system kernel keyring containing these TPM > keys. > Similar to the builtin, secondary, machine, etc. I think when I finally get into working on this again I focus on purely to get ECDSA right. Thanks for the tip, most likely this revamp will happen post your patch set. I'm still busy fixing corner cases with the bus encryption that James Bottomley contributed. BR, Jarkko