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

[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/blacklist.c                      | 83 ++++++++++++++++++--------
 include/keys/system_keyring.h          | 26 ++++----
 security/integrity/digsig_asymmetric.c | 25 ++++----
 security/integrity/ima/Kconfig         | 13 +---
 security/integrity/ima/Makefile        |  1 -
 security/integrity/ima/ima_mok.c       | 49 ---------------
 6 files changed, 84 insertions(+), 113 deletions(-)
 delete mode 100644 security/integrity/ima/ima_mok.c

--
2.56.0

[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>
---
Changelog v1:
- Addressed Sashiko issues: drop Kconfig change, linking issues when
blacklist support configured, but without CONFIG_ASYMMETRIC_KEY_TYPE
configured.

 certs/blacklist.c             | 83 ++++++++++++++++++++++++-----------
 include/keys/system_keyring.h | 12 +++++
 2 files changed, 70 insertions(+), 25 deletions(-)

diff --git a/certs/blacklist.c b/certs/blacklist.c
index fa561c12eabf..9158bd9f9fc9 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,40 @@ int is_hash_blacklisted(const u8 *hash, size_t hash_len,
 }
 EXPORT_SYMBOL_GPL(is_hash_blacklisted);

+#ifdef CONFIG_ASYMMETRIC_KEY_TYPE
+/**
+ * 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);
+#endif
+
 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..2ac1dae2063a 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
@@ -92,6 +94,16 @@ static inline int is_binary_blacklisted(const u8 *hash, size_t hash_len)
 }
 #endif

+#if defined(CONFIG_SYSTEM_BLACKLIST_KEYRING) && \
+    defined(CONFIG_ASYMMETRIC_KEY_TYPE)
+extern int is_key_blacklisted(const struct key *key);
+#else
+static inline int is_key_blacklisted(const struct key *key)
+{
+	return 0;
+}
+#endif
+
 struct pkcs7_message;
 #ifdef CONFIG_SYSTEM_REVOCATION_LIST
 extern int add_key_to_revocation_list(const char *data, size_t size);
--
2.56.0

[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

[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 2ac1dae2063a..61e5aba26e24 100644
--- a/include/keys/system_keyring.h
+++ b/include/keys/system_keyring.h
@@ -119,20 +119,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