linux-integrity.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Mickaël Salaün" <mic@digikod.net>
To: Jarkko Sakkinen <jarkko@kernel.org>
Cc: "David Howells" <dhowells@redhat.com>,
	"David Woodhouse" <dwmw2@infradead.org>,
	"David S . Miller" <davem@davemloft.net>,
	"Eric Snowberg" <eric.snowberg@oracle.com>,
	"Herbert Xu" <herbert@gondor.apana.org.au>,
	"James Morris" <jmorris@namei.org>,
	"Mickaël Salaün" <mic@linux.microsoft.com>,
	"Mimi Zohar" <zohar@linux.ibm.com>,
	"Serge E . Hallyn" <serge@hallyn.com>,
	"Tyler Hicks" <tyhicks@linux.microsoft.com>,
	keyrings@vger.kernel.org, linux-crypto@vger.kernel.org,
	linux-integrity@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-security-module@vger.kernel.org
Subject: Re: [PATCH v8 5/5] certs: Allow root user to append signed hashes to the blacklist keyring
Date: Tue, 8 Mar 2022 13:18:28 +0100	[thread overview]
Message-ID: <995fc93b-531b-9840-1523-21ae2adbe4ba@digikod.net> (raw)
In-Reply-To: <YidDznCPSmFmfNwE@iki.fi>


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?

Calling panic() seems OK here. Is there a better way to stop the kernel 
for such critical error? If the kernel cannot allocate memory at this 
time, it would be useless to try continuing booting.

  reply	other threads:[~2022-03-08 12:14 UTC|newest]

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-12 17:03 [PATCH v8 0/5] Enable root to update " Mickaël Salaün
2021-07-12 17:03 ` [PATCH v8 1/5] tools/certs: Add print-cert-tbs-hash.sh Mickaël Salaün
2021-07-12 17:03 ` [PATCH v8 2/5] certs: Check that builtin blacklist hashes are valid Mickaël Salaün
2021-07-12 17:03 ` [PATCH v8 3/5] certs: Make blacklist_vet_description() more strict Mickaël Salaün
2021-07-12 17:03 ` [PATCH v8 4/5] certs: Factor out the blacklist hash creation Mickaël Salaün
2021-07-12 17:03 ` [PATCH v8 5/5] certs: Allow root user to append signed hashes to the blacklist keyring Mickaël Salaün
2022-03-08 11:53   ` Jarkko Sakkinen
2022-03-08 12:18     ` Mickaël Salaün [this message]
2022-03-08 13:19       ` Jarkko Sakkinen
2022-03-08 16:02         ` Mickaël Salaün
2022-03-09 16:01           ` Jarkko Sakkinen
2022-03-09 18:36             ` Mickaël Salaün
2022-03-09 23:11               ` Jarkko Sakkinen
2022-03-11 16:36                 ` Mickaël Salaün
2022-03-11 16:45                   ` Jarkko Sakkinen
2022-03-30 13:44   ` David Howells
2021-12-13 15:30 ` [PATCH v8 0/5] Enable root to update " Mickaël Salaün
2021-12-21  8:50   ` Jarkko Sakkinen
2022-01-04 15:56     ` Mickaël Salaün
2022-01-06 19:12       ` Jarkko Sakkinen
2022-01-06 19:16         ` Jarkko Sakkinen
2022-01-07 12:14           ` Mickaël Salaün
2022-01-31 11:33             ` Mickaël Salaün
2022-02-17 19:58               ` Jarkko Sakkinen
2022-02-19 11:42                 ` Jarkko Sakkinen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=995fc93b-531b-9840-1523-21ae2adbe4ba@digikod.net \
    --to=mic@digikod.net \
    --cc=davem@davemloft.net \
    --cc=dhowells@redhat.com \
    --cc=dwmw2@infradead.org \
    --cc=eric.snowberg@oracle.com \
    --cc=herbert@gondor.apana.org.au \
    --cc=jarkko@kernel.org \
    --cc=jmorris@namei.org \
    --cc=keyrings@vger.kernel.org \
    --cc=linux-crypto@vger.kernel.org \
    --cc=linux-integrity@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-security-module@vger.kernel.org \
    --cc=mic@linux.microsoft.com \
    --cc=serge@hallyn.com \
    --cc=tyhicks@linux.microsoft.com \
    --cc=zohar@linux.ibm.com \
    --subject='Re: [PATCH v8 5/5] certs: Allow root user to append signed hashes to the blacklist keyring' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).