Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH] fs: Return raw xattr for security.* if there is size
 disagreement with LSMs
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
References: <ee75bde9a17f418984186caa70abd33b@huawei.com>
 <20210616132227.999256-1-roberto.sassu@huawei.com>
From:   Stefan Berger <stefanb@linux.ibm.com>
Message-ID: <6e1c9807-d7e8-7c26-e0ee-975afa4b9515@linux.ibm.com>
Date:   Wed, 16 Jun 2021 10:40:23 -0400
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.10.1
MIME-Version: 1.0
In-Reply-To: <20210616132227.999256-1-roberto.sassu@huawei.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Precedence: bulk

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: