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

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

On Fri, Nov 14 2014 at 00:24, Dmitry Kasatkin wrote:
> 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.

Following up on this patch (very very late), did you want me to update
the docstring referencing IS_ERR_OR_NULL() specifically or was this ok
as it stands? I'm fine with this returning IS_ERR_OR_NULL() if you all
are.

 Cheers,
 peter

--