On Wed, 2022-08-17 at 18:31 +0200, Christian Brauner wrote:
> Hey Mimi,
> Heim Roberto
> Hey everyone,
>
> I'm currently reworking posix acls - well trying to - and so I'm also
> looking at various security hooks. Currently I'm looking at
> security_inode_setxattr() which calls ima_inode_setxattr() and I think
> it's doing strange things when system.posix_acl_{access,deafult} is
> passed as xattr. So we have:
>
> ima_inode_setxattr(system.posix_acl_{access,default})
> -> ima_protect_xattr(system.posix_acl_{access,default})
> -> evm_revalidate_status(system.posix_acl_{access,default})
> -> validate_hash_algo(system.posix_acl_{access,default}))
>
> Since this isn't an ima_protect_xattr() it will return 0. But
> evm_revalidate_status() will return true for posix acls as they seem to
> be included. In any case, this will cause validate_hash_algo() to run:
>
> So the posix acl value is cast to a struct evm_ima_xattr_data:
>
> const struct evm_ima_xattr_data *xvalue = xattr_value;
>
> then passed to:
>
> validate_hash_algo(struct dentry *dentry, const struct evm_ima_xattr_data *xattr_value, size_t xattr_value_len)
>
> which then passes it to:
>
> enum hash_algo ima_get_hash_algo(const struct evm_ima_xattr_data *xattr_value, int xattr_len)
>
> which does:
>
> switch (xattr_value->type) {
> case IMA_VERITY_DIGSIG:
> sig = (typeof(sig))xattr_value;
> if (sig->version != 3 || xattr_len <= sizeof(*sig) ||
> sig->hash_algo >= HASH_ALGO__LAST)
> return ima_hash_algo;
> return sig->hash_algo;
> case EVM_IMA_XATTR_DIGSIG:
> sig = (typeof(sig))xattr_value;
> if (sig->version != 2 || xattr_len <= sizeof(*sig)
> || sig->hash_algo >= HASH_ALGO__LAST)
> return ima_hash_algo;
> return sig->hash_algo;
> case IMA_XATTR_DIGEST_NG:
> /* first byte contains algorithm id */
> ret = xattr_value->data[0];
> if (ret < HASH_ALGO__LAST)
> return ret;
> break;
> case IMA_XATTR_DIGEST:
> /* this is for backward compatibility */
> if (xattr_len == 21) {
> unsigned int zero = 0;
> if (!memcmp(&xattr_value->data[16], &zero, 4))
> return HASH_ALGO_MD5;
> else
> return HASH_ALGO_SHA1;
> } else if (xattr_len == 17)
> return HASH_ALGO_MD5;
> break;
> }
>
> unless I have a fundamental misunderstanding this can't possibly be
> right as you're casting a posix acl value to a struct
> evm_ima_xattr_data and then try to retrieve
>
> Explaining my findings to Seth (Cced) he found that it got introduced by
> 50f742dd9147 "IMA: block writes of the security.ima xattr with unsupported algorithms"
>
> Could you please fix this in ima_inode_setxattr() and please in a way
> that you never operate on raw posix acl values? I really would like to
> prevent further integrity assumptions about posix acls; I know evm
> already has some but let's not get ima involved in them as well...
>
> (They are a pain to deal with and they represent very much structured
> binary data with {g,u}ids stored in them that need to be handled in the
> VFS layer.)
Thank you for the clear explanation of the problem. The hash algorithm
check should have been right after the call to ima_protect_xattr(),
which would have limited it to just security.ima xattrs.
thanks,
Mimi
