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

[Mimi Z. <zo...@li...>]

On Fri, 2014-11-07 at 15:50 +0200, Dmitry Kasatkin wrote: 
> Hi Peter,
> 
> We had a chat with Mimi and thought/discussed about couple of issues...

Thanks, Dmitry, for summarizing our discussion.

> It would be nice that patch description to discuss about use-case scenario.
> It is unclear why, when and how this API is going to be used.
> 
> Also please see comment bellow...
> 
> On 06/11/14 00:08, Peter Moody wrote:
> > It's useful to be able to get the hash of a previously measured file.
> > Querying IMA directly is more efficient than grepping the ascii
> > measurements of the audit logs.

In further discussion we realized that there is a major difference
between the two methods.  Searching the measurement list will return the
hash value(s) at the time the file was used, whereas opening the file to
query IMA could cause IMA to re-calculate the hash, if the file has
changed.

> > ---
> >  include/linux/ima.h               |  6 ++++++
> >  security/integrity/ima/ima_main.c | 35 +++++++++++++++++++++++++++++++++++
> >  2 files changed, 41 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..39a139d 100644
> > --- a/security/integrity/ima/ima_main.c
> > +++ b/security/integrity/ima/ima_main.c
> > @@ -309,6 +309,41 @@ 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. 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 (!file)
> > +		return NULL;
> > +
> > +	inode = file_inode(file);
> > +
> 
> We think mutex is taken too early. It makes sense to verify if inode was
> handled by IMA.
> Locking would be useless because IMA is disabled by policy or 'iint'
> will be NULL...
> 
> Please check ima_file_free(). It makes sense to use the same pattern
> ------------
>     if (!ima_policy_flag || !S_ISREG(inode->i_mode))
>         return NULL;
> 
>     iint = integrity_iint_find(inode);
>     if (!iint)
>         return NULL;
> 
>     mutex_lock(&inode->i_mutex);
> ----------------
> 
> S_ISREG is probably not needed for your hook.
> 
> in reality integrity_iint_find() is just enough but we optimize for not
> calling it when IMA is disabled..
> 
> > +	mutex_lock(&inode->i_mutex);
> > +	if (iint && (iint->flags & IMA_COLLECTED))
> 
> Also we thought that using iint->flags may be inappropriate at all.
> They can be reset and you probably want old hash value anyway.
> In such case it is better to test for "zero" hash...

With the above recommended changes, the 'iint' check is not needed
either.

Mimi

> > +		snprintf(buf, size, "%s:%*phN",
> > +			hash_algo_name[iint->ima_hash->algo],
> > +			iint->ima_hash->length, iint->ima_hash->digest);
> > +	else
> > +		buf = NULL;
> > +	mutex_unlock(&inode->i_mutex);
> > +
> > +	return buf;
> 
> Also may be if you need error code to indicate if IMA is disabled you
> could use 'return ERR_PTR()'.
> 
> 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
> >   *
>