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