[PATCH] crypto: expose needs_key in procfs

[PATCH] crypto: expose needs_key in procfs

[Christoph Böhmwalder]

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@linbit.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");
 }
 
 static const struct crypto_type crypto_shash_type = {
-- 
2.26.2

Re: [PATCH] crypto: expose needs_key in procfs

[Eric Biggers]

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@linbit.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).

Also, what about algorithms of type "ahash"?  Shouldn't this field be added for
them 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.

- Eric

Re: [PATCH] crypto: expose needs_key in procfs

[Christoph Böhmwalder]

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

Re: [PATCH] crypto: expose needs_key in procfs

[Eric Biggers]

On Mon, Mar 01, 2021 at 09:51:56PM +0100, Christoph Böhmwalder wrote:
> > 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

Shouldn't you know ahead of time which algorithm you are using (or set of
algorithms which you might use), and not be parsing /proc/crypto and choosing
some random one (which might be a non-cryptographic algorithm like CRC-32, or
something known to be insecure like MD5)?

Using the algorithm attributes in /proc/crypto only really makes sense if the
decision of which algorithm to use is punted to a higher level and the program
just needs to be able to pass through *any* algorithm available in Linux -- like
how 'cryptsetup' works.  But it's preferable to avoid that sort of design, as it
invites users to start depending on weird or insecure things.

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

BLAKE2 does need a key if it is being used as a keyed hash algorithm.  So it
depends on the user, not the algorithm per se.

- Eric