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@xxxxxxxxxx> > --- > 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) > { >
