Re: [PATCH v7 4/5] IMA: add a policy option to restrict xattr hash algorithms on appraisal

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

Hi Mimi,


On 8/11/21 6:26 PM, Mimi Zohar wrote:
> Hi Simon,
> 
> On Wed, 2021-08-11 at 11:40 +0000, THOBY Simon wrote:
>> +static unsigned int ima_parse_appraise_algos(char *arg)
>> +{
>> +	unsigned int res = 0;
>> +	int idx;
>> +	char *token;
>> +
>> +	while ((token = strsep(&arg, ",")) != NULL) {
>> +		idx = match_string(hash_algo_name, HASH_ALGO__LAST, token);
>> +
>> +		if (idx < 0) {
>> +			pr_err("unknown hash algorithm \"%s\"",
>> +			       token);
>> +			return 0;
> 
> Previous versions of this patch ignored unknown algorithms.  If not all
> of the algorithms are defined in an older kernel, should loading the
> policy fail?   As new IMA policy features are defined, older kernels
> prevent loading newer policies with unknown features.   I hesitated to
> equate the two scenarios.

Yes, that choice isn't easy. I changed the behavior in response to Lakshmi's
remark on v6. It's true that failing to load the policy on an old kernel because
of an unknown algorithm is not very desirable, but loading a partial policy may
be worse.

As an exampe, if we load the policy rule
	appraise func=BPRM_CHECK fowner=0 appraise_algos=sha256,<new_algo>
in a version of the kernel that doesn't know about <new_algo>, a permissive interpretation
would yield
	appraise func=BPRM_CHECK fowner=0 appraise_algos=sha256.
Now if the the system files were already hashes with <new_algo>, then the user is in
for a world of pain as the system can't boot (trying to appraise every executable root
owns - that is, pretty much all of them - will fail).
Admittedly, if the kernel doesn't know about <new_algo>, there is little chances the
filesystem was protected with that algorithm, unless the user did migrate to <new_algo>
at some point and is now trying to rollback to an older kernel for some reason.
So I think that remains a very niche issue.


On the other hand, rejecting the policy protects against typos and human errors
(while trying this patch, I once wrote 'appraise [...] appraise_hashes=sha256,sha384;sha512'
which was accepted and silently updated to 'appraise [...] appraise_hashes=sha256',
and I didn't understood my error until trying to launch a command and being greeted with the
infamous "Permission denied". Had I been monitoring the kernel log, I would have seen the error
right away, but as the policy was accepted I assumed it did what I expected and didn't thought
any more of it until seeing failures).
It is also more consistent, as I think it is a desirable feature to know when writing a policy
to the kernel that reading it back will return the exact same policy, modulo the order of the
fields in the policy rules. Ignoring unknown algorithms breaks that behavior.


Additionally, I don't think digest algorithms are added very frequently to the
kernel (or else the list would be much longer), which mitigate the potential impact
of either solution.


After some thought, I am personally inclined to prefer strict checking, as I think failing fast
and early can save great ordeals later. Of course it's not always easy in the case of the kernel,
where failure is not an option except in some rare catastrophic situations. But as rejecting
invalid algorithms is coherent with the IMA behavior with regard to new features, and rejecting
a policy cannot break a system (although it definitely reduces the trust you can have in the
integrity of the system), I think that's an acceptable behavior.

> 
>> +		}
>> +
>> +		/* Add the hash algorithm to the 'allowed' bitfield */
>> +		res |= (1U << idx);
> 
> This assumes that all the hash algorithms are enabled in the kernel,
> but nothing checks that they are.  In validate_hash_algo(), either the
> allowed_hashes is checked or the hash algorithm must be configured.  Do
> we really want a total separation like this?

I kind of assumed that if the user allowlist some algorithms with 'appraise_algos', he is basically
saying "I know what I'm doing, trust these values", and thus these values take precedence on
the algorithms compiled in the kernel.

In addition, validate_hash_algo() is only ever used for setxattr, not for appraisal
(which is the subject of this specific patch). If a user try to run a file appraised
with an algorithm not present in the kernel, ima_collect_measurement() would fail
before we even checked that the algorithm is in the allowlist (process_measurement()->
ima_collect_measurement()->ima_calc_file_hash()->ima_calc_file_ahash()->ima_alloc_atfm(<algo>)
would fail and log an error message like "Can not allocate <algo> (reason: <result>)").
So that check is already done "for free" on appraisal.

However your comment does applies to the next patch ("IMA: introduce a new policy
option func=SETXATTR_CHECK"), and we probably could restrict the algorithms referenced in
ima_setxattr_allowed_hash_algorithms to ones the current kernel can use. 
The easiest way to enforce this would probably be to check that when parsing 'appraise_algos'
in ima_parse_appraise_algos(), we only add algorithms that are available, ignoring - or
rejecting, according to the outcome of the discussion above - the other algorithms. That way,
we do not have to pay the price of allocating a hash object every time validate_hash_algo() is
called.

Would it be ok if I did that?

> 
> thanks,
> 
> Mimi
> 
>> +	}
>> +
>> +	return res;
>> +}
>> +
> 

Thanks,
Simon