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

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]);
+		if (isspace(desc[i]))
+			desc[i] = 0;
+	}
+
+	prep->description = desc;
+	saved_prep_data = prep->data;
+	saved_prep_datalen = prep->datalen;
+	prep->data = desc;
+	prep->datalen = len;
+	ret = user_preparse(prep);
+	prep->data = saved_prep_data;
+	prep->datalen = saved_prep_datalen;
+	return ret;
+}
+
+static void key_acl_free_preparse(struct key_preparsed_payload *prep)
+{
+	kfree(prep->description);
+	user_free_preparse(prep);
+}
+
+static struct key *clavis_keyring_get(void)
+{
+	return clavis_keyring;
+}
+
 static bool clavis_acl_enforced(void)
 {
 	return clavis_enforced;
 }
+
+static int key_acl_preparse(struct key_preparsed_payload *prep)
+{
+	/*
+	 * Only allow the description to be set via the pkcs7 data contents.
+	 * The exception to this rule is if the entry was builtin, it will have
+	 * the original_description set.  Since we don't have access to the key
+	 * within the preparse step to determine if the entity is builtin, let
+	 * it through now and this will be checked in the instantiate step.
+	 */
+	if (prep->orig_description)
+		return 0;
+
+	return verify_pkcs7_signature(NULL, 0, prep->data, prep->datalen, clavis_keyring_get(),
+				      VERIFYING_CLAVIS_SIGNATURE, pkcs7_preparse_content,
+				      prep);
+}
+
+static int key_acl_instantiate(struct key *key, struct key_preparsed_payload *prep)
+{
+	/*
+	 * The orig_description may only be used for builtin entities.  All
+	 * other entries must have been validated through the pkcs7 signature
+	 * within the preparse stage.
+	 */
+	if (prep->orig_description && !(key->flags & (1 << KEY_FLAG_BUILTIN)))
+		return -EINVAL;
+
+	key->perm = KEY_POS_SEARCH | KEY_POS_VIEW | KEY_USR_SEARCH |
+		    KEY_USR_VIEW;
+	set_bit(KEY_FLAG_KEEP, &key->flags);
+	return generic_key_instantiate(key, prep);
+}
+
+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");
+}
+
+static int key_acl_update(struct key *key, struct key_preparsed_payload *prep)
+{
+	return -EPERM;
+}
+
+static int key_acl_vet_description(const char *desc)
+{
+	int i, desc_len;
+	s16 ktype;
+
+	if (!desc)
+		goto invalid;
+
+	desc_len = sizeof(desc);
+
+	/*
+	 * 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)
+		goto invalid;
+
+	return 0;
+
+invalid:
+	pr_err("Unparsable clavis key id: %s\n", desc);
+	return -EINVAL;
+}
+
+static struct key_type clavis_key_acl = {
+	.name			= "clavis_key_acl",
+	.preparse		= key_acl_preparse,
+	.free_preparse		= key_acl_free_preparse,
+	.instantiate		= key_acl_instantiate,
+	.update			= key_acl_update,
+	.revoke			= key_acl_revoke,
+	.destroy		= key_acl_destroy,
+	.vet_description	= key_acl_vet_description,
+	.read			= user_read,
+};
+
 static int restrict_link_for_clavis(struct key *dest_keyring, const struct key_type *type,
 				    const union key_payload *payload, struct key *restrict_key)
 {
@@ -28,6 +195,9 @@ static int restrict_link_for_clavis(struct key *dest_keyring, const struct key_t
 		return 0;
 	}
 
+	if (type == &clavis_key_acl)
+		return 0;
+
 	return -EOPNOTSUPP;
 }
 
@@ -93,6 +263,9 @@ static int __init clavis_keyring_init(void)
 {
 	struct key_restriction *restriction;
 
+	if (register_key_type(&clavis_key_acl) < 0)
+		panic("Can't allocate clavis key type\n");
+
 	restriction = clavis_restriction_alloc(restrict_link_for_clavis);
 	if (!restriction)
 		panic("Can't allocate clavis keyring restriction\n");
-- 
2.45.0

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

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

On Fri, Oct 18, 2024 at 15:42:15 +0000, Eric Snowberg wrote:
> > On Oct 17, 2024, at 11:21 PM, Ben Boeckel <me@benboeckel.net> wrote:
> > 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.

Ah, I'll look there, thanks.

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

Well, this doesn't check the end for whitespace; any internal whitespace
will terminate the key:

    DEAD BEEF
        ^ becomes NUL

and results in the same thing as `DEAD` being passed.

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

Actually, this could probably be `strnlen` using `CLAVIS_ASCII_KID_MAX +
1` just to avoid running off into la-la land when we're going to error
anyways. Or even `8` because we only actually care about "is at least 7
bytes". Worth a comment either way.

Looking forward to the next cycle.

Thanks,

--Ben

> On Oct 18, 2024, at 10:55 AM, Ben Boeckel <me@benboeckel.net> wrote:
> 
> On Fri, Oct 18, 2024 at 15:42:15 +0000, Eric Snowberg wrote:
>> 
>> 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.
>> +        */
> 
> Well, this doesn't check the end for whitespace; any internal whitespace
> will terminate the key:
> 
>    DEAD BEEF
>        ^ becomes NUL
> 
> and results in the same thing as `DEAD` being passed.

Originally I was thinking I could extract and fix up the data in pkcs7_preparse_content,
later when key_acl_vet_description gets called do the validation. But I see 
your point that it is possible there could be a valid ACL, followed by a space and
some other data, which should trigger an invalid response.  I'll take care of this in
the next round too.  I'll also add a Kunit test for this one. Thanks.