| Message ID | 20191015231750.25992-2-nramas@linux.microsoft.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KEYS: LSM Hook for key_create_or_update | expand |
On Tue, 2019-10-15 at 16:17 -0700, Lakshmi Ramasubramanian wrote: > Add a LSM Hook for key_create_or_update function. The motive behind > this change is enable subsystems to receive notification when > a new key is created or updated. As per Documentation/process/submitting-patches.rst section "2) Describe your changes", please begin the patch description by describing the problem. > > IMA will be one of the subsystems that will use this hook to measure > the newly created key. > > This change set includes helper functions to check if the keyring > (to which a new key is being added, for example) is one of > the trusted keys keyring (builtin, secondary, or platform). > > The change set also includes an IMA function that will be called > from the LSM hook to notify of the newly created key and the keyring > to which the key is being added to. > > Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com> This patch should be broken up even further.[1] In this case to simplify review, separate defining the new LSM hook from any IMA code. Different maintainers need to Ack/sign off on these patches. The new LSM hook patch, with a clear well written patch description, should be posted on the LSM mailing list as well. [1] Refer to Documentation/process/submitting-patches.rst section 3) "Separate your changes". > --- > certs/system_keyring.c | 23 +++++++++++++++++++++++ > include/keys/system_keyring.h | 4 ++++ > include/linux/ima.h | 8 ++++++++ > include/linux/lsm_hooks.h | 14 ++++++++++++++ > include/linux/security.h | 10 ++++++++++ > security/integrity/ima/ima.h | 1 + > security/integrity/ima/ima_main.c | 19 +++++++++++++++++++ > security/keys/key.c | 8 ++++++++ > security/security.c | 11 +++++++++++ > 9 files changed, 98 insertions(+) > > diff --git a/certs/system_keyring.c b/certs/system_keyring.c > index 1eba08a1af82..a89d23fb5d9d 100644 > --- a/certs/system_keyring.c > +++ b/certs/system_keyring.c > @@ -283,3 +283,26 @@ 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); > +} > + > +inline bool is_secondary_trusted_keyring(struct key *keyring) > +{ > + #ifdef CONFIG_SECONDARY_TRUSTED_KEYRING > + return (keyring == secondary_trusted_keys); > + #else > + return false; > + #endif > +} > + > +inline bool is_platform_trusted_keyring(struct key *keyring) > +{ > + #ifdef CONFIG_INTEGRITY_PLATFORM_KEYRING > + return (keyring == platform_trusted_keys); > + #else > + return false; > + #endif > +} Why are these functions defined in a new LSM hook patch? Before posting a patch, please review the patch line by line, making sure that there isn't anything extraneous. > diff --git a/include/keys/system_keyring.h b/include/keys/system_keyring.h > index c1a96fdf598b..2517181d8d6c 100644 > --- a/include/keys/system_keyring.h > +++ b/include/keys/system_keyring.h > @@ -66,4 +66,8 @@ static inline void set_platform_trusted_keys(struct key *keyring) > } > #endif > > +extern bool is_builtin_trusted_keyring(struct key *keyring); > +extern bool is_secondary_trusted_keyring(struct key *keyring); > +extern bool is_platform_trusted_keyring(struct key *keyring); > + Not needed in this patch. Mimi
On 10/15/19 5:30 PM, Mimi Zohar wrote: > As per Documentation/process/submitting-patches.rst section "2) > Describe your changes", please begin the patch description by > describing the problem. Will do. > > This patch should be broken up even further.[1] In this case to > simplify review, separate defining the new LSM hook from any IMA code. > Different maintainers need to Ack/sign off on these patches. > > The new LSM hook patch, with a clear well written patch description, > should be posted on the LSM mailing list as well. Will do >> + >> +inline bool is_platform_trusted_keyring(struct key *keyring) >> +{ >> + #ifdef CONFIG_INTEGRITY_PLATFORM_KEYRING >> + return (keyring == platform_trusted_keys); >> + #else >> + return false; >> + #endif >> +} > > Why are these functions defined in a new LSM hook patch? Before > posting a patch, please review the patch line by line, making sure > that there isn't anything extraneous. Since these are helper functions that will be used by IMA (which I will post shortly), I thought it is appropriate to include this. Sorry about that - I'll move it out of this patch set. Will send an updated change today. thanks, -lakshmi
On Tue, 15 Oct 2019, Lakshmi Ramasubramanian wrote: > +inline bool is_secondary_trusted_keyring(struct key *keyring) > +{ > + #ifdef CONFIG_SECONDARY_TRUSTED_KEYRING > + return (keyring == secondary_trusted_keys); > + #else > + return false; > + #endif > +} > + > +inline bool is_platform_trusted_keyring(struct key *keyring) > +{ > + #ifdef CONFIG_INTEGRITY_PLATFORM_KEYRING > + return (keyring == platform_trusted_keys); > + #else > + return false; > + #endif > +} See "Conditional Compilation" in Documentation/process/coding-style.rst i.e. compile out at the function level, in the header file for these, do not indent the directives, add a matching comment for the #endif.
diff --git a/certs/system_keyring.c b/certs/system_keyring.c index 1eba08a1af82..a89d23fb5d9d 100644 --- a/certs/system_keyring.c +++ b/certs/system_keyring.c @@ -283,3 +283,26 @@ 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); +} + +inline bool is_secondary_trusted_keyring(struct key *keyring) +{ + #ifdef CONFIG_SECONDARY_TRUSTED_KEYRING + return (keyring == secondary_trusted_keys); + #else + return false; + #endif +} + +inline bool is_platform_trusted_keyring(struct key *keyring) +{ + #ifdef CONFIG_INTEGRITY_PLATFORM_KEYRING + return (keyring == platform_trusted_keys); + #else + return false; + #endif +} diff --git a/include/keys/system_keyring.h b/include/keys/system_keyring.h index c1a96fdf598b..2517181d8d6c 100644 --- a/include/keys/system_keyring.h +++ b/include/keys/system_keyring.h @@ -66,4 +66,8 @@ static inline void set_platform_trusted_keys(struct key *keyring) } #endif +extern bool is_builtin_trusted_keyring(struct key *keyring); +extern bool is_secondary_trusted_keyring(struct key *keyring); +extern bool is_platform_trusted_keyring(struct key *keyring); + #endif /* _KEYS_SYSTEM_KEYRING_H */ diff --git a/include/linux/ima.h b/include/linux/ima.h index a20ad398d260..3fbeca697a1f 100644 --- a/include/linux/ima.h +++ b/include/linux/ima.h @@ -25,6 +25,8 @@ extern int ima_post_read_file(struct file *file, void *buf, loff_t size, extern void ima_post_path_mknod(struct dentry *dentry); extern void ima_kexec_cmdline(const void *buf, int size); +extern int ima_post_key_create_or_update(struct key *keyring, + struct key *key); #ifdef CONFIG_IMA_KEXEC extern void ima_add_kexec_buffer(struct kimage *image); #endif @@ -91,6 +93,12 @@ static inline void ima_post_path_mknod(struct dentry *dentry) } static inline void ima_kexec_cmdline(const void *buf, int size) {} + +static inline int ima_post_key_create_or_update(struct key *keyring, + struct key *key) +{ + return 0; +} #endif /* CONFIG_IMA */ #ifndef CONFIG_IMA_KEXEC diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h index df1318d85f7d..a7f680e05d78 100644 --- a/include/linux/lsm_hooks.h +++ b/include/linux/lsm_hooks.h @@ -1066,6 +1066,16 @@ * * Security hooks affecting all Key Management operations * + * @key_create_or_update: + * Notification of key create or update. + * @keyring points to the keyring to which the key belongs + * @key points to the key being created or updated + * @cred current cred + * @flags is the allocation flags + * @builtin_or_secondary flag indicating whether + * the keyring to which the key belongs is the builtin + * or secondary trusted keys keyring + * Return 0 if permission is granted, -ve error otherwise. * @key_alloc: * Permit allocation of a key and assign security data. Note that key does * not have a serial number assigned at this point. @@ -1781,6 +1791,9 @@ union security_list_options { /* key management security hooks */ #ifdef CONFIG_KEYS + int (*key_create_or_update)(struct key *keyring, struct key *key, + const struct cred *cred, + unsigned long flags); int (*key_alloc)(struct key *key, const struct cred *cred, unsigned long flags); void (*key_free)(struct key *key); @@ -2026,6 +2039,7 @@ struct security_hook_heads { struct hlist_head xfrm_decode_session; #endif /* CONFIG_SECURITY_NETWORK_XFRM */ #ifdef CONFIG_KEYS + struct hlist_head key_create_or_update; struct hlist_head key_alloc; struct hlist_head key_free; struct hlist_head key_permission; diff --git a/include/linux/security.h b/include/linux/security.h index 5f7441abbf42..e1cc1c703623 100644 --- a/include/linux/security.h +++ b/include/linux/security.h @@ -1672,6 +1672,9 @@ static inline int security_path_chroot(const struct path *path) #ifdef CONFIG_KEYS #ifdef CONFIG_SECURITY +int security_key_create_or_update(struct key *keyring, struct key *key, + const struct cred *cred, + unsigned long flags); int security_key_alloc(struct key *key, const struct cred *cred, unsigned long flags); void security_key_free(struct key *key); int security_key_permission(key_ref_t key_ref, @@ -1680,6 +1683,13 @@ int security_key_getsecurity(struct key *key, char **_buffer); #else +int security_key_create_or_update(struct key *keyring, struct key *key, + const struct cred *cred, + unsigned long flags) +{ + return 0; +} + static inline int security_key_alloc(struct key *key, const struct cred *cred, unsigned long flags) diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h index b6847ee1f47a..8b519c7a6fed 100644 --- a/security/integrity/ima/ima.h +++ b/security/integrity/ima/ima.h @@ -21,6 +21,7 @@ #include <linux/tpm.h> #include <linux/audit.h> #include <crypto/hash_info.h> +#include <keys/asymmetric-type.h> #include "../integrity.h" diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index 3d5ce4fd4dcc..b77e489cdce7 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -678,6 +678,25 @@ 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 + * IMA hook called when a new key is created. + * This function will measure the key. + * + * On success return 0. + * Return appropriate error code on error + */ +int ima_post_key_create_or_update(struct key *keyring, struct key *key) +{ + if (key->type != &key_type_asymmetric) + return 0; + + /* TODO: Call the function to measure the key */ + return 0; +} + static int __init init_ima(void) { int error; diff --git a/security/keys/key.c b/security/keys/key.c index 764f4c57913e..909554f4d15f 100644 --- a/security/keys/key.c +++ b/security/keys/key.c @@ -936,6 +936,14 @@ key_ref_t key_create_or_update(key_ref_t keyring_ref, goto error_link_end; } + /* let the security module know about the key */ + ret = security_key_create_or_update(keyring, key, cred, flags); + if (ret < 0) { + key_put(key); + key_ref = ERR_PTR(ret); + goto error_link_end; + } + key_ref = make_key_ref(key, is_key_possessed(keyring_ref)); error_link_end: diff --git a/security/security.c b/security/security.c index 250ee2d76406..a6367abb0c76 100644 --- a/security/security.c +++ b/security/security.c @@ -2280,6 +2280,17 @@ EXPORT_SYMBOL(security_skb_classify_flow); #ifdef CONFIG_KEYS +int security_key_create_or_update(struct key *keyring, struct key *key, + const struct cred *cred, + unsigned long flags) +{ + int rc = call_int_hook(key_create_or_update, 0, + keyring, key, cred, flags); + if (rc) + return rc; + return ima_post_key_create_or_update(keyring, key); +} + int security_key_alloc(struct key *key, const struct cred *cred, unsigned long flags) {
Add a LSM Hook for key_create_or_update function. The motive behind this change is enable subsystems to receive notification when a new key is created or updated. IMA will be one of the subsystems that will use this hook to measure the newly created key. This change set includes helper functions to check if the keyring (to which a new key is being added, for example) is one of the trusted keys keyring (builtin, secondary, or platform). The change set also includes an IMA function that will be called from the LSM hook to notify of the newly created key and the keyring to which the key is being added to. Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com> --- certs/system_keyring.c | 23 +++++++++++++++++++++++ include/keys/system_keyring.h | 4 ++++ include/linux/ima.h | 8 ++++++++ include/linux/lsm_hooks.h | 14 ++++++++++++++ include/linux/security.h | 10 ++++++++++ security/integrity/ima/ima.h | 1 + security/integrity/ima/ima_main.c | 19 +++++++++++++++++++ security/keys/key.c | 8 ++++++++ security/security.c | 11 +++++++++++ 9 files changed, 98 insertions(+)