Re: [RFC PATCH v3 05/13] clavis: Introduce a new key type called clavis_key_acl

[Eric Snowberg <eric.snowberg@oracle.com>]

> On Oct 17, 2024, at 11:21 PM, Ben Boeckel <me@benboeckel.net> 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@oracle.com>
>> ---
>> 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.