Re: [Linux-ima-devel] [PATCH v6] ima: add the ability to query ima for the hash of a given file.

[Dmitry K. <d.k...@sa...>]

On 13/11/14 21:09, Peter Moody wrote:
> This allows other parts of the kernel (perhaps a stacked LSM focused on system
> monitoring, eg. the proposed hone LSM [1] designed to monitor fork/exec/exit
> calls to correlate network activity with the sending/receiving process) to
> retrieve the hash of a given file from IMA if it's present in the iint cache.
>
> It's true that the existence of the hash means that it's also either in the
> audit logs or in /sys/kernel/security/ima/ascii_runtime_measurements, but it
> can be difficult to pull that information out for every subsequent exec.
> This is especially true if a given host has been up for a long time and the
> file was first measured a long time ago.
>
> [1] http://marc.info/?l=linux-security-module&m=140685613922280&w=2
> ---
>  include/linux/ima.h               |  6 ++++++
>  security/integrity/ima/ima_main.c | 39 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 45 insertions(+)
>
> diff --git a/include/linux/ima.h b/include/linux/ima.h
> index 120ccc5..27cc4f6 100644
> --- a/include/linux/ima.h
> +++ b/include/linux/ima.h
> @@ -20,6 +20,7 @@ extern void ima_file_free(struct file *file);
>  extern int ima_file_mmap(struct file *file, unsigned long prot);
>  extern int ima_module_check(struct file *file);
>  extern int ima_fw_from_file(struct file *file, char *buf, size_t size);
> +extern char *ima_file_hash(struct file *file, char *buf, size_t size);
>  
>  #else
>  static inline int ima_bprm_check(struct linux_binprm *bprm)
> @@ -52,6 +53,11 @@ static inline int ima_fw_from_file(struct file *file, char *buf, size_t size)
>  	return 0;
>  }
>  
> +static inline char *ima_file_hash(struct file *file, char *buf, size_t size)
> +{
> +	return NULL;
> +}
> +
>  #endif /* CONFIG_IMA */
>  
>  #ifdef CONFIG_IMA_APPRAISE
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 62f59ec..6a1cc80 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -309,6 +309,45 @@ int ima_file_check(struct file *file, int mask, int opened)
>  EXPORT_SYMBOL_GPL(ima_file_check);
>  
>  /**
> + * ima_file_hash - return the stored measurement if a file's been hashed.
> + * @file: pointer to the file.
> + * @buf: buffer in which to store the hash.
> + * @size: length of the buffer.
> + *
> + * On success returns the buffer. If IMA is disabled, return ERR_PTR(-EVINVAL).
> + * Otherwise returns NULL. If the hash is larger than buffer, then only size
> + * bytes will be copied. It generally just makes sense to pass a buffer
> + * capable of holding the largest possible hash string. This is currently 135:
> + * (IMA_MAX_DIGEST_SIZE * 2 + 6 + 1).
> + */
> +char *ima_file_hash(struct file *file, char *buf, size_t size)
> +{
> +	struct inode *inode;
> +	struct integrity_iint_cache *iint;
> +
> +	if (!ima_policy_flag || !file)
> +		return ERR_PTR(-EINVAL);
> +

Hi,

Patch looks good.

May be few thoughts about error codes.
Do you want to return -EINVAL if IMA is disabled?
May be NULL as !iint? because of the same meaning.. inode is not handled
by IMA.

Also just want highlight that normally if you use ERR_PTR then client
checks error as IS_ERR(ptr).
NULL is not an error, just as 0.
So client need to check IS_ERR if it was error and NULL which is not an
error and means that inode is not handled by policy..

Is that what you want?
Or if NULL will be used as error, then IS_ERR_OR_NULL(ptr) can be used.

> +	inode = file_inode(file);
> +
> +	iint = integrity_iint_find(inode);
> +	if (!iint)
> +		return NULL;
                      OK here.
> +
> +	mutex_lock(&inode->i_mutex);
> +	if (iint->ima_hash->length > 0)
> +		snprintf(buf, size, "%s:%*phN",
> +			hash_algo_name[iint->ima_hash->algo],
> +			iint->ima_hash->length, iint->ima_hash->digest);
> +	else
> +		buf = NULL;
                                   buf =  ERR_PTR(-EINVAL);
> +	mutex_unlock(&inode->i_mutex);
> +
> +	return buf;
> +}

Thanks

Dmitry


> +EXPORT_SYMBOL_GPL(ima_file_hash);
> +
> +/**
>   * ima_module_check - based on policy, collect/store/appraise measurement.
>   * @file: pointer to the file to be measured/appraised
>   *