Re: [PATCH v3] crypto: add blake2b generic implementation

[David Sterba <dsterba@suse.cz>]

On Thu, Oct 10, 2019 at 03:12:02PM -0700, Eric Biggers wrote:
> > I'm going to do the selftests next so the above can't happen again.
> 
> The test vectors should be included in this patch.
> 
> > +
> > +	  - blake2b     - the default 512b digest
> > +	  - blake2b-160
> > +	  - blake2b-256
> > +	  - blake2b-384
> > +	  - blake2b-512
> > +
> 
> Why have the "blake2b" algorithm at all, when it's already available under the
> name "blake2b-512"?  It's confusing to have two different names for the same
> algorithm because then people will need to decide which one to use, and both
> will need to be tested.

The idea is that 'blake2b' is a convenience alias for the default digest
size, as it is commonly referred. But I agree it's can cause some
confusion, so I'll remove it.

> > +struct blake2b_param
> > +{
> > +	u8  digest_length; /* 1 */
> > +	u8  key_length;    /* 2 */
> > +	u8  fanout;        /* 3 */
> > +	u8  depth;         /* 4 */
> > +	u32 leaf_length;   /* 8 */
> > +	u32 node_offset;   /* 12 */
> > +	u32 xof_length;    /* 16 */
> 
> The u32 fields need to be __le32, since this struct is interpreted as an array
> of bytes.

For this and several other comments: I tried to minimize changes to the
the reference implementation, not to introduce bugs or remove code that
should be there, unless requested by a reviewer. I hope you understand
that and don't mind.

> > +static int blake2b_init(struct blake2b_state *S, size_t outlen)
> > +{
> > +	struct blake2b_param P[1];
> 
> This shouldn't be an array.

Copied from the original, will switch to simple variable.

> > +	if ((!outlen) || (outlen > BLAKE2B_OUTBYTES))
> > +		return -1;
> 
> No need for these checks, since this patch doesn't provide any way for the user
> to set an arbitrary outlen.  They should either be removed, or replaced with a
> WARN_ON().  As-is, it looks like a valid error, which is bad because some
> callers of the crypto_shash API don't handle errors.

I see and will remove it.

> > +	P->digest_length = (u8)outlen;
> > +	P->key_length    = 0;
> > +	P->fanout        = 1;
> > +	P->depth         = 1;
> > +	put_unaligned_le32(0, &P->leaf_length);
> > +	put_unaligned_le32(0, &P->node_offset);
> > +	put_unaligned_le32(0, &P->xof_length);
> 
> struct blake2b_param is already a packed structure, so these should be direct
> assignments.  No need for put_unaligned_le32().
> 
> > +	P->node_depth    = 0;
> > +	P->inner_length  = 0;
> > +	memset(P->reserved, 0, sizeof(P->reserved));
> > +	memset(P->salt,     0, sizeof(P->salt));
> > +	memset(P->personal, 0, sizeof(P->personal));
> > +	return blake2b_init_param(S, P);
> > +}
> > +
> > +static int blake2b_init_key(struct blake2b_state *S, size_t outlen,
> > +			 const void *key, size_t keylen)
> > +{
> > +	struct blake2b_param P[1];
> > +
> > +	if ((!outlen) || (outlen > BLAKE2B_OUTBYTES))
> > +		return -1;
> > +
> > +	if (!key || !keylen || keylen > BLAKE2B_KEYBYTES)
> > +		return -1;
> 
> More unclear error checks here.  Which are actually valid reachable errors, and
> which are assertions that should never trigger?  See comment above.
> 
> > +
> > +	P->digest_length = (u8)outlen;
> > +	P->key_length    = (u8)keylen;
> > +	P->fanout        = 1;
> > +	P->depth         = 1;
> > +	put_unaligned_le32(0, &P->leaf_length);
> > +	put_unaligned_le32(0, &P->node_offset);
> > +	put_unaligned_le32(0, &P->xof_length);
> 
> Same problem with the unnecessary put_unaligned_le32().
> 
> > +static int blake2b_final(struct blake2b_state *S, void *out, size_t outlen)
> > +{
> > +	u8 buffer[BLAKE2B_OUTBYTES] = {0};
> > +	size_t i;
> > +
> > +	if (out == NULL || outlen < S->outlen)
> > +		return -1;
> 
> More unnecessary error checks.  None of the other hash algorithms check for a
> NULL output buffer, and some users don't check for errors.  So returning -1
> instead of just crashing could hide bugs.

Yeah, removing the checks will allow to remove return values.

> > +	if (blake2b_is_lastblock(S))
> > +		return -1;
> 
> This can't be the case because lastblock is only set by final().
> 
> > +static int digest_setkey(struct crypto_shash *tfm, const u8 *key,
> > +			 unsigned int keylen)
> > +{
> > +	struct digest_tfm_ctx *mctx = crypto_shash_ctx(tfm);
> > +
> > +	if (keylen == 0 || keylen > BLAKE2B_KEYBYTES) {
> > +		crypto_shash_set_flags(tfm, CRYPTO_TFM_RES_BAD_KEY_LEN);
> > +		return -EINVAL;
> > +	}
> > +
> > +	memcpy(mctx->key, key, BLAKE2B_KEYBYTES);
> > +	mctx->keylen = keylen;
> > +
> > +	return 0;
> > +}
> 
> This reads past the end of the key buffer if keylen < BLAKE2B_KEYBYTES.
> 
> Please add tests and run with CONFIG_KASAN=y.

Right, I'll fix it. The selftests did not catch this because all of them
used the maximum key length.

> > +static int digest_update(struct shash_desc *desc, const u8 *data,
> > +			 unsigned int length)
> > +{
> > +	struct digest_desc_ctx *ctx = shash_desc_ctx(desc);
> > +	int ret;
> > +
> > +	ret = blake2b_update(ctx->S, data, length);
> > +	if (ret)
> > +		return -EINVAL;
> > +	return 0;
> > +}
> 
> Why does update() need to fail?  Not all shash API users check for errors.
> 
> > +
> > +static int digest_final(struct shash_desc *desc, u8 *out)
> > +{
> > +	struct digest_desc_ctx *ctx = shash_desc_ctx(desc);
> > +	const int digestsize = crypto_shash_digestsize(desc->tfm);
> > +	int ret;
> > +
> > +	ret = blake2b_final(ctx->S, out, digestsize);
> > +	if (ret)
> > +		return -EINVAL;
> > +	return 0;
> > +}
> 
> Likewise.  Why does final() need to fail?
> 
> > +
> > +static int digest_finup(struct shash_desc *desc, const u8 *data,
> > +			unsigned int len, u8 *out)
> > +{
> > +	struct digest_desc_ctx *ctx = shash_desc_ctx(desc);
> > +	const int digestsize = crypto_shash_digestsize(desc->tfm);
> > +	int ret;
> > +
> > +	ret = blake2b_update(ctx->S, data, len);
> > +	if (ret)
> > +		return -EINVAL;
> > +	ret = blake2b_final(ctx->S, out, digestsize);
> > +	if (ret)
> > +		return -EINVAL;
> > +	return 0;
> > +}
> 
> finup() shouldn't be implemented if it can't be made more efficient than
> update() and final() separately.

Ok, will be removed.

> > +static int blake2b_cra_init(struct crypto_tfm *tfm)
> > +{
> > +	struct digest_tfm_ctx *mctx = crypto_tfm_ctx(tfm);
> > +
> > +	/* Use the unkeyed version by default */
> > +	memset(mctx->key, 0, BLAKE2B_KEYBYTES);
> > +	mctx->keylen = 0;
> > +
> > +	return 0;
> > +}
> 
> No need for this function, since the tfm_ctx starts out zeroed by default.

Ok, will be removed.

> > +static struct shash_alg blake2b_algs[] = {
> > +	{
> > +		.digestsize		= BLAKE2B_512_DIGEST_SIZE,
> > +		.setkey			= digest_setkey,
> > +		.init			= digest_init,
> > +		.update			= digest_update,
> > +		.final			= digest_final,
> > +		.finup			= digest_finup,
> > +		.descsize		= sizeof(struct digest_desc_ctx),
> > +		.base.cra_name		= "blake2b",
> > +		.base.cra_driver_name	= "blake2b-generic",
> > +		.base.cra_priority	= 100,
> > +		.base.cra_flags		= CRYPTO_ALG_OPTIONAL_KEY,
> > +		.base.cra_blocksize	= BLAKE2B_BLOCKBYTES,
> > +		.base.cra_ctxsize	= 0,
> 
> Need to set cra_ctxsize to sizeof(struct digest_tfm_ctx), otherwise the code is
> using an area beyond the end of the buffer for the tfm_ctx.  This would have
> been caught if there were self tests and they were run with CONFIG_KASAN=y.

I found it out the hard way, the sefltests caused memory overwrite,
caught by SLUB_DEBUG, but I did the KASAN pass as well, no further
problems found.

I'll spin v4, adding the test vectors. It'll be probably a series
because single patch I have now is over 400K in size, so we'd need one
more feedback round to decide what test values to include.

Thanks for the comments.