Linux-Security-Module Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] ima: Fix NULL pointer dereference in ima_file_hash
@ 2020-09-16 12:05 KP Singh
  2020-09-16 12:41 ` Florent Revest
  0 siblings, 1 reply; 3+ messages in thread
From: KP Singh @ 2020-09-16 12:05 UTC (permalink / raw)
  To: linux-kernel, linux-security-module
  Cc: Mimi Zohar, James Morris, Kees Cook, Lakshmi Ramasubramanian,
	Jann Horn, Florent Revest

From: KP Singh <kpsingh@google.com>

ima_file_hash can be called when there is no iint->ima_hash available
even though the inode exists in the integrity cache.

An example where this can happen (suggested by Jann Horn):

Process A does:

	while(1) {
		unlink("/tmp/imafoo");
		fd = open("/tmp/imafoo", O_RDWR|O_CREAT|O_TRUNC, 0700);
		if (fd == -1) {
			perror("open");
			continue;
		}
		write(fd, "A", 1);
		close(fd);
	}

and Process B does:

	while (1) {
		int fd = open("/tmp/imafoo", O_RDONLY);
		if (fd == -1)
			continue;
    		char *mapping = mmap(NULL, 0x1000, PROT_READ|PROT_EXEC,
			 	     MAP_PRIVATE, fd, 0);
		if (mapping != MAP_FAILED)
			munmap(mapping, 0x1000);
		close(fd);
  	}

Due to the race to get the iint->mutex between ima_file_hash and
process_measurement iint->ima_hash could still be NULL.

Fixes: 6beea7afcc72 ("ima: add the ability to query the cached hash of a given file")
Signed-off-by: KP Singh <kpsingh@google.com>
---
 security/integrity/ima/ima_main.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 8a91711ca79b..4c86cd4eece0 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -531,6 +531,16 @@ int ima_file_hash(struct file *file, char *buf, size_t buf_size)
 		return -EOPNOTSUPP;
 
 	mutex_lock(&iint->mutex);
+
+	/*
+	 * ima_file_hash can be called when ima_collect_measurement has still
+	 * not been called, we might not always have a hash.
+	 */
+	if (!iint->ima_hash) {
+		mutex_unlock(&iint->mutex);
+		return -EOPNOTSUPP;
+	}
+
 	if (buf) {
 		size_t copied_size;
 
-- 
2.28.0.618.gf4bc123cb7-goog


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] ima: Fix NULL pointer dereference in ima_file_hash
  2020-09-16 12:05 [PATCH] ima: Fix NULL pointer dereference in ima_file_hash KP Singh
@ 2020-09-16 12:41 ` Florent Revest
  2020-09-16 12:47   ` KP Singh
  0 siblings, 1 reply; 3+ messages in thread
From: Florent Revest @ 2020-09-16 12:41 UTC (permalink / raw)
  To: KP Singh, linux-kernel, linux-security-module
  Cc: Mimi Zohar, James Morris, Kees Cook, Lakshmi Ramasubramanian, Jann Horn

Reviewed-by: Florent Revest <revest@chromium.org>

On Wed, 2020-09-16 at 12:05 +0000, KP Singh wrote:
> From: KP Singh <kpsingh@google.com>
> 
> ima_file_hash can be called when there is no iint->ima_hash available
> even though the inode exists in the integrity cache.
> 
> An example where this can happen (suggested by Jann Horn):
> 
> Process A does:
> 
> 	while(1) {
> 		unlink("/tmp/imafoo");
> 		fd = open("/tmp/imafoo", O_RDWR|O_CREAT|O_TRUNC, 0700);
> 		if (fd == -1) {
> 			perror("open");
> 			continue;
> 		}
> 		write(fd, "A", 1);
> 		close(fd);
> 	}
> 
> and Process B does:
> 
> 	while (1) {
> 		int fd = open("/tmp/imafoo", O_RDONLY);
> 		if (fd == -1)
> 			continue;
>     		char *mapping = mmap(NULL, 0x1000, PROT_READ|PROT_EXEC,
> 			 	     MAP_PRIVATE, fd, 0);
> 		if (mapping != MAP_FAILED)
> 			munmap(mapping, 0x1000);
> 		close(fd);
>   	}
> 
> Due to the race to get the iint->mutex between ima_file_hash and
> process_measurement iint->ima_hash could still be NULL.
> 
> Fixes: 6beea7afcc72 ("ima: add the ability to query the cached hash
> of a given file")
> Signed-off-by: KP Singh <kpsingh@google.com>
> ---
>  security/integrity/ima/ima_main.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/security/integrity/ima/ima_main.c
> b/security/integrity/ima/ima_main.c
> index 8a91711ca79b..4c86cd4eece0 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -531,6 +531,16 @@ int ima_file_hash(struct file *file, char *buf,
> size_t buf_size)
>  		return -EOPNOTSUPP;
>  
>  	mutex_lock(&iint->mutex);
> +
> +	/*
> +	 * ima_file_hash can be called when ima_collect_measurement has
> still
> +	 * not been called, we might not always have a hash.
> +	 */
> +	if (!iint->ima_hash) {
> +		mutex_unlock(&iint->mutex);
> +		return -EOPNOTSUPP;
> +	}
> +
>  	if (buf) {
>  		size_t copied_size;
>  


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] ima: Fix NULL pointer dereference in ima_file_hash
  2020-09-16 12:41 ` Florent Revest
@ 2020-09-16 12:47   ` KP Singh
  0 siblings, 0 replies; 3+ messages in thread
From: KP Singh @ 2020-09-16 12:47 UTC (permalink / raw)
  To: Florent Revest
  Cc: open list, Linux Security Module list, Mimi Zohar, James Morris,
	Kees Cook, Lakshmi Ramasubramanian, Jann Horn

Somehow this patch does not seem to have been picked up by any of the
mailing list archives and I am not sure if this was delivered to the
lists. I will send a v2 and add Florent's Reviewed-by tag.

On Wed, Sep 16, 2020 at 2:41 PM Florent Revest <revest@chromium.org> wrote:
>
> Reviewed-by: Florent Revest <revest@chromium.org>
>
> On Wed, 2020-09-16 at 12:05 +0000, KP Singh wrote:
> > From: KP Singh <kpsingh@google.com>
> >
> > ima_file_hash can be called when there is no iint->ima_hash available
> > even though the inode exists in the integrity cache.
> >
> > An example where this can happen (suggested by Jann Horn):
> >
> > Process A does:
> >
> >       while(1) {
> >               unlink("/tmp/imafoo");
> >               fd = open("/tmp/imafoo", O_RDWR|O_CREAT|O_TRUNC, 0700);
> >               if (fd == -1) {
> >                       perror("open");
> >                       continue;
> >               }
> >               write(fd, "A", 1);
> >               close(fd);
> >       }
> >
> > and Process B does:
> >
> >       while (1) {
> >               int fd = open("/tmp/imafoo", O_RDONLY);
> >               if (fd == -1)
> >                       continue;
> >               char *mapping = mmap(NULL, 0x1000, PROT_READ|PROT_EXEC,
> >                                    MAP_PRIVATE, fd, 0);
> >               if (mapping != MAP_FAILED)
> >                       munmap(mapping, 0x1000);
> >               close(fd);
> >       }
> >
> > Due to the race to get the iint->mutex between ima_file_hash and
> > process_measurement iint->ima_hash could still be NULL.
> >
> > Fixes: 6beea7afcc72 ("ima: add the ability to query the cached hash
> > of a given file")
> > Signed-off-by: KP Singh <kpsingh@google.com>
> > ---
> >  security/integrity/ima/ima_main.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> >
> > diff --git a/security/integrity/ima/ima_main.c
> > b/security/integrity/ima/ima_main.c
> > index 8a91711ca79b..4c86cd4eece0 100644
> > --- a/security/integrity/ima/ima_main.c
> > +++ b/security/integrity/ima/ima_main.c
> > @@ -531,6 +531,16 @@ int ima_file_hash(struct file *file, char *buf,
> > size_t buf_size)
> >               return -EOPNOTSUPP;
> >
> >       mutex_lock(&iint->mutex);
> > +
> > +     /*
> > +      * ima_file_hash can be called when ima_collect_measurement has
> > still
> > +      * not been called, we might not always have a hash.
> > +      */
> > +     if (!iint->ima_hash) {
> > +             mutex_unlock(&iint->mutex);
> > +             return -EOPNOTSUPP;
> > +     }
> > +
> >       if (buf) {
> >               size_t copied_size;
> >
>

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-16 12:05 [PATCH] ima: Fix NULL pointer dereference in ima_file_hash KP Singh
2020-09-16 12:41 ` Florent Revest
2020-09-16 12:47   ` KP Singh

Linux-Security-Module Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-security-module/0 linux-security-module/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-security-module linux-security-module/ https://lore.kernel.org/linux-security-module \
		linux-security-module@vger.kernel.org
	public-inbox-index linux-security-module

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-security-module


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git