Fix #849 - SHA256 for IMA allowlist v1

[maugustosilva]

A new parameter, ima_hash_alg is introduced under the [tenant]
section in keylime.conf. This parameter can be overriden by the CLI
option --allowlist_hash_alg on keylime_tenant. All this is, of
course, only applicable to "version 1" IMA allowlists.

In addition to that we fix the WARNING message on the agent. If the IMA
hash algorithm is other than SHA1, and the kernel is older 5.10,
communicate that there is potential problem.

[THS-on]

Why do you rename ima_hash_alg to ima_filedata_hash_alg and then don't use it anymore?
ima_hash_alg is there to explicitly control the template hash, so why hard code it to SHA1?

If we look at an IMA entry like 10 7936eb315fb4e74b99e7d461bc5c96049e1ee092 ima-ng sha1:bc026ae66d81713e4e852465e980784dc96651f8 /usr/lib/systemd/systemd
ima_hash_alg controls which hash algorithm 7936eb315fb4e74b99e7d461bc5c96049e1ee092 is and pcr_hash_alg what is extended into the TPM.
For sha1:bc026ae66d81713e4e852465e980784dc96651f8 we don't need to specify the algorithm because the current validator only compares the digest against the allowlist.

[maugustosilva]

One of the first things failing on the code when I made the change to be able to use algorithms other than sha1 was a failure in the comparison if self.ima_template_hash != self._ima_hash_alg.hash(self._bytes) (L368 in the current code).
The reason for this, as stated on the comment in L332 of the new proposed code, is that, even when the filedata-hash is anything other than sha1, the template-hash is still sha1, and supplying a different algorithm lead to an error (the error was also unprintable due to the way we attempted to "jsonify" it, but I fixed it too).

Other than that, you're right that, on this particular function, ima_filedata_hash_alg is not really used anymore, and could be removed.

[THS-on]

Currently if you change ima_hash_alg to anything other than SHA1 this won't work because the Kernel does indeed hardcode SHA1 for the template digest: https://elixir.bootlin.com/linux/latest/source/security/integrity/ima/ima_fs.c#L237
I've added the option log_hash_alg to set this manually, because this can change in newer Kernel versions.

The boot parameter ima_hash=sha256 modifies how IMA calculates the hashes of a file which has nothing to do with this code, because Keylime only validates the digests and not the hash algorithm of the entries in the allowlist.

[maugustosilva]

Ok, maybe I am missing something here. You seem to agree that, if ima_hash_alg is set to anything other than sha1 in the code as currently is today, it does indeed breaks. So, are we in agreement that it has to be hard-coded, for the time being, as sha1. Would say that simply removing the (admittedly, unused) ima_filedata_hash_alg as parameter is sufficient to make the PR acceptable?

[THS-on]

You seem to agree that, if ima_hash_alg is set to anything other than sha1 in the code as currently is today, it does indeed breaks. So, are we in agreement that it has to be hard-coded, for the time being, as sha1.

Yes, but then we can drop log_hash_alg also from the JSON IMA allowlist format.

Would say that simply removing the (admittedly, unused) ima_filedata_hash_alg as parameter is sufficient to make the PR acceptable?

Dropping that and also the newly introduced tenant option. I agree that it currently does not make sense for this to be configurable, if the kernel does not have the plans to change that in the foreseeable future.

We should probably also document this discussion and what ima_hash as boot parameter does somewhere in the documentation.

[maugustosilva]

Fair enough, I see now that you are correct: most of the code proposed here would remain unused for years. I will revert most of the changes, and instead will add a couple of comments in key points for anyone who happens to make the same mistake I did (i.e., started modifying the code while forgetting Keylime does not validate the hash algorithm of the entries )

[THS-on]

If you squash those three commits together the change LGTM.

What data with which setting is hashed is quite confusing and those comments add more clarity on what is going on.