| Message ID | 20191023001818.3684-6-nramas@linux.microsoft.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KEYS: measure keys when they are created or updated | expand |
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); > }
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
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
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
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); }
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(+)