[RFC PATCH 0/3] ima: use the system blacklist in lieu of the IMA blacklist keyring

[RFC PATCH 0/3] ima: use the system blacklist in lieu of the IMA blacklist keyring

[Mimi Zohar]

The experimental IMA blacklist keyring accepts any certificate signed by
a key on the builtin trusted keyring.  The certificate being revoked was
created by such a key, so root can add it to the blacklist.  Nothing
differentiates the authority to sign from the authority to revoke.

The system blacklist already supports signed updates
(CONFIG_SYSTEM_BLACKLIST_AUTH_UPDATE), but cannot revoke a key that is
already loaded on a keyring.

This series defines "skid:<hex>" entries and adds is_key_blacklisted()
to the system blacklist, has IMA's asymmetric_verify() call it on the
key it finds, and removes the IMA blacklist keyring.

 1/3 certs: blacklist: define skid: entries and add is_key_blacklisted()
 2/3 integrity: use is_key_blacklisted() in asymmetric_verify()
 3/3 ima: remove the experimental IMA blacklist keyring

Revocations for keys on the IMA keyring are now "skid:<hex>" entries in
the system blacklist.

Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>

Mimi Zohar (3):
  certs: blacklist: define skid: entries and add is_key_blacklisted()
  integrity: use is_key_blacklisted() in asymmetric_verify()
  ima: remove the experimental IMA blacklist keyring

 certs/Kconfig                          |  7 ++-
 certs/blacklist.c                      | 81 ++++++++++++++++++--------
 include/keys/system_keyring.h          | 22 +++----
 security/integrity/digsig_asymmetric.c | 25 ++++----
 security/integrity/ima/Kconfig         | 13 +----
 security/integrity/ima/Makefile        |  1 -
 security/integrity/ima/ima_mok.c       | 49 ----------------
 7 files changed, 82 insertions(+), 116 deletions(-)
 delete mode 100644 security/integrity/ima/ima_mok.c

--
2.56.0

[RFC PATCH 1/3] certs: blacklist: define skid: entries and add is_key_blacklisted()

[Mimi Zohar]

A "tbs:" blacklist entry is only compared against certificates being
added to a keyring or presented in a signature chain, so it does not
stop a key that is already loaded from being used as a trust anchor.

Define a new "skid:<hex>" blacklist entry, holding a Subject Key
Identifier, and add is_key_blacklisted(), which looks up the SKID of an
asymmetric key in the system blacklist.  Entries are added like "tbs:"
and "bin:" entries and are subject to CONFIG_SYSTEM_BLACKLIST_AUTH_UPDATE.

blacklist_vet_description() now uses a prefix table, since the prefixes
no longer all have the same length.  "tbs:" and "bin:" entries behave
as before.

For example, to add an entry for a certificate (the signed string must
not include a trailing newline):

  skid=$(openssl x509 -in cert.pem -noout -ext subjectKeyIdentifier |
         sed -n '2s/[ :]//gp' | tr A-F a-f)
  printf 'skid:%s' "$skid" > skid.txt
  openssl smime -sign -binary -noattr -in skid.txt -signer signer.crt \
          -inkey signer.key -outform DER -out skid.p7s
  keyctl padd blacklist "skid:$skid" %:.blacklist < skid.p7s

The SKID is also the hex string in the description of the key on a
keyring, and its last four bytes are the key ID in the security.ima
xattr, so an entry is easy to correlate with the key it revokes:

  # keyctl show %:.ima | grep 073d
  927946571 --als--v  0  0   \_ asymmetric: test signing key: b7809c3372a07fe87248dcec7ff047a40fb3073d
  # keyctl show %:.blacklist | grep skid
  1014808518 ----s--v 0  0   \_ blacklist: skid:b7809c3372a07fe87248dcec7ff047a40fb3073d

Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
---
 certs/Kconfig                 |  7 +--
 certs/blacklist.c             | 81 ++++++++++++++++++++++++-----------
 include/keys/system_keyring.h |  8 ++++
 3 files changed, 68 insertions(+), 28 deletions(-)

diff --git a/certs/Kconfig b/certs/Kconfig
index 9d2bf7fb5b9e..9e51f988ba3b 100644
--- a/certs/Kconfig
+++ b/certs/Kconfig
@@ -155,10 +155,11 @@ config SYSTEM_BLACKLIST_HASH_LIST
 	  If set, this option should be the filename of a list of hashes in the
 	  form "<hash>", "<hash>", ... .  This will be included into a C
 	  wrapper to incorporate the list into the kernel.  Each <hash> must be a
-	  string starting with a prefix ("tbs" or "bin"), then a colon (":"), and
-	  finally an even number of hexadecimal lowercase characters (up to 128).
+	  string starting with a prefix ("tbs", "bin" or "skid"), then a colon
+	  (":"), and finally an even number of hexadecimal lowercase characters
+	  (up to 128).
 	  Certificate hashes can be generated with
-	  tools/certs/print-cert-tbs-hash.sh .
+	  tools/certs/print-cert-tbs-hash.sh.
 
 config SYSTEM_REVOCATION_LIST
 	bool "Provide system-wide ring of revocation certificates"
diff --git a/certs/blacklist.c b/certs/blacklist.c
index fa561c12eabf..b1e2c178d378 100644
--- a/certs/blacklist.c
+++ b/certs/blacklist.c
@@ -32,6 +32,7 @@
 
 static const char tbs_prefix[] = "tbs";
 static const char bin_prefix[] = "bin";
+static const char skid_prefix[] = "skid";
 
 static struct key *blacklist_keyring;
 
@@ -40,36 +41,30 @@ extern __initconst const u8 revocation_certificate_list[];
 extern __initconst const unsigned long revocation_certificate_list_size;
 #endif
 
+static bool blacklist_prefix_valid(const char *desc, size_t len)
+{
+	return (len == sizeof(tbs_prefix) - 1 &&
+		!strncmp(desc, tbs_prefix, len)) ||
+	       (len == sizeof(bin_prefix) - 1 &&
+		!strncmp(desc, bin_prefix, len)) ||
+	       (len == sizeof(skid_prefix) - 1 &&
+		!strncmp(desc, skid_prefix, len));
+}
+
 /*
- * The description must be a type prefix, a colon and then an even number of
- * hex digits.  The hash is kept in the description.
+ * The description must be a type prefix ("tbs", "bin" or "skid"), a colon and
+ * then an even number of lower-case hex digits.  The hash (or, for "skid", the
+ * Subject Key Identifier) is kept in the description.
  */
 static int blacklist_vet_description(const char *desc)
 {
-	int i, prefix_len, tbs_step = 0, bin_step = 0;
-
-	/* The following algorithm only works if prefix lengths match. */
-	BUILD_BUG_ON(sizeof(tbs_prefix) != sizeof(bin_prefix));
-	prefix_len = sizeof(tbs_prefix) - 1;
-	for (i = 0; *desc; desc++, i++) {
-		if (*desc == ':') {
-			if (tbs_step == prefix_len)
-				goto found_colon;
-			if (bin_step == prefix_len)
-				goto found_colon;
-			return -EINVAL;
-		}
-		if (i >= prefix_len)
-			return -EINVAL;
-		if (*desc == tbs_prefix[i])
-			tbs_step++;
-		if (*desc == bin_prefix[i])
-			bin_step++;
-	}
-	return -EINVAL;
+	const char *colon = strchr(desc, ':');
+	int i;
 
-found_colon:
-	desc++;
+	if (!colon || !blacklist_prefix_valid(desc, colon - desc))
+		return -EINVAL;
+
+	desc = colon + 1;
 	for (i = 0; *desc && i < MAX_HASH_LEN; desc++, i++) {
 		if (!isxdigit(*desc) || isupper(*desc))
 			return -EINVAL;
@@ -160,6 +155,10 @@ static char *get_raw_hash(const u8 *hash, size_t hash_len,
 		type_len = sizeof(bin_prefix) - 1;
 		type_prefix = bin_prefix;
 		break;
+	case BLACKLIST_HASH_SKID:
+		type_len = sizeof(skid_prefix) - 1;
+		type_prefix = skid_prefix;
+		break;
 	default:
 		WARN_ON_ONCE(1);
 		return ERR_PTR(-EINVAL);
@@ -244,6 +243,38 @@ int is_hash_blacklisted(const u8 *hash, size_t hash_len,
 }
 EXPORT_SYMBOL_GPL(is_hash_blacklisted);
 
+/**
+ * is_key_blacklisted - Determine if a key is blacklisted by its SKID
+ * @key: The key to be checked, e.g. a trust anchor found in a keyring
+ *
+ * Unlike a "tbs:" entry, which is only compared against certificates being
+ * added or presented in a chain, this checks a key that is already loaded, so
+ * that keys that cannot be unlinked from their keyring can still be revoked.
+ * The identifier is taken from the key itself, not from the caller.
+ *
+ * Returns -EKEYREJECTED if the key's Subject Key Identifier is blacklisted,
+ * 0 if it is not (or the key has no SKID), or another negative error.
+ */
+int is_key_blacklisted(const struct key *key)
+{
+	const struct asymmetric_key_ids *kids;
+	const struct asymmetric_key_id *skid;
+
+	if (!key || key->type != &key_type_asymmetric)
+		return 0;
+
+	kids = asymmetric_key_ids(key);
+	if (!kids)
+		return 0;
+
+	skid = kids->id[1];
+	if (!skid || !skid->len)
+		return 0;
+
+	return is_hash_blacklisted(skid->data, skid->len, BLACKLIST_HASH_SKID);
+}
+EXPORT_SYMBOL_GPL(is_key_blacklisted);
+
 int is_binary_blacklisted(const u8 *hash, size_t hash_len)
 {
 	if (is_hash_blacklisted(hash, hash_len, BLACKLIST_HASH_BINARY) ==
diff --git a/include/keys/system_keyring.h b/include/keys/system_keyring.h
index a6c2897bcc63..48c830850b70 100644
--- a/include/keys/system_keyring.h
+++ b/include/keys/system_keyring.h
@@ -15,6 +15,8 @@ enum blacklist_hash_type {
 	BLACKLIST_HASH_X509_TBS = 1,
 	/* Raw data hash */
 	BLACKLIST_HASH_BINARY = 2,
+	/* Subject Key Identifier of a key (not a hash) */
+	BLACKLIST_HASH_SKID = 3,
 };
 
 #ifdef CONFIG_SYSTEM_TRUSTED_KEYRING
@@ -79,6 +81,7 @@ extern int mark_hash_blacklisted(const u8 *hash, size_t hash_len,
 extern int is_hash_blacklisted(const u8 *hash, size_t hash_len,
 			       enum blacklist_hash_type hash_type);
 extern int is_binary_blacklisted(const u8 *hash, size_t hash_len);
+extern int is_key_blacklisted(const struct key *key);
 #else
 static inline int is_hash_blacklisted(const u8 *hash, size_t hash_len,
 				      enum blacklist_hash_type hash_type)
@@ -90,6 +93,11 @@ static inline int is_binary_blacklisted(const u8 *hash, size_t hash_len)
 {
 	return 0;
 }
+
+static inline int is_key_blacklisted(const struct key *key)
+{
+	return 0;
+}
 #endif
 
 struct pkcs7_message;
-- 
2.56.0

[RFC PATCH 2/3] integrity: use is_key_blacklisted() in asymmetric_verify()

[Mimi Zohar]

request_asymmetric_key() checks the key ID against the experimental IMA
blacklist keyring before looking up the key.  The key ID in the
security.ima xattr is only an abbreviated (32-bit) form of the SKID, so
the key must be looked up first to obtain its complete SKID.

Look the key up, then pass the key found to is_key_blacklisted(), which
compares its complete SKID against the system blacklist.  A blacklisted
key, or any error from the check, rejects the key.

Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
---
 security/integrity/digsig_asymmetric.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/security/integrity/digsig_asymmetric.c b/security/integrity/digsig_asymmetric.c
index b4c23a0ed68f..731f3b0009ab 100644
--- a/security/integrity/digsig_asymmetric.c
+++ b/security/integrity/digsig_asymmetric.c
@@ -23,23 +23,12 @@ static struct key *request_asymmetric_key(struct key *keyring, uint32_t keyid)
 {
 	struct key *key;
 	char name[12];
+	int ret;
 
 	sprintf(name, "id:%08x", keyid);
 
 	pr_debug("key search: \"%s\"\n", name);
 
-	key = get_ima_blacklist_keyring();
-	if (key) {
-		key_ref_t kref;
-
-		kref = keyring_search(make_key_ref(key, 1),
-				      &key_type_asymmetric, name, true);
-		if (!IS_ERR(kref)) {
-			pr_err("Key '%s' is in ima_blacklist_keyring\n", name);
-			return ERR_PTR(-EKEYREJECTED);
-		}
-	}
-
 	if (keyring) {
 		/* search in specific keyring */
 		key_ref_t kref;
@@ -74,6 +63,18 @@ static struct key *request_asymmetric_key(struct key *keyring, uint32_t keyid)
 		}
 	}
 
+	/*
+	 * Check the key actually selected by the lookup against the system
+	 * blacklist, so that a revoked key stops verifying even if it cannot be
+	 * removed from the keyring.  Fail closed on any error.
+	 */
+	ret = is_key_blacklisted(key);
+	if (ret) {
+		pr_err("Key '%s' is blacklisted\n", name);
+		key_put(key);
+		return ERR_PTR(ret);
+	}
+
 	pr_debug("%s() = 0 [%x]\n", __func__, key_serial(key));
 
 	return key;
-- 
2.56.0

[RFC PATCH 3/3] ima: remove the experimental IMA blacklist keyring

[Mimi Zohar]

The experimental IMA blacklist keyring accepts any certificate signed by
a key on the builtin trusted keyring.  Since every certificate issued by
such a key qualifies, root can add any of them, including legitimate
ones.  Nothing differentiates the authority to sign from the authority
to revoke.

Remove CONFIG_IMA_BLACKLIST_KEYRING, the ".ima_blacklist" keyring in
ima_mok.c, get_ima_blacklist_keyring() and the related Kconfig help
text.

Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
---
 include/keys/system_keyring.h    | 14 ---------
 security/integrity/ima/Kconfig   | 13 +--------
 security/integrity/ima/Makefile  |  1 -
 security/integrity/ima/ima_mok.c | 49 --------------------------------
 4 files changed, 1 insertion(+), 76 deletions(-)
 delete mode 100644 security/integrity/ima/ima_mok.c

diff --git a/include/keys/system_keyring.h b/include/keys/system_keyring.h
index 48c830850b70..28695f96c091 100644
--- a/include/keys/system_keyring.h
+++ b/include/keys/system_keyring.h
@@ -115,20 +115,6 @@ static inline int is_key_on_revocation_list(struct pkcs7_message *pkcs7)
 }
 #endif
 
-#ifdef CONFIG_IMA_BLACKLIST_KEYRING
-extern struct key *ima_blacklist_keyring;
-
-static inline struct key *get_ima_blacklist_keyring(void)
-{
-	return ima_blacklist_keyring;
-}
-#else
-static inline struct key *get_ima_blacklist_keyring(void)
-{
-	return NULL;
-}
-#endif /* CONFIG_IMA_BLACKLIST_KEYRING */
-
 #if defined(CONFIG_INTEGRITY_PLATFORM_KEYRING) && \
 	defined(CONFIG_SYSTEM_TRUSTED_KEYRING)
 extern void __init set_platform_trusted_keys(struct key *keyring);
diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
index 72654cf797cd..fc42ddd34385 100644
--- a/security/integrity/ima/Kconfig
+++ b/security/integrity/ima/Kconfig
@@ -255,7 +255,7 @@ config IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY
 	select INTEGRITY_TRUSTED_KEYRING
 	default n
 	help
-	  Keys may be added to the IMA or IMA blacklist keyrings, if the
+	  Keys may be added to the IMA keyring, if the
 	  key is validly signed by a CA cert in the system built-in,
 	  machine (if configured), or secondary trusted keyrings. The
 	  key must also have the digitalSignature usage set.
@@ -265,17 +265,6 @@ config IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY
 	  provided they are validly signed by a key already resident in the
 	  built-in, machine (if configured) or secondary trusted keyrings.
 
-config IMA_BLACKLIST_KEYRING
-	bool "Create IMA machine owner blacklist keyrings (EXPERIMENTAL)"
-	depends on SYSTEM_TRUSTED_KEYRING
-	depends on INTEGRITY_TRUSTED_KEYRING
-	default n
-	help
-	   This option creates an IMA blacklist keyring, which contains all
-	   revoked IMA keys.  It is consulted before any other keyring.  If
-	   the search is successful the requested operation is rejected and
-	   an error is returned to the caller.
-
 config IMA_LOAD_X509
 	bool "Load X509 certificate onto the '.ima' trusted keyring"
 	depends on INTEGRITY_TRUSTED_KEYRING
diff --git a/security/integrity/ima/Makefile b/security/integrity/ima/Makefile
index b376d38b4ee6..ad5840ba085e 100644
--- a/security/integrity/ima/Makefile
+++ b/security/integrity/ima/Makefile
@@ -11,7 +11,6 @@ ima-y := ima_fs.o ima_queue.o ima_init.o ima_main.o ima_crypto.o ima_api.o \
 ima-$(CONFIG_IMA_APPRAISE) += ima_appraise.o
 ima-$(CONFIG_IMA_APPRAISE_MODSIG) += ima_modsig.o
 ima-$(CONFIG_HAVE_IMA_KEXEC) += ima_kexec.o
-ima-$(CONFIG_IMA_BLACKLIST_KEYRING) += ima_mok.o
 ima-$(CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS) += ima_asymmetric_keys.o
 ima-$(CONFIG_IMA_QUEUE_EARLY_BOOT_KEYS) += ima_queue_keys.o
 
diff --git a/security/integrity/ima/ima_mok.c b/security/integrity/ima/ima_mok.c
deleted file mode 100644
index 14d93d573a6a..000000000000
--- a/security/integrity/ima/ima_mok.c
+++ /dev/null
@@ -1,49 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0-only
-/*
- * Copyright (C) 2015 Juniper Networks, Inc.
- *
- * Author:
- * Petko Manolov <petko.manolov@konsulko.com>
- */
-
-#include <linux/export.h>
-#include <linux/kernel.h>
-#include <linux/sched.h>
-#include <linux/cred.h>
-#include <linux/err.h>
-#include <linux/init.h>
-#include <linux/slab.h>
-#include <keys/system_keyring.h>
-
-
-struct key *ima_blacklist_keyring;
-
-/*
- * Allocate the IMA blacklist keyring
- */
-static __init int ima_mok_init(void)
-{
-	struct key_restriction *restriction;
-
-	pr_notice("Allocating IMA blacklist keyring.\n");
-
-	restriction = kzalloc_obj(struct key_restriction);
-	if (!restriction)
-		panic("Can't allocate IMA blacklist restriction.");
-
-	restriction->check = restrict_link_by_builtin_trusted;
-
-	ima_blacklist_keyring = keyring_alloc(".ima_blacklist",
-				KUIDT_INIT(0), KGIDT_INIT(0), current_cred(),
-				(KEY_POS_ALL & ~KEY_POS_SETATTR) |
-				KEY_USR_VIEW | KEY_USR_READ |
-				KEY_USR_WRITE | KEY_USR_SEARCH,
-				KEY_ALLOC_NOT_IN_QUOTA |
-				KEY_ALLOC_SET_KEEP,
-				restriction, NULL);
-
-	if (IS_ERR(ima_blacklist_keyring))
-		panic("Can't allocate IMA blacklist keyring.");
-	return 0;
-}
-device_initcall(ima_mok_init);
-- 
2.56.0

Re: [RFC PATCH 1/3] certs: blacklist: define skid: entries and add is_key_blacklisted()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] certs: blacklist: check-blacklist-hashes.awk rejects skid: prefix
- [Medium] certs: blacklist: undefined key_type_asymmetric in is_key_blacklisted()
--

commit 28646d9ec2c355c55a1dbd687d050b270243210e
Author: Mimi Zohar <zohar@linux.ibm.com>

certs: blacklist: define skid: entries and add is_key_blacklisted()

This commit introduces a new "skid:<hex>" blacklist entry type for revoking
already-loaded asymmetric keys by their Subject Key Identifier and adds
is_key_blacklisted() to query the system blacklist for a key's SKID.

> diff --git a/certs/Kconfig b/certs/Kconfig
> index 9d2bf7fb5b9e4..9e51f988ba3bd 100644
> --- a/certs/Kconfig
> +++ b/certs/Kconfig
> @@ -155,10 +155,11 @@ config SYSTEM_BLACKLIST_HASH_LIST
>  	  If set, this option should be the filename of a list of hashes in the
>  	  form "<hash>", "<hash>", ... .  This will be included into a C
>  	  wrapper to incorporate the list into the kernel.  Each <hash> must be a
> -	  string starting with a prefix ("tbs" or "bin"), then a colon (":"), and
> -	  finally an even number of hexadecimal lowercase characters (up to 128).
> +	  string starting with a prefix ("tbs", "bin" or "skid"), then a colon
> +	  (":"), and finally an even number of hexadecimal lowercase characters
> +	  (up to 128).

[Severity: Medium]
When CONFIG_SYSTEM_BLACKLIST_HASH_LIST is set, certs/Makefile runs
certs/check-blacklist-hashes.awk via cmd_check_and_copy_blacklist_hash_list
to validate the hash list file during the build:

certs/check-blacklist-hashes.awk:
	if (!match(part1[1], "^(tbs|bin):(.*)$", part2)) {
		print "Unknown prefix (item " NR "):", part1[1];
		exit 1;
	}

Since certs/check-blacklist-hashes.awk only allows tbs and bin prefixes,
won't any skid:<hex> entry in the file configured by
CONFIG_SYSTEM_BLACKLIST_HASH_LIST be rejected with an "Unknown prefix" error
and fail the kernel build?

> diff --git a/certs/blacklist.c b/certs/blacklist.c
> index fa561c12eabfe..b1e2c178d3782 100644
> --- a/certs/blacklist.c
> +++ b/certs/blacklist.c

[ ... ]

> @@ -244,6 +243,38 @@ int is_hash_blacklisted(const u8 *hash, size_t hash_len,
>  }
>  EXPORT_SYMBOL_GPL(is_hash_blacklisted);
>  
> +/**
> + * is_key_blacklisted - Determine if a key is blacklisted by its SKID
> + * @key: The key to be checked, e.g. a trust anchor found in a keyring
> + *
> + * Unlike a "tbs:" entry, which is only compared against certificates being
> + * added or presented in a chain, this checks a key that is already loaded, so
> + * that keys that cannot be unlinked from their keyring can still be revoked.
> + * The identifier is taken from the key itself, not from the caller.
> + *
> + * Returns -EKEYREJECTED if the key's Subject Key Identifier is blacklisted,
> + * 0 if it is not (or the key has no SKID), or another negative error.
> + */
> +int is_key_blacklisted(const struct key *key)
> +{
> +	const struct asymmetric_key_ids *kids;
> +	const struct asymmetric_key_id *skid;
> +
> +	if (!key || key->type != &key_type_asymmetric)
> +		return 0;

[Severity: Medium]
In certs/Kconfig, SYSTEM_BLACKLIST_KEYRING only depends on KEYS and does not
depend on or select ASYMMETRIC_KEY_TYPE:

certs/Kconfig:
config SYSTEM_BLACKLIST_KEYRING
	bool "Provide system-wide ring of blacklisted keys"
	depends on KEYS

However, key_type_asymmetric is defined in
crypto/asymmetric_keys/asymmetric_type.c, which is only built when
CONFIG_ASYMMETRIC_KEY_TYPE is enabled.

If the kernel is built with CONFIG_SYSTEM_BLACKLIST_KEYRING=y and
CONFIG_ASYMMETRIC_KEY_TYPE=n, won't this unconditional reference to
key_type_asymmetric in is_key_blacklisted() cause a link failure?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105121227.4089147-1-zohar@linux.ibm.com?part=1