KEYS: Measure keys when they are created or updated

Problem Statement:

Keys created or updated in the system are currently not being measured. Therefore an attestation service, for instance, would not be able to attest whether or not the trusted keys keyring(s), for instance, contain only known good (trusted) keys.

IMA measures system files, command line arguments passed to kexec, boot aggregate, etc. It can be used to measure keys as well. But there is no mechanism available in the kernel for IMA to know when a key is created or updated.

This change aims to address measuring keys created or updated in the system.

To achieve the above the following changes have been made:

- Added a new IMA hook namely, ima_post_key_create_or_update, which measures the key. This IMA hook is called from key_create_or_update function. The key measurement can be controlled through IMA policy.

In this change set a new IMA policy function KEYRING_CHECK has been added to measure keys. The policy can optionally specify a set of keyrings to measure. By default all keyrings are included in the measurement when KEYRING_CHECK policy is specified.

   # measure all keys
   measure func=KEYRING_CHECK

   # measure keys on the IMA keyring
   measure func=KEYRING_CHECK keyring=".ima"

   # measure keys on the BUILTIN and IMA keyrings into a different PCR
   measure func=KEYRING_CHECK keyring=".builtin_trusted_keys|.ima" pcr

Testing performed:

* Booted the kernel with this change.
* Executed keyctl tests from the Linux Test Project (LTP)
* All keys are measured when only KEYRING_CHECK is set.
* Only keys added to the given keyrings are measured when keyrings option is set.
* Keys are not measured when KEYRING_CHECK is not set.
* Key is queued for measurement if IMA is not yet initialized and processed when IMA is initialized.
* Key is measured rightaway when IMA is initialized.
* Added a new key to a keyring and verified "key create" code path. => In this case added a key to .ima keyring.
* Added the same key again and verified "key update" code path. => Add the same key to .ima keyring.

Change Log:

v4:

=> Rebased the changes to v5.4-rc3
=> Applied the following dependent patch set first
and then added new changes.
https://lore.kernel.org/linux-integrity/[email protected]
=> Refactored the patch set to separate out changes related to
func KEYRING_CHECK and options keyrings into different patches.
=> Moved the functions to queue and dequeue keys for measurement
from ima_queue.c to a new file ima_asymmetric_keys.c.
=> Added a new config namely CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS
to enable asymmetric key measurement. When this config option is enabled ima_asymmetric_keys.c is compiled.

v3:

=> Added KEYRING_CHECK for measuring keys. This can optionally specify
keyrings to measure.
=> Updated ima_get_action() and related functions to return
the keyrings if specified in the policy.
=> process_buffer_measurement() function is updated to take keyring
as a parameter. The key will be measured if the policy includes the keyring in the list of measured keyrings. If the policy does not specify any keyrings then all keys are measured.

v2:

=> Per suggestion from Mimi reordered the patch set to first
enable measuring keys added or updated in the system. And, then scope the measurement to keys added to builtin_trusted_keys keyring through ima policy.
=> Removed security_key_create_or_update function and instead
call ima hook, to measure the key, directly from key_create_or_update function.

v1:

=> LSM function for key_create_or_update. It calls ima. => Added ima hook for measuring keys
=> ima measures keys based on ima policy.

v0:

=> Added LSM hook for key_create_or_update.
=> Measure keys added to builtin or secondary trusted keys keyring.

Background:

Currently ima measures file hashes and .ima signatures. ima signatures are validated against keys in the ".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.

Although ima supports the above configuration, not having an insight into what keys are present in these trusted keys keyrings would prevent an attestation service from validating a client machine.

On systems with CONFIG_IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY enabled, measuring keys in the ".builtin_trusted_keys" 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 change, 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.

Questions and concerns raised by reviewers on this 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 .builtin_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 ".builtin_trusted_keys"), 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_MEASURE_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 ".builtin_trusted_keys"

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 => Built-In trusted key attests IMA key on the client => Attestation service attests the Built-In 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 ".builtin_trusted_keys"

Lakshmi Ramasubramanian (10):
KEYS: Defined an IMA hook and a func to measure keys on key create or
update
KEYS: Added KEYRING_CHECK func in IMA policy to measure keys
KEYS: Added keyrings= option in IMA policy to only measure keys added
to the specified keyrings.
KEYS: Read keyrings= option from the IMA policy into ima_rule_entry
KEYS: Updated IMA policy functions to return keyrings option read from
the policy
KEYS: Measure key if the IMA policy allows measurement for the keyring
to which the key is linked to
KEYS: Added a boolean flag to track IMA initialization status
KEYS: Defined functions to queue and dequeue keys for measurement
KEYS: Call queue and dequeue functions to measure keys
KEYS: Call the IMA hook to measure key when a new key is created or an
existing key is updated

 Documentation/ABI/testing/ima_policy         |  16 ++-
 include/linux/ima.h                          |   8 ++
 security/integrity/ima/Kconfig               |  14 ++
 security/integrity/ima/Makefile              |   1 +
 security/integrity/ima/ima.h                 |  34 ++++-
 security/integrity/ima/ima_api.c             |   8 +-
 security/integrity/ima/ima_appraise.c        |   4 +-
 security/integrity/ima/ima_asymmetric_keys.c | 136 +++++++++++++++++++
 security/integrity/ima/ima_init.c            |  10 +-
 security/integrity/ima/ima_main.c            |  47 ++++++-
 security/integrity/ima/ima_policy.c          |  39 +++++-
 security/keys/key.c                          |   9 ++
 12 files changed, 309 insertions(+), 17 deletions(-)
 create mode 100644 security/integrity/ima/ima_asymmetric_keys.c

2.17.1

Asymmetric keys used for verifying file signatures or certificates are currently not included in the IMA measurement list.

This patch defines a new IMA hook namely ima_post_key_create_or_update() to measure asymmetric keys.

---
 security/integrity/ima/ima_main.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

index d7e987baf127..a0e233afe876 100644
@@ -721,6 +721,22 @@ void ima_kexec_cmdline(const void *buf, int size)
 					   KEXEC_CMDLINE, 0);
 }
 
+/**
+ * ima_post_key_create_or_update - measure asymmetric keys
+ * @keyring: keyring to which the key is linked to
+ * @key: created or updated key
+ * @flags: key flags
+ * @create: flag indicating whether the key was created or updated
+ *
+ * Keys can only be measured, not appraised.
+ */
+void ima_post_key_create_or_update(struct key *keyring, struct key *key,
+				   unsigned long flags, bool create)
+{
+	if ((keyring != NULL) && (key != NULL))
+		return;
+}
+
 static int __init init_ima(void)
 {
 	int error;

2.17.1

As I'd stated in the other response, I wanted to isolate all key related code in a separate C file and build it if and only if all CONFIG dependencies are met.

I can do the following:

=> Define the IMA hook in ima_asymmetric_keys.c instead of ima_main.c

=> In include/linux/ima.h declare the IMA hook if CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS is enabled. Else, NOP it.

#ifdef CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS
extern void ima_post_key_create_or_update(struct key *keyring,
struct key *key, unsigned long flags, bool create);
#else
static inline void ima_post_key_create_or_update(struct key *keyring,
struct key *key, unsigned long flags, bool create) {}
#endif

Would that be acceptable?

thanks,
-lakshmi

The basic measuring of keys shouldn't be any different than any other policy rule, other than it is a key and not a file.  This is the reason that I keep saying start out with the basics and then add support to defer measuring keys on the trusted keyrings.

Only the queueing code needed for measuring keys on the trusted keyrings would be in a separate file.

Mimi

I'll make the changes, rearrange the patches and send an updated set.

I do have a few questions since I am still not fully understanding the requirements you are targeting. Appreciate if you could please clarify.

As you already know, I am using the "public key" of the given asymmetric key as the "buffer" to measure in process_buffer_measurement().

The measurement decision is not based on whether the keyring is a trusted one or an untrusted one. As long as the IMA policy allows (through the "keyrings=" option) the key will be measured.

Do you want only trusted keyrings to be allowed in the measurement?
In my opinion, that decision should be deferred to whoever is setting up the IMA policy.

The decision to process key immediately or defer (queue) is based on whether IMA has been initialized or not. Keyring is not used for this decision.

Could you please clarify how queuing is related to keyring's trustworthiness?

The check for whether the key is an asymmetric one or not, and extracting the "public key" if it is an asymmetric key needs to be in a separate file to handle the CONFIG dependencies in IMA.

thanks,
-lakshmi

We should be able to measure all keys being loaded onto any keyring or onto a specific "keyring=".   This shouldn't be any different than any other policy rule.  Once you have this basic feature working, you would address loading keys during early boot.

Right, but it shouldn't be limited to just "trusted" keyrings.  This way you can first test loading keys onto any keyring.

Queuing the keys should be independent of measuring the keys. Initially you would start with just measuring the key.  From a high level it would look like:

    ima_post_key_create_or_update(...)
    {
       "measure key based on
    policy(key, keyring, ...)"
    }

This requires the IMA "keyring=" policy option support be defined first.

Subsequently you would add key queuing support, and then update ima_post_key_create_or_update().  It would look like:

ima_post_key_create_or_update(...)
{
if (custom policy is loaded)
   "measure key based on policy(key, keyring, ...)" else
"queue key(key, keyring)"
}

Mimi

Perfect - that's exactly how I have implemented it right now. Will continue to test it.

Thank you.

Yes - I have the above change working. Will continue testing.

thanks,
-lakshmi

IMA policy needs to support a func to enable measurement of asymmetric keys.

This patch defines a new IMA policy func namely KEYRING_CHECK to measure asymmetric keys.

---
 Documentation/ABI/testing/ima_policy | 6 ++++++
 security/integrity/ima/ima.h         | 1 +
 security/integrity/ima/ima_policy.c  | 2 +-
 3 files changed, 8 insertions(+), 1 deletion(-)

index 29aaedf33246..341df49b5ad1 100644
@@ -30,6 +30,7 @@ Description:
 				[FIRMWARE_CHECK]
 				[KEXEC_KERNEL_CHECK] [KEXEC_INITRAMFS_CHECK]
 				[KEXEC_CMDLINE]
+				[KEYRING_CHECK]
 			mask:= [[^]MAY_READ] [[^]MAY_WRITE] [[^]MAY_APPEND]
 			       [[^]MAY_EXEC]
 			fsmagic:= hex value

@@ -113,3 +114,8 @@ Description:
 		Example of appraise rule allowing modsig appended signatures:
 
 			appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig
+
+		Example of measure rule using KEYRING_CHECK to measure
+		all keys:
+
+			measure func=KEYRING_CHECK

index df4ca482fb53..7f23405b2718 100644
@@ -193,6 +193,7 @@ static inline unsigned long ima_hash_key(u8 *digest)
 	hook(KEXEC_INITRAMFS_CHECK)	\
 	hook(POLICY_CHECK)		\
 	hook(KEXEC_CMDLINE)		\
+	hook(KEYRING_CHECK)		\
 	hook(MAX_CHECK)
 #define __ima_hook_enumify(ENUM)	ENUM,
 

index f19a895ad7cd..4344b7354fc5 100644
@@ -373,7 +373,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 = KEYRING_CHECK)) {
 		if ((rule->flags & IMA_FUNC) && (rule->func = func))
 			return true;
 		return false;

2.17.1

IMA policy needs to support measuring only those keys linked to a specific set of keyrings.

This patch defines a new IMA policy option namely "keyrings=" that can be used to specify a set of keyrings. If this option is specified in the policy for func=KEYRING_CHECK then only the keys linked to the keyrings given in "keyrings=" option are measured.

If "keyrings=" option is not specified for func=KEYRING_CHECK then all keys are measured.

---
 Documentation/ABI/testing/ima_policy | 10 +++++++++-
 security/integrity/ima/ima_policy.c  |  2 ++
 2 files changed, 11 insertions(+), 1 deletion(-)

index 341df49b5ad1..be2874fa3928 100644
@@ -25,7 +25,7 @@ Description:
 			lsm:	[[subj_user=] [subj_role=] [subj_type=]
 				 [obj_user=] [obj_role=] [obj_type=]]
 			option:	[[appraise_type=]] [template=] [permit_directio]
-				[appraise_flag=]
+				[appraise_flag=] [keyrings=]
 		base: 	func:= [BPRM_CHECK][MMAP_CHECK][CREDS_CHECK][FILE_CHECK][MODULE_CHECK]
 				[FIRMWARE_CHECK]
 				[KEXEC_KERNEL_CHECK] [KEXEC_INITRAMFS_CHECK]

@@ -43,6 +43,9 @@ Description:
 			appraise_flag:= [check_blacklist]
 			Currently, blacklist check is only for files signed with appended
 			signature.
+			keyrings:= list of keyrings
+			(eg, .builtin_trusted_keys|.ima). Only valid
+			when action is "measure" and func is KEYRING_CHECK.
 			template:= name of a defined IMA template type
 			(eg, ima-ng). Only valid when action is "measure".
 			pcr:= decimal value

@@ -119,3 +122,8 @@ Description:
 		all keys:
 
 			measure func=KEYRING_CHECK
+
+		Example of measure rule using KEYRING_CHECK to only measure
+		keys added to .builtin_trusted_keys or .ima keyring:
+
+			measure func=KEYRING_CHECK keyrings=.builtin_trusted_keys|.ima

index 4344b7354fc5..4d68ad8ed91c 100644
@@ -34,6 +34,7 @@
 #define IMA_EUID	0x0080
 #define IMA_PCR		0x0100
 #define IMA_FSNAME	0x0200
+#define IMA_KEYRINGS	0x0400
 
 #define UNKNOWN		0
 #define MEASURE		0x0001	/* same as IMA_MEASURE */

@@ -79,6 +80,7 @@ struct ima_rule_entry {
 		int type;	/* audit type */
 	} lsm[MAX_LSM_RULES];
 	char *fsname;
+	char *keyrings; /* Measure keys added to these keyrings */
 	struct ima_template_desc *template;
 };
 

2.17.1

"keyrings=" option, if specified in the IMA policy, needs to be stored in the list of IMA rules when the configured IMA policy is read.

This patch defines a new policy token enum namely Opt_keyrings for reading "keyrings=" option from the IMA policy.

---
 security/integrity/ima/ima_policy.c | 28 +++++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

index 4d68ad8ed91c..74a727dc6030 100644
@@ -768,7 +768,8 @@ enum {
 	Opt_uid_gt, Opt_euid_gt, Opt_fowner_gt,
 	Opt_uid_lt, Opt_euid_lt, Opt_fowner_lt,
 	Opt_appraise_type, Opt_appraise_flag,
-	Opt_permit_directio, Opt_pcr, Opt_template, Opt_err
+	Opt_permit_directio, Opt_pcr, Opt_template, Opt_keyrings,
+	Opt_err
 };
 
 static const match_table_t policy_tokens = {

@@ -804,6 +805,7 @@ static const match_table_t policy_tokens = {
 	{Opt_permit_directio, "permit_directio"},
 	{Opt_pcr, "pcr=%s"},
 	{Opt_template, "template=%s"},
+	{Opt_keyrings, "keyrings=%s"},
 	{Opt_err, NULL}
 };
 

@@ -1051,6 +1053,23 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
 			result = 0;
 			entry->flags |= IMA_FSNAME;
 			break;
+		case Opt_keyrings:
+			ima_log_string(ab, "keyrings", args[0].from);
+
+			if ((entry->keyrings) ||
+			    (entry->action != MEASURE) ||
+			    (entry->func != KEYRING_CHECK)) {
+				result = -EINVAL;
+				break;
+			}
+			entry->keyrings = kstrdup(args[0].from, GFP_KERNEL);
+			if (!entry->keyrings) {
+				result = -ENOMEM;
+				break;
+			}
+			result = 0;
+			entry->flags |= IMA_KEYRINGS;
+			break;
 		case Opt_fsuuid:
 			ima_log_string(ab, "fsuuid", args[0].from);
 

@@ -1426,6 +1445,13 @@ int ima_policy_show(struct seq_file *m, void *v)
 		seq_puts(m, " ");
 	}
 
+	if (entry->flags & IMA_KEYRINGS) {
+		if (entry->keyrings != NULL)
+			snprintf(tbuf, sizeof(tbuf), "%s", entry->keyrings);
+		seq_printf(m, pt(Opt_keyrings), tbuf);
+		seq_puts(m, " ");
+	}
+
 	if (entry->flags & IMA_PCR) {
 		snprintf(tbuf, sizeof(tbuf), "%d", entry->pcr);
 		seq_printf(m, pt(Opt_pcr), tbuf);

2.17.1

keyrings option read from the IMA policy needs to be provided to the callers that determine the action to be performed.

This patch updates ima_get_action() and ima_match_policy() functions to return the keyrings option specified in the IMA policy.

---
 security/integrity/ima/ima.h          | 6 ++++--
 security/integrity/ima/ima_api.c      | 8 +++++---
 security/integrity/ima/ima_appraise.c | 2 +-
 security/integrity/ima/ima_main.c     | 5 +++--
 security/integrity/ima/ima_policy.c   | 7 ++++++-
 5 files changed, 19 insertions(+), 9 deletions(-)

index 7f23405b2718..387829afb9a2 100644
@@ -208,7 +208,8 @@ struct modsig;
 /* LIM API function definitions */
 int ima_get_action(struct inode *inode, const struct cred *cred, u32 secid,
 		   int mask, enum ima_hooks func, int *pcr,
-		   struct ima_template_desc **template_desc);
+		   struct ima_template_desc **template_desc,
+		   char **keyrings);
 int ima_must_measure(struct inode *inode, int mask, enum ima_hooks func);
 int ima_collect_measurement(struct integrity_iint_cache *iint,
 			    struct file *file, void *buf, loff_t size,

@@ -235,7 +236,8 @@ const char *ima_d_path(const struct path *path, char **pathbuf, char *filename);
 /* IMA policy related functions */
 int ima_match_policy(struct inode *inode, const struct cred *cred, u32 secid,
 		     enum ima_hooks func, int mask, int flags, int *pcr,
-		     struct ima_template_desc **template_desc);
+		     struct ima_template_desc **template_desc,
+		     char **keyrings);
 void ima_init_policy(void);
 void ima_update_policy(void);
 void ima_update_policy_flag(void);

index 610759fe63b8..fa2cd71ddf1a 100644
@@ -169,12 +169,13 @@ void ima_add_violation(struct file *file, const unsigned char *filename,
  * @func: caller identifier
  * @pcr: pointer filled in if matched measure policy sets pcr  * @template_desc: pointer filled in if matched measure policy sets template+ * @keyrings: pointer filled in if matched measure policy sets keyrings  *
  * The policy is defined in terms of keypairs:
  *		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 | KEYRING_CHECK
  *	mask: contains the permission mask
  *	fsmagic: hex value
  *

@@ -183,14 +184,15 @@ void ima_add_violation(struct file *file, const unsigned char *filename,
  */
 int ima_get_action(struct inode *inode, const struct cred *cred, u32 secid,
 		   int mask, enum ima_hooks func, int *pcr,
-		   struct ima_template_desc **template_desc)
+		   struct ima_template_desc **template_desc,
+		   char **keyrings)
 {
 	int flags = IMA_MEASURE | IMA_AUDIT | IMA_APPRAISE | IMA_HASH;
 
 	flags &= ima_policy_flag;
 
 	return ima_match_policy(inode, cred, secid, func, mask, flags, pcr,
-				template_desc);
+				template_desc, keyrings);
 }
 
 /*

index 300c8d2943c5..47ad4f56c0a8 100644
@@ -55,7 +55,7 @@ int ima_must_appraise(struct inode *inode, int mask, enum ima_hooks func)
 
 	security_task_getsecid(current, &secid);
 	return ima_match_policy(inode, current_cred(), secid, func, mask,
-				IMA_APPRAISE | IMA_HASH, NULL, NULL);
+				IMA_APPRAISE | IMA_HASH, NULL, NULL, NULL);
 }
 
 static int ima_fix_xattr(struct dentry *dentry,

index a0e233afe876..b6d17f37ba61 100644
@@ -215,7 +215,7 @@ static int process_measurement(struct file *file, const struct cred *cred,
 	 * Included is the appraise submask.
 	 */
 	action = ima_get_action(inode, cred, secid, mask, func, &pcr,
-				&template_desc);
+				&template_desc, NULL);
 	violation_check = ((func = FILE_CHECK || func = MMAP_CHECK) &&
 			   (ima_policy_flag & IMA_MEASURE));
 	if (!action && !violation_check)

@@ -647,6 +647,7 @@ void process_buffer_measurement(const void *buf, int size,
 					    .buf = buf,
 					    .buf_len = size};
 	struct ima_template_desc *template = NULL;
+	char *keyrings = NULL;
 	struct {
 		struct ima_digest_data hdr;
 		char digest[IMA_MAX_DIGEST_SIZE];

@@ -665,7 +666,7 @@ void process_buffer_measurement(const void *buf, int size,
 	if (func) {
 		security_task_getsecid(current, &secid);
 		action = ima_get_action(NULL, current_cred(), secid, 0, func,
-					&pcr, &template);
+					&pcr, &template, &keyrings);
 		if (!(action & IMA_MEASURE))
 			return;
 	}

index 74a727dc6030..53379a19de43 100644
@@ -481,6 +481,7 @@ static int get_subaction(struct ima_rule_entry *rule, enum ima_hooks func)
  * @mask: requested action (MAY_READ | MAY_WRITE | MAY_APPEND | MAY_EXEC)
  * @pcr: set the pcr to extend
  * @template_desc: the template that should be used for this rule
+ * @keyrings: set the keyrings for this rule, if specified
  *
  * Measure decision based on func/mask/fsmagic and LSM(subj/obj/type)
  * conditions.

@@ -491,7 +492,8 @@ static int get_subaction(struct ima_rule_entry *rule, enum ima_hooks func)
  */
 int ima_match_policy(struct inode *inode, const struct cred *cred, u32 secid,
 		     enum ima_hooks func, int mask, int flags, int *pcr,
-		     struct ima_template_desc **template_desc)
+		     struct ima_template_desc **template_desc,
+		     char **keyrings)
 {
 	struct ima_rule_entry *entry;
 	int action = 0, actmask = flags | (flags << 1);

@@ -527,6 +529,9 @@ int ima_match_policy(struct inode *inode, const struct cred *cred, u32 secid,
 		if ((pcr) && (entry->flags & IMA_PCR))
 			*pcr = entry->pcr;
 
+		if ((keyrings) && (entry->flags & IMA_KEYRINGS))
+			*keyrings = entry->keyrings;
+
 		if (template_desc && entry->template)
 			*template_desc = entry->template;
 

2.17.1

process_buffer_measurement() needs to check if the keyring to which the given key is linked to is listed in the keyrings option in the IMA policy.

This patch adds a new parameter "keyring" to process_buffer_measurement().

If process_buffer_measurement() is called with func KEYRING_CHECK and the name of the keyring to which the key is linked to, then the given key is measured if:
1, IMA policy did not specify "keyrings=" option.
2, Or, the given keyring name is listed in the "keyrings=" option.

---
 security/integrity/ima/ima.h          |  2 +-
 security/integrity/ima/ima_appraise.c |  2 +-
 security/integrity/ima/ima_main.c     | 26 ++++++++++++++++++++++++--
 3 files changed, 26 insertions(+), 4 deletions(-)

index 387829afb9a2..f15199f7ff2a 100644
@@ -221,7 +221,7 @@ void ima_store_measurement(struct integrity_iint_cache *iint, struct file *file,
 			   struct ima_template_desc *template_desc);
 void process_buffer_measurement(const void *buf, int size,
 				const char *eventname, enum ima_hooks func,
-				int pcr);
+				int pcr, const char *keyring);
 void ima_audit_measurement(struct integrity_iint_cache *iint,
 			   const unsigned char *filename);
 int ima_alloc_init_template(struct ima_event_data *event_data,

index 47ad4f56c0a8..a9649b04b9f1 100644
@@ -330,7 +330,7 @@ int ima_check_blacklist(struct integrity_iint_cache *iint,
 		if ((rc = -EPERM) && (iint->flags & IMA_MEASURE))
 			process_buffer_measurement(digest, digestsize,
 						   "blacklisted-hash", NONE,
-						   pcr);
+						   pcr, NULL);
 	}
 
 	return rc;

index b6d17f37ba61..56540357c854 100644
@@ -632,12 +632,22 @@ int ima_load_data(enum kernel_load_data_id id)
  * @eventname: event name to be used for the buffer entry.
  * @func: IMA hook
  * @pcr: pcr to extend the measurement
+ * @keyring: keyring for the measurement
+ *
+ *	The following scenarios are possible with respect to
+ *	the parameter "keyring":
+ *	1, keyring is NULL. In this case buffer is measured.
+ *	2, keyring is not NULL, but ima_get_action returned
+ *	   a NULL keyrings. In this case also the buffer is measured.
+ *	3, keyring is not NULL and ima_get_action returned
+ *	   a non-NULL keyrings. In this case measure the buffer
+ *	   only if the given keyring is present in the keyrings.
  *
  * Based on policy, the buffer is measured into the ima log.
  */
 void process_buffer_measurement(const void *buf, int size,
 				const char *eventname, enum ima_hooks func,
-				int pcr)
+				int pcr, const char *keyring)
 {
 	int ret = 0;
 	struct ima_template_entry *entry = NULL;

@@ -656,6 +666,13 @@ void process_buffer_measurement(const void *buf, int size,
 	int action = 0;
 	u32 secid;
 
+	/*
+	 * If IMA is not yet initialized or IMA policy is empty
+	 * then there is no need to measure.
+	 */
+	if (!ima_policy_flag)
+		return;
+
 	/*
 	 * Both LSM hooks and auxilary based buffer measurements are
 	 * based on policy.  To avoid code duplication, differentiate

@@ -671,6 +688,11 @@ void process_buffer_measurement(const void *buf, int size,
 			return;
 	}
 
+	if ((keyring != NULL) && (keyrings != NULL)
+	    && (strstr(keyrings, keyring) = NULL)) {
+		return;
+	}
+
 	if (!pcr)
 		pcr = CONFIG_IMA_MEASURE_PCR_IDX;
 

@@ -719,7 +741,7 @@ void ima_kexec_cmdline(const void *buf, int size)
 {
 	if (buf && size != 0)
 		process_buffer_measurement(buf, size, "kexec-cmdline",
-					   KEXEC_CMDLINE, 0);
+					   KEXEC_CMDLINE, 0, NULL);
 }
 
 /**

2.17.1

IMA initialization status need to be checked before attempting to determine the action (measure, appraise, etc.) and any related options specified in the IMA policy.

This patch defines a flag namely ima_initialized to track IMA initialization status.

ima_policy_flag cannot be relied upon for knowing IMA initialization status because ima_policy_flag will be set to 0 when either IMA is not initialized or the IMA policy itself is empty.

---
 security/integrity/ima/ima.h      | 1 +
 security/integrity/ima/ima_init.c | 9 ++++++++-
 2 files changed, 9 insertions(+), 1 deletion(-)

index f15199f7ff2a..6a86daa62c5b 100644
@@ -52,6 +52,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 {

index 5d55ade5f3b9..a810af6df587 100644
@@ -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,11 @@ int __init ima_init(void)
 
 	ima_init_policy();
 
-	return ima_fs_init();
+	rc = ima_fs_init();
+	if (rc != 0)
+		return rc;
+
+	ima_initialized = true;
+
+	return 0;
 }

2.17.1

A key can be measured right away only if IMA is initialized. Otherwise, the key should be queued up and processed when IMA initialization is completed.

This patch defines functions to queue and dequeue keys for measurement and a config to enable these functions. These functions are defined in a new file namely ima_asymmetric_keys.c

Note that currently IMA subsystem can be enabled without enabling KEYS subsystem.

Adding support for measuring asymmetric keys in IMA requires KEYS subsystem to be enabled. To handle this dependency a new config namely CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS has been added. Enabling this config requires the following configs to be enabled:
CONFIG_IMA, CONFIG_KEYS, CONFIG_ASYMMETRIC_KEY_TYPE, and CONFIG_ASYMMETRIC_PUBLIC_KEY_SUBTYPE.

The new file ima_asymmetric_keys.c is built only if CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS is enabled.

This config is turned off by default.

---
 security/integrity/ima/Kconfig               |  14 ++
 security/integrity/ima/Makefile              |   1 +
 security/integrity/ima/ima.h                 |  24 ++++
 security/integrity/ima/ima_asymmetric_keys.c | 136 +++++++++++++++++++
 4 files changed, 175 insertions(+)
 create mode 100644 security/integrity/ima/ima_asymmetric_keys.c

index 838476d780e5..c6d14884bc19 100644
@@ -310,3 +310,17 @@ config IMA_APPRAISE_SIGNED_INIT
 	default n
 	help
 	   This option requires user-space init to be signed.
+
+config IMA_MEASURE_ASYMMETRIC_KEYS
+	bool "Enable measuring asymmetric keys on key create or update"
+	depends on IMA
+	depends on KEYS
+	depends on ASYMMETRIC_KEY_TYPE
+	depends on ASYMMETRIC_PUBLIC_KEY_SUBTYPE
+	default n
+	help
+	   This option enables measuring asymmetric keys when
+	   the key is created or updated. Additionally, IMA policy
+	   needs to be configured to either measure keys linked to
+	   any keyring or only measure keys linked to the keyrings
+	   specified in the IMA policy through the keyrings= option.

index 31d57cdf2421..3e9d0ad68c7b 100644
@@ -12,3 +12,4 @@ ima-$(CONFIG_IMA_APPRAISE) += ima_appraise.o
 ima-$(CONFIG_IMA_APPRAISE_MODSIG) += ima_modsig.o
 ima-$(CONFIG_HAVE_IMA_KEXEC) += ima_kexec.o
 obj-$(CONFIG_IMA_BLACKLIST_KEYRING) += ima_mok.o
+obj-$(CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS) += ima_asymmetric_keys.o

index 6a86daa62c5b..872883520ea6 100644
@@ -206,6 +206,30 @@ extern const char *const func_tokens[];
 
 struct modsig;
 
+#ifdef CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS
+/*
+ * To track keys that need to be measured.
+ */
+struct ima_measure_key_entry {
+	struct list_head list;
+	void *public_key;
+	u32  public_key_len;
+	char *keyring_name;
+};
+
+int ima_queue_or_process_key_for_measurement(struct key *keyring,
+					     struct key *key);
+void ima_measure_queued_keys(void);
+#else
+static inline int ima_queue_or_process_key_for_measurement(
+					     struct key *keyring,
+					     struct key *key)
+{
+	return 0;
+}
+static inline void ima_measure_queued_keys(void) {}
+#endif /* CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS */
+
 /* LIM API function definitions */
 int ima_get_action(struct inode *inode, const struct cred *cred, u32 secid,
 		   int mask, enum ima_hooks func, int *pcr,

2.17.1

Reason the lock is taken even in the case the key is not queued is to avoid the following race condition:

=> ima_init() sets ima_initialized flag and calls the dequeue function

=> If IMA hook checks ima_initialized flag outside the lock and sees
the flag is not set, it will call the queue function.

=> If the above two steps race, the key could get added to the queue
after ima_init() has processed the queued keys.

That's the reason I named the function called by the IMA hook to ima_queue_or_process_key_for_measurement().

But I can make the following change:

=> IMA hook checks the flag.
=> If it is set, process key immediately
=> If the flag is not set, call ima_queue_or_process_key_for_measurement()

ima_queue_or_process_key_for_measurement() will do the following:

=> With the lock held check ima_initialized flag
=> If true release the lock and call process_buffer_measurement() => If false, queue the key and then release the lock

Would that be acceptable?

Just wanted to confirm:
Rename ima_post_key_create_or_update() to a more appropriate name?

Another reason for doing all key related operations in ima_queue_or_process_key_for_measurement() is to isolate key related code in a separate C file and build it if and only if the CONFIG dependencies are met.

With respect to loading custom policy, I will take a look at how to handle that case. Thanks for pointing that out.

thanks,
-lakshmi

On 11/6/19 2:44 PM, Mimi Zohar wrote:

Hi Mimi,

ima_init_policy() is called before ima_initialized flag is set.

As far as I understand ima_init_policy() loads custom policies as well. So custom policies (such as arch specific policies, secure boot policies, etc.) are loaded before the queued keys are processed.

But if CONFIG_IMA_WRITE_POLICY is enabled, the policy can be updated anytime. This scenario is not handled in my implementation.

Please correct me if my understanding is wrong.

thanks,
-lakshmi

Keys should be queued for measurement if IMA is not yet initialized. Keys queued for measurement, if any, need to be processed when IMA initialization is completed.

This patch updates the IMA hook for key_create_or_update to call ima_queue_or_process_key_for_measurement() and adds the call to process queued keys upon IMA initialization completion.

---
 security/integrity/ima/ima_init.c | 1 +
 security/integrity/ima/ima_main.c | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

index a810af6df587..74817a9f78e5 100644
@@ -137,6 +137,7 @@ int __init ima_init(void)
 		return rc;
 
 	ima_initialized = true;
+	ima_measure_queued_keys();
 
 	return 0;
 }

index 56540357c854..8733990867f2 100644
@@ -757,7 +757,7 @@ void ima_post_key_create_or_update(struct key *keyring, struct key *key,
 				   unsigned long flags, bool create)
 {
 	if ((keyring != NULL) && (key != NULL))
-		return;
+		ima_queue_or_process_key_for_measurement(keyring, key);
 }
 
 static int __init init_ima(void)

2.17.1

key_create_or_update function needs to call the IMA hook to measure the key when a new key is created or an existing key is updated.

This patch adds the call to the IMA hook from key_create_or_update function.

---
 include/linux/ima.h | 8 ++++++++
 security/keys/key.c | 9 +++++++++
 2 files changed, 17 insertions(+)

index 6d904754d858..069879242a15 100644
@@ -24,6 +24,9 @@ extern int ima_post_read_file(struct file *file, void *buf, loff_t size,
 			      enum kernel_read_file_id id);
 extern void ima_post_path_mknod(struct dentry *dentry);
 extern void ima_kexec_cmdline(const void *buf, int size);
+extern void ima_post_key_create_or_update(struct key *keyring,
+					  struct key *key,
+					  unsigned long flags, bool create);
 
 #ifdef CONFIG_IMA_KEXEC
 extern void ima_add_kexec_buffer(struct kimage *image);

@@ -92,6 +95,11 @@ static inline void ima_post_path_mknod(struct dentry *dentry)
 }
 
 static inline void ima_kexec_cmdline(const void *buf, int size) {}
+
+static inline void ima_post_key_create_or_update(struct key *keyring,
+						 struct key *key,
+						 unsigned long flags,
+						 bool create) {}
 #endif /* CONFIG_IMA */
 
 #ifndef CONFIG_IMA_KEXEC

index 764f4c57913e..9782d4d046fd 100644
@@ -13,6 +13,7 @@
 #include <linux/security.h>
 #include <linux/workqueue.h>
 #include <linux/random.h>
+#include <linux/ima.h>
 #include <linux/err.h>
 #include "internal.h"
 

@@ -936,6 +937,9 @@ key_ref_t key_create_or_update(key_ref_t keyring_ref,
 		goto error_link_end;
 	}
 
+	/* let the ima module know about the created key. */
+	ima_post_key_create_or_update(keyring, key, flags, true);
+
 	key_ref = make_key_ref(key, is_key_possessed(keyring_ref));
 
 error_link_end:

@@ -965,6 +969,11 @@ key_ref_t key_create_or_update(key_ref_t keyring_ref,
 	}
 
 	key_ref = __key_update(key_ref, &prep);
+
+	/* let the ima module know about the updated key. */
+	if (!IS_ERR(key_ref))
+		ima_post_key_create_or_update(keyring, key, flags, false);
+
 	goto error_free_prep;
 }
 EXPORT_SYMBOL(key_create_or_update);

2.17.1