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

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

Hi Simon,

On Thu, 2021-08-12 at 08:06 +0000, THOBY Simon wrote:
> On 8/11/21 6:26 PM, Mimi Zohar wrote:
> > 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.

Somehow I missed Lakshmi's comment.  Thank you for the really well
written and thought out explanation below.

> 
> 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.

Agreed, but having this discussion is important too.

> 
> 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?

Without knowing and understanding all the environments in which IMA is
enabled (e.g. front end, back end build system), you're correct -
protecting the user from themselves -might not be the right answer.

What you suggested, above, would be the correct solution.  Perhaps post
that change as a separate patch, on top of this patch set, for
additional discussion.

thanks,

Mimi