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 Sun, Apr 26 2015 at 13:13, Mimi Zohar wrote:
> On Mon, 2015-03-30 at 22:24 -0400, Mimi Zohar wrote:
>> On Mon, 2015-03-30 at 19:14 -0700, Peter Moody wrote: 
>> > On Mon, Mar 16 2015 at 10:25, Peter Moody wrote:
>> > > 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.
>> > 
>> > (re-pining because I'm not sure if my last message made it through).
>> 
>> Sorry for the delay in responding...  Let me refresh my memory a bit
>> before commenting.
>> 
>> Mimi
>> 
>> > > 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.
>
> The IMA (and security) hooks normally return 0 on success and an error
> code otherwise.  Is there any benefit in returning 'buf'?

I was originally thinking that it would nice to be able to call
something like

  pr_info("hash %s\n", ima_file_hash(file, buf, size));

but I have to check the return value before I use it anyway, so no,
there's no real benefit. I'll update with the 0/1 return value.

> Mimi
>

-- 
peter moody