exec: Set file unwritable before LSM check

PATCH In review Kees Cook 4 participants 6 messages Mar 9, 2018 – Mar 19, 2018

Subsystems (4)

VFS Security (LSM) exec and binfmt Keys-trusted

lore

Share & tools

lore

Kees Cook

author

2018-03-09 19:30 UTC #0 lore raw

PATCH v2 1/1 exec: Set file unwritable before LSM check

The LSM check should happen after the file has been confirmed to be unchanging. Without this, we could have a race between the Time of Check (the call to security_kernel_read_file() which could read the file and make access policy decisions) and the Time of Use (starting with kernel_read_file()'s reading of the file contents). In theory, file contents could change between the two.

Signed-off-by Kees Cook <[email protected]>

---
v2: Clarify the ToC/ToU race (Linus)

Only loadpin and SELinux currently implement this hook. From what I can see, this won't change anything for either of them. IMA calls kernel_read_file(), but looking there it seems those callers won't be negatively impacted either. Can folks double-check this and send an Ack please?

---
 fs/exec.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

fs/exec.c +3 -3

index 7eb8d21bcab9..a919a827d181 100644
@@ -895,13 +895,13 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
 	if (!S_ISREG(file_inode(file)->i_mode) || max_size < 0)
 		return -EINVAL;
 
-	ret = security_kernel_read_file(file, id);
+	ret = deny_write_access(file);
 	if (ret)
 		return ret;
 
-	ret = deny_write_access(file);
+	ret = security_kernel_read_file(file, id);
 	if (ret)
-		return ret;
+		goto out;
 
 	i_size = i_size_read(file_inode(file));
 	if (max_size > 0 && i_size > max_size) {

Signature · 10 lines

2.7.4


-- 
Kees Cook
Pixel Security
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Linus Torvalds

2018-03-09 19:47 UTC #1 lore raw

Quoted 6 lines · Kees Cook

The LSM check should happen after the file has been confirmed to be unchanging. Without this, we could have a race between the Time of Check (the call to security_kernel_read_file() which could read the file and make access policy decisions) and the Time of Use (starting with kernel_read_file()'s reading of the file contents). In theory, file contents could change between the two.

I'm going to assume I get this for 4.17 from the security tree.

Because I'm guessing there are actually no existing users that care? selinux seems to just look at file state, not actually at contents or anything that write access denial would care about.

And the only other security module that even registers this is loadpin, and again it just seems to check things like "on the right filesystem" that aren't actually impacted by write access (in fact, the documented reason is to check that it's a read-only filesystem so that write access is simply _irrelevant_).

So this issue seems to be mainly a cleanliness thing, not an actual bug.

Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Kees Cook

author

2018-03-09 19:54 UTC #2 lore raw

Quoted 14 lines · Linus Torvalds

Quoted 6 lines · Kees Cook
The LSM check should happen after the file has been confirmed to be unchanging. Without this, we could have a race between the Time of Check (the call to security_kernel_read_file() which could read the file and make access policy decisions) and the Time of Use (starting with kernel_read_file()'s reading of the file contents). In theory, file contents could change between the two.
I'm going to assume I get this for 4.17 from the security tree.
Because I'm guessing there are actually no existing users that care? selinux seems to just look at file state, not actually at contents or anything that write access denial would care about.

Quoted 7 lines

And the only other security module that even registers this is loadpin, and again it just seems to check things like "on the right filesystem" that aren't actually impacted by write access (in fact, the documented reason is to check that it's a read-only filesystem so that write access is simply _irrelevant_).
So this issue seems to be mainly a cleanliness thing, not an actual bug.

That is my assumption too (I left off the Cc: stable as a result). I'm much less familiar with IMA, though, but it's a caller of kernel_read_file(), not hooking it, etc.

-Kees

Signature · 6 lines

Kees Cook
Pixel Security
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Mimi Zohar

2018-03-09 21:54 UTC #3 lore raw

Quoted 9 lines · Kees Cook

Quoted 7 lines · Linus Torvalds
Quoted 6 lines · Kees Cook
The LSM check should happen after the file has been confirmed to be unchanging. Without this, we could have a race between the Time of Check (the call to security_kernel_read_file() which could read the file and make access policy decisions) and the Time of Use (starting with kernel_read_file()'s reading of the file contents). In theory, file contents could change between the two.

For files opened by userspace, IMA refers to the problem as "Time of Measure, Time of Use" (ToMToU) and emits an audit message.

security_kernel_read_file() is being called by the kernel to read the kexec kernel image and initramfs, kernel modules (new syscall), ima_policy, EVM x509 certificate, and firmware.

If these files are signed, like they should be, then IMA prevents them from being opened for write. ?Modifying the file via the filesystem should not be possible. ?Other sorts of attacks, would probably be possible.

If these files aren't signed, then in terms of IMA-measurement the file measured, might not be the file used. ?The ToMToU audit message is not being generated for these files.

Quoted 12 lines

Quoted 12 lines
I'm going to assume I get this for 4.17 from the security tree.
Because I'm guessing there are actually no existing users that care? selinux seems to just look at file state, not actually at contents or anything that write access denial would care about.
And the only other security module that even registers this is loadpin, and again it just seems to check things like "on the right filesystem" that aren't actually impacted by write access (in fact, the documented reason is to check that it's a read-only filesystem so that write access is simply _irrelevant_).

Quoted 5 lines

Quoted 1 line
So this issue seems to be mainly a cleanliness thing, not an actual bug.
That is my assumption too (I left off the Cc: stable as a result). I'm much less familiar with IMA, though, but it's a caller of kernel_read_file(), not hooking it, etc.

Please add my reviewed-by.

Mimi

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

James Morris

maintainer

2018-03-13 05:16 UTC #4 lore raw

In reply to Kees Cook

Quoted 10 lines · Kees Cook

The LSM check should happen after the file has been confirmed to be unchanging. Without this, we could have a race between the Time of Check (the call to security_kernel_read_file() which could read the file and make access policy decisions) and the Time of Use (starting with kernel_read_file()'s reading of the file contents). In theory, file contents could change between the two.
Signed-off-by Kees Cook <[email protected]>
---
v2: Clarify the ToC/ToU race (Linus)

I'll merge this unless Al objects (cc'd).

Quoted 32 lines

---
 fs/exec.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)