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
