Re: Problem with new X.509 is_hash_blacklisted() interface

[Ard Biesheuvel]

On 21 June 2017 at 14:49, David Howells <dhowells@xxxxxxxxxx> wrote:
> Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx> wrote:
>
>> >      This can be told to skip a particular algorithm for when the caller
>> >      has one precalculated.  The precalculated hash can be passed to
>> >      is_hash_blacklisted().  This would typically be the case for a signed
>> >      X.509 message.
>>
>> This last part seems a premature optimization to me. Is there a
>> performance concern preventing us from using (4) only?
>
> Crypto stuff is relatively slow - and in the case of X.509 and PKCS#7 the
> caller will already have calculated a hash.  The most likely situation
> currently, I think, is that we will only have sha256 hashes in the blacklist,
> and whatever we're checking will have a sha256 hash also.
>
> Possibly, I could just pass the precalculated hash into is_data_blacklisted()
> and so avoid having to call is_hash_blacklisted() from outside.
>

That would be cleaner, yes. As I said, it looks correct to me, but I
would simply prefer to keep the code as simple as possible. If there
is a good reason for the additional fast path, then I have no
objections at all.

>> In any case, the approach and the code look sound to me, although I
>> think adding a hash of a type that we don't know how to calculate
>> deserves a warning at least.
>
> There are two issues with that:
>
>  (1) We don't know what hashes are available without checking to see what
>      modules are available.  However, to do this would involve loading the
>      hash algorithm module - but we might not be in a position to do this yet
>      (the blacklist is loaded before we start userspace).
>

Ah ok. That does complicate matters, indeed.

>  (2) A module implementing a hash algorithm might be blacklisted by the hash
>      that we've been given to add to the blacklist.  I think this is a more
>      general problem - and might require us to restrict blacklisting to hash
>      algorithms that are built in.
>

That makes sense, but deserves another big fat warning when it turns
out that the blacklist contains an entry we cannot verify against.