Hi Simon,

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

<snip>
 

quoted

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