Re: [PATCH v2 14/25] evm: add support for fscaps security hooks

From: Roberto Sassu <roberto.sassu@huaweicloud.com>
To: "Seth Forshee (DigitalOcean)" <sforshee@kernel.org>,
	Christian Brauner <brauner@kernel.org>,
	Serge Hallyn <serge@hallyn.com>, Paul Moore <paul@paul-moore.com>,
	Eric Paris <eparis@redhat.com>, James Morris <jmorris@namei.org>,
	Alexander Viro <viro@zeniv.linux.org.uk>, Jan Kara <jack@suse.cz>,
	Stephen Smalley <stephen.smalley.work@gmail.com>,
	Ondrej Mosnacek <omosnace@redhat.com>,
	Casey Schaufler <casey@schaufler-ca.com>,
	Mimi Zohar <zohar@linux.ibm.com>,
	Roberto Sassu <roberto.sassu@huawei.com>,
	Dmitry Kasatkin <dmitry.kasatkin@gmail.com>,
	Eric Snowberg <eric.snowberg@oracle.com>,
	"Matthew Wilcox (Oracle)" <willy@infradead.org>,
	Jonathan Corbet <corbet@lwn.net>,
	Miklos Szeredi <miklos@szeredi.hu>,
	 Amir Goldstein <amir73il@gmail.com>
Cc: linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org,
	 linux-security-module@vger.kernel.org, audit@vger.kernel.org,
	 selinux@vger.kernel.org, linux-integrity@vger.kernel.org,
	 linux-doc@vger.kernel.org, linux-unionfs@vger.kernel.org
Subject: Re: [PATCH v2 14/25] evm: add support for fscaps security hooks
Date: Fri, 01 Mar 2024 10:19:13 +0100	[thread overview]
Message-ID: <15a69385b49c4f8626f082bc9b957132388414fb.camel@huaweicloud.com> (raw)
In-Reply-To: <20240221-idmap-fscap-refactor-v2-14-3039364623bd@kernel.org>

On Wed, 2024-02-21 at 15:24 -0600, Seth Forshee (DigitalOcean) wrote:
> Support the new fscaps security hooks by converting the vfs_caps to raw
> xattr data and then handling them the same as other xattrs.

Hi Seth

I started looking at this patch set.

The first question I have is if you are also going to update libcap
(and also tar, I guess), since both deal with the raw xattr.

From IMA/EVM perspective (Mimi will add on that), I guess it is
important that files with a signature/HMAC continue to be accessible
after applying this patch set.

Looking at the code, it seems the case (if I understood correctly,
vfs_getxattr_alloc() is still allowed).

To be sure that everything works, it would be really nice if you could
also extend our test suite:

https://github.com/mimizohar/ima-evm-utils/blob/next-testing/tests/portable_signatures.test

and

https://github.com/mimizohar/ima-evm-utils/blob/next-testing/tests/evm_hmac.test

The first test we would need to extend is check_cp_preserve_xattrs,
which basically does a cp -a. We would need to set fscaps in the
origin, copy to the destination, and see if the latter is accessible.

I would also extend:

check_tar_extract_xattrs_different_owner
check_tar_extract_xattrs_same_owner
check_metadata_change
check_evm_revalidate
check_evm_portable_sig_ima_appraisal
check_evm_portable_sig_ima_measurement_list

It should not be too complicated. The purpose would be to exercise your
code below.

Regarding the second test, we would need to extend just check_evm_hmac.

Just realized, before extending the tests, it would be necessary to
modify also evmctl.c, to retrieve fscaps through the new interfaces,
and to let users provide custom fscaps the HMAC or portable signature
is calculated on.

You can run the tests locally (even with UML linux), or make a PR in
Github for both linux and ima-evm-utils, and me and Mimi will help to
run them. For Github, for now please use:

https://github.com/linux-integrity/linux
https://github.com/mimizohar/ima-evm-utils/

Thanks

Roberto

> Signed-off-by: Seth Forshee (DigitalOcean) <sforshee@kernel.org>
> ---
>  include/linux/evm.h               | 39 +++++++++++++++++++++++++
>  security/integrity/evm/evm_main.c | 60 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 99 insertions(+)
> 
> diff --git a/include/linux/evm.h b/include/linux/evm.h
> index 36ec884320d9..aeb9ff52ad22 100644
> --- a/include/linux/evm.h
> +++ b/include/linux/evm.h
> @@ -57,6 +57,20 @@ static inline void evm_inode_post_set_acl(struct dentry *dentry,
>  {
>  	return evm_inode_post_setxattr(dentry, acl_name, NULL, 0);
>  }
> +extern int evm_inode_set_fscaps(struct mnt_idmap *idmap,
> +				struct dentry *dentry,
> +				const struct vfs_caps *caps, int flags);
> +static inline int evm_inode_remove_fscaps(struct dentry *dentry)
> +{
> +	return evm_inode_set_fscaps(&nop_mnt_idmap, dentry, NULL, XATTR_REPLACE);
> +}
> +extern void evm_inode_post_set_fscaps(struct mnt_idmap *idmap,
> +				      struct dentry *dentry,
> +				      const struct vfs_caps *caps, int flags);
> +static inline void evm_inode_post_remove_fscaps(struct dentry *dentry)
> +{
> +	return evm_inode_post_set_fscaps(&nop_mnt_idmap, dentry, NULL, 0);
> +}
>  
>  int evm_inode_init_security(struct inode *inode, struct inode *dir,
>  			    const struct qstr *qstr, struct xattr *xattrs,
> @@ -164,6 +178,31 @@ static inline void evm_inode_post_set_acl(struct dentry *dentry,
>  	return;
>  }
>  
> +static inline int evm_inode_set_fscaps(struct mnt_idmap *idmap,
> +				       struct dentry *dentry,
> +				       const struct vfs_caps *caps, int flags)
> +{
> +	return 0;
> +}
> +
> +static inline int evm_inode_remove_fscaps(struct dentry *dentry)
> +{
> +	return 0;
> +}
> +
> +static inline void evm_inode_post_set_fscaps(struct mnt_idmap *idmap,
> +					     struct dentry *dentry,
> +					     const struct vfs_caps *caps,
> +					     int flags)
> +{
> +	return;
> +}
> +
> +static inline void evm_inode_post_remove_fscaps(struct dentry *dentry)
> +{
> +	return;
> +}
> +
>  static inline int evm_inode_init_security(struct inode *inode, struct inode *dir,
>  					  const struct qstr *qstr,
>  					  struct xattr *xattrs,
> diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
> index cc7956d7878b..ecf4634a921a 100644
> --- a/security/integrity/evm/evm_main.c
> +++ b/security/integrity/evm/evm_main.c
> @@ -805,6 +805,66 @@ void evm_inode_post_removexattr(struct dentry *dentry, const char *xattr_name)
>  	evm_update_evmxattr(dentry, xattr_name, NULL, 0);
>  }
>  
> +int evm_inode_set_fscaps(struct mnt_idmap *idmap, struct dentry *dentry,
> +			 const struct vfs_caps *caps, int flags)
> +{
> +	struct inode *inode = d_inode(dentry);
> +	struct vfs_ns_cap_data nscaps;
> +	const void *xattr_data = NULL;
> +	int size = 0;
> +
> +	/* Policy permits modification of the protected xattrs even though
> +	 * there's no HMAC key loaded
> +	 */
> +	if (evm_initialized & EVM_ALLOW_METADATA_WRITES)
> +		return 0;
> +
> +	if (caps) {
> +		size = vfs_caps_to_xattr(idmap, i_user_ns(inode), caps, &nscaps,
> +					 sizeof(nscaps));
> +		if (size < 0)
> +			return size;
> +		xattr_data = &nscaps;
> +	}
> +
> +	return evm_protect_xattr(idmap, dentry, XATTR_NAME_CAPS, xattr_data, size);
> +}
> +
> +void evm_inode_post_set_fscaps(struct mnt_idmap *idmap, struct dentry *dentry,
> +			       const struct vfs_caps *caps, int flags)
> +{
> +	struct inode *inode = d_inode(dentry);
> +	struct vfs_ns_cap_data nscaps;
> +	const void *xattr_data = NULL;
> +	int size = 0;
> +
> +	if (!evm_revalidate_status(XATTR_NAME_CAPS))
> +		return;
> +
> +	evm_reset_status(dentry->d_inode);
> +
> +	if (!(evm_initialized & EVM_INIT_HMAC))
> +		return;
> +
> +	if (is_unsupported_fs(dentry))
> +		return;
> +
> +	if (caps) {
> +		size = vfs_caps_to_xattr(idmap, i_user_ns(inode), caps, &nscaps,
> +					 sizeof(nscaps));
> +		/*
> +		 * The fscaps here should have been converted to an xattr by
> +		 * evm_inode_set_fscaps() already, so a failure to convert
> +		 * here is a bug.
> +		 */
> +		if (WARN_ON_ONCE(size < 0))
> +			return;
> +		xattr_data = &nscaps;
> +	}
> +
> +	evm_update_evmxattr(dentry, XATTR_NAME_CAPS, xattr_data, size);
> +}
> +
>  static int evm_attr_change(struct mnt_idmap *idmap,
>  			   struct dentry *dentry, struct iattr *attr)
>  {
> 