On 6/16/21 9:22 AM, Roberto Sassu wrote:
vfs_getxattr() differs from vfs_setxattr() in the way it obtains the xattr
value. The former gives precedence to the LSMs, and if the LSMs don't
provide a value, obtains it from the filesystem handler. The latter does
the opposite, first invokes the filesystem handler, and if the filesystem
does not support xattrs, passes the xattr value to the LSMs.
The problem is that not necessarily the user gets the same xattr value that
he set. For example, if he sets security.selinux with a value not
terminated with '\0', he gets a value terminated with '\0' because SELinux
adds it during the translation from xattr to internal representation
(vfs_setxattr()) and from internal representation to xattr
(vfs_getxattr()).
Normally, this does not have an impact unless the integrity of xattrs is
verified with EVM. The kernel and the user see different values due to the
different functions used to obtain them:
kernel (EVM): uses vfs_getxattr_alloc() which obtains the xattr value from
the filesystem handler (raw value);
user (ima-evm-utils): uses vfs_getxattr() which obtains the xattr value
from the LSMs (normalized value).
Maybe there should be another implementation similar to
vfs_getxattr_alloc() (or modify it) to behave like vfs_getxattr() but do
the memory allocation part so that the kernel sees what user space see
rather than modifying it with your patch so that user space now sees
something different than what it has been for years (previous
NUL-terminated SELinux xattr may not be NUL-terminated anymore)?
Stefan
Given that the difference between the raw value and the normalized value
should be just the additional '\0' not the rest of the content, this patch
modifies vfs_getxattr() to compare the size of the xattr value obtained
from the LSMs to the size of the raw xattr value. If there is a mismatch
and the filesystem handler does not return an error, vfs_getxattr() returns
the raw value.
This patch should have a minimal impact on existing systems, because if the
SELinux label is written with the appropriate tools such as setfiles or
restorecon, there will not be a mismatch (because the raw value also has
'\0').
In the case where the SELinux label is written directly with setfattr and
without '\0', this patch helps to align EVM and ima-evm-utils in terms of
result provided (due to the fact that they both verify the integrity of
xattrs from raw values).
Signed-off-by: Roberto Sassu <roberto.sassu@xxxxxxxxxx>
Tested-by: Mimi Zohar <zohar@xxxxxxxxxxxxx>
---
fs/xattr.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/fs/xattr.c b/fs/xattr.c
index 5c8c5175b385..412ec875aa07 100644
--- a/fs/xattr.c
+++ b/fs/xattr.c
@@ -420,12 +420,27 @@ vfs_getxattr(struct user_namespace *mnt_userns, struct dentry *dentry,
const char *suffix = name + XATTR_SECURITY_PREFIX_LEN;
int ret = xattr_getsecurity(mnt_userns, inode, suffix, value,
size);
+ int ret_raw;
+
/*
* Only overwrite the return value if a security module
* is actually active.
*/
if (ret == -EOPNOTSUPP)
goto nolsm;
+
+ if (ret < 0)
+ return ret;
+
+ /*
+ * Read raw xattr if the size from the filesystem handler
+ * differs from that returned by xattr_getsecurity() and is
+ * equal or greater than zero.
+ */
+ ret_raw = __vfs_getxattr(dentry, inode, name, NULL, 0);
+ if (ret_raw >= 0 && ret_raw != ret)
+ goto nolsm;
+
return ret;
}
nolsm:
