[PATCH] evm: fix TOCTOU heap OOB in protected xattr reads

[PATCH] evm: fix TOCTOU heap OOB in protected xattr reads

[Jérémy Jean]

ima_eventinodexattrs_init_common() queries the total size of protected
xattrs, allocates that amount, and calls evm_read_protected_xattrs()
again to fill the buffer. This creates a TOCTOU window: if the
protected xattr changes between the two calls to
evm_read_protected_xattrs(), names and lengths are written without
checking whether the data still fits in the target buffer. This can
yield an OOB heap write that KASAN reports as:

  BUG: KASAN: slab-out-of-bounds in evm_read_protected_xattrs+0x15e/0x280
  Write of size 20 at addr ffff888002c59bcd by task init/62

Reject fields that no longer fit in the target buffer. For xattr
values, use the length actually returned by the copy.

Fixes: 8314b6732ae4e ("ima: Define new template fields xattrnames, xattrlengths and xattrvalues")
Cc: stable@vger.kernel.org
Signed-off-by: Jérémy Jean <Jeremy.Jean@oss.cyber.gouv.fr>
---
 security/integrity/evm/evm_main.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index ac8328812e6ef..4b01ca0b4753a 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -389,6 +389,10 @@ int evm_read_protected_xattrs(struct dentry *dentry, u8 *buffer,
 		case 'n':
 			size = strlen(xattr->name) + 1;
 			if (buffer) {
+				if (total_size > buffer_size ||
+				    size > buffer_size - total_size)
+					return -ERANGE;
+
 				if (total_size)
 					*(buffer + total_size - 1) = '|';
 
@@ -398,6 +402,10 @@ int evm_read_protected_xattrs(struct dentry *dentry, u8 *buffer,
 		case 'l':
 			size = sizeof(u32);
 			if (buffer) {
+				if (total_size > buffer_size ||
+				    size > buffer_size - total_size)
+					return -ERANGE;
+
 				if (canonical_fmt)
 					rc = (__force int)cpu_to_le32(rc);
 
@@ -407,12 +415,17 @@ int evm_read_protected_xattrs(struct dentry *dentry, u8 *buffer,
 		case 'v':
 			size = rc;
 			if (buffer) {
+				if (total_size > buffer_size ||
+				    size > buffer_size - total_size)
+					return -ERANGE;
+
 				rc = __vfs_getxattr(dentry,
 					d_backing_inode(dentry), xattr->name,
 					buffer + total_size,
 					buffer_size - total_size);
 				if (rc < 0)
 					return rc;
+				size = rc;
 			}
 			break;
 		default:
-- 
2.47.3

Re: [PATCH] evm: fix TOCTOU heap OOB in protected xattr reads

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] evm: incomplete TOCTOU bounds check in
  evm_read_protected_xattrs()
--

commit 94961f480ee0e6be15409d6ea100ede9af939446
Author: Jérémy Jean <Jeremy.Jean@oss.cyber.gouv.fr>

evm: fix TOCTOU heap OOB in protected xattr reads

This commit adds bounds checks in evm_read_protected_xattrs() to reject
protected xattr names, lengths, and values that no longer fit in the
caller's buffer if an xattr changes between the size query and the read.

> diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
> index b59e3f121b8a8..d00b050edb811 100644
> --- a/security/integrity/evm/evm_main.c
> +++ b/security/integrity/evm/evm_main.c
[ ... ]
> @@ -407,12 +415,17 @@ int evm_read_protected_xattrs(struct dentry *dentry, u8 *buffer,
>  		case 'v':
>  			size = rc;
>  			if (buffer) {
> +				if (total_size > buffer_size ||
> +				    size > buffer_size - total_size)
> +					return -ERANGE;
> +
>  				rc = __vfs_getxattr(dentry,
>  					d_backing_inode(dentry), xattr->name,
>  					buffer + total_size,
>  					buffer_size - total_size);
>  				if (rc < 0)
>  					return rc;
> +				size = rc;
>  			}
>  			break;

[Severity: Medium]
Can total_size still exceed buffer_size when total_size == buffer_size and
an xattr grows from 0 bytes between the two __vfs_getxattr() calls?

When the first __vfs_getxattr() returns 0 for an empty xattr and
total_size == buffer_size, the bounds check passes and the second
__vfs_getxattr() is called with a size argument of
buffer_size - total_size == 0. Many filesystem xattr handlers (such as
btrfs_getxattr(), xfs_attr_copy_value(), and fuse_getxattr()) treat a zero
size argument as a length query rather than an empty buffer, so if the
xattr concurrently grew to N > 0 bytes, __vfs_getxattr() returns N instead
of -ERANGE.

Assigning size = rc then advances total_size to buffer_size + N, causing
ima_eventinodexattrs_init_common() -> ima_write_template_field_data() to
memcpy() past the end of buffer (or dereference ZERO_SIZE_PTR when
buffer_size == 0). Skipping the second __vfs_getxattr() call when size == 0
or checking that rc <= buffer_size - total_size would avoid this.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261107194909.1061154-2-Jeremy.Jean@oss.cyber.gouv.fr?part=1