Message-ID: <20190114175315.GB7644@gmail.com>
Date:   Mon, 14 Jan 2019 09:53:16 -0800
From:   Eric Biggers <ebiggers@...nel.org>
To:     Stephan Müller <smueller@...onox.de>
Cc:     Herbert Xu <herbert@...dor.apana.org.au>,
        James Bottomley <James.Bottomley@...senpartnership.com>,
        Andy Lutomirski <luto@...capital.net>,
        "Lee, Chun-Yi" <joeyli.kernel@...il.com>,
        "Rafael J . Wysocki" <rjw@...ysocki.net>,
        Pavel Machek <pavel@....cz>, linux-kernel@...r.kernel.org,
        linux-pm@...r.kernel.org, keyrings@...r.kernel.org,
        "Rafael J. Wysocki" <rafael.j.wysocki@...el.com>,
        Chen Yu <yu.c.chen@...el.com>,
        Oliver Neukum <oneukum@...e.com>,
        Ryan Chen <yu.chen.surf@...il.com>,
        David Howells <dhowells@...hat.com>,
        Giovanni Gherdovich <ggherdovich@...e.cz>,
        Randy Dunlap <rdunlap@...radead.org>,
        Jann Horn <jannh@...gle.com>,
        Andy Lutomirski <luto@...nel.org>, linux-crypto@...r.kernel.org
Subject: Re: [PATCH 4/6] crypto: hkdf - RFC5869 Key Derivation Function

On Mon, Jan 14, 2019 at 10:30:39AM +0100, Stephan Müller wrote:
> Am Samstag, 12. Januar 2019, 06:12:54 CET schrieb Eric Biggers:
> 
> Hi Eric,
> 
> [...]
> 
> > > The extract and expand phases use different instances of the underlying
> > > keyed message digest cipher to ensure that while the extraction phase
> > > generates a new key for the expansion phase, the cipher for the
> > > expansion phase can still be used. This approach is intended to aid
> > > multi-threaded uses cases.
> > 
> > I think you partially misunderstood what I was asking for.  One HMAC tfm is
> > sufficient as long as HKDF-Expand is separated from HKDF-Extract, which
> > you've done.  So just use one HMAC tfm, and in crypto_hkdf_seed() key it
> > with the 'salt', and then afterwards with the 'prk'.
> 
> Ok, thanks for the clarification. I will remove the 2nd HMAC TFM then.
> > 
> > Also everywhere in this patchset, please avoid using the word "cipher" to
> > refer to algorithms that are not encryption/decryption.  I know a lot of
> > the crypto API docs do this, but I think it is a mistake and confusing. 
> > Hash algorithms and KDFs are not "ciphers".
> 
> As you wish, I will refer to specific name of the cryptographic operation.
> 
> [...]
> 
> > > + * NOTE: In-place cipher operations are not supported.
> > > + */
> > 
> > What does an "in-place cipher operation" mean in this context?  That the
> > 'info' buffer must not overlap the 'dst' buffer? 
> 
> Correct, no overlapping.
> 
> > Maybe
> > crypto_rng_generate() should check that for all crypto_rngs?  Or is it
> > different for different crypto_rngs?
> 
> This is the case in general for all KDFs (and even RNGs). It is no technical 
> or cryptographic error to have overlapping buffers. The only issue is that the 
> result will not match the expected value.
> 
> The issue is that the input buffer to the generate function is an input to 
> every round of the KDF. If the input and output buffer overlap, starting with 
> the 2nd iteration of the KDF, the input is the output of the 1st round. Again, 
> I do not think it is a cryptographic error though.
> 
> (To support my conclusion: A colleague of mine has proposed an update to the 
> HKDF specification where the input data changes for each KDF round. This 
> proposal was considered appropriate by one of the authors of HKDF.)
> 
> If the requested output is smaller or equal to the output block size of the 
> KDF, overlapping buffers are even harmless since the implementation will 
> calculate the correct output.
> 
> Due to that, I removed the statement. But I am not sure we should add a 
> technical block to deny overlapping input/output buffers.
> 
> [...]
> > > 
> > > +	desc->flags = crypto_shash_get_flags(expand_kmd) &
> > > +		      CRYPTO_TFM_REQ_MAY_SLEEP;
> > 
> > This line setting desc->flags doesn't make sense.  How is the user meant to
> > control whether crypto_rng_generate() can sleep or not?  Or can it always
> > sleep? Either way this part is wrong since the user can't get access to the
> > HMAC tfm to set this flag being checked for.
> 
> Could you please help me why a user should set this flag? Isn't the 
> implementation specifying that flag to allow identifying whether the 
> implementation could or could not sleep? Thus, we simply copy the sleeping 
> flag from the lower level keyed message digest implementation.
> 
> At least that is also the implementation found in crypto/hmac.c.
> 
> [...]

Whether the crypto_shash* stuff can sleep is controlled on a per-request basis,
not a per-implementation basis.  So I don't understand what you are talking
about here.

> 
> > > +		if (dlen < h) {
> > > +			u8 tmpbuffer[CRYPTO_HKDF_MAX_DIGESTSIZE];
> > > +
> > > +			err = crypto_shash_finup(desc, &ctr, 1, tmpbuffer);
> > > +			if (err)
> > > +				goto out;
> > > +			memcpy(dst, tmpbuffer, dlen);
> > > +			memzero_explicit(tmpbuffer, h);
> > > +			goto out;
> > > +		} else {
> > 
> > No need for the 'else'.
> 
> Could you please help me why that else branch is not needed? If the buffer to 
> be generated is equal or larger than the output block length of the keyed 
> message digest, I would like to directly operate on the output buffer to avoid 
> a memcpy.

I'm simply saying you don't need the 'else' keyword as the previous block ends
with a goto.

> > 
> > > +			err = crypto_shash_finup(desc, &ctr, 1, dst);
> > > +			if (err)
> > > +				goto out;
> > > +
> > > +			prev = dst;
> > > +			dst += h;
> > > +			dlen -= h;
> > > +			ctr++;
> > > +		}
> > > +	}
> 
> [...]
> > 
> > > +	struct crypto_shash *extract_kmd = ctx->extract_kmd;
> > > +	struct crypto_shash *expand_kmd = ctx->expand_kmd;
> > > +	struct rtattr *rta = (struct rtattr *)seed;
> > > +	SHASH_DESC_ON_STACK(desc, extract_kmd);
> > > +	u32 saltlen;
> > > +	unsigned int h = crypto_shash_digestsize(extract_kmd);
> > > +	int err;
> > > +	const uint8_t null_salt[CRYPTO_HKDF_MAX_DIGESTSIZE] = { 0 };
> > 
> > static const
> > 
> 
> Why would I want to turn that buffer into a static variable? All we need it 
> for is in case there is no salt provided.
> 
> [...]
> 
> > > +
> > > +	if (!RTA_OK(rta, slen))
> > > +		return -EINVAL;
> > > +	if (rta->rta_type != 1)
> > > +		return -EINVAL;
> > > +	if (RTA_PAYLOAD(rta) < sizeof(saltlen))
> > > +		return -EINVAL;
> > > +	saltlen = *((u32 *)RTA_DATA(rta));
> > 
> > I'm guessing you copied the weird "length as a rtattr payload" approach from
> > the authenc template.  I think it's not necessary.  And it's overly
> > error-prone, as shown by the authenc template getting the parsing wrong for
> > years and you making the exact same mistake again here...
> > (See https://patchwork.kernel.org/patch/10732803/)  How about just using a
> > u32 at the beginning without the 'rtattr' preceding it?
> 
> I was not sure whether this approach would be acceptable. I very much would 
> love to have a u32 pre-pended only without the RTA business.
> 
> I updated the implementation accordingly.
> > 
> [...]
> 
> > 
> > > +	alg = &salg->base;
> > 
> > Check here that the underlying algorithm really is "hmac(" something?
> 
> I added a check for the presence of salg->setkey.
> > 
> > Alternatively it may be a good idea to simplify usage by making the template
> > just take the unkeyed hash directly, like "hkdf(sha512)".  And if any users
> > really need to specify a specific HMAC implementation then another template
> > usable as "hkdf_base(hmac(sha512))" could be added later.
> > 
> 
> I would not suggest this, because that rounds contrary to the concept of the 
> kernel crypto API IMHO. The caller has to provide the wrapping cipher. It is 
> perfectly viable to allow a caller to invoke a specific keyed message digest.
> 

Sure, but it would not conform to the HKDF specification.  Are you sure it is
okay to specify an arbitrary keyed hash?

> [...]
> 
> Thank you very much for your code review.
> 
> Ciao
> Stephan
> 
> 

- Eric

Powered by blists - more mailing lists