linux-kernel - [PATCH 11/11] IMA: fix the ToMToU logic

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 25 Oct 2010 14:42:25 -0400
From:	Eric Paris <eparis@...hat.com>
To:	linux-kernel@...r.kernel.org,
	linux-security-module@...r.kernel.org,
	linux-fsdevel@...r.kernel.org
Cc:	hch@...radead.org, zohar@...ibm.com, warthog9@...nel.org,
	david@...morbit.com, jmorris@...ei.org, kyle@...artin.ca,
	hpa@...or.com, akpm@...ux-foundation.org,
	torvalds@...ux-foundation.org, mingo@...e.hu, eparis@...hat.com,
	viro@...iv.linux.org.uk
Subject: [PATCH 11/11] IMA: fix the ToMToU logic

Current logic looks like this:

        rc = ima_must_measure(NULL, inode, MAY_READ, FILE_CHECK);
        if (rc < 0)
                goto out;

        if (mode & FMODE_WRITE) {
                if (inode->i_readcount)
                        send_tomtou = true;
                goto out;
        }

        if (atomic_read(&inode->i_writecount) > 0)
                send_writers = true;

Lets assume we have a policy which states that all files opened for read by
root must be measured.

Lets assume the file has permissions 777.

Lets assume that root has the given file open for read.

Lets assume that a non-root process opens the file write.

The non-root process will get to ima_counts_get() and will check the
ima_must_measure().  Since it is not supposed to measure it will goto out.

We should check the i_readcount no matter what since we might be causing a
ToMToU voilation!

This is close to correct , but still not quite perfect.  The situation could have
been that root, which was interested in the mesurement opened and closed the
file and another process which is not interested in the measurement is the one
holding the i_readcount ATM.  This is just overly strict on ToMToU violations,
which is better than not strict enough...

Signed-off-by: Eric Paris <eparis@...hat.com>
Acked-by: Mimi Zohar <zohar@...ux.vnet.ibm.com>
---

 security/integrity/ima/ima_main.c |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 60dd615..203de97 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -112,22 +112,23 @@ void ima_counts_get(struct file *file)
 	if (!ima_initialized)
 		goto out;
 
-	rc = ima_must_measure(NULL, inode, MAY_READ, FILE_CHECK);
-	if (rc < 0)
-		goto out;
-
 	if (mode & FMODE_WRITE) {
-		if (inode->i_readcount)
+		if (inode->i_readcount && IS_IMA(inode))
 			send_tomtou = true;
 		goto out;
 	}
 
+	rc = ima_must_measure(NULL, inode, MAY_READ, FILE_CHECK);
+	if (rc < 0)
+		goto out;
+
 	if (atomic_read(&inode->i_writecount) > 0)
 		send_writers = true;
 out:
 	/* remember the vfs deals with i_writecount */
 	if ((mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ)
 		inode->i_readcount++;
+
 	spin_unlock(&inode->i_lock);
 
 	if (send_tomtou)

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/