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

[Ben Boeckel <me@benboeckel.net>]

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. 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.

> 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?

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.

> +		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.

> +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?

> +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?

> +	/*
> +	 * 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…).

Thanks,

--Ben