[v1,5/6] KEYS: measure queued keys
diff mbox series

Message ID 20191023001818.3684-6-nramas@linux.microsoft.com
State New
Headers show
Series
  • KEYS: measure keys when they are created or updated
Related show

Commit Message

Lakshmi Ramasubramanian Oct. 23, 2019, 12:18 a.m. UTC
Call process_buffer_measurement to measure keys that
are added and updated in the system.

Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
---
 security/integrity/ima/ima_main.c  | 23 +++++++++++++++++++++
 security/integrity/ima/ima_queue.c | 32 ++++++++++++++++++++++++++++++
 2 files changed, 55 insertions(+)

Comments

Mimi Zohar Oct. 23, 2019, 1:23 p.m. UTC | #1
On Tue, 2019-10-22 at 17:18 -0700, Lakshmi Ramasubramanian wrote:
> Call process_buffer_measurement to measure keys that
> are added and updated in the system.

This patch description doesn't describe what the patch actually does
(eg. it not only calls process_buffer_measurement, but defines the IMA
hook itself.)

The ordering of this patch set is awkward.  It should first introduce
a generic method for measuring keys based on the keyring.  Then add
the additional support needed for the specific builtin_trusted_keys
keyring usecase.

> 
> Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
> ---
>  security/integrity/ima/ima_main.c  | 23 +++++++++++++++++++++
>  security/integrity/ima/ima_queue.c | 32 ++++++++++++++++++++++++++++++
>  2 files changed, 55 insertions(+)
> 
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 8e965d18fb21..7c2afb954f19 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -678,6 +678,29 @@ void ima_kexec_cmdline(const void *buf, int size)
>  	}
>  }
>  
> +/*
> + * ima_post_key_create_or_update
> + * @keyring points to the keyring to which the key belongs
> + * @key points to the key being created or updated
> + * @cred cred structure
> + * @flags flags passed to key_create_or_update function
> + * @create flag to indicate whether the key was created or updated
> + *
> + * IMA hook called when a new key is created or updated.
> + *
> + * On success return 0.
> + * Return appropriate error code on error
> + */
> +int ima_post_key_create_or_update(struct key *keyring, struct key *key,
> +				  const struct cred *cred,
> +				  unsigned long flags, bool create)
> +{
> +	if (key->type != &key_type_asymmetric)
> +		return 0;
> +
> +	return ima_measure_key(keyring, key);
> +}
> +

Here is the new IMA hook, not "[PATCH v1 3/6] KEYS: ima hook to
measure builtin_trusted_keys".  The new hook should call
process_buffer_measurement() directly.  A subsequent patch, based on
the keyring, would determine if it needs to be queued.

Mimi


>  static int __init init_ima(void)
>  {
>  	int error;
> diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c
> index a262e289615b..0da11a292f99 100644
> --- a/security/integrity/ima/ima_queue.c
> +++ b/security/integrity/ima/ima_queue.c
> @@ -322,7 +322,12 @@ static struct ima_trusted_key_entry *ima_alloc_trusted_queue_entry(
>  int ima_measure_key(struct key *keyring, struct key *key)
>  {
>  	int rc = 0;
> +	int pcr = CONFIG_IMA_MEASURE_PCR_IDX;
> +	struct ima_template_desc *template_desc = ima_template_desc_current();
> +	int action;
>  	struct ima_trusted_key_entry *entry = NULL;
> +	const struct public_key *pk;
> +	u32 secid;
>  	enum ima_hooks func;
>  	bool queued = false;
>  
> @@ -344,16 +349,43 @@ int ima_measure_key(struct key *keyring, struct key *key)
>  
>  	mutex_unlock(&ima_trusted_keys_mutex);
>  
> +	if ((rc == 0) && !queued) {
> +		security_task_getsecid(current, &secid);
> +		action = ima_get_action(NULL, current_cred(), secid, 0,
> +					func, &pcr, &template_desc);
> +		if (action & IMA_MEASURE) {
> +			pk = key->payload.data[asym_crypto];
> +			process_buffer_measurement(pk->key, pk->keylen,
> +						   key->description,
> +						   pcr, template_desc);
> +		}
> +	}
> +
>  	return rc;
>  }
>  
>  void ima_measure_queued_trusted_keys(void)
>  {
>  	struct ima_trusted_key_entry *entry, *tmp;
> +	int pcr = CONFIG_IMA_MEASURE_PCR_IDX;
> +	struct ima_template_desc *template_desc = ima_template_desc_current();
> +	int action;
> +	u32 secid;
>  
>  	mutex_lock(&ima_trusted_keys_mutex);
>  
>  	list_for_each_entry_safe(entry, tmp, &ima_trusted_keys, list) {
> +		security_task_getsecid(current, &secid);
> +		action = ima_get_action(NULL, current_cred(), secid, 0,
> +					entry->func, &pcr,
> +					&template_desc);
> +		if (action & IMA_MEASURE) {
> +			process_buffer_measurement(entry->public_key,
> +						   entry->public_key_len,
> +						   entry->key_description,
> +						   pcr,
> +						   template_desc);
> +		}
>  		list_del(&entry->list);
>  		ima_free_trusted_key_entry(entry);
>  	}
Lakshmi Ramasubramanian Oct. 23, 2019, 5:34 p.m. UTC | #2
On 10/23/19 6:23 AM, Mimi Zohar wrote:

> The ordering of this patch set is awkward.  It should first introduce
> a generic method for measuring keys based on the keyring.  Then add
> the additional support needed for the specific builtin_trusted_keys
> keyring usecase.

Would the following ordering of the patch set be acceptable:

  => PATCH 0/5: Cover letter

  => PATCH 1/5: Define the enum "hook(BUILTIN_TRUSTED_KEYS)" in ima.h

  => PATCH 2/5: Define ima hook
                This will initially do nothing if ima is not yet
                initialized.
                Call process_buffer_measurement() if ima is initialized.

  => PATCH 3/5: key_create_or_update change and the call to ima hook

  => PATCH 4/5: Queue\De-Queue of key measurement requests.
                Enable queuing of key in the ima hook if ima is not
                initialized.

  => PATCH 5/5: ima policy to enable measurement of keys which will
                enable end-to-end working of this feature.

thanks,
  -lakshmi
Mimi Zohar Oct. 23, 2019, 5:52 p.m. UTC | #3
On Wed, 2019-10-23 at 10:34 -0700, Lakshmi Ramasubramanian wrote:
> On 10/23/19 6:23 AM, Mimi Zohar wrote:
> 
> > The ordering of this patch set is awkward.  It should first introduce
> > a generic method for measuring keys based on the keyring.  Then add
> > the additional support needed for the specific builtin_trusted_keys
> > keyring usecase.
> 
> Would the following ordering of the patch set be acceptable:
> 
>   => PATCH 0/5: Cover letter
> 
>   => PATCH 1/5: Define the enum "hook(BUILTIN_TRUSTED_KEYS)" in ima.h
> 
>   => PATCH 2/5: Define ima hook
>                 This will initially do nothing if ima is not yet
>                 initialized.
>                 Call process_buffer_measurement() if ima is initialized.
> 
>   => PATCH 3/5: key_create_or_update change and the call to ima hook
> 
>   => PATCH 4/5: Queue\De-Queue of key measurement requests.
>                 Enable queuing of key in the ima hook if ima is not
>                 initialized.
> 
>   => PATCH 5/5: ima policy to enable measurement of keys which will
>                 enable end-to-end working of this feature.

The first patches need to introduce the generic concept of measuring
keys based on policy.  Only afterwards would you add any builtin
trusted keyring specific code.

Mimi
Mimi Zohar Oct. 23, 2019, 6:49 p.m. UTC | #4
On Wed, 2019-10-23 at 13:52 -0400, Mimi Zohar wrote:
> On Wed, 2019-10-23 at 10:34 -0700, Lakshmi Ramasubramanian wrote:
> > On 10/23/19 6:23 AM, Mimi Zohar wrote:
> > 
> > > The ordering of this patch set is awkward.  It should first introduce
> > > a generic method for measuring keys based on the keyring.  Then add
> > > the additional support needed for the specific builtin_trusted_keys
> > > keyring usecase.
> > 
> > Would the following ordering of the patch set be acceptable:
> > 
> >   => PATCH 0/5: Cover letter
> > 
> >   => PATCH 1/5: Define the enum "hook(BUILTIN_TRUSTED_KEYS)" in ima.h
> > 
> >   => PATCH 2/5: Define ima hook
> >                 This will initially do nothing if ima is not yet
> >                 initialized.
> >                 Call process_buffer_measurement() if ima is initialized.
> > 
> >   => PATCH 3/5: key_create_or_update change and the call to ima hook
> > 
> >   => PATCH 4/5: Queue\De-Queue of key measurement requests.
> >                 Enable queuing of key in the ima hook if ima is not
> >                 initialized.
> > 
> >   => PATCH 5/5: ima policy to enable measurement of keys which will
> >                 enable end-to-end working of this feature.
> 
> The first patches need to introduce the generic concept of measuring
> keys based on policy.  Only afterwards would you add any builtin
> trusted keyring specific code.

1. Extend the IMA policy language to support identifying keyrings
2. Define a new IMA hook which calls process_buffer_measurement()
3. Call the new IMA hook (eg. from post_key_create_or_update)
4. Define an early workqueue for saving keys loaded prior to IMA is
initialized.  (Remember we don't hard code policy in the kernel.)

I'll be pushing out linux-integrity shortly.  For the time being,
please base your patches on -rc3.

thanks,

Mimi

Patch
diff mbox series

diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 8e965d18fb21..7c2afb954f19 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -678,6 +678,29 @@  void ima_kexec_cmdline(const void *buf, int size)
 	}
 }
 
+/*
+ * ima_post_key_create_or_update
+ * @keyring points to the keyring to which the key belongs
+ * @key points to the key being created or updated
+ * @cred cred structure
+ * @flags flags passed to key_create_or_update function
+ * @create flag to indicate whether the key was created or updated
+ *
+ * IMA hook called when a new key is created or updated.
+ *
+ * On success return 0.
+ * Return appropriate error code on error
+ */
+int ima_post_key_create_or_update(struct key *keyring, struct key *key,
+				  const struct cred *cred,
+				  unsigned long flags, bool create)
+{
+	if (key->type != &key_type_asymmetric)
+		return 0;
+
+	return ima_measure_key(keyring, key);
+}
+
 static int __init init_ima(void)
 {
 	int error;
diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c
index a262e289615b..0da11a292f99 100644
--- a/security/integrity/ima/ima_queue.c
+++ b/security/integrity/ima/ima_queue.c
@@ -322,7 +322,12 @@  static struct ima_trusted_key_entry *ima_alloc_trusted_queue_entry(
 int ima_measure_key(struct key *keyring, struct key *key)
 {
 	int rc = 0;
+	int pcr = CONFIG_IMA_MEASURE_PCR_IDX;
+	struct ima_template_desc *template_desc = ima_template_desc_current();
+	int action;
 	struct ima_trusted_key_entry *entry = NULL;
+	const struct public_key *pk;
+	u32 secid;
 	enum ima_hooks func;
 	bool queued = false;
 
@@ -344,16 +349,43 @@  int ima_measure_key(struct key *keyring, struct key *key)
 
 	mutex_unlock(&ima_trusted_keys_mutex);
 
+	if ((rc == 0) && !queued) {
+		security_task_getsecid(current, &secid);
+		action = ima_get_action(NULL, current_cred(), secid, 0,
+					func, &pcr, &template_desc);
+		if (action & IMA_MEASURE) {
+			pk = key->payload.data[asym_crypto];
+			process_buffer_measurement(pk->key, pk->keylen,
+						   key->description,
+						   pcr, template_desc);
+		}
+	}
+
 	return rc;
 }
 
 void ima_measure_queued_trusted_keys(void)
 {
 	struct ima_trusted_key_entry *entry, *tmp;
+	int pcr = CONFIG_IMA_MEASURE_PCR_IDX;
+	struct ima_template_desc *template_desc = ima_template_desc_current();
+	int action;
+	u32 secid;
 
 	mutex_lock(&ima_trusted_keys_mutex);
 
 	list_for_each_entry_safe(entry, tmp, &ima_trusted_keys, list) {
+		security_task_getsecid(current, &secid);
+		action = ima_get_action(NULL, current_cred(), secid, 0,
+					entry->func, &pcr,
+					&template_desc);
+		if (action & IMA_MEASURE) {
+			process_buffer_measurement(entry->public_key,
+						   entry->public_key_len,
+						   entry->key_description,
+						   pcr,
+						   template_desc);
+		}
 		list_del(&entry->list);
 		ima_free_trusted_key_entry(entry);
 	}