Bug in ima_inode_setxattr()

[Christian Brauner <brauner@xxxxxxxxxx>]

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.)

Christian