[Cc'ing John Johansen]
Hi Roberto,
On Fri, 2020-09-04 at 11:26 +0200, Roberto Sassu wrote:
> When EVM_ALLOW_METADATA_WRITES is set, EVM allows any operation on
> metadata. Its main purpose is to allow users to freely set metadata when
> they are protected by a portable signature, until the HMAC key is loaded.
>
> However, IMA is not notified about metadata changes and, after the first
> successful appraisal, always allows access to the files without checking
> metadata again.
>
> This patch introduces the new atomic flag EVM_RESET_STATUS in
> integrity_iint_cache that is set in the EVM post hooks and cleared in
> evm_verify_hmac(). IMA checks the new flag in process_measurement() and if
> it is set, it clears the appraisal flags.
>
> Although the flag could be cleared also by evm_inode_setxattr() and
> evm_inode_setattr() before IMA sees it, this does not happen if
> EVM_ALLOW_METADATA_WRITES is set. Since the only remaining caller is
> evm_verifyxattr(), this ensures that IMA always sees the flag set before it
> is cleared.
>
> This patch also adds a call to evm_reset_status() in
> evm_inode_post_setattr() so that EVM won't return the cached status the
> next time appraisal is performed.
>
> Cc: stable@xxxxxxxxxxxxxxx # 4.16.x
> Fixes: ae1ba1676b88e ("EVM: Allow userland to permit modification of EVM-protected metadata")
> Signed-off-by: Roberto Sassu <roberto.sassu@xxxxxxxxxx>
> ---
> security/integrity/evm/evm_main.c | 17 +++++++++++++++--
> security/integrity/ima/ima_main.c | 8 ++++++--
> security/integrity/integrity.h | 1 +
> 3 files changed, 22 insertions(+), 4 deletions(-)
>
> diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
> index 4e9f5e8b21d5..05be1ad3e6f3 100644
> --- a/security/integrity/evm/evm_main.c
> +++ b/security/integrity/evm/evm_main.c
> @@ -221,8 +221,15 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
> evm_status = (rc == -ENODATA) ?
> INTEGRITY_NOXATTRS : INTEGRITY_FAIL;
> out:
> - if (iint)
> + if (iint) {
> + /*
> + * EVM_RESET_STATUS can be cleared only by evm_verifyxattr()
> + * when EVM_ALLOW_METADATA_WRITES is set. This guarantees that
> + * IMA sees the EVM_RESET_STATUS flag set before it is cleared.
> + */
> + clear_bit(EVM_RESET_STATUS, &iint->atomic_flags);
> iint->evm_status = evm_status;
True IMA is currently the only caller of evm_verifyxattr() in the
upstreamed kernel, but it is an exported function, which may be called
from elsewhere. The previous version crossed the boundary between EVM
& IMA with EVM modifying the IMA flag directly. This version assumes
that IMA will be the only caller. Otherwise, I like this version.
Mimi
> + }
> kfree(xattr_data);
> return evm_status;
> }
> @@ -418,8 +425,12 @@ static void evm_reset_status(struct inode *inode)
> struct integrity_iint_cache *iint;
>
> iint = integrity_iint_find(inode);
> - if (iint)
> + if (iint) {
> + if (evm_initialized & EVM_ALLOW_METADATA_WRITES)
> + set_bit(EVM_RESET_STATUS, &iint->atomic_flags);
> +
> iint->evm_status = INTEGRITY_UNKNOWN;
> + }
> }
>
> /**
> @@ -513,6 +524,8 @@ void evm_inode_post_setattr(struct dentry *dentry, int ia_valid)
> if (!evm_key_loaded())
> return;
>
> + evm_reset_status(dentry->d_inode);
> +
> if (ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID))
> evm_update_evmxattr(dentry, NULL, NULL, 0);
> }
