[PATCH v0 0/2] KEYS: Measure keys added to builtin or secondary trusted keys keyring

[PATCH v0 0/2] KEYS: Measure keys added to builtin or secondary trusted keys keyring

[Lakshmi Ramasubramanian]

Created using linux v5.3.0

Motive:

Motive behind this patch set is to measure the public keys added to
the trusted keys keyring (builtin or secondary trusted keys keyring).
This feature can be enabled through CONFIG_IMA and through
the IMA Hook TRUSTED_KEYS.

Measurement of the trusted keys is an addition to
the existing IMA measurements and not a replacement for it.

Background:

Currently IMA measures file hashes and .ima signatures. IMA signatures
are validated against keys in ".ima" keyring. If the kernel is built with
CONFIG_IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY enabled,
then all keys in ".ima" keyring must be signed by a key in
".builtin_trusted_keys" or ".secondary_trusted_keys" keyrings.

On systems with CONFIG_IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY
enabled, measuring keys in the  trusted keyring provides a mechanism
to attest that the client's system binaries are indeed signed by signers
that chain to known trusted keys.

Without this patch set, to attest the clients one needs to maintain
an "allowed list" of file hashes of all versions of all client binaries
that are deployed on the clients in the enterprise. That is a huge
operational challenge in a large scale environment of clients with
heterogenous builds. This also limits scalability and agility of
rolling out frequent client binary updates.

Current patch:

This patch set to measure the public keys added to the trusted keys
keyring can be enabled through CONFIG_IMA and through the IMA Hook
TRUSTED_KEYS. A new LSM hook has been added that will be invoked
when a new key is created. If CONFIG_IMA is enabled, the IMA function
to measure the key is called. This function will measure the key if
  => The key is being added to builtin or secondary trusted keys keyring
  => The key is an asymmetric key
  => IMA hook TRUSTED_KEYS is enabled through ima policy.

Questions and concerns raised by reviewers on the patch set:

Question 1:
Is "Signed with a trusted key" equal to "Trusted file"?
Doesn't the service need the hashes of the system files to determine
whether a file is trusted or not?

"Signed with a trusted key" does not equal "Trusted"

Answer:
Agree "Signed with a trusted key" may not equal "Trusted".
To address this, the attesting service can maintain a small
manageable set of bad hashes (a "Blocked list") and a list of
trusted keys expected in client's trusted keys keyring.
Using this data, the service can detect the presence of
"Disallowed (untrusted) version of client binaries".

Question 2:
Providing more data to the service (such as the keys in trusted keyring)
empowers the service to deny access to clients (block clients).
IMA walks a fine line in enforcing and measuring file integrity.
This patchset breaches that fine line and in doing so brings back
the fears of trusted computing.

Answer:
Any new measurement we add in IMA will provide more data to service
and can enable it to deny access to clients. It is not clear why
this patch set would breach the fine line between measuring
and enforcing.

Since this patch set is disabled by default and enabled through
CONFIG_IMA and through the IMA hook TRUSTED_KEYS, only those
enterprises that require this new measurement can opt-in for it.
Since it is disabled by default, it does not restrict the autonomy
of independent users who are unaffected by attestation.

Question 3:
IMA log already contains a pointer to the IMA keys used for signature
verification. Why does the service need to care what keys were used
to sign (install) the IMA keys? What is gained by measuring the keys
in the trusted keyring?

Answer:
To attest the clients using the current IMA log, service needs to maintain
hashes of all the deployed versions of all the system binaries for their
enterprise. This will introduce a very high operational overhead in
a large scale environment of clients with heterogenous builds.
This limits scalability and agility of rolling out frequent client
binary updates.

On the other hand, with the current patch set, we will have IMA
validate the file signature on the clients and the service validate
that the IMA keys were installed using trusted keys.

This provides a chain of trust:
    => IMA Key validates file signature on the client
    => Key in the trusted keyring attests IMA key on the client
    => Attestation service attests the trusted keys
       reported by the client in the IMA log

This approach, therefore, would require the service to maintain
a manageble set of trusted keys that it receives from a trusted source.
And, verify if the clients only have keys from that set of trusted keys.

Question 4:
Where will the attestation service receive the keys to validate against?

Answer:
Attestation service will receive the keys from a trusted source such as
the enterprise build services that provides the client builds.
The service will use this set of keys to verify that the keys reported by
the clients in the IMA log contains only keys from this trusted list.

Question 5:
What is changing in the IMA log through this patch set?

Answer:
This patch set does not remove any data that is currently included
in the IMA log. It only adds more data to the IMA log - the data on
keys in the trusted keyring

Lakshmi Ramasubramanian (2):
  Measure keys added to builtin or secondary trusted keys keyring
  LSM Hook for key_create_or_update

 Documentation/ABI/testing/ima_policy |   1 +
 certs/system_keyring.c               |  14 ++
 include/keys/system_keyring.h        |   2 +
 include/linux/ima.h                  |  10 ++
 include/linux/lsm_hooks.h            |  15 ++
 include/linux/security.h             |  12 ++
 security/integrity/ima/ima.h         |  14 ++
 security/integrity/ima/ima_api.c     |   2 +-
 security/integrity/ima/ima_init.c    |  11 +-
 security/integrity/ima/ima_main.c    | 230 ++++++++++++++++++++++-----
 security/integrity/ima/ima_policy.c  |   4 +-
 security/keys/key.c                  |  12 ++
 security/security.c                  |  14 ++
 13 files changed, 295 insertions(+), 46 deletions(-)

-- 
2.17.1

[PATCH v0 1/2] KEYS: Measure keys added to builtin or secondary trusted keys keyring

[Lakshmi Ramasubramanian]

IMA hook TRUSTED_KEYS to measure keys added to builtin or secondary
trusted keys keyring. This can be enabled through IMA policy.

The key data is queued up if IMA is not yet initialized and measured
when IMA is initialized. If IMA is initialized then the key is
measured immediately.

Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
---
 Documentation/ABI/testing/ima_policy |   1 +
 include/linux/ima.h                  |  10 ++
 security/integrity/ima/ima.h         |  14 ++
 security/integrity/ima/ima_api.c     |   2 +-
 security/integrity/ima/ima_init.c    |  11 +-
 security/integrity/ima/ima_main.c    | 230 ++++++++++++++++++++++-----
 security/integrity/ima/ima_policy.c  |   4 +-
 7 files changed, 226 insertions(+), 46 deletions(-)

diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
index fc376a323908..cc25c0f41d6e 100644
--- a/Documentation/ABI/testing/ima_policy
+++ b/Documentation/ABI/testing/ima_policy
@@ -29,6 +29,7 @@ Description:
 				[FIRMWARE_CHECK]
 				[KEXEC_KERNEL_CHECK] [KEXEC_INITRAMFS_CHECK]
 				[KEXEC_CMDLINE]
+				[TRUSTED_KEYS]
 			mask:= [[^]MAY_READ] [[^]MAY_WRITE] [[^]MAY_APPEND]
 			       [[^]MAY_EXEC]
 			fsmagic:= hex value
diff --git a/include/linux/ima.h b/include/linux/ima.h
index a20ad398d260..eb95743ada7d 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -25,6 +25,9 @@ extern int ima_post_read_file(struct file *file, void *buf, loff_t size,
 extern void ima_post_path_mknod(struct dentry *dentry);
 extern void ima_kexec_cmdline(const void *buf, int size);
 
+extern int ima_post_key_create_or_update(struct key *keyring,
+					 struct key *key,
+					 bool builtin_or_secondary);
 #ifdef CONFIG_IMA_KEXEC
 extern void ima_add_kexec_buffer(struct kimage *image);
 #endif
@@ -91,6 +94,13 @@ static inline void ima_post_path_mknod(struct dentry *dentry)
 }
 
 static inline void ima_kexec_cmdline(const void *buf, int size) {}
+
+static inline int ima_post_key_create_or_update(struct key *keyring,
+						struct key *key,
+						bool builtin_or_secondary)
+{
+	return 0;
+}
 #endif /* CONFIG_IMA */
 
 #ifndef CONFIG_IMA_KEXEC
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 011b91c79351..f0c1801da890 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -21,6 +21,7 @@
 #include <linux/tpm.h>
 #include <linux/audit.h>
 #include <crypto/hash_info.h>
+#include <keys/asymmetric-type.h>
 
 #include "../integrity.h"
 
@@ -52,6 +53,7 @@ extern int ima_policy_flag;
 extern int ima_hash_algo;
 extern int ima_appraise;
 extern struct tpm_chip *ima_tpm_chip;
+extern bool ima_initialized;
 
 /* IMA event related data */
 struct ima_event_data {
@@ -104,6 +106,16 @@ struct ima_queue_entry {
 };
 extern struct list_head ima_measurements;	/* list of all measurements */
 
+/*
+ * To track trusted keys that need to be measured when IMA is initialized.
+ */
+struct ima_trusted_key_entry {
+	struct list_head list;
+	void *public_key;
+	u32  public_key_len;
+	char *key_description;
+};
+
 /* Some details preceding the binary serialized measurement list */
 struct ima_kexec_hdr {
 	u16 version;
@@ -158,6 +170,7 @@ void ima_init_template_list(void);
 int __init ima_init_digests(void);
 int ima_lsm_policy_change(struct notifier_block *nb, unsigned long event,
 			  void *lsm_data);
+void ima_measure_queued_trusted_keys(void);
 
 /*
  * used to protect h_table and sha_table
@@ -189,6 +202,7 @@ static inline unsigned long ima_hash_key(u8 *digest)
 	hook(KEXEC_INITRAMFS_CHECK)	\
 	hook(POLICY_CHECK)		\
 	hook(KEXEC_CMDLINE)		\
+	hook(TRUSTED_KEYS)		\
 	hook(MAX_CHECK)
 #define __ima_hook_enumify(ENUM)	ENUM,
 
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index f614e22bf39f..704a048ff925 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -174,7 +174,7 @@ void ima_add_violation(struct file *file, const unsigned char *filename,
  *		subj=, obj=, type=, func=, mask=, fsmagic=
  *	subj,obj, and type: are LSM specific.
  *	func: FILE_CHECK | BPRM_CHECK | CREDS_CHECK | MMAP_CHECK | MODULE_CHECK
- *	| KEXEC_CMDLINE
+ *	| KEXEC_CMDLINE | TRUSTED_KEYS
  *	mask: contains the permission mask
  *	fsmagic: hex value
  *
diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c
index 5d55ade5f3b9..32b9147fe410 100644
--- a/security/integrity/ima/ima_init.c
+++ b/security/integrity/ima/ima_init.c
@@ -23,6 +23,7 @@
 /* name for boot aggregate entry */
 static const char boot_aggregate_name[] = "boot_aggregate";
 struct tpm_chip *ima_tpm_chip;
+bool ima_initialized;
 
 /* Add the boot aggregate to the IMA measurement list and extend
  * the PCR register.
@@ -131,5 +132,13 @@ int __init ima_init(void)
 
 	ima_init_policy();
 
-	return ima_fs_init();
+	rc = ima_fs_init();
+	if (rc != 0)
+		return rc;
+
+	ima_initialized = true;
+
+	ima_measure_queued_trusted_keys();
+
+	return 0;
 }
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 584019728660..b0ee5c82207a 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -27,6 +27,7 @@
 #include <linux/ima.h>
 #include <linux/iversion.h>
 #include <linux/fs.h>
+#include <crypto/public_key.h>
 
 #include "ima.h"
 
@@ -43,6 +44,13 @@ static struct notifier_block ima_lsm_policy_notifier = {
 	.notifier_call = ima_lsm_policy_change,
 };
 
+/*
+ * Used to synchronize access to the list of trusted keys (ima_trusted_keys)
+ * that need to be measured when IMA is initialized.
+ */
+static DEFINE_MUTEX(ima_trusted_keys_mutex);
+static LIST_HEAD(ima_trusted_keys);
+
 static int __init hash_setup(char *str)
 {
 	struct ima_template_desc *template_desc = ima_template_desc_current();
@@ -351,6 +359,65 @@ static int process_measurement(struct file *file, const struct cred *cred,
 	return 0;
 }
 
+
+/*
+ * process_buffer_measurement - Measure the buffer to ima log.
+ * @buf: pointer to the buffer that needs to be added to the log.
+ * @size: size of buffer(in bytes).
+ * @eventname: event name to be used for the buffer entry.
+ * @func: IMA Hook function
+ * @cred: a pointer to a credentials structure for user validation.
+ * @secid: the secid of the task to be validated.
+ *
+ * Based on policy, the buffer is measured into the ima log.
+ */
+static void process_buffer_measurement(const void *buf, u32 size,
+				       const char *eventname,
+				       enum ima_hooks func,
+				       const struct cred *cred, u32 secid)
+{
+	int ret = 0;
+	struct ima_template_entry *entry = NULL;
+	struct integrity_iint_cache iint = {};
+	struct ima_event_data event_data = {.iint = &iint,
+					    .filename = eventname,
+					    .buf = buf,
+					    .buf_len = size};
+	struct ima_template_desc *template_desc = ima_template_desc_current();
+	struct {
+		struct ima_digest_data hdr;
+		char digest[IMA_MAX_DIGEST_SIZE];
+	} hash = {};
+	int violation = 0;
+	int pcr = CONFIG_IMA_MEASURE_PCR_IDX;
+	int action = 0;
+
+	action = ima_get_action(NULL, cred, secid, 0, func, &pcr,
+				&template_desc);
+	if (!(action & IMA_MEASURE))
+		return;
+
+	iint.ima_hash = &hash.hdr;
+	iint.ima_hash->algo = ima_hash_algo;
+	iint.ima_hash->length = hash_digest_size[ima_hash_algo];
+
+	ret = ima_calc_buffer_hash(buf, size, iint.ima_hash);
+	if (ret < 0)
+		goto out;
+
+	ret = ima_alloc_init_template(&event_data, &entry, template_desc);
+	if (ret < 0)
+		goto out;
+
+	ret = ima_store_template(entry, violation, NULL, buf, pcr);
+
+	if (ret < 0)
+		ima_free_template_entry(entry);
+
+out:
+	return;
+}
+
 /**
  * ima_file_mmap - based on policy, collect/store measurement.
  * @file: pointer to the file to be measured (May be NULL)
@@ -605,60 +672,136 @@ int ima_load_data(enum kernel_load_data_id id)
 	return 0;
 }
 
+static void ima_free_trusted_key_entry(struct ima_trusted_key_entry *entry)
+{
+	if (entry != NULL) {
+		if (entry->public_key != NULL)
+			kzfree(entry->public_key);
+		if (entry->key_description != NULL)
+			kzfree(entry->key_description);
+		kzfree(entry);
+	}
+}
+
+static struct ima_trusted_key_entry *ima_alloc_trusted_queue_entry(
+	struct key *key)
+{
+	int rc = 0;
+	const struct public_key *pk;
+	size_t key_description_len;
+	struct ima_trusted_key_entry *entry = NULL;
+
+	pk = key->payload.data[asym_crypto];
+	key_description_len = strlen(key->description) + 1;
+	entry = kzalloc(sizeof(*entry), GFP_KERNEL);
+	if (entry != NULL) {
+		entry->public_key = kzalloc(pk->keylen, GFP_KERNEL);
+		entry->key_description =
+			kzalloc(key_description_len, GFP_KERNEL);
+	}
+
+	if ((entry == NULL) || (entry->public_key == NULL) ||
+	    (entry->key_description == NULL)) {
+		rc = -ENOMEM;
+		goto out;
+	}
+
+	strcpy(entry->key_description, key->description);
+	memcpy(entry->public_key, pk->key, pk->keylen);
+	entry->public_key_len = pk->keylen;
+	rc = 0;
+
+out:
+	if (rc) {
+		ima_free_trusted_key_entry(entry);
+		entry = NULL;
+	}
+
+	return entry;
+}
+
 /*
- * process_buffer_measurement - Measure the buffer to ima log.
- * @buf: pointer to the buffer that needs to be added to the log.
- * @size: size of buffer(in bytes).
- * @eventname: event name to be used for the buffer entry.
- * @cred: a pointer to a credentials structure for user validation.
- * @secid: the secid of the task to be validated.
+ * ima_post_key_create_or_update
+ *     @keyring points to the keyring to which the key belongs
+ *     @key points to the key being created or updated
+ *     @builtin_or_secondary flag indicating whether
+ *     the keyring to which the key belongs is the builtin
+ *     or secondary trusted keys keyring
+ * Measure keys added to the builtin or secondary trusted keyring
  *
- * Based on policy, the buffer is measured into the ima log.
+ * On success return 0.
+ * Return appropriate error code on error
  */
-static void process_buffer_measurement(const void *buf, int size,
-				       const char *eventname,
-				       const struct cred *cred, u32 secid)
+int ima_post_key_create_or_update(struct key *keyring,
+				  struct key *key,
+				  bool builtin_or_secondary)
 {
-	int ret = 0;
-	struct ima_template_entry *entry = NULL;
-	struct integrity_iint_cache iint = {};
-	struct ima_event_data event_data = {.iint = &iint,
-					    .filename = eventname,
-					    .buf = buf,
-					    .buf_len = size};
-	struct ima_template_desc *template_desc = NULL;
-	struct {
-		struct ima_digest_data hdr;
-		char digest[IMA_MAX_DIGEST_SIZE];
-	} hash = {};
-	int violation = 0;
-	int pcr = CONFIG_IMA_MEASURE_PCR_IDX;
-	int action = 0;
+	int rc = 0;
+	struct ima_trusted_key_entry *entry = NULL;
+	const struct public_key *pk;
+	u32 secid;
+	bool queued = false;
 
-	action = ima_get_action(NULL, cred, secid, 0, KEXEC_CMDLINE, &pcr,
-				&template_desc);
-	if (!(action & IMA_MEASURE))
-		return;
+	/*
+	 * We only measure asymmetric keys added to either
+	 * the builtin or the secondary trusted keys keyring.
+	 */
+	if (!builtin_or_secondary ||
+	    (key->type != &key_type_asymmetric))
+		return 0;
 
-	iint.ima_hash = &hash.hdr;
-	iint.ima_hash->algo = ima_hash_algo;
-	iint.ima_hash->length = hash_digest_size[ima_hash_algo];
+	mutex_lock(&ima_trusted_keys_mutex);
 
-	ret = ima_calc_buffer_hash(buf, size, iint.ima_hash);
-	if (ret < 0)
-		goto out;
+	if (!ima_initialized) {
+		entry = ima_alloc_trusted_queue_entry(key);
+		if (entry != NULL) {
+			INIT_LIST_HEAD(&entry->list);
+			list_add_tail(&entry->list, &ima_trusted_keys);
+			queued = true;
+		} else
+			rc = -ENOMEM;
+	}
 
-	ret = ima_alloc_init_template(&event_data, &entry, template_desc);
-	if (ret < 0)
-		goto out;
+	mutex_unlock(&ima_trusted_keys_mutex);
 
-	ret = ima_store_template(entry, violation, NULL, buf, pcr);
+	if ((rc == 0) && !queued) {
+		security_task_getsecid(current, &secid);
+		pk = key->payload.data[asym_crypto];
+		process_buffer_measurement(pk->key, pk->keylen,
+					   key->description,
+					   TRUSTED_KEYS,
+					   current_cred(), secid);
+	}
 
-	if (ret < 0)
-		ima_free_template_entry(entry);
+	return rc;
+}
 
-out:
-	return;
+void ima_measure_queued_trusted_keys(void)
+{
+	struct ima_trusted_key_entry *entry, *tmp;
+	u32 secid;
+
+	mutex_lock(&ima_trusted_keys_mutex);
+
+	security_task_getsecid(current, &secid);
+	list_for_each_entry_safe(entry, tmp, &ima_trusted_keys, list) {
+		if ((entry != NULL) &&
+		    (entry->public_key != NULL) &&
+		    (entry->key_description != NULL)) {
+
+			process_buffer_measurement(entry->public_key,
+						   entry->public_key_len,
+						   entry->key_description,
+						   TRUSTED_KEYS,
+						   current_cred(),
+						   secid);
+		}
+
+		list_del(&entry->list);
+		ima_free_trusted_key_entry(entry);
+	}
+
+	mutex_unlock(&ima_trusted_keys_mutex);
 }
 
 /**
@@ -675,6 +818,7 @@ void ima_kexec_cmdline(const void *buf, int size)
 	if (buf && size != 0) {
 		security_task_getsecid(current, &secid);
 		process_buffer_measurement(buf, size, "kexec-cmdline",
+					   KEXEC_CMDLINE,
 					   current_cred(), secid);
 	}
 }
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 6df7f641ff66..1752f7bd1717 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -370,7 +370,7 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
 {
 	int i;
 
-	if (func == KEXEC_CMDLINE) {
+	if ((func == KEXEC_CMDLINE) || (func == TRUSTED_KEYS)) {
 		if ((rule->flags & IMA_FUNC) && (rule->func == func))
 			return true;
 		return false;
@@ -959,6 +959,8 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
 				entry->func = POLICY_CHECK;
 			else if (strcmp(args[0].from, "KEXEC_CMDLINE") == 0)
 				entry->func = KEXEC_CMDLINE;
+			else if (strcmp(args[0].from, "TRUSTED_KEYS") == 0)
+				entry->func = TRUSTED_KEYS;
 			else
 				result = -EINVAL;
 			if (!result)
-- 
2.17.1

[PATCH v0 2/2] KEYS: LSM Hook for key_create_or_update

[Lakshmi Ramasubramanian]

LSM Hook to key_create_or_update. The LSM hook will call_int_hook
IMA function ima_post_key_create_or_update.

Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
---
 certs/system_keyring.c        | 14 ++++++++++++++
 include/keys/system_keyring.h |  2 ++
 include/linux/lsm_hooks.h     | 15 +++++++++++++++
 include/linux/security.h      | 12 ++++++++++++
 security/keys/key.c           | 12 ++++++++++++
 security/security.c           | 14 ++++++++++++++
 6 files changed, 69 insertions(+)

diff --git a/certs/system_keyring.c b/certs/system_keyring.c
index 1eba08a1af82..bed19a2f5dd1 100644
--- a/certs/system_keyring.c
+++ b/certs/system_keyring.c
@@ -283,3 +283,17 @@ void __init set_platform_trusted_keys(struct key *keyring)
 	platform_trusted_keys = keyring;
 }
 #endif
+
+bool is_builtin_or_secondary_trusted_keyring(struct key *keyring)
+{
+	bool trustedkeyring;
+
+	trustedkeyring = (keyring == builtin_trusted_keys);
+
+	#ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
+	if (!trustedkeyring)
+		trustedkeyring = (keyring == secondary_trusted_keys);
+	#endif
+
+	return trustedkeyring;
+}
diff --git a/include/keys/system_keyring.h b/include/keys/system_keyring.h
index c1a96fdf598b..eb7aa81ec5d3 100644
--- a/include/keys/system_keyring.h
+++ b/include/keys/system_keyring.h
@@ -66,4 +66,6 @@ static inline void set_platform_trusted_keys(struct key *keyring)
 }
 #endif
 
+extern bool is_builtin_or_secondary_trusted_keyring(struct key *keyring);
+
 #endif /* _KEYS_SYSTEM_KEYRING_H */
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index df1318d85f7d..d447ee689910 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -1066,6 +1066,16 @@
  *
  * Security hooks affecting all Key Management operations
  *
+ * @key_create_or_update:
+ *      Notification of key create or update.
+ *      @keyring points to the keyring to which the key belongs
+ *      @key points to the key being created or updated
+ *      @cred current cred
+ *      @flags is the allocation flags
+ *      @builtin_or_secondary flag indicating whether
+ *      the keyring to which the key belongs is the builtin
+ *      or secondary trusted keys keyring
+ *      Return 0 if permission is granted, -ve error otherwise.
  * @key_alloc:
  *	Permit allocation of a key and assign security data. Note that key does
  *	not have a serial number assigned at this point.
@@ -1781,6 +1791,10 @@ union security_list_options {
 
 	/* key management security hooks */
 #ifdef CONFIG_KEYS
+	int (*key_create_or_update)(struct key *keyring, struct key *key,
+				    const struct cred *cred,
+				    unsigned long flags,
+				    bool builtin_or_secondary);
 	int (*key_alloc)(struct key *key, const struct cred *cred,
 				unsigned long flags);
 	void (*key_free)(struct key *key);
@@ -2026,6 +2040,7 @@ struct security_hook_heads {
 	struct hlist_head xfrm_decode_session;
 #endif	/* CONFIG_SECURITY_NETWORK_XFRM */
 #ifdef CONFIG_KEYS
+	struct hlist_head key_create_or_update;
 	struct hlist_head key_alloc;
 	struct hlist_head key_free;
 	struct hlist_head key_permission;
diff --git a/include/linux/security.h b/include/linux/security.h
index 5f7441abbf42..0db4316e236e 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -1672,6 +1672,10 @@ static inline int security_path_chroot(const struct path *path)
 #ifdef CONFIG_KEYS
 #ifdef CONFIG_SECURITY
 
+int security_key_create_or_update(struct key *keyring, struct key *key,
+				  const struct cred *cred,
+				  unsigned long flags,
+				  bool builtin_or_secondary);
 int security_key_alloc(struct key *key, const struct cred *cred, unsigned long flags);
 void security_key_free(struct key *key);
 int security_key_permission(key_ref_t key_ref,
@@ -1680,6 +1684,14 @@ int security_key_getsecurity(struct key *key, char **_buffer);
 
 #else
 
+int security_key_create_or_update(struct key *keyring, struct key *key,
+				  const struct cred *cred,
+				  unsigned long flags,
+				  bool builtin_or_secondary)
+{
+	return 0;
+}
+
 static inline int security_key_alloc(struct key *key,
 				     const struct cred *cred,
 				     unsigned long flags)
diff --git a/security/keys/key.c b/security/keys/key.c
index 764f4c57913e..973dfead490c 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -14,6 +14,7 @@
 #include <linux/workqueue.h>
 #include <linux/random.h>
 #include <linux/err.h>
+#include <keys/system_keyring.h>
 #include "internal.h"
 
 struct kmem_cache *key_jar;
@@ -823,6 +824,7 @@ key_ref_t key_create_or_update(key_ref_t keyring_ref,
 	key_ref_t key_ref;
 	int ret;
 	struct key_restriction *restrict_link = NULL;
+	bool trusted_keyring = false;
 
 	/* look up the key type to see if it's one of the registered kernel
 	 * types */
@@ -936,6 +938,16 @@ key_ref_t key_create_or_update(key_ref_t keyring_ref,
 		goto error_link_end;
 	}
 
+	/* let the security module know about the key */
+	trusted_keyring = is_builtin_or_secondary_trusted_keyring(keyring);
+	ret = security_key_create_or_update(keyring, key, cred, flags,
+					    trusted_keyring);
+	if (ret < 0) {
+		key_put(key);
+		key_ref = ERR_PTR(ret);
+		goto error_link_end;
+	}
+
 	key_ref = make_key_ref(key, is_key_possessed(keyring_ref));
 
 error_link_end:
diff --git a/security/security.c b/security/security.c
index 250ee2d76406..df6f561e4418 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2280,6 +2280,20 @@ EXPORT_SYMBOL(security_skb_classify_flow);
 
 #ifdef CONFIG_KEYS
 
+int security_key_create_or_update(struct key *keyring, struct key *key,
+				  const struct cred *cred,
+				  unsigned long flags,
+				  bool builtin_or_secondary)
+{
+	int rc = call_int_hook(key_create_or_update, 0,
+			       keyring, key, cred, flags,
+			       builtin_or_secondary);
+	if (rc)
+		return rc;
+	return ima_post_key_create_or_update(keyring, key,
+					     builtin_or_secondary);
+}
+
 int security_key_alloc(struct key *key, const struct cred *cred,
 		       unsigned long flags)
 {
-- 
2.17.1

Re: [PATCH v0 1/2] KEYS: Measure keys added to builtin or secondary trusted keys keyring

[Mimi Zohar]

On Fri, 2019-10-11 at 10:35 -0700, Lakshmi Ramasubramanian wrote:
> IMA hook TRUSTED_KEYS to measure keys added to builtin or secondary
> trusted keys keyring. This can be enabled through IMA policy.
> 
> The key data is queued up if IMA is not yet initialized and measured
> when IMA is initialized. If IMA is initialized then the key is
> measured immediately.
> 
> Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>

This patch needs to be broken up into multiple, smaller patches.  As
discussed, measuring keys should be separate from the early
measurement workqueue, at minimum as separate patches, if not separate
patch sets.  A new LSM hook definitely needs to be defined in a
separate patch.

>  /*
> - * process_buffer_measurement - Measure the buffer to ima log.
> - * @buf: pointer to the buffer that needs to be added to the log.
> - * @size: size of buffer(in bytes).
> - * @eventname: event name to be used for the buffer entry.
> - * @cred: a pointer to a credentials structure for user validation.
> - * @secid: the secid of the task to be validated.
> + * ima_post_key_create_or_update
> + *     @keyring points to the keyring to which the key belongs
> + *     @key points to the key being created or updated
> + *     @builtin_or_secondary flag indicating whether
> + *     the keyring to which the key belongs is the builtin
> + *     or secondary trusted keys keyring
> + * Measure keys added to the builtin or secondary trusted keyring
>   *
> - * Based on policy, the buffer is measured into the ima log.
> + * On success return 0.
> + * Return appropriate error code on error
>   */
> -static void process_buffer_measurement(const void *buf, int size,
> -				       const char *eventname,
> -				       const struct cred *cred, u32 secid)
> +int ima_post_key_create_or_update(struct key *keyring,
> +				  struct key *key,
> +				  bool builtin_or_secondary)
>  {
> -	int ret = 0;
> -	struct ima_template_entry *entry = NULL;
> -	struct integrity_iint_cache iint = {};
> -	struct ima_event_data event_data = {.iint = &iint,
> -					    .filename = eventname,
> -					    .buf = buf,
> -					    .buf_len = size};
> -	struct ima_template_desc *template_desc = NULL;
> -	struct {
> -		struct ima_digest_data hdr;
> -		char digest[IMA_MAX_DIGEST_SIZE];
> -	} hash = {};
> -	int violation = 0;
> -	int pcr = CONFIG_IMA_MEASURE_PCR_IDX;
> -	int action = 0;
> +	int rc = 0;
> +	struct ima_trusted_key_entry *entry = NULL;
> +	const struct public_key *pk;
> +	u32 secid;
> +	bool queued = false;
>  
> -	action = ima_get_action(NULL, cred, secid, 0, KEXEC_CMDLINE, &pcr,
> -				&template_desc);
> -	if (!(action & IMA_MEASURE))
> -		return;
> +	/*
> +	 * We only measure asymmetric keys added to either
> +	 * the builtin or the secondary trusted keys keyring.
> +	 */
> +	if (!builtin_or_secondary ||
> +	    (key->type != &key_type_asymmetric))
> +		return 0;

Measuring keys should be generic, independent of the keyring that it
is being added to.  Please do not hard code policy.

Mimi

>  
> -	iint.ima_hash = &hash.hdr;
> -	iint.ima_hash->algo = ima_hash_algo;
> -	iint.ima_hash->length = hash_digest_size[ima_hash_algo];
> +	mutex_lock(&ima_trusted_keys_mutex);

Re: [PATCH v0 2/2] KEYS: LSM Hook for key_create_or_update

[Mimi Zohar]

On Fri, 2019-10-11 at 10:35 -0700, Lakshmi Ramasubramanian wrote:
> diff --git a/security/keys/key.c b/security/keys/key.c
> index 764f4c57913e..973dfead490c 100644
> --- a/security/keys/key.c
> +++ b/security/keys/key.c
> @@ -14,6 +14,7 @@
>  #include <linux/workqueue.h>
>  #include <linux/random.h>
>  #include <linux/err.h>
> +#include <keys/system_keyring.h>
>  #include "internal.h"
>  
>  struct kmem_cache *key_jar;
> @@ -823,6 +824,7 @@ key_ref_t key_create_or_update(key_ref_t keyring_ref,
>  	key_ref_t key_ref;
>  	int ret;
>  	struct key_restriction *restrict_link = NULL;
> +	bool trusted_keyring = false;
>  
>  	/* look up the key type to see if it's one of the registered kernel
>  	 * types */
> @@ -936,6 +938,16 @@ key_ref_t key_create_or_update(key_ref_t keyring_ref,
>  		goto error_link_end;
>  	}
>  
> +	/* let the security module know about the key */
> +	trusted_keyring = is_builtin_or_secondary_trusted_keyring(keyring);

Nothing should be added to the keys subsystem, other than the LSM
hook.

Mimi

> +	ret = security_key_create_or_update(keyring, key, cred, flags,
> +					    trusted_keyring);
> +	if (ret < 0) {
> +		key_put(key);
> +		key_ref = ERR_PTR(ret);
> +		goto error_link_end;
> +	}
> +

Re: [PATCH v0 1/2] KEYS: Measure keys added to builtin or secondary trusted keys keyring

[Nayna]

Hi Lakshmi,


On 10/11/2019 01:35 PM, Lakshmi Ramasubramanian wrote:
> IMA hook TRUSTED_KEYS to measure keys added to builtin or secondary
> trusted keys keyring. This can be enabled through IMA policy.
>
> The key data is queued up if IMA is not yet initialized and measured
> when IMA is initialized. If IMA is initialized then the key is
> measured immediately.
>
> Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
> ---
>   Documentation/ABI/testing/ima_policy |   1 +
>   include/linux/ima.h                  |  10 ++
>   security/integrity/ima/ima.h         |  14 ++
>   security/integrity/ima/ima_api.c     |   2 +-
>   security/integrity/ima/ima_init.c    |  11 +-
>   security/integrity/ima/ima_main.c    | 230 ++++++++++++++++++++++-----
>   security/integrity/ima/ima_policy.c  |   4 +-
>   7 files changed, 226 insertions(+), 46 deletions(-)
>
> diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
> index fc376a323908..cc25c0f41d6e 100644
> --- a/Documentation/ABI/testing/ima_policy
> +++ b/Documentation/ABI/testing/ima_policy
> @@ -29,6 +29,7 @@ Description:
>   				[FIRMWARE_CHECK]
>   				[KEXEC_KERNEL_CHECK] [KEXEC_INITRAMFS_CHECK]
>   				[KEXEC_CMDLINE]
> +				[TRUSTED_KEYS]
>   			mask:= [[^]MAY_READ] [[^]MAY_WRITE] [[^]MAY_APPEND]
>   			       [[^]MAY_EXEC]
>   			fsmagic:= hex value
> diff --git a/include/linux/ima.h b/include/linux/ima.h
> index a20ad398d260..eb95743ada7d 100644
> --- a/include/linux/ima.h
> +++ b/include/linux/ima.h
> @@ -25,6 +25,9 @@ extern int ima_post_read_file(struct file *file, void *buf, loff_t size,
>   extern void ima_post_path_mknod(struct dentry *dentry);
>   extern void ima_kexec_cmdline(const void *buf, int size);
>
> +extern int ima_post_key_create_or_update(struct key *keyring,
> +					 struct key *key,
> +					 bool builtin_or_secondary);
>   #ifdef CONFIG_IMA_KEXEC
>   extern void ima_add_kexec_buffer(struct kimage *image);
>   #endif
> @@ -91,6 +94,13 @@ static inline void ima_post_path_mknod(struct dentry *dentry)
>   }
>
>   static inline void ima_kexec_cmdline(const void *buf, int size) {}
> +
> +static inline int ima_post_key_create_or_update(struct key *keyring,
> +						struct key *key,
> +						bool builtin_or_secondary)
> +{
> +	return 0;
> +}
>   #endif /* CONFIG_IMA */
>
>   #ifndef CONFIG_IMA_KEXEC
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index 011b91c79351..f0c1801da890 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -21,6 +21,7 @@
>   #include <linux/tpm.h>
>   #include <linux/audit.h>
>   #include <crypto/hash_info.h>
> +#include <keys/asymmetric-type.h>
>
>   #include "../integrity.h"
>
> @@ -52,6 +53,7 @@ extern int ima_policy_flag;
>   extern int ima_hash_algo;
>   extern int ima_appraise;
>   extern struct tpm_chip *ima_tpm_chip;
> +extern bool ima_initialized;
>
>   /* IMA event related data */
>   struct ima_event_data {
> @@ -104,6 +106,16 @@ struct ima_queue_entry {
>   };
>   extern struct list_head ima_measurements;	/* list of all measurements */
>
> +/*
> + * To track trusted keys that need to be measured when IMA is initialized.
> + */
> +struct ima_trusted_key_entry {
> +	struct list_head list;
> +	void *public_key;
> +	u32  public_key_len;
> +	char *key_description;
> +};
> +
>   /* Some details preceding the binary serialized measurement list */
>   struct ima_kexec_hdr {
>   	u16 version;
> @@ -158,6 +170,7 @@ void ima_init_template_list(void);
>   int __init ima_init_digests(void);
>   int ima_lsm_policy_change(struct notifier_block *nb, unsigned long event,
>   			  void *lsm_data);
> +void ima_measure_queued_trusted_keys(void);
>
>   /*
>    * used to protect h_table and sha_table
> @@ -189,6 +202,7 @@ static inline unsigned long ima_hash_key(u8 *digest)
>   	hook(KEXEC_INITRAMFS_CHECK)	\
>   	hook(POLICY_CHECK)		\
>   	hook(KEXEC_CMDLINE)		\
> +	hook(TRUSTED_KEYS)		\
>   	hook(MAX_CHECK)
>   #define __ima_hook_enumify(ENUM)	ENUM,
>
> diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
> index f614e22bf39f..704a048ff925 100644
> --- a/security/integrity/ima/ima_api.c
> +++ b/security/integrity/ima/ima_api.c
> @@ -174,7 +174,7 @@ void ima_add_violation(struct file *file, const unsigned char *filename,
>    *		subj=, obj=, type=, func=, mask=, fsmagic=
>    *	subj,obj, and type: are LSM specific.
>    *	func: FILE_CHECK | BPRM_CHECK | CREDS_CHECK | MMAP_CHECK | MODULE_CHECK
> - *	| KEXEC_CMDLINE
> + *	| KEXEC_CMDLINE | TRUSTED_KEYS
>    *	mask: contains the permission mask
>    *	fsmagic: hex value
>    *
> diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c
> index 5d55ade5f3b9..32b9147fe410 100644
> --- a/security/integrity/ima/ima_init.c
> +++ b/security/integrity/ima/ima_init.c
> @@ -23,6 +23,7 @@
>   /* name for boot aggregate entry */
>   static const char boot_aggregate_name[] = "boot_aggregate";
>   struct tpm_chip *ima_tpm_chip;
> +bool ima_initialized;
>
>   /* Add the boot aggregate to the IMA measurement list and extend
>    * the PCR register.
> @@ -131,5 +132,13 @@ int __init ima_init(void)
>
>   	ima_init_policy();
>
> -	return ima_fs_init();
> +	rc = ima_fs_init();
> +	if (rc != 0)
> +		return rc;
> +
> +	ima_initialized = true;
> +
> +	ima_measure_queued_trusted_keys();
> +
> +	return 0;
>   }
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 584019728660..b0ee5c82207a 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -27,6 +27,7 @@
>   #include <linux/ima.h>
>   #include <linux/iversion.h>
>   #include <linux/fs.h>
> +#include <crypto/public_key.h>
>
>   #include "ima.h"
>
> @@ -43,6 +44,13 @@ static struct notifier_block ima_lsm_policy_notifier = {
>   	.notifier_call = ima_lsm_policy_change,
>   };
>
> +/*
> + * Used to synchronize access to the list of trusted keys (ima_trusted_keys)
> + * that need to be measured when IMA is initialized.
> + */
> +static DEFINE_MUTEX(ima_trusted_keys_mutex);
> +static LIST_HEAD(ima_trusted_keys);
> +
>   static int __init hash_setup(char *str)
>   {
>   	struct ima_template_desc *template_desc = ima_template_desc_current();
> @@ -351,6 +359,65 @@ static int process_measurement(struct file *file, const struct cred *cred,
>   	return 0;
>   }
>
> +
> +/*
> + * process_buffer_measurement - Measure the buffer to ima log.
> + * @buf: pointer to the buffer that needs to be added to the log.
> + * @size: size of buffer(in bytes).
> + * @eventname: event name to be used for the buffer entry.
> + * @func: IMA Hook function
> + * @cred: a pointer to a credentials structure for user validation.
> + * @secid: the secid of the task to be validated.
> + *
> + * Based on policy, the buffer is measured into the ima log.
> + */
> +static void process_buffer_measurement(const void *buf, u32 size,
> +				       const char *eventname,
> +				       enum ima_hooks func,
> +				       const struct cred *cred, u32 secid)
> +{
> +	int ret = 0;
> +	struct ima_template_entry *entry = NULL;
> +	struct integrity_iint_cache iint = {};
> +	struct ima_event_data event_data = {.iint = &iint,
> +					    .filename = eventname,
> +					    .buf = buf,
> +					    .buf_len = size};
> +	struct ima_template_desc *template_desc = ima_template_desc_current();
> +	struct {
> +		struct ima_digest_data hdr;
> +		char digest[IMA_MAX_DIGEST_SIZE];
> +	} hash = {};
> +	int violation = 0;
> +	int pcr = CONFIG_IMA_MEASURE_PCR_IDX;
> +	int action = 0;
> +
> +	action = ima_get_action(NULL, cred, secid, 0, func, &pcr,
> +				&template_desc);
> +	if (!(action & IMA_MEASURE))
> +		return;
> +
> +	iint.ima_hash = &hash.hdr;
> +	iint.ima_hash->algo = ima_hash_algo;
> +	iint.ima_hash->length = hash_digest_size[ima_hash_algo];
> +
> +	ret = ima_calc_buffer_hash(buf, size, iint.ima_hash);
> +	if (ret < 0)
> +		goto out;
> +
> +	ret = ima_alloc_init_template(&event_data, &entry, template_desc);
> +	if (ret < 0)
> +		goto out;
> +
> +	ret = ima_store_template(entry, violation, NULL, buf, pcr);
> +
> +	if (ret < 0)
> +		ima_free_template_entry(entry);
> +
> +out:
> +	return;
> +}
> +

I am wondering that even though there is just one argument change in the 
process_buffer_measurement() function, a full new function is defined. 
This makes reviewing the function more difficult than it should be. Can 
you please check on how the patch is getting formatted ?

Moreover, I am already modifying this function as part of the blacklist 
patchset, but in a different way. Please refer to the Patch [5/8] in the 
patchset titled "powerpc: Enabling IMA arch specific secure boot 
policies". It was posted on 8th October.

I will modify the process_buffer_measurement() function in a way that 
can work for both of our requirements and will post a new version soon.

Thanks & Regards,
      - Nayna

Re: [PATCH v0 1/2] KEYS: Measure keys added to builtin or secondary trusted keys keyring

[Lakshmi Ramasubramanian]

On 10/13/19 11:34 AM, Nayna wrote:

> Hi Lakshmi,
> I am wondering that even though there is just one argument change in the 
> process_buffer_measurement() function, a full new function is defined. 
> This makes reviewing the function more difficult than it should be. Can 
> you please check on how the patch is getting formatted ?
> 
> Moreover, I am already modifying this function as part of the blacklist 
> patchset, but in a different way. Please refer to the Patch [5/8] in the 
> patchset titled "powerpc: Enabling IMA arch specific secure boot 
> policies". It was posted on 8th October.
> 
> I will modify the process_buffer_measurement() function in a way that 
> can work for both of our requirements and will post a new version soon.
> 
> Thanks & Regards,
>       - Nayna

Hi Nayna,

I think it is because I moved the function towards the top of the file - 
so it appears like a delete and an add instead of a minor change :(

Please let me know when you send your change. I'll take a look.

thanks,
  -lakshmi