diff mbox series

[v7,5/5] certs: Allow root user to append signed hashes to the blacklist keyring

Message ID 20210312171232.2681989-6-mic@digikod.net (mailing list archive)
State Not Applicable
Delegated to: Herbert Xu
Headers show
Series Enable root to update the blacklist keyring | expand

Commit Message

Mickaël Salaün March 12, 2021, 5:12 p.m. UTC
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/20210312171232.2681989-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(-)

Comments

Eric Snowberg March 15, 2021, 4:59 p.m. UTC | #1
> On Mar 12, 2021, at 10:12 AM, Mickaël Salaün <mic@digikod.net> 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/20210312171232.2681989-6-mic@digikod.net

I tried testing this, it doesn’t work as I would expect.  
Here is my test setup:

Kernel built with two keys compiled into the builtin_trusted_keys keyring

Generated a tbs cert from one of the keys and signed it with the other key

As root, added the tbs cert hash to the blacklist keyring

Verified the tbs hash is in the blacklist keyring

Enabled lockdown to enforce kernel module signature checking

Signed a kernel module with the key I just blacklisted

Load the kernel module 

I’m seeing the kernel module load, I would expect this to fail, since the 
key is now blacklisted.  Or is this change just supposed to prevent new keys 
from being added in the future?
Mickaël Salaün March 15, 2021, 6:01 p.m. UTC | #2
On 15/03/2021 17:59, Eric Snowberg wrote:
> 
>> On Mar 12, 2021, at 10:12 AM, Mickaël Salaün <mic@digikod.net> 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/20210312171232.2681989-6-mic@digikod.net
> 
> I tried testing this, it doesn’t work as I would expect.  
> Here is my test setup:
> 
> Kernel built with two keys compiled into the builtin_trusted_keys keyring
> 
> Generated a tbs cert from one of the keys and signed it with the other key
> 
> As root, added the tbs cert hash to the blacklist keyring
> 
> Verified the tbs hash is in the blacklist keyring
> 
> Enabled lockdown to enforce kernel module signature checking
> 
> Signed a kernel module with the key I just blacklisted
> 
> Load the kernel module 
> 
> I’m seeing the kernel module load, I would expect this to fail, since the 
> key is now blacklisted.  Or is this change just supposed to prevent new keys 
> from being added in the future?

This is the expected behavior and the way the blacklist keyring is
currently used, as explained in the commit message:
"This enables to invalidate new certificates, either from being loaded
in a keyring, or from being trusted in a PKCS#7 certificate chain."

If you want a (trusted root) key to be untrusted, you need to remove it
from the keyring, which is not allowed for the builtin trusted keyring.
Eric Snowberg March 17, 2021, 2:48 p.m. UTC | #3
> On Mar 15, 2021, at 12:01 PM, Mickaël Salaün <mic@digikod.net> wrote:
> 
> 
> On 15/03/2021 17:59, Eric Snowberg wrote:
>> 
>>> On Mar 12, 2021, at 10:12 AM, Mickaël Salaün <mic@digikod.net> 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/20210312171232.2681989-6-mic@digikod.net
>> 
>> I tried testing this, it doesn’t work as I would expect.  
>> Here is my test setup:
>> 
>> Kernel built with two keys compiled into the builtin_trusted_keys keyring
>> 
>> Generated a tbs cert from one of the keys and signed it with the other key
>> 
>> As root, added the tbs cert hash to the blacklist keyring
>> 
>> Verified the tbs hash is in the blacklist keyring
>> 
>> Enabled lockdown to enforce kernel module signature checking
>> 
>> Signed a kernel module with the key I just blacklisted
>> 
>> Load the kernel module 
>> 
>> I’m seeing the kernel module load, I would expect this to fail, since the 
>> key is now blacklisted.  Or is this change just supposed to prevent new keys 
>> from being added in the future?
> 
> This is the expected behavior and the way the blacklist keyring is
> currently used, as explained in the commit message:
> "This enables to invalidate new certificates, either from being loaded
> in a keyring, or from being trusted in a PKCS#7 certificate chain."
> 
> If you want a (trusted root) key to be untrusted, you need to remove it
> from the keyring, which is not allowed for the builtin trusted keyring.

Is there a non technical reason why this can not be changed to also apply to
builtin trusted keys? If a user had the same tbs cert hash in their dbx and 
soon mokx, the hash would show up in the .blacklist keyring and invalidate 
any key in the builtin_trusted_keys keyring. After adding the same hash with 
this series, it shows up in the .blacklist_keyring but the value is ignored 
by operations using the builtin_trusted_keys keyring.  It just seems 
incomplete to me, or did I miss an earlier discussion on this topic?
Mickaël Salaün March 17, 2021, 3:45 p.m. UTC | #4
On 17/03/2021 15:48, Eric Snowberg wrote:
> 
>> On Mar 15, 2021, at 12:01 PM, Mickaël Salaün <mic@digikod.net> wrote:
>>
>>
>> On 15/03/2021 17:59, Eric Snowberg wrote:
>>>
>>>> On Mar 12, 2021, at 10:12 AM, Mickaël Salaün <mic@digikod.net> 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/20210312171232.2681989-6-mic@digikod.net
>>>
>>> I tried testing this, it doesn’t work as I would expect.  
>>> Here is my test setup:
>>>
>>> Kernel built with two keys compiled into the builtin_trusted_keys keyring
>>>
>>> Generated a tbs cert from one of the keys and signed it with the other key
>>>
>>> As root, added the tbs cert hash to the blacklist keyring
>>>
>>> Verified the tbs hash is in the blacklist keyring
>>>
>>> Enabled lockdown to enforce kernel module signature checking
>>>
>>> Signed a kernel module with the key I just blacklisted
>>>
>>> Load the kernel module 
>>>
>>> I’m seeing the kernel module load, I would expect this to fail, since the 
>>> key is now blacklisted.  Or is this change just supposed to prevent new keys 
>>> from being added in the future?
>>
>> This is the expected behavior and the way the blacklist keyring is
>> currently used, as explained in the commit message:
>> "This enables to invalidate new certificates, either from being loaded
>> in a keyring, or from being trusted in a PKCS#7 certificate chain."
>>
>> If you want a (trusted root) key to be untrusted, you need to remove it
>> from the keyring, which is not allowed for the builtin trusted keyring.
> 
> Is there a non technical reason why this can not be changed to also apply to
> builtin trusted keys? If a user had the same tbs cert hash in their dbx and 
> soon mokx, the hash would show up in the .blacklist keyring and invalidate 
> any key in the builtin_trusted_keys keyring. After adding the same hash with 
> this series, it shows up in the .blacklist_keyring but the value is ignored 
> by operations using the builtin_trusted_keys keyring.  It just seems 
> incomplete to me, or did I miss an earlier discussion on this topic?

The purpose of the blacklist keyring is to block new keys from being
loaded in the kernel. For builtin and dbx/mokx hashes, they are loaded
in the blacklist keyring before builtin certificates are loaded in the
trusted builtin keyring, which can reject them if there is a match. I
think that being able to load a blocked hash at run time should not
change the semantic of the blacklist keyring, which is to block the
loading of certificates. If someone wants to change this semantic, I
think it should be configurable. Indeed, we should keep in mind the
temporal dimension and the hierarchy of trust: dbx/mokx -> builtin
hashes -> run time hashes.
diff mbox series

Patch

diff --git a/certs/Kconfig b/certs/Kconfig
index cf3740c1b22b..3aa5e597cfae 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");
+	restriction->check = restrict_link_for_blacklist;
+
 	blacklist_keyring =
 		keyring_alloc(".blacklist",
 			      GLOBAL_ROOT_UID, GLOBAL_ROOT_GID, current_cred(),
-			      (KEY_POS_ALL & ~KEY_POS_SETATTR) |
-			      KEY_USR_VIEW | KEY_USR_READ |
-			      KEY_USR_SEARCH,
-			      KEY_ALLOC_NOT_IN_QUOTA |
+			      KEY_POS_VIEW | KEY_POS_READ | KEY_POS_SEARCH |
+			      KEY_POS_WRITE |
+			      KEY_USR_VIEW | KEY_USR_READ | KEY_USR_SEARCH
+#ifdef CONFIG_SYSTEM_BLACKLIST_AUTH_UPDATE
+			      | KEY_USR_WRITE
+#endif
+			      , KEY_ALLOC_NOT_IN_QUOTA |
 			      KEY_ALLOC_SET_KEEP,
-			      NULL, NULL);
+			      restriction, NULL);
 	if (IS_ERR(blacklist_keyring))
 		panic("Can't allocate system blacklist keyring\n");