On Tue Apr 16, 2024 at 3:51 AM EEST, Stefan Berger wrote:
>
>
> On 4/15/24 14:53, Jarkko Sakkinen wrote:
> > On Mon Apr 15, 2024 at 3:30 AM EEST, Stefan Berger wrote:
> >> ecc_is_key_valid expects a key with the most significant digit in the last
> >> entry of the digit array. Currently ecdh_set_secret passes a reversed key
> >> to ecc_is_key_valid that then passes the rather simple test checking
> >> whether the private key is in range [2, n-3]. For all current ecdh-
> >> supported curves (NIST P192/256/384) the 'n' parameter is a rather large
> >> number, therefore easily passing this test.
> >>
> >> Throughout the ecdh and ecc codebase the variable 'priv' is used for a
> >> private_key holding the bytes in proper byte order. Therefore, introduce
> >> priv in ecdh_set_secret and copy the bytes from ctx->private_key into
> >> priv in proper byte order by using ecc_swap_digits. Pass priv to
> >> ecc_is_valid_key.
> >>
> >> Cc: Ard Biesheuvel <[email protected]>
> >> Cc: Salvatore Benedetto <[email protected]>
> >> Signed-off-by: Stefan Berger <[email protected]>
> >> ---
> >> crypto/ecdh.c | 4 +++-
> >> 1 file changed, 3 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/crypto/ecdh.c b/crypto/ecdh.c
> >> index 3049f147e011..a73853bd44de 100644
> >> --- a/crypto/ecdh.c
> >> +++ b/crypto/ecdh.c
> >> @@ -27,6 +27,7 @@ static int ecdh_set_secret(struct crypto_kpp *tfm, const void *buf,
> >> unsigned int len)
> >> {
> >> struct ecdh_ctx *ctx = ecdh_get_ctx(tfm);
> >> + u64 priv[ECC_MAX_DIGITS];
> >> struct ecdh params;
> >>
> >> if (crypto_ecdh_decode_key(buf, len, ¶ms) < 0 ||
> >> @@ -40,9 +41,10 @@ static int ecdh_set_secret(struct crypto_kpp *tfm, const void *buf,
> >> ctx->private_key);
> >>
> >> memcpy(ctx->private_key, params.key, params.key_size);
> >> + ecc_swap_digits(ctx->private_key, priv, ctx->ndigits);
> >
> > Does swapping speed up the test that follows are what effect does it
> > have to the ecc_is_key_valid() call?
> The goal of this particular patch is to fix an issue with the byte order
> (as description says) and, as you can see in the 2nd patch, private key
> is always copied into priv using ecc_swap_digits before priv is being
> used instead of ctx->private_key (or whatever it is called in the
> function it was passed to). This patch here has nothing to do with speed
> up but a) fixing an issue and b) using priv here as well, so fixing this
> 'outlier' here. The speed-up comes in the 2nd patch when the bytes in
> ctx->private_key are put into proper order right away and we can get rid
> if priv, taking the swapped bytes of ctx->private_key, everywhere and we
> can use ctx->private_key directly.
>
> The test harness (testmgr.c) runs through part of this code here
> providing the private key that is copied into ctx->private_key, so it's
> being used and when you make a mistake (making the changes I did) the
> ecdh test cases will fail.
OK, thanks for the explanation :-) No opposition on the change itself.
Acked-by: Jarkko Sakkinen <[email protected]>
BR, Jarkko
Hi,
Apologies for hijacking this thread, but I don't have access to the
older emails.
Should the new priv variable not be zeroized before the end of the
function? As it contains private keying material?
Kind regards,
Joachim
On 4/16/24 9:25 AM, Jarkko Sakkinen wrote:
> On Tue Apr 16, 2024 at 3:51 AM EEST, Stefan Berger wrote:
>>
>> On 4/15/24 14:53, Jarkko Sakkinen wrote:
>>> On Mon Apr 15, 2024 at 3:30 AM EEST, Stefan Berger wrote:
>>>> ecc_is_key_valid expects a key with the most significant digit in the last
>>>> entry of the digit array. Currently ecdh_set_secret passes a reversed key
>>>> to ecc_is_key_valid that then passes the rather simple test checking
>>>> whether the private key is in range [2, n-3]. For all current ecdh-
>>>> supported curves (NIST P192/256/384) the 'n' parameter is a rather large
>>>> number, therefore easily passing this test.
>>>>
>>>> Throughout the ecdh and ecc codebase the variable 'priv' is used for a
>>>> private_key holding the bytes in proper byte order. Therefore, introduce
>>>> priv in ecdh_set_secret and copy the bytes from ctx->private_key into
>>>> priv in proper byte order by using ecc_swap_digits. Pass priv to
>>>> ecc_is_valid_key.
>>>>
>>>> Cc: Ard Biesheuvel <[email protected]>
>>>> Cc: Salvatore Benedetto <[email protected]>
>>>> Signed-off-by: Stefan Berger <[email protected]>
>>>> ---
>>>> crypto/ecdh.c | 4 +++-
>>>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/crypto/ecdh.c b/crypto/ecdh.c
>>>> index 3049f147e011..a73853bd44de 100644
>>>> --- a/crypto/ecdh.c
>>>> +++ b/crypto/ecdh.c
>>>> @@ -27,6 +27,7 @@ static int ecdh_set_secret(struct crypto_kpp *tfm, const void *buf,
>>>> unsigned int len)
>>>> {
>>>> struct ecdh_ctx *ctx = ecdh_get_ctx(tfm);
>>>> + u64 priv[ECC_MAX_DIGITS];
>>>> struct ecdh params;
>>>>
>>>> if (crypto_ecdh_decode_key(buf, len, ¶ms) < 0 ||
>>>> @@ -40,9 +41,10 @@ static int ecdh_set_secret(struct crypto_kpp *tfm, const void *buf,
>>>> ctx->private_key);
>>>>
>>>> memcpy(ctx->private_key, params.key, params.key_size);
>>>> + ecc_swap_digits(ctx->private_key, priv, ctx->ndigits);
>>> Does swapping speed up the test that follows are what effect does it
>>> have to the ecc_is_key_valid() call?
>> The goal of this particular patch is to fix an issue with the byte order
>> (as description says) and, as you can see in the 2nd patch, private key
>> is always copied into priv using ecc_swap_digits before priv is being
>> used instead of ctx->private_key (or whatever it is called in the
>> function it was passed to). This patch here has nothing to do with speed
>> up but a) fixing an issue and b) using priv here as well, so fixing this
>> 'outlier' here. The speed-up comes in the 2nd patch when the bytes in
>> ctx->private_key are put into proper order right away and we can get rid
>> if priv, taking the swapped bytes of ctx->private_key, everywhere and we
>> can use ctx->private_key directly.
>>
>> The test harness (testmgr.c) runs through part of this code here
>> providing the private key that is copied into ctx->private_key, so it's
>> being used and when you make a mistake (making the changes I did) the
>> ecdh test cases will fail.
> OK, thanks for the explanation :-) No opposition on the change itself.
>
> Acked-by: Jarkko Sakkinen <[email protected]>
>
> BR, Jarkko
>