Re: [PATCH 17/20] ima: make IMA policy replaceable at runtime

[Mimi Zohar]

On Wed, 2014-04-23 at 16:30 +0300, Dmitry Kasatkin wrote: 
> This patch provides functionality to replace the IMA policy at runtime.
> 
> By default, the IMA policy can be successfully updated only once,
> but with this patch when the kernel configuration option
> CONFIG_IMA_POLICY_REPLACEABLE is enabled, the IMA policy can be replaced
> multiple times at runtime.
> 
> Signed-off-by: Dmitry Kasatkin <d.kasatkin@xxxxxxxxxxx>

I have a couple of concerns with replacing the IMA policy.

- Currently opened files might now be in policy, that previously
weren't.  Do these files need to be measured, appraised, or audited?
These files could have been modified, but the 'security.ima' xattr
hasn't been updated yet. In such cases, appraisal would fail.

- At minimum, after replacing the policy, the iint cache entry flags
need to be reset.

Please provide the motivation for such a use case scenario.

Mimi

> ---
>  security/integrity/ima/Kconfig      |  8 ++++++++
>  security/integrity/ima/ima_fs.c     |  2 ++
>  security/integrity/ima/ima_policy.c | 23 +++++++++++++++++++----
>  3 files changed, 29 insertions(+), 4 deletions(-)
> 
> diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
> index b00044f..b60a315 100644
> --- a/security/integrity/ima/Kconfig
> +++ b/security/integrity/ima/Kconfig
> @@ -160,3 +160,11 @@ config IMA_KERNEL_POLICY
>  	default n
>  	help
>  	  This option enables IMA policy loading from the kernel.
> +
> +config IMA_POLICY_REPLACEABLE
> +	bool "Allows to replace policy at runtime"
> +	depends on IMA_POLICY_LOADER
> +	default n
> +	help
> +	  Enabling this option allows to replace policy at runtime.
> +	  Only signed policy is allowed.
> diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
> index d050a5c..b4144b4 100644
> --- a/security/integrity/ima/ima_fs.c
> +++ b/security/integrity/ima/ima_fs.c
> @@ -304,11 +304,13 @@ static int ima_open_policy(struct inode *inode, struct file *filp)
>  		return -EACCES;
>  	if (test_and_set_bit(IMA_FS_BUSY, &ima_fs_flags))
>  		return -EBUSY;
> +#ifndef CONFIG_IMA_POLICY_REPLACEABLE
>  	if (!ima_default_policy()) {
>  		/* policy was already set*/
>  		clear_bit(IMA_FS_BUSY, &ima_fs_flags);
>  		return -EACCES;
>  	}
> +#endif
>  	return 0;
>  }
> 
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index c6da801..981e953 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -108,11 +108,14 @@ static struct ima_rule_entry default_appraise_rules[] = {
> 
>  static LIST_HEAD(ima_default_rules);
>  static LIST_HEAD(ima_policy_rules);
> +static LIST_HEAD(ima_active_rules);
>  static struct list_head *ima_rules;
>  static bool path_rules;
> 
>  static DEFINE_MUTEX(ima_rules_mutex);
> 
> +static void ima_do_delete_rules(struct list_head *rules);
> +
>  static bool ima_use_tcb __initdata;
>  static int __init default_measure_policy_setup(char *str)
>  {
> @@ -367,7 +370,14 @@ void ima_update_policy(void)
>  	int result = 0;
>  	int audit_info = 0;
> 
> -	ima_rules = &ima_policy_rules;
> +	if (ima_default_policy()) {
> +		/* set new policy head */
> +		ima_rules = &ima_active_rules;
> +	} else {
> +		/* FIXME: must be protected by lock */
> +		ima_do_delete_rules(ima_rules);
> +	}
> +	list_replace_init(&ima_policy_rules, ima_rules);
> 
>  	integrity_audit_msg(AUDIT_INTEGRITY_STATUS, NULL,
>  			    NULL, op, cause, result, audit_info);
> @@ -734,14 +744,14 @@ ssize_t ima_parse_add_rule(char *rule)
>  	return len;
>  }
> 
> -/* ima_delete_rules called to cleanup invalid policy */
> -void ima_delete_rules(void)
> +/* ima_delete_rules called to cleanup invalid or old policy */
> +static void ima_do_delete_rules(struct list_head *rules)
>  {
>  	struct ima_rule_entry *entry, *tmp;
>  	int i;
> 
>  	mutex_lock(&ima_rules_mutex);
> -	list_for_each_entry_safe(entry, tmp, &ima_policy_rules, list) {
> +	list_for_each_entry_safe(entry, tmp, rules, list) {
>  		for (i = 0; i < MAX_LSM_RULES; i++)
>  			kfree(entry->lsm[i].args_p);
> 
> @@ -751,6 +761,11 @@ void ima_delete_rules(void)
>  	mutex_unlock(&ima_rules_mutex);
>  }
> 
> +void ima_delete_rules(void)
> +{
> +	ima_do_delete_rules(&ima_policy_rules);
> +}
> +
>  #ifdef CONFIG_IMA_POLICY_LOADER
> 
>  ssize_t ima_read_policy(char *path)


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/