linux-integrity.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/4] KEYS: measure keys when they are created or updated
@ 2019-10-23 23:39 Lakshmi Ramasubramanian
  2019-10-23 23:39 ` [PATCH v2 1/4] KEYS: Defined an ima hook for measuring keys on key create or update Lakshmi Ramasubramanian
                   ` (3 more replies)
  0 siblings, 4 replies; 14+ messages in thread
From: Lakshmi Ramasubramanian @ 2019-10-23 23:39 UTC (permalink / raw)
  To: zohar, dhowells, casey, sashal, jamorris, linux-security-module,
	linux-integrity, linux-kernel, keyrings
  Cc: nramas

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, and
boot aggregate. 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. The measurement can be controlled through ima policy.

   In this change set a new ima policy hook BUILTIN_TRUSTED_KEYS has been
   added to measure keys added to the builtin_trusted_keys keyring.
   In future, this can be extended to measure keys added to other
   keyrings.

Change Log:

  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.

Testing performed:

  * Booted the kernel with this change.
  * Executed keyctl tests from the Linux Test Project (LTP)
  * Added a new key to a keyring and verified "key create" code path.
    => In this case added a key to builtin_trusted_keys keyring.
    => Verified ima measured this key only when a policy is set.
  * Added the same key again and verified "key update" code path.
    => Add the same key to builtin_trusted_keys keyring.
    => Verified ima measured the key only when a policy is set.

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 (4):
  KEYS: Defined an ima hook for measuring keys on key create or update
  KEYS: Queue key for measurement if ima is not initialized. Measure
    queued keys when ima is initialized
  KEYS: Added BUILTIN_TRUSTED_KEYS enum to measure keys added to
    builtin_trusted_keys keyring
  KEYS: Enabled ima policy to measure keys added to builtin_trusted_keys
    keyring

 Documentation/ABI/testing/ima_policy |  1 +
 certs/system_keyring.c               |  5 ++
 include/keys/system_keyring.h        |  2 +
 include/linux/ima.h                  |  8 +++
 security/integrity/ima/ima.h         | 18 ++++++
 security/integrity/ima/ima_api.c     |  1 +
 security/integrity/ima/ima_init.c    | 10 ++-
 security/integrity/ima/ima_main.c    | 49 +++++++++++++++
 security/integrity/ima/ima_policy.c  |  5 +-
 security/integrity/ima/ima_queue.c   | 94 ++++++++++++++++++++++++++++
 security/keys/key.c                  |  9 +++
 11 files changed, 200 insertions(+), 2 deletions(-)

-- 
2.17.1


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH v2 1/4] KEYS: Defined an ima hook for measuring keys on key create or update
  2019-10-23 23:39 [PATCH v2 0/4] KEYS: measure keys when they are created or updated Lakshmi Ramasubramanian
@ 2019-10-23 23:39 ` Lakshmi Ramasubramanian
  2019-10-25 19:40   ` Mimi Zohar
  2019-10-23 23:39 ` [PATCH v2 2/4] KEYS: Queue key for measurement if ima is not initialized. Measure queued keys when ima is initialized Lakshmi Ramasubramanian
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 14+ messages in thread
From: Lakshmi Ramasubramanian @ 2019-10-23 23:39 UTC (permalink / raw)
  To: zohar, dhowells, casey, sashal, jamorris, linux-security-module,
	linux-integrity, linux-kernel, keyrings
  Cc: nramas

Defined an ima hook to measure keys created or updated in the system.

Call this ima hook from key_create_or_update function when a new key
is created or an existing key is updated.

ima hook calls process_buffer_measurement function to measure the key
if ima is initialized. If ima is not yet initialized, the ima hook
currently does nothing. The change to queue the key for measurement
and measure after ima is initialized is implemented in a later patch.

This patch set depends on the following patch set provided by
Nayna Jain from IBM (nayna@linux.ibm.com). That patch set is
currently being reviewed:

[PATCH v8 5/8] ima: make process_buffer_measurement() generic
https://lore.kernel.org/linux-integrity/1569594360-7141-7-git-send-email-nayna@linux.ibm.com/

Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
---
 include/linux/ima.h               |  8 ++++++++
 security/integrity/ima/ima.h      |  3 +++
 security/integrity/ima/ima_init.c |  1 +
 security/integrity/ima/ima_main.c | 26 ++++++++++++++++++++++++++
 security/keys/key.c               |  9 +++++++++
 5 files changed, 47 insertions(+)

diff --git a/include/linux/ima.h b/include/linux/ima.h
index a20ad398d260..4df39aefcd06 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -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);
@@ -91,6 +94,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
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 997a57137351..2d4130ff5655 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -21,6 +21,8 @@
 #include <linux/tpm.h>
 #include <linux/audit.h>
 #include <crypto/hash_info.h>
+#include <crypto/public_key.h>
+#include <keys/asymmetric-type.h>
 
 #include "../integrity.h"
 
@@ -52,6 +54,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 {
diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c
index 5d55ade5f3b9..52847ce765a4 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.
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 2b60d8fd017a..8bde12385912 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -693,6 +693,32 @@ void ima_kexec_cmdline(const void *buf, int size)
 	}
 }
 
+/**
+ * ima_post_key_create_or_update - measure 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)
+{
+	const struct public_key *pk;
+
+	if (key->type != &key_type_asymmetric)
+		return;
+
+	if (!ima_initialized)
+		return;
+
+	pk = key->payload.data[asym_crypto];
+	process_buffer_measurement(pk->key, pk->keylen,
+				   key->description,
+				   NONE, 0);
+}
+
 static int __init init_ima(void)
 {
 	int error;
diff --git a/security/keys/key.c b/security/keys/key.c
index 764f4c57913e..7c39054d8da6 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -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);
+	if (!IS_ERR(key_ref)) {
+		/* let the ima module know about the updated key. */
+		ima_post_key_create_or_update(keyring, key, flags, false);
+	}
+
 	goto error_free_prep;
 }
 EXPORT_SYMBOL(key_create_or_update);
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v2 2/4] KEYS: Queue key for measurement if ima is not initialized. Measure queued keys when ima is initialized
  2019-10-23 23:39 [PATCH v2 0/4] KEYS: measure keys when they are created or updated Lakshmi Ramasubramanian
  2019-10-23 23:39 ` [PATCH v2 1/4] KEYS: Defined an ima hook for measuring keys on key create or update Lakshmi Ramasubramanian
@ 2019-10-23 23:39 ` Lakshmi Ramasubramanian
  2019-10-23 23:39 ` [PATCH v2 3/4] KEYS: Added BUILTIN_TRUSTED_KEYS enum to measure keys added to builtin_trusted_keys keyring Lakshmi Ramasubramanian
  2019-10-23 23:39 ` [PATCH v2 4/4] KEYS: Enabled ima policy " Lakshmi Ramasubramanian
  3 siblings, 0 replies; 14+ messages in thread
From: Lakshmi Ramasubramanian @ 2019-10-23 23:39 UTC (permalink / raw)
  To: zohar, dhowells, casey, sashal, jamorris, linux-security-module,
	linux-integrity, linux-kernel, keyrings
  Cc: nramas

Defined functions to queue key for measurement if ima is not yet
initialized. ima hook function ima_post_key_create_or_update will
queue the key if ima is not yet initialized.

Process queued keys and measure them when ima initialization
is completed.

Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
---
 security/integrity/ima/ima.h       | 13 +++++
 security/integrity/ima/ima_init.c  |  9 ++-
 security/integrity/ima/ima_main.c  |  4 +-
 security/integrity/ima/ima_queue.c | 94 ++++++++++++++++++++++++++++++
 4 files changed, 118 insertions(+), 2 deletions(-)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 2d4130ff5655..38279707632a 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -199,6 +199,17 @@ enum ima_hooks {
 	__ima_hooks(__ima_hook_enumify)
 };
 
+/*
+ * 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;
+	enum ima_hooks func;
+};
+
 /* 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,
@@ -225,6 +236,8 @@ int ima_store_template(struct ima_template_entry *entry, int violation,
 		       const unsigned char *filename, int pcr);
 void ima_free_template_entry(struct ima_template_entry *entry);
 const char *ima_d_path(const struct path *path, char **pathbuf, char *filename);
+int ima_queue_key_for_measurement(struct key *key, enum ima_hooks func);
+void ima_measure_queued_keys(void);
 
 /* IMA policy related functions */
 int ima_match_policy(struct inode *inode, const struct cred *cred, u32 secid,
diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c
index 52847ce765a4..8734ed5322c7 100644
--- a/security/integrity/ima/ima_init.c
+++ b/security/integrity/ima/ima_init.c
@@ -132,5 +132,12 @@ 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_keys();
+	return 0;
 }
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 8bde12385912..bce430b3386e 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -710,8 +710,10 @@ void ima_post_key_create_or_update(struct key *keyring, struct key *key,
 	if (key->type != &key_type_asymmetric)
 		return;
 
-	if (!ima_initialized)
+	if (!ima_initialized) {
+		ima_queue_key_for_measurement(key, NONE);
 		return;
+	}
 
 	pk = key->payload.data[asym_crypto];
 	process_buffer_measurement(pk->key, pk->keylen,
diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c
index 1ce8b1701566..d42987022c12 100644
--- a/security/integrity/ima/ima_queue.c
+++ b/security/integrity/ima/ima_queue.c
@@ -46,6 +46,13 @@ struct ima_h_table ima_htable = {
  */
 static DEFINE_MUTEX(ima_extend_list_mutex);
 
+/*
+ * 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);
+
 /* lookup up the digest value in the hash table, and return the entry */
 static struct ima_queue_entry *ima_lookup_digest_entry(u8 *digest_value,
 						       int pcr)
@@ -232,3 +239,90 @@ int __init ima_init_digests(void)
 
 	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,
+	enum ima_hooks func)
+{
+	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;
+	entry->func = func;
+	rc = 0;
+
+out:
+	if (rc) {
+		ima_free_trusted_key_entry(entry);
+		entry = NULL;
+	}
+
+	return entry;
+}
+
+int ima_queue_key_for_measurement(struct key *key, enum ima_hooks func)
+{
+	int rc = 0;
+	struct ima_trusted_key_entry *entry = NULL;
+
+	mutex_lock(&ima_trusted_keys_mutex);
+
+	entry = ima_alloc_trusted_queue_entry(key, func);
+	if (entry != NULL) {
+		INIT_LIST_HEAD(&entry->list);
+		list_add_tail(&entry->list, &ima_trusted_keys);
+	} else
+		rc = -ENOMEM;
+
+	mutex_unlock(&ima_trusted_keys_mutex);
+
+	return rc;
+}
+
+void ima_measure_queued_keys(void)
+{
+	struct ima_trusted_key_entry *entry, *tmp;
+
+	mutex_lock(&ima_trusted_keys_mutex);
+
+	list_for_each_entry_safe(entry, tmp, &ima_trusted_keys, list) {
+		process_buffer_measurement(entry->public_key,
+					   entry->public_key_len,
+					   entry->key_description,
+					   NONE, 0);
+		list_del(&entry->list);
+		ima_free_trusted_key_entry(entry);
+	}
+
+	mutex_unlock(&ima_trusted_keys_mutex);
+}
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v2 3/4] KEYS: Added BUILTIN_TRUSTED_KEYS enum to measure keys added to builtin_trusted_keys keyring
  2019-10-23 23:39 [PATCH v2 0/4] KEYS: measure keys when they are created or updated Lakshmi Ramasubramanian
  2019-10-23 23:39 ` [PATCH v2 1/4] KEYS: Defined an ima hook for measuring keys on key create or update Lakshmi Ramasubramanian
  2019-10-23 23:39 ` [PATCH v2 2/4] KEYS: Queue key for measurement if ima is not initialized. Measure queued keys when ima is initialized Lakshmi Ramasubramanian
@ 2019-10-23 23:39 ` Lakshmi Ramasubramanian
  2019-10-27 14:33   ` Mimi Zohar
  2019-10-23 23:39 ` [PATCH v2 4/4] KEYS: Enabled ima policy " Lakshmi Ramasubramanian
  3 siblings, 1 reply; 14+ messages in thread
From: Lakshmi Ramasubramanian @ 2019-10-23 23:39 UTC (permalink / raw)
  To: zohar, dhowells, casey, sashal, jamorris, linux-security-module,
	linux-integrity, linux-kernel, keyrings
  Cc: nramas

Added an ima policy hook BUILTIN_TRUSTED_KEYS to measure keys added
to builtin_trusted_keys keyring.

Added a helper function to check if the given keyring is
the builtin_trusted_keys keyring.

Defined a function to map the keyring to ima policy hook function
and use it when measuring the key.

Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
---
 Documentation/ABI/testing/ima_policy |  1 +
 certs/system_keyring.c               |  5 +++++
 include/keys/system_keyring.h        |  2 ++
 security/integrity/ima/ima.h         |  2 ++
 security/integrity/ima/ima_api.c     |  1 +
 security/integrity/ima/ima_main.c    | 25 +++++++++++++++++++++++--
 security/integrity/ima/ima_queue.c   |  2 +-
 7 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
index fc376a323908..25566c74e679 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]
+				[BUILTIN_TRUSTED_KEYS]
 			mask:= [[^]MAY_READ] [[^]MAY_WRITE] [[^]MAY_APPEND]
 			       [[^]MAY_EXEC]
 			fsmagic:= hex value
diff --git a/certs/system_keyring.c b/certs/system_keyring.c
index 1eba08a1af82..5533c7f92fef 100644
--- a/certs/system_keyring.c
+++ b/certs/system_keyring.c
@@ -283,3 +283,8 @@ void __init set_platform_trusted_keys(struct key *keyring)
 	platform_trusted_keys = keyring;
 }
 #endif
+
+inline bool is_builtin_trusted_keyring(struct key *keyring)
+{
+	return (keyring == builtin_trusted_keys);
+}
diff --git a/include/keys/system_keyring.h b/include/keys/system_keyring.h
index c1a96fdf598b..2bc0aaa07f05 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_trusted_keyring(struct key *keyring);
+
 #endif /* _KEYS_SYSTEM_KEYRING_H */
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 38279707632a..92c25a6b4da7 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -23,6 +23,7 @@
 #include <crypto/hash_info.h>
 #include <crypto/public_key.h>
 #include <keys/asymmetric-type.h>
+#include <keys/system_keyring.h>
 
 #include "../integrity.h"
 
@@ -192,6 +193,7 @@ static inline unsigned long ima_hash_key(u8 *digest)
 	hook(KEXEC_INITRAMFS_CHECK)	\
 	hook(POLICY_CHECK)		\
 	hook(KEXEC_CMDLINE)		\
+	hook(BUILTIN_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..cc04706b7e7a 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -175,6 +175,7 @@ void ima_add_violation(struct file *file, const unsigned char *filename,
  *	subj,obj, and type: are LSM specific.
  *	func: FILE_CHECK | BPRM_CHECK | CREDS_CHECK | MMAP_CHECK | MODULE_CHECK
  *	| KEXEC_CMDLINE
+ *	| BUILTIN_TRUSTED_KEYS
  *	mask: contains the permission mask
  *	fsmagic: hex value
  *
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index bce430b3386e..986f80eead4d 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -605,6 +605,24 @@ int ima_load_data(enum kernel_load_data_id id)
 	return 0;
 }
 
+/*
+ * Maps the given keyring to a IMA Hook.
+ * @keyring: A keyring to which a key maybe linked to.
+ *
+ * This function currently handles only builtin_trusted_keys.
+ * To handle more keyrings, this function, ima hook and
+ * ima policy handler need to be updated.
+ */
+static enum ima_hooks keyring_policy_map(struct key *keyring)
+{
+	enum ima_hooks func = NONE;
+
+	if (is_builtin_trusted_keyring(keyring))
+		func = BUILTIN_TRUSTED_KEYS;
+
+	return func;
+}
+
 /*
  * process_buffer_measurement - Measure the buffer to ima log.
  * @buf: pointer to the buffer that needs to be added to the log.
@@ -706,19 +724,22 @@ void ima_post_key_create_or_update(struct key *keyring, struct key *key,
 				   unsigned long flags, bool create)
 {
 	const struct public_key *pk;
+	enum ima_hooks func;
 
 	if (key->type != &key_type_asymmetric)
 		return;
 
+	func = keyring_policy_map(keyring);
+
 	if (!ima_initialized) {
-		ima_queue_key_for_measurement(key, NONE);
+		ima_queue_key_for_measurement(key, func);
 		return;
 	}
 
 	pk = key->payload.data[asym_crypto];
 	process_buffer_measurement(pk->key, pk->keylen,
 				   key->description,
-				   NONE, 0);
+				   func, 0);
 }
 
 static int __init init_ima(void)
diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c
index d42987022c12..ed77c4dc0520 100644
--- a/security/integrity/ima/ima_queue.c
+++ b/security/integrity/ima/ima_queue.c
@@ -319,7 +319,7 @@ void ima_measure_queued_keys(void)
 		process_buffer_measurement(entry->public_key,
 					   entry->public_key_len,
 					   entry->key_description,
-					   NONE, 0);
+					   entry->func, 0);
 		list_del(&entry->list);
 		ima_free_trusted_key_entry(entry);
 	}
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v2 4/4] KEYS: Enabled ima policy to measure keys added to builtin_trusted_keys keyring
  2019-10-23 23:39 [PATCH v2 0/4] KEYS: measure keys when they are created or updated Lakshmi Ramasubramanian
                   ` (2 preceding siblings ...)
  2019-10-23 23:39 ` [PATCH v2 3/4] KEYS: Added BUILTIN_TRUSTED_KEYS enum to measure keys added to builtin_trusted_keys keyring Lakshmi Ramasubramanian
@ 2019-10-23 23:39 ` Lakshmi Ramasubramanian
  3 siblings, 0 replies; 14+ messages in thread
From: Lakshmi Ramasubramanian @ 2019-10-23 23:39 UTC (permalink / raw)
  To: zohar, dhowells, casey, sashal, jamorris, linux-security-module,
	linux-integrity, linux-kernel, keyrings
  Cc: nramas

Updated ima policy handler to check if the ima policy enables
measurement of keys added to the builtin_trusted_keys keyring.

With this patch measurement of keys added to the builtin_trusted_keys
keyring is enabled end-to-end.

Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
---
 security/integrity/ima/ima_policy.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 6df7f641ff66..944636076152 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 == BUILTIN_TRUSTED_KEYS)) {
 		if ((rule->flags & IMA_FUNC) && (rule->func == func))
 			return true;
 		return false;
@@ -959,6 +959,9 @@ 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,
+					"BUILTIN_TRUSTED_KEYS") == 0)
+				entry->func = BUILTIN_TRUSTED_KEYS;
 			else
 				result = -EINVAL;
 			if (!result)
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 1/4] KEYS: Defined an ima hook for measuring keys on key create or update
  2019-10-23 23:39 ` [PATCH v2 1/4] KEYS: Defined an ima hook for measuring keys on key create or update Lakshmi Ramasubramanian
@ 2019-10-25 19:40   ` Mimi Zohar
  2019-10-25 19:49     ` Lakshmi Ramasubramanian
  2019-10-25 22:28     ` Lakshmi Ramasubramanian
  0 siblings, 2 replies; 14+ messages in thread
From: Mimi Zohar @ 2019-10-25 19:40 UTC (permalink / raw)
  To: Lakshmi Ramasubramanian, dhowells, casey, sashal, jamorris,
	linux-security-module, linux-integrity, linux-kernel, keyrings

On Wed, 2019-10-23 at 16:39 -0700, Lakshmi Ramasubramanian wrote:
> Defined an ima hook to measure keys created or updated in the system.

"IMA" is an anacronym.  Unless it is a part of a function name, it
should be capitalized.

Before describing "what" you're doing, describe the problem.  For
example, "The 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 named ima_key_create_or_update() to
measure keys."

> Call this ima hook from key_create_or_update function when a new key
> is created or an existing key is updated.
> 
> ima hook calls process_buffer_measurement function to measure the key
> if ima is initialized. If ima is not yet initialized, the ima hook
> currently does nothing. The change to queue the key for measurement
> and measure after ima is initialized is implemented in a later patch.

> This patch set depends on the following patch set provided by
> Nayna Jain from IBM (nayna@linux.ibm.com). That patch set is
> currently being reviewed:
> 
> [PATCH v8 5/8] ima: make process_buffer_measurement() generic
> https://lore.kernel.org/linux-integrity/1569594360-7141-7-git-send-email-nayna@linux.ibm.com/

Unnecessary information for this patch.


> Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
> ---
>  include/linux/ima.h               |  8 ++++++++
>  security/integrity/ima/ima.h      |  3 +++
>  security/integrity/ima/ima_init.c |  1 +
>  security/integrity/ima/ima_main.c | 26 ++++++++++++++++++++++++++
>  security/keys/key.c               |  9 +++++++++
>  5 files changed, 47 insertions(+)
> 
> diff --git a/include/linux/ima.h b/include/linux/ima.h
> index a20ad398d260..4df39aefcd06 100644
> --- a/include/linux/ima.h
> +++ b/include/linux/ima.h
> @@ -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);
> @@ -91,6 +94,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
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index 997a57137351..2d4130ff5655 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -21,6 +21,8 @@
>  #include <linux/tpm.h>
>  #include <linux/audit.h>
>  #include <crypto/hash_info.h>
> +#include <crypto/public_key.h>
> +#include <keys/asymmetric-type.h>
>  
>  #include "../integrity.h"
>  
> @@ -52,6 +54,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 {
> diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c
> index 5d55ade5f3b9..52847ce765a4 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.
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 2b60d8fd017a..8bde12385912 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -693,6 +693,32 @@ void ima_kexec_cmdline(const void *buf, int size)
>  	}
>  }
>  
> +/**
> + * ima_post_key_create_or_update - measure 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)
> +{
> +	const struct public_key *pk;
> +
> +	if (key->type != &key_type_asymmetric)
> +		return;
> +
> +	if (!ima_initialized)
> +		return;

There's no reason to define a new variable to determine if IMA is
initialized.  Use ima_policy_flag.  Like process_measurements, the
test should be in process_buffer_measurement(), not here.

> +
> +	pk = key->payload.data[asym_crypto];
> +	process_buffer_measurement(pk->key, pk->keylen,
> +				   key->description,
> +				   NONE, 0);
> +}
> +
>  static int __init init_ima(void)
>  {
>  	int error;
> diff --git a/security/keys/key.c b/security/keys/key.c
> index 764f4c57913e..7c39054d8da6 100644
> --- a/security/keys/key.c
> +++ b/security/keys/key.c
> @@ -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));

This patch defines the new IMA hook.  This call and the subsequent one
below can be defined in a separate patch.  The subject line of that
patch would be "keys: Add ima_key_create_or_update call to measure
keys".

Mimi

>  
>  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);
> +	if (!IS_ERR(key_ref)) {
> +		/* let the ima module know about the updated key. */
> +		ima_post_key_create_or_update(keyring, key, flags, false);
> +	}
> +
>  	goto error_free_prep;
>  }
>  EXPORT_SYMBOL(key_create_or_update);


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 1/4] KEYS: Defined an ima hook for measuring keys on key create or update
  2019-10-25 19:40   ` Mimi Zohar
@ 2019-10-25 19:49     ` Lakshmi Ramasubramanian
  2019-10-25 22:28     ` Lakshmi Ramasubramanian
  1 sibling, 0 replies; 14+ messages in thread
From: Lakshmi Ramasubramanian @ 2019-10-25 19:49 UTC (permalink / raw)
  To: Mimi Zohar, dhowells, casey, sashal, jamorris,
	linux-security-module, linux-integrity, linux-kernel, keyrings

On 10/25/2019 12:40 PM, Mimi Zohar wrote:

> On Wed, 2019-10-23 at 16:39 -0700, Lakshmi Ramasubramanian wrote:
>> Defined an ima hook to measure keys created or updated in the system.
> 
> "IMA" is an anacronym.  Unless it is a part of a function name, it
> should be capitalized.
Will fix that.

> 
> Before describing "what" you're doing, describe the problem.  For
> example, "The 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 named ima_key_create_or_update() to
> measure keys."
Agree - will update.


>> +
>> +	if (!ima_initialized)
>> +		return;
> 
> There's no reason to define a new variable to determine if IMA is
> initialized.  Use ima_policy_flag.
Will change it.

>  Like process_measurements, the test should be in process_buffer_measurement(), not here.

Currently, queuing of requests when IMA is not initialized is done for 
keys only. Moving that check inside process_buffer_measurement would 
mean handling queuing for all buffer measurements.

Can that be done as a separate patch set and not in this one?

>> @@ -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));
> 
> This patch defines the new IMA hook.  This call and the subsequent one
> below can be defined in a separate patch.  The subject line of that
> patch would be "keys: Add ima_key_create_or_update call to measure
> keys".
> 
> Mimi
Agree - will change it.

thanks,
  -lakshmi

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 1/4] KEYS: Defined an ima hook for measuring keys on key create or update
  2019-10-25 19:40   ` Mimi Zohar
  2019-10-25 19:49     ` Lakshmi Ramasubramanian
@ 2019-10-25 22:28     ` Lakshmi Ramasubramanian
  2019-10-27 14:47       ` Mimi Zohar
  1 sibling, 1 reply; 14+ messages in thread
From: Lakshmi Ramasubramanian @ 2019-10-25 22:28 UTC (permalink / raw)
  To: Mimi Zohar, dhowells, casey, sashal, jamorris,
	linux-security-module, linux-integrity, linux-kernel, keyrings

On 10/25/2019 12:40 PM, Mimi Zohar wrote:

>> +void ima_post_key_create_or_update(struct key *keyring, struct key *key,
>> +				   unsigned long flags, bool create)
>> +{
>> +	const struct public_key *pk;
>> +
>> +	if (key->type != &key_type_asymmetric)
>> +		return;
>> +
>> +	if (!ima_initialized)
>> +		return;
> 
> There's no reason to define a new variable to determine if IMA is
> initialized.  Use ima_policy_flag.  

Please correct me if I am wrong -

ima_policy_flag will be set to 0 if IMA is not yet initialized
OR
IMA is initialized, but ima_policy_flag could be still set to 0 (say, 
due to the configured policy).

In the latter case the measurement request should be a NOP immediately.

  -lakshmi


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 3/4] KEYS: Added BUILTIN_TRUSTED_KEYS enum to measure keys added to builtin_trusted_keys keyring
  2019-10-23 23:39 ` [PATCH v2 3/4] KEYS: Added BUILTIN_TRUSTED_KEYS enum to measure keys added to builtin_trusted_keys keyring Lakshmi Ramasubramanian
@ 2019-10-27 14:33   ` Mimi Zohar
  2019-10-28 15:12     ` Lakshmi Ramasubramanian
  2019-10-28 15:56     ` Lakshmi Ramasubramanian
  0 siblings, 2 replies; 14+ messages in thread
From: Mimi Zohar @ 2019-10-27 14:33 UTC (permalink / raw)
  To: Lakshmi Ramasubramanian, dhowells, casey, sashal, jamorris,
	linux-security-module, linux-integrity, linux-kernel, keyrings

On Wed, 2019-10-23 at 16:39 -0700, Lakshmi Ramasubramanian wrote:
> Added an ima policy hook BUILTIN_TRUSTED_KEYS to measure keys added
> to builtin_trusted_keys keyring.
> 
> Added a helper function to check if the given keyring is
> the builtin_trusted_keys keyring.
> 
> Defined a function to map the keyring to ima policy hook function
> and use it when measuring the key.
 
.builtin_trusted_keys is a trusted keyring, which is created by the
kernel.  It cannot be deleted or replaced by userspace, so it should
be possible to correlate a keyring name with a keyring number on
policy load.

Other examples of trusted keyrings are: .ima, .evm, .platform,
.blacklist, .builtin_regdb_keys.  Instead of defining a keyring
specific method of getting the keyring number, define a generic
method.  For example, the userspace command "keyctl describe
%keyring:.builtin_trusted_keys" searches /proc/keys, but the kernel
shouldn't need to access /proc/keys.

> 
> Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
> ---
>  Documentation/ABI/testing/ima_policy |  1 +
>  certs/system_keyring.c               |  5 +++++
>  include/keys/system_keyring.h        |  2 ++
>  security/integrity/ima/ima.h         |  2 ++
>  security/integrity/ima/ima_api.c     |  1 +
>  security/integrity/ima/ima_main.c    | 25 +++++++++++++++++++++++--
>  security/integrity/ima/ima_queue.c   |  2 +-
>  7 files changed, 35 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
> index fc376a323908..25566c74e679 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]
> +				[BUILTIN_TRUSTED_KEYS]

The .builtin_trusted_keys is the name of a keyring, not of an IMA
hook.  Define a new IMA policy "keyring=" option, where keyring is
optional.  Some IMA policy rules might look like:

# 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=11


>  			mask:= [[^]MAY_READ] [[^]MAY_WRITE] [[^]MAY_APPEND]
>  			       [[^]MAY_EXEC]
>  			fsmagic:= hex value
> 
> 
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index bce430b3386e..986f80eead4d 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -605,6 +605,24 @@ int ima_load_data(enum kernel_load_data_id id)
>  	return 0;
>  }
>  
> +/*
> + * Maps the given keyring to a IMA Hook.
> + * @keyring: A keyring to which a key maybe linked to.
> + *
> + * This function currently handles only builtin_trusted_keys.
> + * To handle more keyrings, this function, ima hook and
> + * ima policy handler need to be updated.
> + */
> +static enum ima_hooks keyring_policy_map(struct key *keyring)
> +{
> +	enum ima_hooks func = NONE;
> +
> +	if (is_builtin_trusted_keyring(keyring))
> +		func = BUILTIN_TRUSTED_KEYS;
> +
> +	return func;
> +}
> +
>  /*
>   * process_buffer_measurement - Measure the buffer to ima log.
>   * @buf: pointer to the buffer that needs to be added to the log.
> @@ -706,19 +724,22 @@ void ima_post_key_create_or_update(struct key *keyring, struct key *key,
>  				   unsigned long flags, bool create)
>  {
>  	const struct public_key *pk;
> +	enum ima_hooks func;
>  
>  	if (key->type != &key_type_asymmetric)
>  		return;
>  
> +	func = keyring_policy_map(keyring);
> +

"func", in this case, should be something like "KEYRING_CHECK".  No
mapping is necessary.

>  	if (!ima_initialized) {
> -		ima_queue_key_for_measurement(key, NONE);
> +		ima_queue_key_for_measurement(key, func);
>  		return;
>  	}
>  
>  	pk = key->payload.data[asym_crypto];
>  	process_buffer_measurement(pk->key, pk->keylen,
>  				   key->description,
> -				   NONE, 0);
> +				   func, 0);

Pass the "keyring" to process_buffer_measurement() and on to
ima_get_action(), so that ima_get_action() determines whether the
keyring is in policy.

Mimi

>  }
>  


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 1/4] KEYS: Defined an ima hook for measuring keys on key create or update
  2019-10-25 22:28     ` Lakshmi Ramasubramanian
@ 2019-10-27 14:47       ` Mimi Zohar
  2019-10-28 14:58         ` Lakshmi Ramasubramanian
  0 siblings, 1 reply; 14+ messages in thread
From: Mimi Zohar @ 2019-10-27 14:47 UTC (permalink / raw)
  To: Lakshmi Ramasubramanian, dhowells, casey, sashal, jamorris,
	linux-security-module, linux-integrity, linux-kernel, keyrings

On Fri, 2019-10-25 at 15:28 -0700, Lakshmi Ramasubramanian wrote:
> On 10/25/2019 12:40 PM, Mimi Zohar wrote:
> 
> >> +void ima_post_key_create_or_update(struct key *keyring, struct key *key,
> >> +				   unsigned long flags, bool create)
> >> +{
> >> +	const struct public_key *pk;
> >> +
> >> +	if (key->type != &key_type_asymmetric)
> >> +		return;
> >> +
> >> +	if (!ima_initialized)
> >> +		return;
> > 
> > There's no reason to define a new variable to determine if IMA is
> > initialized.  Use ima_policy_flag.  
> 
> Please correct me if I am wrong -
> 
> ima_policy_flag will be set to 0 if IMA is not yet initialized
> OR
> IMA is initialized, but ima_policy_flag could be still set to 0 (say, 
> due to the configured policy).
> 
> In the latter case the measurement request should be a NOP immediately.

I'm not sure.  The builtin keys most likely will be loaded prior to a
custom IMA policy containing "keyring" rules are defined.

Mimi


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 1/4] KEYS: Defined an ima hook for measuring keys on key create or update
  2019-10-27 14:47       ` Mimi Zohar
@ 2019-10-28 14:58         ` Lakshmi Ramasubramanian
  0 siblings, 0 replies; 14+ messages in thread
From: Lakshmi Ramasubramanian @ 2019-10-28 14:58 UTC (permalink / raw)
  To: Mimi Zohar, dhowells, casey, sashal, jamorris,
	linux-security-module, linux-integrity, linux-kernel, keyrings

On 10/27/19 7:47 AM, Mimi Zohar wrote:

>>> There's no reason to define a new variable to determine if IMA is
>>> initialized.  Use ima_policy_flag.
>>
>> Please correct me if I am wrong -
>>
>> ima_policy_flag will be set to 0 if IMA is not yet initialized
>> OR
>> IMA is initialized, but ima_policy_flag could be still set to 0 (say,
>> due to the configured policy).
>>
>> In the latter case the measurement request should be a NOP immediately.
> 
> I'm not sure.  The builtin keys most likely will be loaded prior to a
> custom IMA policy containing "keyring" rules are defined.
> 
> Mimi

I am not sure if I described it clearly - let me clarify:

Say, we use ima_policy_flag to determine whether to
measure the key immediately or
queue the key for measurement and, measure when IMA is initialized.

We can incorrectly keep queuing keys in the case when IMA is 
initialized, but due to the way IMA policy is configured ima_policy_flag 
is still 0.

That's why I feel a separate boolean flag would be needed to know 
whether IMA is initialized or not.

If IMA is initialized, ima_policy_flag will dictate whether to measure 
the key or not.

thanks,
  -lakshmi


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 3/4] KEYS: Added BUILTIN_TRUSTED_KEYS enum to measure keys added to builtin_trusted_keys keyring
  2019-10-27 14:33   ` Mimi Zohar
@ 2019-10-28 15:12     ` Lakshmi Ramasubramanian
  2019-10-28 17:08       ` Mimi Zohar
  2019-10-28 15:56     ` Lakshmi Ramasubramanian
  1 sibling, 1 reply; 14+ messages in thread
From: Lakshmi Ramasubramanian @ 2019-10-28 15:12 UTC (permalink / raw)
  To: Mimi Zohar, dhowells, casey, sashal, jamorris,
	linux-security-module, linux-integrity, linux-kernel, keyrings

On 10/27/19 7:33 AM, Mimi Zohar wrote:

> .builtin_trusted_keys is a trusted keyring, which is created by the
> kernel.  It cannot be deleted or replaced by userspace, so it should
> be possible to correlate a keyring name with a keyring number on
> policy load.

Yes - at policy load we can map a keyring name to a keyring number.

But at runtime we still need to know if the keyring parameter passed to 
the IMA hook function is configured to be measured.

void ima_post_key_create_or_update(struct key *keyring, struct key *key,
				   unsigned long flags, bool create);
{
    => Get the keyring number for the given "keyring".
    => Check if the keyring number is in the configured IMA policy.
    => If yes, measure the key.
    => Else, do nothing.
}

Did I misunderstand what you had stated?

>> diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
>> index fc376a323908..25566c74e679 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]
>> +				[BUILTIN_TRUSTED_KEYS]
> 
> The .builtin_trusted_keys is the name of a keyring, not of an IMA
> hook.  Define a new IMA policy "keyring=" option, where keyring is
> optional.  Some IMA policy rules might look like:
> 
> # 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=11

I agree - I'll take a look at this.

> "func", in this case, should be something like "KEYRING_CHECK".  No
> mapping is necessary.
Agree.

> 
>>   	if (!ima_initialized) {
>> -		ima_queue_key_for_measurement(key, NONE);
>> +		ima_queue_key_for_measurement(key, func);
>>   		return;
>>   	}
>>   
>>   	pk = key->payload.data[asym_crypto];
>>   	process_buffer_measurement(pk->key, pk->keylen,
>>   				   key->description,
>> -				   NONE, 0);
>> +				   func, 0);
> 
> Pass the "keyring" to process_buffer_measurement() and on to
> ima_get_action(), so that ima_get_action() determines whether the
> keyring is in policy.

Agree.

thanks,
  -lakshmi



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 3/4] KEYS: Added BUILTIN_TRUSTED_KEYS enum to measure keys added to builtin_trusted_keys keyring
  2019-10-27 14:33   ` Mimi Zohar
  2019-10-28 15:12     ` Lakshmi Ramasubramanian
@ 2019-10-28 15:56     ` Lakshmi Ramasubramanian
  1 sibling, 0 replies; 14+ messages in thread
From: Lakshmi Ramasubramanian @ 2019-10-28 15:56 UTC (permalink / raw)
  To: Mimi Zohar, dhowells, casey, sashal, jamorris,
	linux-security-module, linux-integrity, linux-kernel, keyrings

On 10/27/19 7:33 AM, Mimi Zohar wrote:

> Other examples of trusted keyrings are: .ima, .evm, .platform,
> .blacklist, .builtin_regdb_keys.  Instead of defining a keyring
> specific method of getting the keyring number, define a generic
> method.  For example, the userspace command "keyctl describe
> %keyring:.builtin_trusted_keys" searches /proc/keys, but the kernel
> shouldn't need to access /proc/keys.

"description" field in "struct key" is set to ".builtin_trusted_keys" 
for Built-In Trusted Keys keyring.

Similarly, for other keyrings such as .ima, .evm, .blacklist, 
.builtin_regdb_keys, etc.

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

With IMA policy set like above, the keyring to keyring number mapping 
can be set at IMA policy load.

My earlier point about mapping the "keyring" to "keyring number" at 
runtime (when the IMA hook is called) still needs to be done to know if 
the given keyring is in the policy or not.

void ima_post_key_create_or_update(struct key *keyring, struct key *key,
				   unsigned long flags, bool create)

thanks,
  -lakshmi

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 3/4] KEYS: Added BUILTIN_TRUSTED_KEYS enum to measure keys added to builtin_trusted_keys keyring
  2019-10-28 15:12     ` Lakshmi Ramasubramanian
@ 2019-10-28 17:08       ` Mimi Zohar
  0 siblings, 0 replies; 14+ messages in thread
From: Mimi Zohar @ 2019-10-28 17:08 UTC (permalink / raw)
  To: Lakshmi Ramasubramanian, dhowells, casey, sashal, jamorris,
	linux-security-module, linux-integrity, linux-kernel, keyrings

On Mon, 2019-10-28 at 08:12 -0700, Lakshmi Ramasubramanian wrote:
> On 10/27/19 7:33 AM, Mimi Zohar wrote:
> 
> > .builtin_trusted_keys is a trusted keyring, which is created by the
> > kernel.  It cannot be deleted or replaced by userspace, so it should
> > be possible to correlate a keyring name with a keyring number on
> > policy load.
> 
> Yes - at policy load we can map a keyring name to a keyring number.
> 
> But at runtime we still need to know if the keyring parameter passed to 
> the IMA hook function is configured to be measured.
> 
> void ima_post_key_create_or_update(struct key *keyring, struct key *key,
> 				   unsigned long flags, bool create);
> {
>     => Get the keyring number for the given "keyring".

There is no "getting" involved here.  Pass "keyring" to
process_buffer_measurement and on to ima_get_action().

>     => Check if the keyring number is in the configured IMA policy.

ima_get_action() should do a simple compare of the valued stored in
the IMA policy with the value returned by key_serial().

Mimi

>     => If yes, measure the key.
>     => Else, do nothing.
> }

> Did I misunderstand what you had stated?


^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2019-10-28 17:08 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-23 23:39 [PATCH v2 0/4] KEYS: measure keys when they are created or updated Lakshmi Ramasubramanian
2019-10-23 23:39 ` [PATCH v2 1/4] KEYS: Defined an ima hook for measuring keys on key create or update Lakshmi Ramasubramanian
2019-10-25 19:40   ` Mimi Zohar
2019-10-25 19:49     ` Lakshmi Ramasubramanian
2019-10-25 22:28     ` Lakshmi Ramasubramanian
2019-10-27 14:47       ` Mimi Zohar
2019-10-28 14:58         ` Lakshmi Ramasubramanian
2019-10-23 23:39 ` [PATCH v2 2/4] KEYS: Queue key for measurement if ima is not initialized. Measure queued keys when ima is initialized Lakshmi Ramasubramanian
2019-10-23 23:39 ` [PATCH v2 3/4] KEYS: Added BUILTIN_TRUSTED_KEYS enum to measure keys added to builtin_trusted_keys keyring Lakshmi Ramasubramanian
2019-10-27 14:33   ` Mimi Zohar
2019-10-28 15:12     ` Lakshmi Ramasubramanian
2019-10-28 17:08       ` Mimi Zohar
2019-10-28 15:56     ` Lakshmi Ramasubramanian
2019-10-23 23:39 ` [PATCH v2 4/4] KEYS: Enabled ima policy " Lakshmi Ramasubramanian

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).