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

[THOBY Simon <Simon.THOBY@viveris.fr>]

Hello Mimi,

On 7/23/21 1:46 PM, Mimi Zohar wrote:
> Hi Simon,
> 
> On Tue, 2021-07-20 at 09:25 +0000, THOBY Simon wrote:
>> By default, writes of the extended attributes security.ima will be
>> forbidden if the xattr value uses a hash algorithm not compiled in the
>> kernel. Disabling weak hashes when building the kernel will thus prevent
>> their use in IMA (these hashes will not only be blocked for setxattr,
>> but they won't be allowed for measurement/appraisal either as the kernel
>> will obviously not be able to measure files hashed with them). Note
>> however that CONFIG_IMA depends on CONFIG_CRYPTO_MD5 and
>> CONFIG_CRYPTO_SHA1, so this limits the security benefits of this
>> measure.
>> To bypass that limitation, if secure boot is enabled on the system,
>> the allowed algorithms are further restricted: only writes performed
>> with the algorithm specified in the ima_hash parameter (defined at
>> build-time with CONFIG_IMA_DEFAULT_HASH or overwritten with a boot
>> cmdline option) are allowed.
> 
> Although the intention of this patch is to prevent writing file
> signatures based on weak hash algorithms, there are two logical
> changes.  Each should be a separate patch.  For example, one patch
> would only allow writing security.ima signatures based on configured
> hash algorithms, while the other patch would limit writing security.ima
> signatures based on the IMA default hash algorithm.
> 

Will do, this will be fixed in the next iteration of this patchset.

> Basing the decision on whether to limit the security.ima signature to
> the IMA default hash algorithm based on the secure boot flag, seems
> rather arbitrary.   Instead perhaps base it on whether the IMA policy
> contains any new policy rule "appraise_hash" options.  A policy without
> the new "appraise_hash" option would permit both writing and verifying
> signatures based on any configured hash algorithm.   A policy
> containing "appraise_hash", would both limit the hash algorithms
> writing the security.ima signatures and verifying them.
> 
> 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.

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.


[snip]

>> +
>> +	/**
> 
> "/**" is used to indicate the start of kernel-doc comments.  Please use
> the normal "/*"
> comment here.
> 

Noted, thanks.

[snip]>> +		path = dentry_path(dentry, pathbuf, PATH_MAX);
>> +
>> +	integrity_audit_msg(AUDIT_INTEGRITY_DATA, d_inode(dentry),
>> +		path, "collect_data", "forbidden-hash-algorithm", res, 0);
> 
> This function is writing security xattrs, not collecting/calculating
> the file hash.  Please update the audit message.  Instead of
> "forbidden", perhaps use something a little less dramatic, like
> "unsupported" or even "denied".
> 

Noted.

Thanks,
I hope my explanations weren't too confused,
Simon