Re: KPP questions and confusion

[Tudor Ambarus <tudor.ambarus@microchip.com>]

Hi, Marcel, Kyle,

On 07/17/2017 09:17 PM, Marcel Holtmann wrote:
> Hi Kyle,
> 
>> I am confused about several things in the new key agreement code.
>>
>> net/bluetooth/smp.c in two places generates random bytes for the
>> private_key argument to
>> net/bluetooth/ecdh_helper.c:generate_ecdh_keys, which suggests the
>> private key is static within the function. However, there is a do ...
>> while(true) loop within generate_ecdh_keys, with the following near
>> the end:
>>
>> /* Private key is not valid. Regenerate */
>> if (err == -EINVAL)
>>             continue;
>>
>> which suggests that it expects a different private key to be generated
>> on each iteration of the loop. But it looks like it runs through the
>> loop yet again with the same private key generated in the caller,
>> which suggests it would infinitely loop on a bad private key value. Is
>> this incorrect?
> 
> actually it seems that I screwed that up with commit 71653eb64bcca6110c42aadfd50b8d54d3a88079 where I moved the seeding of private_key outside of generate_ecdh_keys() function.
> 
>> Furthermore, since req->src == NULL via the call to
>> kpp_request_set_input, ecc_make_pub_key will always be called in
>> ecdh.c:ecdh_compute_value, in which case -EINVAL would be returned
>> only by invalid input (!private_key or bad curve_id) that AFAICT
>> cannot happen, or at least wouldn't be resolved by another run through
>> the loop.
>>
>> OTOH, -EAGAIN would be returned by ecc_make_pub_key if the public key
>> turns out to be the zero point, which is at least one reason why you'd
>> want to generate a new private key (if that loop were to do that.)
>>
>> I'm a little confused about some other things:
>>
>> * The bluetooth code doesn't seem to use ecc_gen_privkey, opting to
>> instead just generate random bytes and hope for the best.
>> * There doesn't appear to be any way for ecc_gen_privkey to get called
>> from crypto/ecdh.c:ecdh_set_secret, as it starts with a call to
>> crypto/ecdh_helper.c:crypto_ecdh_decode_key that returns -EINVAL if
>> decoding fails, meaning that both params.key != NULL and (if I'm
>> reading this correctly) params.key_size > 0. Is that dead code, or is
>> there a way it is intended to be used?
> 
> I am fine switching to ecc_gen_privkey. Care to provide a patch for it?
> 

Marcel, regarding the ecdh offload to hw from Bluetooth SMP code, this
is not possible as of know because SMP code uses it's own private keys.
atmel-ecc driver will fallback to the ecdh software implementation if
user wants to use it's own private keys.

I can jump in the Bluetooth's ecdh handling, but at best effort, my
primary goal is to add support for KPP in af_alg.

>> The context for this email is that I have need for the same basic
>> functionality in net/bluetooth/ecdh_helper.c for a non-BT purpose, so
>> it seems like this should be part of crypto/ecdh_helper.c (abstracted
>> to work for any curve). Basically, I need to do basic ECDH key
>> agreement:
>>
>> * Generate a new (valid) ephemeral private key, or potentially re-use
>> an existing one
>> * Compute the corresponding public key for the curve
>> * Compute the shared secret based on my private and peer's public
>>
>> Is KPP intended to be an abstract interface to all of the above, e.g.,
>> for both ECDH and FFDH? Right now it seems like layer violations are
>> required as there doesn't appear to be any way to (for example)
>> generate a fresh private key via kpp_*.
> 

Kyle, you can do all of the above for ecdh. Look at alg_test_kpp from
crypto/testmgr.c.

FFDH private key generation is not supported yet.

Cheers,
ta

> I think the whole goal is to abstract this. Mainly so that ECDH can also be offload to hardware.
> 
> Regards
> 
> Marcel
>