| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ZxHwaGeDCBSp3Dzx@farprobe>
Date: Fri, 18 Oct 2024 01:21:44 -0400
From: Ben Boeckel <me@...boeckel.net>
To: Eric Snowberg <eric.snowberg@...cle.com>
Cc: linux-security-module@...r.kernel.org, dhowells@...hat.com,
dwmw2@...radead.org, herbert@...dor.apana.org.au,
davem@...emloft.net, ardb@...nel.org, jarkko@...nel.org,
paul@...l-moore.com, jmorris@...ei.org, serge@...lyn.com,
zohar@...ux.ibm.com, roberto.sassu@...wei.com,
dmitry.kasatkin@...il.com, mic@...ikod.net, casey@...aufler-ca.com,
stefanb@...ux.ibm.com, ebiggers@...nel.org, rdunlap@...radead.org,
linux-kernel@...r.kernel.org, keyrings@...r.kernel.org,
linux-crypto@...r.kernel.org, linux-efi@...r.kernel.org,
linux-integrity@...r.kernel.org
Subject: Re: [RFC PATCH v3 05/13] clavis: Introduce a new key type called
clavis_key_acl
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@...cle.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
Powered by blists - more mailing lists