diff mbox series

ima: add the ability to query ima for the hash of a given file.

Message ID 20191220163136.25010-1-revest@chromium.org (mailing list archive)
State New, archived
Headers show
Series ima: add the ability to query ima for the hash of a given file. | expand

Commit Message

Florent Revest Dec. 20, 2019, 4:31 p.m. UTC 
From: Florent Revest <revest@google.com>

This allows other parts of the kernel (perhaps a stacked LSM allowing
system monitoring, eg. the proposed KRSI LSM [1]) 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 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.

This is based on Peter Moody's patch:
 https://sourceforge.net/p/linux-ima/mailman/message/33036180/

[1] https://lkml.org/lkml/2019/9/10/393

Signed-off-by: Florent Revest <revest@google.com>
---
 include/linux/ima.h               |  6 +++++
 security/integrity/ima/ima_main.c | 41 +++++++++++++++++++++++++++++++
 2 files changed, 47 insertions(+)

Comments

Lakshmi Ramasubramanian Dec. 20, 2019, 4:48 p.m. UTC | #1 
On 12/20/2019 8:31 AM, Florent Revest wrote:

>   
> +/**
> + * ima_file_hash - return the stored measurement if a file has been hashed.
> + * @file: pointer to the file
> + * @buf: buffer in which to store the hash
> + * @buf_size: length of the buffer
> + *
> + * On success, output the hash into buf and return the hash algorithm (as
> + * defined in the enum hash_algo).

> + * If the hash is larger than buf, then only size bytes will be copied. It
> + * generally just makes sense to pass a buffer capable of holding the largest
> + * possible hash: IMA_MAX_DIGEST_SIZE

If the given buffer is smaller than the hash length, wouldn't it be 
better to return the required size and a status indicating the buffer is 
not enough. The caller can then call back with the required buffer.

If the hash is truncated the caller may not know if the hash is partial 
or not.

> + *
> + * If IMA is disabled or if no measurement is available, return -EOPNOTSUPP.
> + * If the parameters are incorrect, return -EINVAL.
> + */
> +int ima_file_hash(struct file *file, char *buf, size_t buf_size)
> +{
> +	struct inode *inode;
> +	struct integrity_iint_cache *iint;
> +	size_t copied_size;
> +
> +	if (!file || !buf)
> +		return -EINVAL;
> +
> +	if (!ima_policy_flag)
> +		return -EOPNOTSUPP;
> +
> +	inode = file_inode(file);
> +	iint = integrity_iint_find(inode);
> +	if (!iint)
> +		return -EOPNOTSUPP;
> +
> +	mutex_lock(&iint->mutex);
> +	copied_size = min_t(size_t, iint->ima_hash->length, buf_size);
> +	memcpy(buf, iint->ima_hash->digest, copied_size);
> +	mutex_unlock(&iint->mutex);
> +
> +	return iint->ima_hash->algo;

Should the hash algorithm be copied from iinit->ima_hash to a local 
variable while holding the mutex and that one returned?

I assume iinit->mutex  is taken to ensure iinit->ima_hash is not removed 
while this function is accessing it.

thanks,
  -lakshmi
Mimi Zohar Dec. 23, 2019, 5:39 p.m. UTC | #2 
On Fri, 2019-12-20 at 08:48 -0800, Lakshmi Ramasubramanian wrote:
> On 12/20/2019 8:31 AM, Florent Revest wrote:
> 
> >   
> > +/**
> > + * ima_file_hash - return the stored measurement if a file has been hashed.
> > + * @file: pointer to the file
> > + * @buf: buffer in which to store the hash
> > + * @buf_size: length of the buffer
> > + *
> > + * On success, output the hash into buf and return the hash algorithm (as
> > + * defined in the enum hash_algo).
> 
> > + * If the hash is larger than buf, then only size bytes will be copied. It
> > + * generally just makes sense to pass a buffer capable of holding the largest
> > + * possible hash: IMA_MAX_DIGEST_SIZE
> 
> If the given buffer is smaller than the hash length, wouldn't it be 
> better to return the required size and a status indicating the buffer is 
> not enough. The caller can then call back with the required buffer.
> 
> If the hash is truncated the caller may not know if the hash is partial 
> or not.

Based on the hash algorithm, the caller would know if the buffer
provided was too small and was truncated.

> 
> > + *
> > + * If IMA is disabled or if no measurement is available, return -EOPNOTSUPP.
> > + * If the parameters are incorrect, return -EINVAL.
> > + */
> > +int ima_file_hash(struct file *file, char *buf, size_t buf_size)
> > +{
> > +	struct inode *inode;
> > +	struct integrity_iint_cache *iint;
> > +	size_t copied_size;
> > +
> > +	if (!file || !buf)
> > +		return -EINVAL;
> > +

Other kernel functions provide a means of determining the needed
buffer size by passing a NULL field.  Instead of failing here, if buf
is NULL, how about returning the hash algorithm?

Mimi

> > +	if (!ima_policy_flag)
> > +		return -EOPNOTSUPP;
> > +
Florent Revest Jan. 6, 2020, 4:10 p.m. UTC | #3 
On Fri, 2019-12-20 at 08:48 -0800, Lakshmi Ramasubramanian wrote:
> On 12/20/2019 8:31 AM, Florent Revest wrote:
> 
> >   
> > +/**
> > + * ima_file_hash - return the stored measurement if a file has
> > been hashed.
> > + * @file: pointer to the file
> > + * @buf: buffer in which to store the hash
> > + * @buf_size: length of the buffer
> > + *
> > + * On success, output the hash into buf and return the hash
> > algorithm (as
> > + * defined in the enum hash_algo).
> > + * If the hash is larger than buf, then only size bytes will be
> > copied. It
> > + * generally just makes sense to pass a buffer capable of holding
> > the largest
> > + * possible hash: IMA_MAX_DIGEST_SIZE
> 
> If the given buffer is smaller than the hash length, wouldn't it be 
> better to return the required size and a status indicating the buffer
> is not enough. The caller can then call back with the required
> buffer.
> 
> If the hash is truncated the caller may not know if the hash is
> partial or not.

I agree with Mimi's answer that the caller would know based on the
returned hash algorithm.

> > + *
> > + * If IMA is disabled or if no measurement is available, return
> > -EOPNOTSUPP.
> > + * If the parameters are incorrect, return -EINVAL.
> > + */
> > +int ima_file_hash(struct file *file, char *buf, size_t buf_size)
> > +{
> > +	struct inode *inode;
> > +	struct integrity_iint_cache *iint;
> > +	size_t copied_size;
> > +
> > +	if (!file || !buf)
> > +		return -EINVAL;
> > +
> > +	if (!ima_policy_flag)
> > +		return -EOPNOTSUPP;
> > +
> > +	inode = file_inode(file);
> > +	iint = integrity_iint_find(inode);
> > +	if (!iint)
> > +		return -EOPNOTSUPP;
> > +
> > +	mutex_lock(&iint->mutex);
> > +	copied_size = min_t(size_t, iint->ima_hash->length, buf_size);
> > +	memcpy(buf, iint->ima_hash->digest, copied_size);
> > +	mutex_unlock(&iint->mutex);
> > +
> > +	return iint->ima_hash->algo;
> 
> Should the hash algorithm be copied from iinit->ima_hash to a local 
> variable while holding the mutex and that one returned?
> 
> I assume iinit->mutex  is taken to ensure iinit->ima_hash is not
> removed while this function is accessing it.

Ah! Good catch, thank you :)
Florent Revest Jan. 6, 2020, 4:15 p.m. UTC | #4 
On Mon, 2019-12-23 at 12:39 -0500, Mimi Zohar wrote:
> On Fri, 2019-12-20 at 08:48 -0800, Lakshmi Ramasubramanian wrote:
> > On 12/20/2019 8:31 AM, Florent Revest wrote:
> > 
> > >   
> > > +/**
> > > + * ima_file_hash - return the stored measurement if a file has
> > > been hashed.
> > > + * @file: pointer to the file
> > > + * @buf: buffer in which to store the hash
> > > + * @buf_size: length of the buffer
> > > + *
> > > + * On success, output the hash into buf and return the hash
> > > algorithm (as
> > > + * defined in the enum hash_algo).
> > > + * If the hash is larger than buf, then only size bytes will be
> > > copied. It
> > > + * generally just makes sense to pass a buffer capable of
> > > holding the largest
> > > + * possible hash: IMA_MAX_DIGEST_SIZE
> > 
> > If the given buffer is smaller than the hash length, wouldn't it
> > be 
> > better to return the required size and a status indicating the
> > buffer is 
> > not enough. The caller can then call back with the required buffer.
> > 
> > If the hash is truncated the caller may not know if the hash is
> > partial 
> > or not.
> 
> Based on the hash algorithm, the caller would know if the buffer
> provided was too small and was truncated.
> 
> > > + *
> > > + * If IMA is disabled or if no measurement is available, return
> > > -EOPNOTSUPP.
> > > + * If the parameters are incorrect, return -EINVAL.
> > > + */
> > > +int ima_file_hash(struct file *file, char *buf, size_t buf_size)
> > > +{
> > > +	struct inode *inode;
> > > +	struct integrity_iint_cache *iint;
> > > +	size_t copied_size;
> > > +
> > > +	if (!file || !buf)
> > > +		return -EINVAL;
> > > +
> 
> Other kernel functions provide a means of determining the needed
> buffer size by passing a NULL field.  Instead of failing here, if buf
> is NULL, how about returning the hash algorithm?

I wasn't aware of that. This is nice to have indeed! I'll send a v2.
Thanks!
diff mbox series

Patch

diff --git a/include/linux/ima.h b/include/linux/ima.h
index 6d904754d858..d621c65ba9a5 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -23,6 +23,7 @@  extern int ima_read_file(struct file *file, enum kernel_read_file_id id);
 extern int ima_post_read_file(struct file *file, void *buf, loff_t size,
 			      enum kernel_read_file_id id);
 extern void ima_post_path_mknod(struct dentry *dentry);
+extern int ima_file_hash(struct file *file, char *buf, size_t buf_size);
 extern void ima_kexec_cmdline(const void *buf, int size);
 
 #ifdef CONFIG_IMA_KEXEC
@@ -91,6 +92,11 @@  static inline void ima_post_path_mknod(struct dentry *dentry)
 	return;
 }
 
+static inline int ima_file_hash(struct file *file, char *buf, size_t buf_size)
+{
+	return -EOPNOTSUPP;
+}
+
 static inline void ima_kexec_cmdline(const void *buf, int size) {}
 #endif /* CONFIG_IMA */
 
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index d7e987baf127..f054ddf4364e 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -445,6 +445,47 @@  int ima_file_check(struct file *file, int mask)
 }
 EXPORT_SYMBOL_GPL(ima_file_check);
 
+/**
+ * ima_file_hash - return the stored measurement if a file has been hashed.
+ * @file: pointer to the file
+ * @buf: buffer in which to store the hash
+ * @buf_size: length of the buffer
+ *
+ * On success, output the hash into buf and return the hash algorithm (as
+ * defined in the enum hash_algo).
+ * If the hash is larger than buf, then only size bytes will be copied. It
+ * generally just makes sense to pass a buffer capable of holding the largest
+ * possible hash: IMA_MAX_DIGEST_SIZE
+ *
+ * If IMA is disabled or if no measurement is available, return -EOPNOTSUPP.
+ * If the parameters are incorrect, return -EINVAL.
+ */
+int ima_file_hash(struct file *file, char *buf, size_t buf_size)
+{
+	struct inode *inode;
+	struct integrity_iint_cache *iint;
+	size_t copied_size;
+
+	if (!file || !buf)
+		return -EINVAL;
+
+	if (!ima_policy_flag)
+		return -EOPNOTSUPP;
+
+	inode = file_inode(file);
+	iint = integrity_iint_find(inode);
+	if (!iint)
+		return -EOPNOTSUPP;
+
+	mutex_lock(&iint->mutex);
+	copied_size = min_t(size_t, iint->ima_hash->length, buf_size);
+	memcpy(buf, iint->ima_hash->digest, copied_size);
+	mutex_unlock(&iint->mutex);
+
+	return iint->ima_hash->algo;
+}
+EXPORT_SYMBOL_GPL(ima_file_hash);
+
 /**
  * ima_post_create_tmpfile - mark newly created tmpfile as new
  * @file : newly created tmpfile