Re: [RFC] kexec: Allow kexec_file() with appropriate IMA policy when locked down

[Mimi Zohar <zohar@linux.ibm.com>]

On Fri, 2019-03-15 at 15:03 -0700, Matthew Garrett wrote:
> Systems in lockdown mode should block the kexec of untrusted kernels.
> For x86 and ARM we can ensure that a kernel is trustworthy by validating
> a PE signature, but this isn't possible on other architectures. On those
> platforms we can use IMA digital signatures instead. Add a function to
> determine whether IMA will verify signatures for a given event type,

In both the kexec and kernel modules cases, this should be in the past
tense.  Perhaps change it to something like, "whether IMA has already
or will verify signatures ...".

>  and
> if so permit kexec_file() even if the kernel is otherwise locked down.
> This is restricted to cases where CONFIG_INTEGRITY_TRUSTED_KEYRING is set
> in order to prevent an attacker from loading additional keys at runtime.
> 
> Signed-off-by: Matthew Garrett <mjg59@google.com>
> ---
>  include/linux/evm.h                 |  6 ++++
>  include/linux/ima.h                 | 28 +++++++++++++++++++
>  kernel/kexec_file.c                 |  9 ++++--
>  security/integrity/evm/evm_main.c   |  2 +-
>  security/integrity/ima/ima.h        | 20 +-------------
>  security/integrity/ima/ima_policy.c | 43 +++++++++++++++++++++++++++++
>  6 files changed, 86 insertions(+), 22 deletions(-)
> 
> diff --git a/include/linux/evm.h b/include/linux/evm.h
> index 8302bc29bb35..6e89d046b716 100644
> --- a/include/linux/evm.h
> +++ b/include/linux/evm.h
> @@ -15,6 +15,7 @@
>  struct integrity_iint_cache;
>  
>  #ifdef CONFIG_EVM
> +extern bool evm_key_loaded(void);
>  extern int evm_set_key(void *key, size_t keylen);
>  extern enum integrity_status evm_verifyxattr(struct dentry *dentry,
>  					     const char *xattr_name,
> @@ -45,6 +46,11 @@ static inline int posix_xattr_acl(const char *xattrname)
>  #endif
>  #else
>  
> +static inline bool evm_key_loaded(void)
> +{
> +	return false;
> +}
> +

Remove remaining EVM fragment.

>  static inline int evm_set_key(void *key, size_t keylen)
>  {
>  	return -EOPNOTSUPP;
> diff --git a/include/linux/ima.h b/include/linux/ima.h
> index dc12fbcf484c..a42e2a9a08b7 100644
> --- a/include/linux/ima.h
> +++ b/include/linux/ima.h
> @@ -27,6 +27,25 @@ 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);
>  
> +#define __ima_hooks(hook)		\
> +	hook(NONE)			\
> +	hook(FILE_CHECK)		\
> +	hook(MMAP_CHECK)		\
> +	hook(BPRM_CHECK)		\
> +	hook(CREDS_CHECK)		\
> +	hook(POST_SETATTR)		\
> +	hook(MODULE_CHECK)		\
> +	hook(FIRMWARE_CHECK)		\
> +	hook(KEXEC_KERNEL_CHECK)	\
> +	hook(KEXEC_INITRAMFS_CHECK)	\
> +	hook(POLICY_CHECK)		\
> +	hook(MAX_CHECK)
> +#define __ima_hook_enumify(ENUM)	ENUM,
> +
> +enum ima_hooks {
> +	__ima_hooks(__ima_hook_enumify)
> +};
> +
>  #ifdef CONFIG_IMA_KEXEC
>  extern void ima_add_kexec_buffer(struct kimage *image);
>  #endif
> @@ -132,4 +151,13 @@ static inline int ima_inode_removexattr(struct dentry *dentry,
>  	return 0;
>  }
>  #endif /* CONFIG_IMA_APPRAISE */
> +
> +#if defined(CONFIG_IMA_APPRAISE) && defined(CONFIG_INTEGRITY_TRUSTED_KEYRING)
> +extern bool ima_appraise_signature(enum ima_hooks func);
> +#else
> +static inline bool ima_appraise_kexec_signature(enum ima_hooks func)
> +{
> +	return false;
> +}
> +#endif /* CONFIG_IMA_APPRAISE && CONFIG_INTEGRITY_TRUSTED_KEYRING */
>  #endif /* _LINUX_IMA_H */
> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> index 0cfe4f6f7f85..3e04506a00a2 100644
> --- a/kernel/kexec_file.c
> +++ b/kernel/kexec_file.c
> @@ -20,11 +20,11 @@
>  #include <linux/mutex.h>
>  #include <linux/list.h>
>  #include <linux/fs.h>
> -#include <linux/ima.h>
>  #include <crypto/hash.h>
>  #include <crypto/sha.h>
>  #include <linux/elf.h>
>  #include <linux/elfcore.h>
> +#include <linux/ima.h>
>  #include <linux/kernel.h>
>  #include <linux/syscalls.h>
>  #include <linux/vmalloc.h>
> @@ -240,7 +240,12 @@ kimage_file_prepare_segments(struct kimage *image, int kernel_fd, int initrd_fd,
>  
>  		ret = 0;
>  
> -		if (kernel_is_locked_down(reason)) {
> +		/* If IMA is guaranteed to appraise a signature on the kexec
> +		 * image, permit it even if the kernel is otherwise locked
> +		 * down.
> +		 */
> +		if (!ima_appraise_signature(KEXEC_KERNEL_CHECK) &&
> +		    kernel_is_locked_down(reason)) {
>  			ret = -EPERM;
>  			goto out;

[Cc'ing Dave Young, Eric Biederman, kexec mailing list]

There was a discussion about using KEXEC_KERNEL_CHECK as an argument
when replacing copy_file_from_fd() with kernel_read_file_from_fd().
There was a subsequent discussion when adding a security call in
kexec_load_check.  The end result was defining two enumerations named
kernel_read_file_id and kernel_load_data_id with READING_KEXEC_IMAGE
and LOADING_KECEC_IMAGE respectively.

Instead of making the ima_hooks enumeration global, as we're already
relying on READING_KEXEC_IMAGE, use it.

>  		}
> diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
> index b6d9f14bc234..aad61bc0f774 100644
> --- a/security/integrity/evm/evm_main.c
> +++ b/security/integrity/evm/evm_main.c
> @@ -87,7 +87,7 @@ static void __init evm_init_config(void)
>  	pr_info("HMAC attrs: 0x%x\n", evm_hmac_attrs);
>  }
>  
> -static bool evm_key_loaded(void)
> +bool evm_key_loaded(void)
>  {
>  	return (bool)(evm_initialized & EVM_KEY_MASK);
>  }

Remove remaining EVM fragment.

> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index cc12f3449a72..71614a8ed2aa 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -20,6 +20,7 @@
>  #include <linux/types.h>
>  #include <linux/crypto.h>
>  #include <linux/fs.h>
> +#include <linux/ima.h>
>  #include <linux/security.h>
>  #include <linux/hash.h>
>  #include <linux/tpm.h>
> @@ -171,25 +172,6 @@ static inline unsigned long ima_hash_key(u8 *digest)
>  	return hash_long(*digest, IMA_HASH_BITS);
>  }
>  
> -#define __ima_hooks(hook)		\
> -	hook(NONE)			\
> -	hook(FILE_CHECK)		\
> -	hook(MMAP_CHECK)		\
> -	hook(BPRM_CHECK)		\
> -	hook(CREDS_CHECK)		\
> -	hook(POST_SETATTR)		\
> -	hook(MODULE_CHECK)		\
> -	hook(FIRMWARE_CHECK)		\
> -	hook(KEXEC_KERNEL_CHECK)	\
> -	hook(KEXEC_INITRAMFS_CHECK)	\
> -	hook(POLICY_CHECK)		\
> -	hook(MAX_CHECK)
> -#define __ima_hook_enumify(ENUM)	ENUM,
> -
> -enum ima_hooks {
> -	__ima_hooks(__ima_hook_enumify)
> -};
> -
>  /* 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);
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index 8bc8a1c8cb3f..adeae1ab9ee9 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -21,6 +21,7 @@
>  #include <linux/genhd.h>
>  #include <linux/seq_file.h>
>  #include <linux/ima.h>
> +#include <linux/evm.h>
>  
>  #include "ima.h"
>  
> @@ -1336,4 +1337,46 @@ int ima_policy_show(struct seq_file *m, void *v)
>  	seq_puts(m, "\n");
>  	return 0;
>  }
> +
>  #endif	/* CONFIG_IMA_READ_POLICY */
> +
> +#if defined(CONFIG_IMA_APPRAISE) && defined(CONFIG_INTEGRITY_TRUSTED_KEYRING)
> +/*
> + * ima_appraise_signature: whether IMA will appraise a given function using
> + * an IMA digital signature. This is restricted to cases where the kernel
> + * has a set of built-in trusted keys in order to avoid an attacker simply
> + * loading additional keys.
> + */
> +bool ima_appraise_signature(enum ima_hooks func)
> +{
> +	struct ima_rule_entry *entry;
> +	bool found = false;
> +
> +	rcu_read_lock();
> +	list_for_each_entry_rcu(entry, ima_rules, list) {
> +		if (entry->action != APPRAISE)
> +			continue;
> +
> +		/* A generic entry will match, but otherwise require that it
> +		 * match the func we're looking for
> +		 */
> +		if (entry->func && entry->func != func)
> +			continue;
> +
> +		/* We require this to be a digital signature, not a raw IMA
> +		 * hash.
> +		 */

Comments should either be a single line or "/*" on a separate line.

> +		if (entry->flags & IMA_DIGSIG_REQUIRED)
> +			found = true;
> +
> +		/* We've found a rule that matches, so break now even if it
> +		 * didn't require a digital signature - a later rule that does
> +		 * won't override it, so would be a false positive.
> +		 */
> +		break;
> +	}
> +

Much better.

thanks,

Mimi

> +	rcu_read_unlock();
> +	return found;
> +}
> +#endif /* CONFIG_IMA_APPRAISE && CONFIG_INTEGRITY_TRUSTED_KEYRING */