| Message ID | 20201111092302.1589-7-roberto.sassu@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | evm: Improve usability of portable signatures | expand |
Hi Roberto, On Wed, 2020-11-11 at 10:22 +0100, Roberto Sassu wrote: > When a file is being created, LSMs can set the initial label with the > inode_init_security hook. If no HMAC key is loaded, the new file will have > LSM xattrs but not the HMAC. > > Unfortunately, EVM will deny any further metadata operation on new files, > as evm_protect_xattr() will always return the INTEGRITY_NOLABEL error. This > would limit the usability of EVM when only a public key is loaded, as > commands such as cp or tar with the option to preserve xattrs won't work. > > Ignoring this error won't be an issue if no HMAC key is loaded, as the > inode is locked until the post hook, and EVM won't calculate the HMAC on > metadata that wasn't previously verified. Thus this patch checks if an > HMAC key is loaded and if not, ignores INTEGRITY_NOLABEL. I'm not sure what problem this patch is trying to solve. evm_protect_xattr() is only called by evm_inode_setxattr() and evm_inode_removexattr(), which first checks whether EVM_ALLOW_METADATA_WRITES is enabled. Mimi
> From: Mimi Zohar [mailto:zohar@linux.ibm.com] > Sent: Thursday, December 3, 2020 9:43 PM > Hi Roberto, > > On Wed, 2020-11-11 at 10:22 +0100, Roberto Sassu wrote: > > When a file is being created, LSMs can set the initial label with the > > inode_init_security hook. If no HMAC key is loaded, the new file will have > > LSM xattrs but not the HMAC. > > > > Unfortunately, EVM will deny any further metadata operation on new > files, > > as evm_protect_xattr() will always return the INTEGRITY_NOLABEL error. > This > > would limit the usability of EVM when only a public key is loaded, as > > commands such as cp or tar with the option to preserve xattrs won't work. > > > > Ignoring this error won't be an issue if no HMAC key is loaded, as the > > inode is locked until the post hook, and EVM won't calculate the HMAC on > > metadata that wasn't previously verified. Thus this patch checks if an > > HMAC key is loaded and if not, ignores INTEGRITY_NOLABEL. > > I'm not sure what problem this patch is trying to solve. > evm_protect_xattr() is only called by evm_inode_setxattr() and > evm_inode_removexattr(), which first checks whether > EVM_ALLOW_METADATA_WRITES is enabled. The idea is to also support EVM verification when only a public key is loaded. An advantage to do that is that for example we can prevent accidental metadata changes when the signature is portable. Roberto HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063 Managing Director: Li Peng, Li Jian, Shi Yanli
On Fri, 2020-12-04 at 08:05 +0000, Roberto Sassu wrote: > > From: Mimi Zohar [mailto:zohar@linux.ibm.com] > > Sent: Thursday, December 3, 2020 9:43 PM > > Hi Roberto, > > > > On Wed, 2020-11-11 at 10:22 +0100, Roberto Sassu wrote: > > > When a file is being created, LSMs can set the initial label with the > > > inode_init_security hook. If no HMAC key is loaded, the new file will have > > > LSM xattrs but not the HMAC. > > > > > > Unfortunately, EVM will deny any further metadata operation on new > > files, > > > as evm_protect_xattr() will always return the INTEGRITY_NOLABEL error. > > This > > > would limit the usability of EVM when only a public key is loaded, as > > > commands such as cp or tar with the option to preserve xattrs won't work. > > > > > > Ignoring this error won't be an issue if no HMAC key is loaded, as the > > > inode is locked until the post hook, and EVM won't calculate the HMAC on > > > metadata that wasn't previously verified. Thus this patch checks if an > > > HMAC key is loaded and if not, ignores INTEGRITY_NOLABEL. > > > > I'm not sure what problem this patch is trying to solve. > > evm_protect_xattr() is only called by evm_inode_setxattr() and > > evm_inode_removexattr(), which first checks whether > > EVM_ALLOW_METADATA_WRITES is enabled. > > The idea is to also support EVM verification when only a public key > is loaded. An advantage to do that is that for example we can prevent > accidental metadata changes when the signature is portable. Right, there are a couple of scenarios. Let's be more specific as to which scenario this patch is addressing. - a public key is loaded and EVM_ALLOW_METADATA_WRITES is enabled, - a public key is loaded and EVM_ALLOW_METADATA_WRITES is disabled, - an HMAC key is loaded For the first and last case, this patch shouldn't be necessary. Only the second case, with EVM_ALLOW_METADATA_WRITES disabled, probably does not work. I would claim that is working as designed. thanks, Mimi
> From: Mimi Zohar [mailto:zohar@linux.ibm.com] > Sent: Friday, December 4, 2020 2:05 PM > On Fri, 2020-12-04 at 08:05 +0000, Roberto Sassu wrote: > > > From: Mimi Zohar [mailto:zohar@linux.ibm.com] > > > Sent: Thursday, December 3, 2020 9:43 PM > > > Hi Roberto, > > > > > > On Wed, 2020-11-11 at 10:22 +0100, Roberto Sassu wrote: > > > > When a file is being created, LSMs can set the initial label with the > > > > inode_init_security hook. If no HMAC key is loaded, the new file will > have > > > > LSM xattrs but not the HMAC. > > > > > > > > Unfortunately, EVM will deny any further metadata operation on new > > > files, > > > > as evm_protect_xattr() will always return the INTEGRITY_NOLABEL > error. > > > This > > > > would limit the usability of EVM when only a public key is loaded, as > > > > commands such as cp or tar with the option to preserve xattrs won't > work. > > > > > > > > Ignoring this error won't be an issue if no HMAC key is loaded, as the > > > > inode is locked until the post hook, and EVM won't calculate the HMAC > on > > > > metadata that wasn't previously verified. Thus this patch checks if an > > > > HMAC key is loaded and if not, ignores INTEGRITY_NOLABEL. > > > > > > I'm not sure what problem this patch is trying to solve. > > > evm_protect_xattr() is only called by evm_inode_setxattr() and > > > evm_inode_removexattr(), which first checks whether > > > EVM_ALLOW_METADATA_WRITES is enabled. > > > > The idea is to also support EVM verification when only a public key > > is loaded. An advantage to do that is that for example we can prevent > > accidental metadata changes when the signature is portable. > > Right, there are a couple of scenarios. Let's be more specific as to > which scenario this patch is addressing. > > - a public key is loaded and EVM_ALLOW_METADATA_WRITES is enabled, > - a public key is loaded and EVM_ALLOW_METADATA_WRITES is disabled, > - an HMAC key is loaded > > For the first and last case, this patch shouldn't be necessary. Only > the second case, with EVM_ALLOW_METADATA_WRITES disabled, probably > does > not work. I would claim that is working as designed. If there is no HMAC key loaded and a file is created, I think EVM should not expect an HMAC and return an error. If we do metadata verification only when an HMAC key is loaded, we miss a functionality that could be useful also when only a public key is loaded. Roberto HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063 Managing Director: Li Peng, Li Jian, Shi Yanli
diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c index b38ffa39faa8..4f4404a12bbd 100644 --- a/security/integrity/evm/evm_main.c +++ b/security/integrity/evm/evm_main.c @@ -354,6 +354,14 @@ static int evm_protect_xattr(struct dentry *dentry, const char *xattr_name, -EPERM, 0); } out: + /* + * Ignoring INTEGRITY_NOLABEL is safe if no HMAC key is loaded, as + * EVM won't calculate the HMAC of metadata that wasn't previously + * verified. + */ + if (evm_status == INTEGRITY_NOLABEL && + !(evm_initialized & EVM_INIT_HMAC)) + return 0; if (evm_status != INTEGRITY_PASS) integrity_audit_msg(AUDIT_INTEGRITY_METADATA, d_backing_inode(dentry), dentry->d_name.name, "appraise_metadata", @@ -514,8 +522,15 @@ 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); + /* + * Ignoring INTEGRITY_NOLABEL is safe if no HMAC key is loaded, as + * EVM won't calculate the HMAC of metadata that wasn't previously + * verified. + */ if ((evm_status == INTEGRITY_PASS) || - (evm_status == INTEGRITY_NOXATTRS)) + (evm_status == INTEGRITY_NOXATTRS) || + (evm_status == INTEGRITY_NOLABEL && + !(evm_initialized & EVM_INIT_HMAC))) return 0; integrity_audit_msg(AUDIT_INTEGRITY_METADATA, d_backing_inode(dentry), dentry->d_name.name, "appraise_metadata",
When a file is being created, LSMs can set the initial label with the inode_init_security hook. If no HMAC key is loaded, the new file will have LSM xattrs but not the HMAC. Unfortunately, EVM will deny any further metadata operation on new files, as evm_protect_xattr() will always return the INTEGRITY_NOLABEL error. This would limit the usability of EVM when only a public key is loaded, as commands such as cp or tar with the option to preserve xattrs won't work. Ignoring this error won't be an issue if no HMAC key is loaded, as the inode is locked until the post hook, and EVM won't calculate the HMAC on metadata that wasn't previously verified. Thus this patch checks if an HMAC key is loaded and if not, ignores INTEGRITY_NOLABEL. Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> --- security/integrity/evm/evm_main.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-)