Messages in this thread | | | Date | Wed, 9 Mar 2022 19:36:50 +0100 | From | Mickaël Salaün <> | Subject | Re: [PATCH v8 5/5] certs: Allow root user to append signed hashes to the blacklist keyring |
| |
On 09/03/2022 17:01, Jarkko Sakkinen wrote: > On Tue, Mar 08, 2022 at 05:02:23PM +0100, Mickaël Salaün wrote: >> >> On 08/03/2022 14:19, Jarkko Sakkinen wrote: >>> On Tue, Mar 08, 2022 at 01:18:28PM +0100, Mickaël Salaün wrote: >>>> >>>> On 08/03/2022 12:53, Jarkko Sakkinen wrote: >>>>> On Mon, Jul 12, 2021 at 07:03:13PM +0200, Mickaël Salaün wrote: >>>>>> From: Mickaël Salaün <mic@linux.microsoft.com> >>>>>> >>>>>> Add a kernel option SYSTEM_BLACKLIST_AUTH_UPDATE to enable the root user >>>>>> to dynamically add new keys to the blacklist keyring. This enables to >>>>>> invalidate new certificates, either from being loaded in a keyring, or >>>>>> from being trusted in a PKCS#7 certificate chain. This also enables to >>>>>> add new file hashes to be denied by the integrity infrastructure. >>>>>> >>>>>> Being able to untrust a certificate which could have normaly been >>>>>> trusted is a sensitive operation. This is why adding new hashes to the >>>>>> blacklist keyring is only allowed when these hashes are signed and >>>>>> vouched by the builtin trusted keyring. A blacklist hash is stored as a >>>>>> key description. The PKCS#7 signature of this description must be >>>>>> provided as the key payload. >>>>>> >>>>>> Marking a certificate as untrusted should be enforced while the system >>>>>> is running. It is then forbiden to remove such blacklist keys. >>>>>> >>>>>> Update blacklist keyring, blacklist key and revoked certificate access rights: >>>>>> * allows the root user to search for a specific blacklisted hash, which >>>>>> make sense because the descriptions are already viewable; >>>>>> * forbids key update (blacklist and asymmetric ones); >>>>>> * restricts kernel rights on the blacklist keyring to align with the >>>>>> root user rights. >>>>>> >>>>>> See help in tools/certs/print-cert-tbs-hash.sh . >>>>>> >>>>>> Cc: David Howells <dhowells@redhat.com> >>>>>> Cc: David Woodhouse <dwmw2@infradead.org> >>>>>> Cc: Eric Snowberg <eric.snowberg@oracle.com> >>>>>> Cc: Jarkko Sakkinen <jarkko@kernel.org> >>>>>> Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com> >>>>>> Link: https://lore.kernel.org/r/20210712170313.884724-6-mic@digikod.net >>>>>> --- >>>>>> >>>>>> Changes since v6: >>>>>> * Rebase on keys-cve-2020-26541-v3: commit ebd9c2ae369a ("integrity: >>>>>> Load mokx variables into the blacklist keyring"). >>>>>> >>>>>> Changes since v5: >>>>>> * Rebase on keys-next, fix Kconfig conflict, and update the asymmetric >>>>>> key rights added to the blacklist keyring by the new >>>>>> add_key_to_revocation_list(): align with blacklist key rights by >>>>>> removing KEY_POS_WRITE as a safeguard, and add >>>>>> KEY_ALLOC_BYPASS_RESTRICTION to not be subject to >>>>>> restrict_link_for_blacklist() that only allows blacklist key types to >>>>>> be added to the keyring. >>>>>> * Change the return code for restrict_link_for_blacklist() from -EPERM >>>>>> to -EOPNOTSUPP to align with asymmetric key keyrings. >>>>>> >>>>>> Changes since v3: >>>>>> * Update commit message for print-cert-tbs-hash.sh . >>>>>> >>>>>> Changes since v2: >>>>>> * Add comment for blacklist_key_instantiate(). >>>>>> --- >>>>>> certs/Kconfig | 10 +++++ >>>>>> certs/blacklist.c | 96 ++++++++++++++++++++++++++++++++++++----------- >>>>>> 2 files changed, 85 insertions(+), 21 deletions(-) >>>>>> >>>>>> diff --git a/certs/Kconfig b/certs/Kconfig >>>>>> index 0fbe184ceca5..e0e524b7eff9 100644 >>>>>> --- a/certs/Kconfig >>>>>> +++ b/certs/Kconfig >>>>>> @@ -103,4 +103,14 @@ config SYSTEM_REVOCATION_KEYS >>>>>> containing X.509 certificates to be included in the default blacklist >>>>>> keyring. >>>>>> +config SYSTEM_BLACKLIST_AUTH_UPDATE >>>>>> + bool "Allow root to add signed blacklist keys" >>>>>> + depends on SYSTEM_BLACKLIST_KEYRING >>>>>> + depends on SYSTEM_DATA_VERIFICATION >>>>>> + help >>>>>> + If set, provide the ability to load new blacklist keys at run time if >>>>>> + they are signed and vouched by a certificate from the builtin trusted >>>>>> + keyring. The PKCS#7 signature of the description is set in the key >>>>>> + payload. Blacklist keys cannot be removed. >>>>>> + >>>>>> endmenu >>>>>> diff --git a/certs/blacklist.c b/certs/blacklist.c >>>>>> index b254c87ceb3a..486ce0dd8e9c 100644 >>>>>> --- a/certs/blacklist.c >>>>>> +++ b/certs/blacklist.c >>>>>> @@ -15,6 +15,7 @@ >>>>>> #include <linux/err.h> >>>>>> #include <linux/seq_file.h> >>>>>> #include <linux/uidgid.h> >>>>>> +#include <linux/verification.h> >>>>>> #include <keys/system_keyring.h> >>>>>> #include "blacklist.h" >>>>>> #include "common.h" >>>>>> @@ -26,6 +27,9 @@ >>>>>> */ >>>>>> #define MAX_HASH_LEN 128 >>>>>> +#define BLACKLIST_KEY_PERM (KEY_POS_SEARCH | KEY_POS_VIEW | \ >>>>>> + KEY_USR_SEARCH | KEY_USR_VIEW) >>>>>> + >>>>>> static const char tbs_prefix[] = "tbs"; >>>>>> static const char bin_prefix[] = "bin"; >>>>>> @@ -80,19 +84,51 @@ static int blacklist_vet_description(const char *desc) >>>>>> return 0; >>>>>> } >>>>>> -/* >>>>>> - * The hash to be blacklisted is expected to be in the description. There will >>>>>> - * be no payload. >>>>>> - */ >>>>>> -static int blacklist_preparse(struct key_preparsed_payload *prep) >>>>>> +static int blacklist_key_instantiate(struct key *key, >>>>>> + struct key_preparsed_payload *prep) >>>>>> { >>>>>> - if (prep->datalen > 0) >>>>>> - return -EINVAL; >>>>>> - return 0; >>>>>> +#ifdef CONFIG_SYSTEM_BLACKLIST_AUTH_UPDATE >>>>>> + int err; >>>>>> +#endif >>>>>> + >>>>>> + /* Sets safe default permissions for keys loaded by user space. */ >>>>>> + key->perm = BLACKLIST_KEY_PERM; >>>>>> + >>>>>> + /* >>>>>> + * Skips the authentication step for builtin hashes, they are not >>>>>> + * signed but still trusted. >>>>>> + */ >>>>>> + if (key->flags & (1 << KEY_FLAG_BUILTIN)) >>>>>> + goto out; >>>>>> + >>>>>> +#ifdef CONFIG_SYSTEM_BLACKLIST_AUTH_UPDATE >>>>>> + /* >>>>>> + * Verifies the description's PKCS#7 signature against the builtin >>>>>> + * trusted keyring. >>>>>> + */ >>>>>> + err = verify_pkcs7_signature(key->description, >>>>>> + strlen(key->description), prep->data, prep->datalen, >>>>>> + NULL, VERIFYING_UNSPECIFIED_SIGNATURE, NULL, NULL); >>>>>> + if (err) >>>>>> + return err; >>>>>> +#else >>>>>> + /* >>>>>> + * It should not be possible to come here because the keyring doesn't >>>>>> + * have KEY_USR_WRITE and the only other way to call this function is >>>>>> + * for builtin hashes. >>>>>> + */ >>>>>> + WARN_ON_ONCE(1); >>>>>> + return -EPERM; >>>>>> +#endif >>>>>> + >>>>>> +out: >>>>>> + return generic_key_instantiate(key, prep); >>>>>> } >>>>>> -static void blacklist_free_preparse(struct key_preparsed_payload *prep) >>>>>> +static int blacklist_key_update(struct key *key, >>>>>> + struct key_preparsed_payload *prep) >>>>>> { >>>>>> + return -EPERM; >>>>>> } >>>>>> static void blacklist_describe(const struct key *key, struct seq_file *m) >>>>>> @@ -103,9 +139,8 @@ static void blacklist_describe(const struct key *key, struct seq_file *m) >>>>>> static struct key_type key_type_blacklist = { >>>>>> .name = "blacklist", >>>>>> .vet_description = blacklist_vet_description, >>>>>> - .preparse = blacklist_preparse, >>>>>> - .free_preparse = blacklist_free_preparse, >>>>>> - .instantiate = generic_key_instantiate, >>>>>> + .instantiate = blacklist_key_instantiate, >>>>>> + .update = blacklist_key_update, >>>>>> .describe = blacklist_describe, >>>>>> }; >>>>>> @@ -154,8 +189,7 @@ static int mark_raw_hash_blacklisted(const char *hash) >>>>>> hash, >>>>>> NULL, >>>>>> 0, >>>>>> - ((KEY_POS_ALL & ~KEY_POS_SETATTR) | >>>>>> - KEY_USR_VIEW), >>>>>> + BLACKLIST_KEY_PERM, >>>>>> KEY_ALLOC_NOT_IN_QUOTA | >>>>>> KEY_ALLOC_BUILT_IN); >>>>>> if (IS_ERR(key)) { >>>>>> @@ -232,8 +266,10 @@ int add_key_to_revocation_list(const char *data, size_t size) >>>>>> NULL, >>>>>> data, >>>>>> size, >>>>>> - ((KEY_POS_ALL & ~KEY_POS_SETATTR) | KEY_USR_VIEW), >>>>>> - KEY_ALLOC_NOT_IN_QUOTA | KEY_ALLOC_BUILT_IN); >>>>>> + KEY_POS_VIEW | KEY_POS_READ | KEY_POS_SEARCH >>>>>> + | KEY_USR_VIEW, >>>>>> + KEY_ALLOC_NOT_IN_QUOTA | KEY_ALLOC_BUILT_IN >>>>>> + | KEY_ALLOC_BYPASS_RESTRICTION); >>>>>> if (IS_ERR(key)) { >>>>>> pr_err("Problem with revocation key (%ld)\n", PTR_ERR(key)); >>>>>> @@ -260,25 +296,43 @@ int is_key_on_revocation_list(struct pkcs7_message *pkcs7) >>>>>> } >>>>>> #endif >>>>>> +static int restrict_link_for_blacklist(struct key *dest_keyring, >>>>>> + const struct key_type *type, const union key_payload *payload, >>>>>> + struct key *restrict_key) >>>>>> +{ >>>>>> + if (type == &key_type_blacklist) >>>>>> + return 0; >>>>>> + return -EOPNOTSUPP; >>>>>> +} >>>>>> + >>>>>> /* >>>>>> * Initialise the blacklist >>>>>> */ >>>>>> static int __init blacklist_init(void) >>>>>> { >>>>>> const char *const *bl; >>>>>> + struct key_restriction *restriction; >>>>>> if (register_key_type(&key_type_blacklist) < 0) >>>>>> panic("Can't allocate system blacklist key type\n"); >>>>>> + restriction = kzalloc(sizeof(*restriction), GFP_KERNEL); >>>>>> + if (!restriction) >>>>>> + panic("Can't allocate blacklist keyring restriction\n"); >>>>> >>>>> >>>>> This prevents me from taking this to my pull request. In moderns standards, >>>>> no new BUG_ON(), panic() etc. should never added to the kernel. >>>>> >>>>> I missed this in my review. >>>>> >>>>> This should rather be e.g. >>>>> >>>>> restriction = kzalloc(sizeof(*restriction), GFP_KERNEL); >>>>> if (!restriction) { >>>>> pr_err("Can't allocate blacklist keyring restriction\n"); >>>>> return 0; >>>>> } >>>>> >>>>> Unfortunately I need to drop this patch set, because adding new panic() >>>>> is simply a no-go. >>>> >>>> I agree that panic() is not great in general, but I followed the other part >>>> of the code (just above) that do the same. This part of the kernel should >>>> failed if critical memory allocation failed at boot time (only). It doesn't >>>> impact the kernel once it is running. I don't think that just ignoring this >>>> error with return 0 is fine, after all it's a critical error right? >>> >>> It's not good reason enough to crash the whole kernel, even if it is a >>> critical error (e.g. run-time foresincs). Even WARN() is not recommended >>> these days [*]. >> >> I think that what Greg said in this email is that WARN*() should only be >> used for cases that should never happen, it is definitely not deprecated, >> but WARN_ON_ONCE() may be a better idea though. WARN*() helps detect such >> thought-to-be-impossible cases, that can happen e.g. with code refactoring. >> >> A lot of initialization/boot code (e.g. without user space nor external >> interactions, mostly __init functions) do panic if there is unexpected and >> unrecoverable errors like failed memory allocations. I think handling such >> errors otherwise would be more complex for no benefit. Moreover, delegating >> such error handling to user space could create new (silent) issues. > > To crash the whole kernel, you should be able to clearly explain why it > makes sense in the situation.
If there is no enough memory to allocate 24 bytes (struct key_restriction) during early boot, I really doubt the kernel can do much.
> >>> >>> For the existing panic()-statements: I'm happy to review patches that >>> render them out. > >>> Not sure tho, if this fails should it be then "everything blacklisted". >>> Just one thing to consider. >> >> Well, if it fail it will be "nothing will work afterwards". Do you have a >> working and useful scenario for this kind of error? > > So you have zero chances to get a shell without blacklist just to do > kernel forensics?
Right, I don't think the kernel can launch any process (nor continue early boot) if it cannot allocate 24 bytes.
| |