On Thu, Sep 29, 2022 at 03:14:42PM -0400, Paul Moore wrote: > On Thu, Sep 29, 2022 at 11:33 AM Christian Brauner <brauner@xxxxxxxxxx> wrote: > > > > The current way of setting and getting posix acls through the generic > > xattr interface is error prone and type unsafe. The vfs needs to > > interpret and fixup posix acls before storing or reporting it to > > userspace. Various hacks exist to make this work. The code is hard to > > understand and difficult to maintain in it's current form. Instead of > > making this work by hacking posix acls through xattr handlers we are > > building a dedicated posix acl api around the get and set inode > > operations. This removes a lot of hackiness and makes the codepaths > > easier to maintain. A lot of background can be found in [1]. > > > > So far posix acls were passed as a void blob to the security and > > integrity modules. Some of them like evm then proceed to interpret the > > void pointer and convert it into the kernel internal struct posix acl > > representation to perform their integrity checking magic. This is > > obviously pretty problematic as that requires knowledge that only the > > vfs is guaranteed to have and has lead to various bugs. Add a proper > > security hook for setting posix acls and pass down the posix acls in > > their appropriate vfs format instead of hacking it through a void > > pointer stored in the uapi format. > > > > I spent considerate time in the security module and integrity > > infrastructure and audited all codepaths. EVM is the only part that > > really has restrictions based on the actual posix acl values passed > > through it. Before this dedicated hook EVM used to translate from the > > uapi posix acl format sent to it in the form of a void pointer into the > > vfs format. This is not a good thing. Instead of hacking around in the > > uapi struct give EVM the posix acls in the appropriate vfs format and > > perform sane permissions checks that mirror what it used to to in the > > generic xattr hook. > > > > IMA doesn't have any restrictions on posix acls. When posix acls are > > changed it just wants to update its appraisal status. > > > > The removal of posix acls is equivalent to passing NULL to the posix set > > acl hooks. This is the same as before through the generic xattr api. > > > > Link: https://lore.kernel.org/all/20220801145520.1532837-1-brauner@xxxxxxxxxx [1] > > Signed-off-by: Christian Brauner (Microsoft) <brauner@xxxxxxxxxx> > > --- > > > > Notes: > > /* v2 */ > > unchanged > > > > /* v3 */ > > Paul Moore <paul@xxxxxxxxxxxxxx>: > > - Add get, and remove acl hook > > > > /* v4 */ > > unchanged > > > > include/linux/evm.h | 23 +++++++++ > > include/linux/ima.h | 21 ++++++++ > > security/integrity/evm/evm_main.c | 70 ++++++++++++++++++++++++++- > > security/integrity/ima/ima_appraise.c | 9 ++++ > > security/security.c | 21 +++++++- > > 5 files changed, 141 insertions(+), 3 deletions(-) > > ... > > > diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c > > index 23d484e05e6f..7904786b610f 100644 > > --- a/security/integrity/evm/evm_main.c > > +++ b/security/integrity/evm/evm_main.c > > @@ -8,7 +8,7 @@ > > * > > * File: evm_main.c > > * implements evm_inode_setxattr, evm_inode_post_setxattr, > > - * evm_inode_removexattr, and evm_verifyxattr > > + * evm_inode_removexattr, evm_verifyxattr, and evm_inode_set_acl. > > */ > > > > #define pr_fmt(fmt) "EVM: "fmt > > @@ -670,6 +670,74 @@ int evm_inode_removexattr(struct user_namespace *mnt_userns, > > return evm_protect_xattr(mnt_userns, dentry, xattr_name, NULL, 0); > > } > > > > +static int evm_inode_set_acl_change(struct user_namespace *mnt_userns, > > + struct dentry *dentry, const char *name, > > + struct posix_acl *kacl) > > +{ > > +#ifdef CONFIG_FS_POSIX_ACL > > + int rc; > > + > > + umode_t mode; > > + struct inode *inode = d_backing_inode(dentry); > > + > > + if (!kacl) > > + return 1; > > + > > + rc = posix_acl_update_mode(mnt_userns, inode, &mode, &kacl); > > + if (rc || (inode->i_mode != mode)) > > + return 1; > > +#endif > > + return 0; > > +} > > I'm not too bothered by it either way, but one might consider pulling > the #ifdef outside the function definition, for example: > > #ifdef CONFIG_FS_POSIX_ACL > static int evm_inode_foo(...) > { > /* ... stuff ... */ > } > #else > static int evm_inode_foo(...) > { > return 0; > } > #endif /* CONFIG_FS_POSIX_ACL */ > > > diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c > > index bde74fcecee3..698a8ae2fe3e 100644 > > --- a/security/integrity/ima/ima_appraise.c > > +++ b/security/integrity/ima/ima_appraise.c > > @@ -770,6 +770,15 @@ int ima_inode_setxattr(struct dentry *dentry, const char *xattr_name, > > return result; > > } > > > > +int ima_inode_set_acl(struct user_namespace *mnt_userns, struct dentry *dentry, > > + const char *acl_name, struct posix_acl *kacl) > > +{ > > + if (evm_revalidate_status(acl_name)) > > + ima_reset_appraise_flags(d_backing_inode(dentry), 0); > > + > > + return 0; > > +} > > While the ima_inode_set_acl() implementation above looks okay for the > remove case, I do see that the ima_inode_setxattr() function has a > call to validate_hash_algo() before calling > ima_reset_appraise_flags(). IANAIE (I Am Not An Ima Expert), but it > seems like we would still want that check in the ACL case. Ah, you might've missed this bug... The fact that they call validate_hash_algo() on posix acls is a bug in ima. It's a type safety bug. IMA uses posix acls passed through the void pointer as struct evm_ima_xattr: const struct evm_ima_xattr_data *xvalue = xattr_value; result = validate_hash_algo(dentry, xvalue, xattr_value_len); I reported this to them a little while ago and Mimi sent a fix for it that's in -next: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=5926586f291b53cb8a0c9631fc19489be1186e2d IOW, what I have here seems correct.
