On Thu, 2020-06-18 at 18:01 +0200, Roberto Sassu wrote: > If files with portable signatures are copied from one location to another > or are extracted from an archive, verification can temporarily fail until > all xattrs/attrs are set in the destination. Portable signatures are the > only ones that can be moved to different files, as they don't depend on > system-specific information such as the inode generation.
^Only portable signatures may be moved or copied from one file to another, as they ... Instead portable signatures must include "security.ima". > Unlike other security.evm types, portable signatures ^, EVM portable signatures are also immutable. They > can never be replaced > even if an xattr/attr operation is granted, as once evm_update_evmxattr() > detects this type, it returns without updating the HMAC. Thus, it wouldn't > be a problem to allow those operations so that verification passes on the > destination after all xattrs/attrs are copied. This needs to be reworded a bit. > > This patch first introduces a new integrity status called > INTEGRITY_FAIL_IMMUTABLE, that allows callers of > evm_verify_current_integrity() to detect that a portable signature didn't > pass verification and then adds an exception in evm_protect_xattr() and > evm_inode_setattr() for this status and returns 0 instead of -EPERM. > > Signed-off-by: Roberto Sassu <[email protected]> After this patch, nothing prevents modifying the xattrs after all of them are in place and the signature verification would be successful. (Ok, that is being addressed in subsequent patches.) > --- > include/linux/integrity.h | 1 + > security/integrity/evm/evm_main.c | 25 ++++++++++++++++++++----- > security/integrity/ima/ima_appraise.c | 1 + > 3 files changed, 22 insertions(+), 5 deletions(-) > > diff --git a/include/linux/integrity.h b/include/linux/integrity.h > index 2271939c5c31..2ea0f2f65ab6 100644 > --- a/include/linux/integrity.h > +++ b/include/linux/integrity.h > @@ -13,6 +13,7 @@ enum integrity_status { > INTEGRITY_PASS = 0, > INTEGRITY_PASS_IMMUTABLE, > INTEGRITY_FAIL, > + INTEGRITY_FAIL_IMMUTABLE, > INTEGRITY_NOLABEL, > INTEGRITY_NOXATTRS, > INTEGRITY_UNKNOWN, > diff --git a/security/integrity/evm/evm_main.c > b/security/integrity/evm/evm_main.c > index 4e9f5e8b21d5..30072030f05d 100644 > --- a/security/integrity/evm/evm_main.c > +++ b/security/integrity/evm/evm_main.c > @@ -27,7 +27,8 @@ > int evm_initialized; > > static const char * const integrity_status_msg[] = { > - "pass", "pass_immutable", "fail", "no_label", "no_xattrs", "unknown" > + "pass", "pass_immutable", "fail", "fail_immutable", "no_label", > + "no_xattrs", "unknown" > }; > int evm_hmac_attrs; > > @@ -134,7 +135,7 @@ static enum integrity_status evm_verify_hmac(struct > dentry *dentry, > enum integrity_status evm_status = INTEGRITY_PASS; > struct evm_digest digest; > struct inode *inode; > - int rc, xattr_len; > + int rc, xattr_len, evm_immutable = 0; > > if (iint && (iint->evm_status == INTEGRITY_PASS || > iint->evm_status == INTEGRITY_PASS_IMMUTABLE)) > @@ -179,8 +180,10 @@ static enum integrity_status evm_verify_hmac(struct > dentry *dentry, > if (rc) > rc = -EINVAL; > break; > - case EVM_IMA_XATTR_DIGSIG: > case EVM_XATTR_PORTABLE_DIGSIG: > + evm_immutable = 1; > + fallthrough; > + case EVM_IMA_XATTR_DIGSIG: > /* accept xattr with non-empty signature field */ > if (xattr_len <= sizeof(struct signature_v2_hdr)) { > evm_status = INTEGRITY_FAIL; > @@ -219,7 +222,8 @@ static enum integrity_status evm_verify_hmac(struct > dentry *dentry, > > if (rc) > evm_status = (rc == -ENODATA) ? > - INTEGRITY_NOXATTRS : INTEGRITY_FAIL; > + INTEGRITY_NOXATTRS : evm_immutable ? > + INTEGRITY_FAIL_IMMUTABLE : INTEGRITY_FAIL; Embedded ternary operator should be replaced with normal C syntax. > out: > if (iint) > iint->evm_status = evm_status; > @@ -351,6 +355,12 @@ static int evm_protect_xattr(struct dentry *dentry, > const char *xattr_name, > -EPERM, 0); > } > out: > + /* It is safe to allow fail_immutable, portable signatures can never be > + * updated > + */ Replace "It" with "Writing other xattrs". Writing other xattrs is safe for portable signatures, as portable signatures are immutable and ...." > + if (evm_status == INTEGRITY_FAIL_IMMUTABLE) > + return 0; > + > if (evm_status != INTEGRITY_PASS) > integrity_audit_msg(AUDIT_INTEGRITY_METADATA, > d_backing_inode(dentry), > dentry->d_name.name, "appraise_metadata", > @@ -488,9 +498,14 @@ int evm_inode_setattr(struct dentry *dentry, struct > iattr *attr) > if (!(ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID))) > return 0; > evm_status = evm_verify_current_integrity(dentry); > + /* It is safe to allow fail_immutable, portable signatures can never > + * be updated > + */ Replace "It" with what is safe. Mimi > if ((evm_status == INTEGRITY_PASS) || > - (evm_status == INTEGRITY_NOXATTRS)) > + (evm_status == INTEGRITY_NOXATTRS) || > + (evm_status == INTEGRITY_FAIL_IMMUTABLE)) > 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/ima/ima_appraise.c > b/security/integrity/ima/ima_appraise.c > index a9649b04b9f1..21bda264fc30 100644 > --- a/security/integrity/ima/ima_appraise.c > +++ b/security/integrity/ima/ima_appraise.c > @@ -393,6 +393,7 @@ int ima_appraise_measurement(enum ima_hooks func, > case INTEGRITY_NOLABEL: /* No security.evm xattr. */ > cause = "missing-HMAC"; > goto out; > + case INTEGRITY_FAIL_IMMUTABLE: > case INTEGRITY_FAIL: /* Invalid HMAC/signature. */ > cause = "invalid-HMAC"; > goto out;
