linux-kernel - Re: [PATCH] crypto: expose needs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <e82c30b0-e96f-d5cd-f7a3-d97f4e049b83@linbit.com>
Date:   Mon, 1 Mar 2021 21:51:56 +0100
From:   Christoph Böhmwalder 
        <christoph.boehmwalder@...bit.com>
To:     Eric Biggers <ebiggers@...nel.org>
Cc:     Herbert Xu <herbert@...dor.apana.org.au>,
        "David S. Miller" <davem@...emloft.net>,
        linux-crypto@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] crypto: expose needs_key in procfs

On 01.03.21 19:47, Eric Biggers wrote:
> On Mon, Mar 01, 2021 at 05:59:17PM +0100, Christoph Böhmwalder wrote:
>> Currently, it is not apparent for userspace users which hash algorithms
>> require a key and which don't. We have /proc/crypto, so add a field
>> with this information there.
>>
>> Signed-off-by: Christoph Böhmwalder <christoph.boehmwalder@...bit.com>
>>
>> ---
>>   crypto/shash.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/crypto/shash.c b/crypto/shash.c
>> index 2e3433ad9762..d3127a0618f2 100644
>> --- a/crypto/shash.c
>> +++ b/crypto/shash.c
>> @@ -477,6 +477,9 @@ static void crypto_shash_show(struct seq_file *m, struct crypto_alg *alg)
>>   	seq_printf(m, "type         : shash\n");
>>   	seq_printf(m, "blocksize    : %u\n", alg->cra_blocksize);
>>   	seq_printf(m, "digestsize   : %u\n", salg->digestsize);
>> +	seq_printf(m, "needs key    : %s\n",
>> +		   crypto_shash_alg_needs_key(salg) ?
>> +		   "yes" : "no");
>>   }
>>   
> 
> Do you have a specific use case in mind for this information?  Normally, users
> should already know which algorithm they want to use (or set of algorithms they
> might want to use).

I have a pretty specific use case in mind, yes. For DRBD, we use crypto 
algorithms for peer authentication and for the online-verify mechanism 
(to verify data integrity). The peer authentication algos require a 
shared secret (HMAC), while the verify algorithms are just hash 
functions without keys (we don't configure a shared secret here, so 
these must explicitly be "keyless").

Now, we also have a solution which sits on top of DRBD (LINSTOR), which 
resides purely in userspace. We recently implemented a feature where 
LINSTOR automatically chooses the "best" verify algorithm for all nodes 
in a cluster. It does this by parsing /proc/crypto and prioritizing 
accordingly. The problem is that /proc/crypto currently doesn't contain 
information about whether or not an algorithm requires a key – i.e. 
whether or not it is suitable for DRBD's online-verify mechanism.

See this commit for some context:
https://github.com/LINBIT/drbd/commit/34ee32e6922994c8e9390859e1790ca

> 
> Also, what about algorithms of type "ahash"?  Shouldn't this field be added for
> them too?

You're right. Since we only work with shash in DRBD, I blindly only 
considered this. I will add the field for ahash too.

> 
> Also, what about algorithms such as blake2b-256 which optionally take a key (as
> indicated by CRYPTO_ALG_OPTIONAL_KEY being set)?  So it's not really "yes" or
> "no"; there is a third state as well.

Correct me if I'm missing something, but crypto_shash_alg_needs_key reads:

static inline bool crypto_shash_alg_needs_key(struct shash_alg *alg)
{
	return crypto_shash_alg_has_setkey(alg) &&
		!(alg->base.cra_flags & CRYPTO_ALG_OPTIONAL_KEY);
}

So this already accounts for optional keys. It just returns "no" for an 
optional key, which seems like reasonable behavior to me (it doesn't 
*need* a key after all).

Another option would be to make it "yes/no/optional". I'm not sure if 
that's more desirable for most people.

> 
> - Eric
> 
Thanks,
--
Christoph Böhmwalder
LINBIT | Keeping the Digital World Running
DRBD HA —  Disaster Recovery — Software defined Storage