> On Oct 17, 2024, at 11:21 PM, Ben Boeckel <me@xxxxxxxxxxxxxx> wrote:
>
> On Thu, Oct 17, 2024 at 09:55:08 -0600, Eric Snowberg wrote:
>> Introduce a new key type for keyring access control. The new key type
>> is called clavis_key_acl. The clavis_key_acl contains the subject key
>> identifier along with the allowed usage type for the key.
>>
>> The format is as follows:
>>
>> XX:YYYYYYYYYYY
>>
>> XX - Single byte of the key type
>> VERIFYING_MODULE_SIGNATURE 00
>> VERIFYING_FIRMWARE_SIGNATURE 01
>> VERIFYING_KEXEC_PE_SIGNATURE 02
>> VERIFYING_KEY_SIGNATURE 03
>> VERIFYING_KEY_SELF_SIGNATURE 04
>> VERIFYING_UNSPECIFIED_SIGNATURE 05
>> : - ASCII colon
>> YY - Even number of hexadecimal characters representing the key id
>
> This is expected to be *lowercase* hexadecimal characters in the code;
> can that restriction please be documented? (Coming back here, there is a
> `tolower` pass performed when copying from userspace, so this seems to
> be an internal requirement, not userspace.
Correct
> Might be worth documenting
> somewhere in case the kernel wants to make such a key internally.)
>
> I also see a 32-byte (64 hex characters) limit in the code; that should
> also be documented somewhere.
I didn't want to burden the end-user with getting everything in
lowercase, since OpenSSL typically returns the result in uppercase.
I will add some comments as you suggested incase the kernel
wants to make such a key internally.
>> This key type will be used in the clavis keyring for access control. To
>> be added to the clavis keyring, the clavis_key_acl must be S/MIME signed
>> by the sole asymmetric key contained within it.
>>
>> Below is an example of how this could be used. Within the example, the
>> key (b360d113c848ace3f1e6a80060b43d1206f0487d) is already in the machine
>> keyring. The intended usage for this key is to validate a signed kernel
>> for kexec:
>>
>> echo "02:b360d113c848ace3f1e6a80060b43d1206f0487d" > kernel-acl.txt
>>
>> The next step is to sign it:
>>
>> openssl smime -sign -signer clavis-lsm.x509 -inkey clavis-lsm.priv -in \
>> kernel-acl.txt -out kernel-acl.pkcs7 -binary -outform DER \
>> -nodetach -noattr
>>
>> The final step is how to add the acl to the .clavis keyring:
>>
>> keyctl padd clavis_key_acl "" %:.clavis < kernel-acl.pkcs7
>>
>> Afterwards the new clavis_key_acl can be seen in the .clavis keyring:
>>
>> keyctl show %:.clavis
>> Keyring
>> keyring: .clavis
>> \_ asymmetric: Clavis LSM key: 4a00ab9f35c9dc3aed7c225d22bafcbd9285e1e8
>> \_ clavis_key_acl: 02:b360d113c848ace3f1e6a80060b43d1206f0487d
>
> Can this be committed to `Documentation/` and not just the Git history
> please?
This is documented, but it doesn't come in until the 8th patch in the series.
Hopefully that is not an issue.
> Code comments inline below.
>
>> Signed-off-by: Eric Snowberg <eric.snowberg@xxxxxxxxxx>
>> ---
>> security/clavis/clavis.h | 1 +
>> security/clavis/clavis_keyring.c | 173 +++++++++++++++++++++++++++++++
>> 2 files changed, 174 insertions(+)
>>
>> diff --git a/security/clavis/clavis.h b/security/clavis/clavis.h
>> index 5e397b55a60a..7b55a6050440 100644
>> --- a/security/clavis/clavis.h
>> +++ b/security/clavis/clavis.h
>> @@ -5,6 +5,7 @@
>>
>> /* Max length for the asymmetric key id contained on the boot param */
>> #define CLAVIS_BIN_KID_MAX 32
>> +#define CLAVIS_ASCII_KID_MAX 64
>>
>> struct asymmetric_setup_kid {
>> struct asymmetric_key_id id;
>> diff --git a/security/clavis/clavis_keyring.c b/security/clavis/clavis_keyring.c
>> index 400ed455a3a2..00163e7f0fe9 100644
>> --- a/security/clavis/clavis_keyring.c
>> +++ b/security/clavis/clavis_keyring.c
>> @@ -2,8 +2,12 @@
>>
>> #include <linux/security.h>
>> #include <linux/integrity.h>
>> +#include <linux/ctype.h>
>> #include <keys/asymmetric-type.h>
>> +#include <keys/asymmetric-subtype.h>
>> #include <keys/system_keyring.h>
>> +#include <keys/user-type.h>
>> +#include <crypto/pkcs7.h>
>> #include "clavis.h"
>>
>> static struct key *clavis_keyring;
>> @@ -11,10 +15,173 @@ static struct asymmetric_key_id *clavis_boot_akid;
>> static struct asymmetric_setup_kid clavis_setup_akid;
>> static bool clavis_enforced;
>>
>> +static int pkcs7_preparse_content(void *ctx, const void *data, size_t len, size_t asn1hdrlen)
>> +{
>> + struct key_preparsed_payload *prep = ctx;
>> + const void *saved_prep_data;
>> + size_t saved_prep_datalen;
>> + char *desc;
>> + int ret, i;
>> +
>> + /* key_acl_free_preparse will free this */
>> + desc = kmemdup(data, len, GFP_KERNEL);
>> + if (!desc)
>> + return -ENOMEM;
>> +
>> + /* Copy the user supplied contents and remove any white space. */
>> + for (i = 0; i < len; i++) {
>> + desc[i] = tolower(desc[i]);
>
> Ah, looking here it seems that userspace can provide upper or lowercase.
> THat this is being performed should be added to the comment here.
I'll update the comment.
>
>> + if (isspace(desc[i]))
>> + desc[i] = 0;
>
> How is setting a space to `0` *removing* it? Surely the `isxdigit` check
> internally is going to reject this. Perhaps you meant to have two
> indices into `desc`, one read and one write and to stall the write index
> as long as we're reading whitespace?
>
> Also, that whitespace is stripped is a userspace-relevant detail that
> should be documented.
This was done incase the end-user has a trailing carriage return at the
end of their ACL. I have updated the comment as follows:
+ /*
+ * Copy the user supplied contents, if uppercase is used, convert it to
+ * lowercase. Also if the end of the ACL contains any whitespace, strip
+ * it out.
+ */
>
>> +static void key_acl_destroy(struct key *key)
>> +{
>> + /* It should not be possible to get here */
>> + pr_info("destroy clavis_key_acl denied\n");
>> +}
>> +
>> +static void key_acl_revoke(struct key *key)
>> +{
>> + /* It should not be possible to get here */
>> + pr_info("revoke clavis_key_acl denied\n");
>> +}
>
> These keys cannot be destroyed or revoked? This seems…novel to me. What
> if there's a timeout on the key? If such keys are immortal, timeouts
> should also be refused?
All the system kernel keyrings work this way. But now that you bring this up, neither of
these functions are really necessary, so I will remove them in the next round.
>> +static int key_acl_vet_description(const char *desc)
>> +{
>> + int i, desc_len;
>> + s16 ktype;
>> +
>> + if (!desc)
>> + goto invalid;
>> +
>> + desc_len = sizeof(desc);
>
> This should be `strlen`, no?
I will change this to strlen
>> + /*
>> + * clavis_acl format:
>> + * xx:yyyy...
>> + *
>> + * xx - Single byte of the key type
>> + * : - Ascii colon
>> + * yyyy.. - Even number of hexadecimal characters representing the keyid
>> + */
>> +
>> + /* The min clavis acl is 7 characters. */
>> + if (desc_len < 7)
>> + goto invalid;
>> +
>> + /* Check the first byte is a valid key type. */
>> + if (sscanf(desc, "%2hx", &ktype) != 1)
>> + goto invalid;
>> +
>> + if (ktype >= VERIFYING_CLAVIS_SIGNATURE)
>> + goto invalid;
>> +
>> + /* Check that there is a colon following the key type */
>> + if (desc[2] != ':')
>> + goto invalid;
>> +
>> + /* Move past the colon. */
>> + desc += 3;
>> +
>> + for (i = 0; *desc && i < CLAVIS_ASCII_KID_MAX; desc++, i++) {
>> + /* Check if lowercase hex number */
>> + if (!isxdigit(*desc) || isupper(*desc))
>> + goto invalid;
>> + }
>> +
>> + /* Check if the has is greater than CLAVIS_ASCII_KID_MAX. */
>> + if (*desc)
>> + goto invalid;
>> +
>> + /* Check for even number of hex characters. */
>> + if (i == 0 || i & 1)
>
> FWIW< the `i == 0` is impossible due to the `desc_len < 7` check above
> (well, once `strlen` is used…).
and remove the unnecessary check. Thanks for your review.
