> From: Mimi Zohar [mailto:zohar@xxxxxxxxxxxxx] > Sent: Monday, August 24, 2020 2:17 PM > On Thu, 2020-06-18 at 18:04 +0200, Roberto Sassu wrote: > > If metadata are immutable, they cannot be changed. If metadata are > already > > set to the final value before cp and tar restore the value from the source, > > those applications display an error even if the operation is legitimate > > (they don't change the value). > > "metadata" is singular. The first sentence would be clearer by using > the specific metadata. What problem are you trying to solve? It > doesn't look like you're trying to solve the problem of writing the EVM > portable signatures without an exiting HMAC. > > > > > This patch determines whether setxattr()/setattr() change metadata and, > if > > not, allows the operations even if metadata are immutable. > > Doesn't setxattr/setattr always change file metadata? Please describe > the real problem. Yes. The problem is that tar/cp change metadata even if its value is already correct after the file has been created. These operations will be denied because metadata is immutable and verification succeeds. > > Signed-off-by: Roberto Sassu <roberto.sassu@xxxxxxxxxx> > > --- > > security/integrity/evm/evm_main.c | 72 > +++++++++++++++++++++++++++++++ > > 1 file changed, 72 insertions(+) > > > > diff --git a/security/integrity/evm/evm_main.c > b/security/integrity/evm/evm_main.c > > index 30072030f05d..41cc6a4aaaab 100644 > > --- a/security/integrity/evm/evm_main.c > > +++ b/security/integrity/evm/evm_main.c > > @@ -18,6 +18,7 @@ > > #include <linux/integrity.h> > > #include <linux/evm.h> > > #include <linux/magic.h> > > +#include <linux/posix_acl_xattr.h> > > > > #include <crypto/hash.h> > > #include <crypto/hash_info.h> > > @@ -305,6 +306,56 @@ static enum integrity_status > evm_verify_current_integrity(struct dentry *dentry) > > return evm_verify_hmac(dentry, NULL, NULL, 0, NULL); > > } > > > > +static int evm_xattr_acl_change(struct dentry *dentry, const char > *xattr_name, > > + const void *xattr_value, size_t > xattr_value_len) > > +{ > > + umode_t mode; > > + struct posix_acl *acl = NULL, *acl_res; > > + struct inode *inode = d_backing_inode(dentry); > > + int rc; > > + > > + /* UID/GID in ACL have been already converted from user to init ns > */ > > + acl = posix_acl_from_xattr(&init_user_ns, xattr_value, > xattr_value_len); > > + if (!acl) > > + return 1; > > + > > + acl_res = acl; > > + rc = posix_acl_update_mode(inode, &mode, &acl_res); > > + > > + posix_acl_release(acl); > > + > > + if (rc) > > + return 1; > > + > > + if (acl_res && inode->i_mode != mode) > > + return 1; > > + > > + return 0; > > +} > > + > > +static int evm_xattr_change(struct dentry *dentry, const char > *xattr_name, > > + const void *xattr_value, size_t xattr_value_len) > > +{ > > + char *xattr_data = NULL; > > + int rc = 0; > > + > > + if (posix_xattr_acl(xattr_name)) > > + return evm_xattr_acl_change(dentry, xattr_name, > xattr_value, > > + xattr_value_len); > > + > > + rc = vfs_getxattr_alloc(dentry, xattr_name, &xattr_data, 0, > GFP_NOFS); > > + if (rc < 0) > > + return 1; > > + > > + if (rc == xattr_value_len) > > + rc = memcmp(xattr_value, xattr_data, rc); > > + else > > + rc = 1; > > + > > + kfree(xattr_data); > > + return rc; > > +} > > + > > /* > > * evm_protect_xattr - protect the EVM extended attribute > > * > > @@ -361,6 +412,10 @@ static int evm_protect_xattr(struct dentry > *dentry, const char *xattr_name, > > if (evm_status == INTEGRITY_FAIL_IMMUTABLE) > > return 0; > > > > + if (evm_status == INTEGRITY_PASS_IMMUTABLE && > > + !evm_xattr_change(dentry, xattr_name, xattr_value, > xattr_value_len)) > > + return 0; > > + > > if (evm_status != INTEGRITY_PASS) > > integrity_audit_msg(AUDIT_INTEGRITY_METADATA, > d_backing_inode(dentry), > > dentry->d_name.name, > "appraise_metadata", > > @@ -477,6 +532,19 @@ void evm_inode_post_removexattr(struct dentry > *dentry, const char *xattr_name) > > evm_update_evmxattr(dentry, xattr_name, NULL, 0); > > } > > > > +static int evm_attr_change(struct dentry *dentry, struct iattr *attr) > > static functions don't normally require a function comment, but in this > case it wouldn't hurt to clarify why the uid, gid, mode bits are set, > but aren't being modified. > Similarly a function comment would help with the readability of > evm_xattr_acl_change(). Ok. Roberto HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063 Managing Director: Li Peng, Li Jian, Shi Yanli > Mimi > > > +{ > > + struct inode *inode = d_backing_inode(dentry); > > + unsigned int ia_valid = attr->ia_valid; > > + > > + if ((!(ia_valid & ATTR_UID) || uid_eq(attr->ia_uid, inode->i_uid)) > && > > + (!(ia_valid & ATTR_GID) || gid_eq(attr->ia_gid, inode->i_gid)) && > > + (!(ia_valid & ATTR_MODE) || attr->ia_mode == inode->i_mode)) > > + return 0; > > + > > + return 1; > > +} > > + > > /** > > * evm_inode_setattr - prevent updating an invalid EVM extended > attribute > > * @dentry: pointer to the affected dentry > > @@ -506,6 +574,10 @@ int evm_inode_setattr(struct dentry *dentry, > struct iattr *attr) > > (evm_status == INTEGRITY_FAIL_IMMUTABLE)) > > return 0; > > > > + if (evm_status == INTEGRITY_PASS_IMMUTABLE && > > + !evm_attr_change(dentry, attr)) > > + return 0; > > + > > integrity_audit_msg(AUDIT_INTEGRITY_METADATA, > d_backing_inode(dentry), > > dentry->d_name.name, "appraise_metadata", > > integrity_status_msg[evm_status], -EPERM, 0); >
