Re: [RFC][PATCH 6/8] ima: Use digest cache for measurement

[Mimi Zohar]

Hi Roberto,

> diff --git a/security/integrity/ima/ima_main.c
> b/security/integrity/ima/ima_main.c
> index 3fc48214850a..48a09747ae7a 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -222,7 +222,9 @@ static int process_measurement(struct file *file, const
> struct cred *cred,
>  	bool violation_check;
>  	enum hash_algo hash_algo;
>  	unsigned int allowed_algos = 0;
> -	u64 verif_mask = 0;
> +	u64 verif_mask = 0, *verif_mask_ptr, policy_mask = 0, allow_mask = 0;
> +	struct digest_cache *digest_cache = NULL, *found_cache;
> +	digest_cache_found_t found;
>  
>  	if (!ima_policy_flag || !S_ISREG(inode->i_mode))
>  		return 0;
> @@ -233,7 +235,7 @@ static int process_measurement(struct file *file, const
> struct cred *cred,
>  	 */
>  	action = ima_get_action(file_mnt_idmap(file), inode, cred, secid,
>  				mask, func, &pcr, &template_desc, NULL,
> -				&allowed_algos, NULL);
> +				&allowed_algos, &policy_mask);
>  	violation_check = ((func == FILE_CHECK || func == MMAP_CHECK ||
>  			    func == MMAP_CHECK_REQPROT) &&
>  			   (ima_policy_flag & IMA_MEASURE));
> @@ -364,10 +366,34 @@ static int process_measurement(struct file *file, const
> struct cred *cred,
>  	if (!pathbuf)	/* ima_rdwr_violation possibly pre-fetched */
>  		pathname = ima_d_path(&file->f_path, &pathbuf, filename);
>  
> +	/*
> +	 * For now we don't support nested verification with digest caches.

I haven't reviewed the digest_cache LSM patch set yet.  What does 'nested' mean
in this context?  Why mention it here?

> +	 * Since we allow IMA policy rules without func=, we have to enforce
> +	 * this restriction here.
> +	 */
> +	if (rc == 0 && policy_mask && func != DIGEST_LIST_CHECK)
> +		digest_cache = digest_cache_get(file_dentry(file));

So whether or not a DIGEST_LIST_CHECK policy rule even exists,
digest_cache_get() will be called.  Similarly, even if a digest_cache list
hasn't been measured or appraised, digest_cache_get() will be called.

Basically every file in policy will check the digest_cache.

> +
> +	if (digest_cache) {
> +		found = digest_cache_lookup(file_dentry(file), digest_cache,
> +					    iint->ima_hash->digest,
> +					    iint->ima_hash->algo);
> +		/* AND what is allowed by the policy, and what IMA verified. */
> +		if (found) {
> +			found_cache = digest_cache_from_found_t(found);
> +			verif_mask_ptr = digest_cache_verif_get(found_cache,
> +								"ima");

Instead of using "verif_{set,get}' consider using '{set,get}_usage', where usage
here means measure or appraise.

> +			if (verif_mask_ptr)
> +				allow_mask = policy_mask & *verif_mask_ptr;
> +		}
> +
> +		digest_cache_put(digest_cache);
> +	}
> +

I'm wondering if it makes sense to create IMA wrappers for each of the
digest_cache functions - checking the digest_cache for the hash, setting the
digest_cache permitted usage, etc - and put all of them in a separate
ima_digest_cache.c file.  The file would only be included in the Makefile if
digest_cache is configured.

In this file you could define a static local global variable to detect whether
the digest_cache is ready to be used.  Only after successfully measuring and
appraising a digest_cache list, based on policy, set the variable.

>  	if (action & IMA_MEASURE)
>  		ima_store_measurement(iint, file, pathname,
>  				      xattr_value, xattr_len, modsig, pcr,
> -				      template_desc);
> +				      template_desc, allow_mask);

'allowed_usage'?

>  	if (rc == 0 && (action & IMA_APPRAISE_SUBMASK)) {
>  		rc = ima_check_blacklist(iint, modsig, pcr);
>  		if (rc != -EPERM) {

thanks,

Mimi