linux-integrity.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Stefan Berger <stefanb@linux.ibm.com>
To: Roberto Sassu <roberto.sassu@huawei.com>,
	viro@zeniv.linux.org.uk, zohar@linux.ibm.com,
	paul@paul-moore.com, stephen.smalley.work@gmail.com,
	casey@schaufler-ca.com
Cc: linux-fsdevel@vger.kernel.org, linux-integrity@vger.kernel.org,
	linux-security-module@vger.kernel.org,
	linux-kernel@vger.kernel.org, selinux@vger.kernel.org
Subject: Re: [PATCH] fs: Return raw xattr for security.* if there is size disagreement with LSMs
Date: Wed, 16 Jun 2021 10:40:23 -0400	[thread overview]
Message-ID: <6e1c9807-d7e8-7c26-e0ee-975afa4b9515@linux.ibm.com> (raw)
In-Reply-To: <20210616132227.999256-1-roberto.sassu@huawei.com>

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@huawei.com>
> Tested-by: Mimi Zohar <zohar@linux.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:

  reply	other threads:[~2021-06-16 14:40 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-11  9:44 Size mismatch between vfs_getxattr_alloc() and vfs_getxattr() Roberto Sassu
2021-06-16 13:22 ` [PATCH] fs: Return raw xattr for security.* if there is size disagreement with LSMs Roberto Sassu
2021-06-16 14:40   ` Stefan Berger [this message]
2021-06-17  7:09     ` Roberto Sassu
2021-06-17 15:27       ` Mimi Zohar
2021-06-17 16:05         ` Roberto Sassu
2021-06-18  3:18         ` Paul Moore
2021-06-18 16:04           ` Mimi Zohar
2021-06-18 16:10             ` Roberto Sassu
2021-06-18 16:35             ` Paul Moore
2021-06-18 17:22               ` Mimi Zohar

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=6e1c9807-d7e8-7c26-e0ee-975afa4b9515@linux.ibm.com \
    --to=stefanb@linux.ibm.com \
    --cc=casey@schaufler-ca.com \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-integrity@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-security-module@vger.kernel.org \
    --cc=paul@paul-moore.com \
    --cc=roberto.sassu@huawei.com \
    --cc=selinux@vger.kernel.org \
    --cc=stephen.smalley.work@gmail.com \
    --cc=viro@zeniv.linux.org.uk \
    --cc=zohar@linux.ibm.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).