linux-kernel - [PATCH] fs: Return raw xattr for security.* if there is size disagreement with LSMs

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:   Wed, 16 Jun 2021 15:22:27 +0200
From:   Roberto Sassu <roberto.sassu@...wei.com>
To:     <viro@...iv.linux.org.uk>, <zohar@...ux.ibm.com>,
        <paul@...l-moore.com>, <stephen.smalley.work@...il.com>,
        <casey@...aufler-ca.com>, <stefanb@...ux.ibm.com>
CC:     <linux-fsdevel@...r.kernel.org>, <linux-integrity@...r.kernel.org>,
        <linux-security-module@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <selinux@...r.kernel.org>,
        Roberto Sassu <roberto.sassu@...wei.com>
Subject: [PATCH] fs: Return raw xattr for security.* if there is size disagreement with LSMs

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).

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@...wei.com>
Tested-by: Mimi Zohar <zohar@...ux.ibm.com>
---
 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:
-- 
2.25.1