Re: [PATCH v2 1/3] IMA: block writes of the security.ima xattr with weak hash algorithms

[Mimi Zohar <zohar@linux.ibm.com>]

Hi Simon,

On Mon, 2021-07-26 at 09:49 +0000, THOBY Simon wrote:

<snip>
 
> > A new builtin policy could be defined based on the new "appraise_hash"
> > option or simply a flag (e.g. ima_policy=).
> 
> I have started to take a look at what I might do in that regard. I think your
> idea to filter writes with the ima policy is definitely better than my secure
> boot "hack". However I still wonder the form this might take to be correct.
> 
> IMHO we cannot simply consider whether there is one rule in the policy that uses the
> 'appraise_hash' option, and apply that hash algorithm policy everywhere: we do not
> want to constrain files that rule doesn't impact.
> e.g. if a rule constrains every file owned by root to be valid only if the IMA
> signature was generated with sha256, another user shouldn't be constrained by that
> rule. Consider this policy:
> appraise func=MODULE_CHECK appraise_hash=sha256
> appraise func=BPRM_CHECK fowner=0
> 
> Here we do not want to constrain xattr writes to arbitrary files because we want
> more insurances on the the kernel modules.
> This would be a behavior hard to understand for users, and probably lead to
> unexpected system breakage if someone were to upgrade their ima policy and change the
> 'appraise_hash' value, because it would apply to files that the user didn't expect
> to be impacted.
> 
> For this reason, I believe there must be a way for the setxattr hook to determine if a
> file should be affected by the hash policy or not.
> 
> At first I thought about using 'ima_get_action' in the 'ima_inode_setxattr' hook
> to extract the rule that matches the file, verify if there is a list of allowed
> hash algorithms in that rule and apply the hash restriction to the xattr being
> written.
> But then I hit a significant setback: as I understand it, IMA cannot
> detect if a given rule apply to a file *outside* of trying to executing that rule.
> Let me explain what I mean with an example. Let us suppose we have the following
> ima policy:
> appraise func=BPRM_CHECK euid=0 appraise_hash=sha1,sha256 # (1)
> appraise func=FILE_MMAP fowner=0 mask=MAY_EXEC appraise_hash=sha256,sha512 # (3)
> appraise func=FILE_MMAP euid=0 mask=MAY_EXEC appraise_hash=sha384  # (3)
> 
> (I agree that such a diversity of hashes is quite implausible on a single system
> in practice, but I also think it best to try to think of degenerate usecases
> before implementing that feature, as users will tend to rely on them)
> 
> When a user try to update the ima hash (or ima signature) of a file, how can we
> know the hash algorithms that the user can use ? How do we know if the users uses 
> a rule or another, and thus the algorithm that should apply ?
> There is no one-to-one mapping between files and rules in IMA (I understand that is not
> at all the philosophy of IMA), so the answer is "We cannot".
> Worse, two rules could both apply to the same file (e.g. he could both mmap the dynamic
> loader and run it directly, so rules (1) and (2) would both apply.
> Except they do not use the same appraise_hash parameter!
> So the step "extract the rule that matches a file" is not possible, and I need to get
> back to the drawing board.
> 
> Technically, we could try every possible combination of mask/func to determine which
> would apply to the file whose xattr is being updated, but that would be absolutely
> terrible performance wise, and it would still have bad semantics:
> - either we would choose the first rule that match, and in that case the order of the
>  policy (and the order of our exhaustive search) would impact the resulting algorithms 
>  allowed;
> - or we could consider the intersection of hash algorithms allowed in each rule
>  (it might be null) or their union (it might be overly broad and we might choose
>  an algorithm not part of the intersection, thus the will will not be usable in
>  some situations).
> 
> In short, I believe both situations would be a nightmare, for user experience,
> performance, maintainability and probable the sanity of maintainers/code reviewers.

Agreed.

> 
> I think one possible way of getting out of this conundrum would be to extend the ima
> policy further by adding a new value for the 'func' policy option (something like
> WRITE_XATTR_HASH maybe ?). In that mode, the 'mask' option would have no effect, the
> appraise_hash parameter would be mandatory, and any file matching this policy would
> have the corresponding 'appraise_hash' policy enforced.
> This might give policy rules of the following sort:
> appraise func=BPRM_CHECK euid=0 appraise_hash=sha1,sha256
> appraise func=FILE_MMAP fowner=0 mask=MAY_EXEC appraise_hash=sha256,sha512
> appraise func=FILE_MMAP euid=0 mask=MAY_EXEC appraise_hash=sha384
> appraise func=WRITE_XATTR_HASH fowner=0 obj_type=bin_t appraise_hash=sha256
> 
> The first three rules would just impact executions/mmap()s, and the last one
> would restrict xattr writes.
> 
> I agree that would add quite a bit of complexity (and a performance hit to check
> if a IMA policy matches) to the setxattr hook, that I don't see yet another way out
> of this issue.
> 
> Please let me know what you think, I certainly would prefer it if someone came up
> with a much simpler option that I could then implement.

Implementing complete setxattr policy rules, including LSM labels,
would be the safest, but as you said, it would impact performance. 
Most systems could have a simpler rule to limit the hash algorithm(s).

For example,
appraise func=SETXATTR_CHECK appraise_hash=sha256
appraise func=SETXATTR_CHECK appraise_hash=sha256,sha384,sha512

Without a SETXATTR_CHECK rule, the default would be to limit it to the
configured crypto algorithms.

(The LSM hook is named security_inode_setxattr.)

thanks,

Mimi