RE: [PATCH 06/11] evm: Allow setxattr() and setattr() if metadata digest won't change

From: Roberto Sassu
Date: Mon Aug 31 2020 - 04:51:55 EST


> 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);
>