On Thu, 2020-06-18 at 18:04 +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
> appraisal, always allows access to the files without checking metadata
> again.
^after the first successful appraisal
>
> This patch checks in evm_reset_status() if EVM_ALLOW_METADATA WRITES is
> enabled and if it is, sets the IMA_CHANGE_XATTR/ATTR bits depending on the
> operation performed. At the next appraisal, metadata are revalidated.
EVM modifying IMA bits crosses the boundary between EVM and IMA. There
is already an IMA post_setattr hook. IMA could reset its own bit
there. If necessary EVM could export as a function it's status info.
Mimi
>
> 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 | 14 ++++++++++----
> 1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
> index 41cc6a4aaaab..d4d918183094 100644
> --- a/security/integrity/evm/evm_main.c
> +++ b/security/integrity/evm/evm_main.c
> @@ -478,13 +478,17 @@ int evm_inode_removexattr(struct dentry *dentry, const char *xattr_name)
> return evm_protect_xattr(dentry, xattr_name, NULL, 0);
> }
>
> -static void evm_reset_status(struct inode *inode)
> +static void evm_reset_status(struct inode *inode, int bit)
> {
> struct integrity_iint_cache *iint;
>
> iint = integrity_iint_find(inode);
> - if (iint)
> + if (iint) {
> + if (evm_initialized & EVM_ALLOW_METADATA_WRITES)
> + set_bit(bit, &iint->atomic_flags);
> +
> iint->evm_status = INTEGRITY_UNKNOWN;
> + }
> }
>
> /**:q
> @@ -507,7 +511,7 @@ void evm_inode_post_setxattr(struct dentry *dentry, const char *xattr_name,
> && !posix_xattr_acl(xattr_name)))
> return;
>
> - evm_reset_status(dentry->d_inode);
> + evm_reset_status(dentry->d_inode, IMA_CHANGE_XATTR);
>
> evm_update_evmxattr(dentry, xattr_name, xattr_value, xattr_value_len);
> }
> @@ -527,7 +531,7 @@ void evm_inode_post_removexattr(struct dentry *dentry, const char *xattr_name)
> if (!evm_key_loaded() || !evm_protected_xattr(xattr_name))
> return;
>
> - evm_reset_status(dentry->d_inode);
> + evm_reset_status(dentry->d_inode, IMA_CHANGE_XATTR);
>
> evm_update_evmxattr(dentry, xattr_name, NULL, 0);
> }
> @@ -600,6 +604,8 @@ void evm_inode_post_setattr(struct dentry *dentry, int ia_valid)
> if (!evm_key_loaded())
> return;
>
> + evm_reset_status(dentry->d_inode, IMA_CHANGE_ATTR);
> +
> if (ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID))
> evm_update_evmxattr(dentry, NULL, NULL, 0);
> }
