Re: [PATCH v6 3/5] IMA: add support to restrict the hash algorithms used for file appraisal

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

Hi Mimi,

On 8/9/21 7:43 PM, Mimi Zohar wrote:
> On Wed, 2021-08-04 at 09:20 +0000, THOBY Simon wrote:
>> The kernel accepts any hash algorithm as a value for the security.ima
>> xattr. Users may wish to restrict the accepted algorithms to only
>> support strong cryptographic ones.
>>
>> Provide the plumbing to restrict the permitted set of hash algorithms
>> used for verifying file hashes and digest algorithms stored in
>> security.ima xattr.
> 
> simplify by saying "file hashes and signatures stored ..."

Will fix in the next iteration.

> 
>>
>> This do not apply only to IMA in hash mode, it also works with digital
>> signatures, where the hash from which the signature is derived (by
>> signing it with the trusted private key) must obey the same
>> restrictions.
> 
> The patch is limited to appraisal.  Is the above paragraph needed? 

Yes, what I was (badly) trying to say is that the appraisal check work both when
the security.ima xattr contains digital signatures and when it contains hashes, as
digital signatures can also be generated with different algorithms.
Except, as you pointed out, by talking about "hash mode" here, I introduced a confusion
where the reader can believe I'm talking of the "hash" IMA policy action. I will try
to clarify that.

> 
>>
>> Signed-off-by: Simon Thoby <simon.thoby@viveris.fr>
>> Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
> 
> This patch restricts the "hash algorithms".  Looking this over again
> after some time, does truncating variable names here, and in the other
> patches, to just "_hash|_hashes" make sense?  Perhaps the emphasis
> should not be on "hash", but on "algo".

I will update that (and patch 4/5 consequently). 

> 
>> @@ -684,8 +695,11 @@ int ima_match_policy(struct user_namespace *mnt_userns, struct inode *inode,
>>  			action &= ~IMA_HASH;
>>  			if (ima_fail_unverifiable_sigs)
>>  				action |= IMA_FAIL_UNVERIFIABLE_SIGS;
>> -		}
>>  
>> +			if (allowed_hashes &&
>> +			    entry->flags & IMA_VALIDATE_HASH)
>> +				*allowed_hashes = entry->allowed_hashes;
>> +		}
>>  
>>  		if (entry->action & IMA_DO_MASK)
>>  			actmask &= ~(entry->action | entry->action << 1);
> 
> 
> "allowed_hashes" sounds like a set of digests.  Instead of
> "allowed_hashes" and "IMA_VALIDATE_HASH", should it be "allowed_algo"
> and "IMA_ALLOWED_ALGO"?
> 
> thanks,
> 
> Mimi
> 

Thanks,
Simon