| Message ID | 20200618160458.1579-6-roberto.sassu@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/11] evm: Execute evm_inode_init_security() only when the HMAC key is loaded | expand |
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. > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > --- > 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(). 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);
> From: Mimi Zohar [mailto:zohar@linux.ibm.com] > 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@huawei.com> > > --- > > 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); >
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) +{ + 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);
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). This patch determines whether setxattr()/setattr() change metadata and, if not, allows the operations even if metadata are immutable. Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> --- security/integrity/evm/evm_main.c | 72 +++++++++++++++++++++++++++++++ 1 file changed, 72 insertions(+)