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

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

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'?

Mimi