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

[Peter M. <pm...@go...>]

On Fri, Nov 07 2014 at 05:50, Dmitry Kasatkin wrote:
> Hi Peter,
>
> We had a chat with Mimi and thought/discussed about couple of issues...
>
> 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.

Hey Dmitry, Mimi.

so the usecase would be for me to call this from something like hone
(http://marc.info/?l=linux-security-module&m=140685613922280&w=2, I need
to resend that patchset). The basic idea is that I have a stacked
security module that monitors forks/execs/exits and packets sent &
received.

The purpose of the module is to help an admin (or an incident response
team) to:

 a) correlate a packet seen on the network with the process that sent or
 received it.
 b) determine the process ancestry of a potentially malicious process (eg.
 an ntpd exploit that led to wget being called that led to a rootkit being
 installed)

Part of what hone does is send events for forks/execs/exits to a
userland daemon, along with the pertinents like uid, euid, gid, egid,
ppid, path, exit status .. etc. What I'm trying to do here is, for
systems with IMA enabled with an appropriate policy, include the
calculated hash with the exec events. It's true that the existence of
the hash in the iint_cache means that the hash is 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 give host is up for a long time and
the binary was first measured a long time ago.

Does that make sense?


> 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.
>> ---
>>  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...
>
>> +		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()'.

Ack. I'll make these changes.

> 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
>>   *