The 1st patch fixes a byte ordering issue where ctx->private_key is
currently passed to ecc_is_key_valid but the key is in reverse byte order.
To solve this issue it introduces the variable 'priv' that is already used
throughout the ecc and ecdh code bases for a private key in proper byte
order and calls the function with 'priv'.
The 2nd patch gets rid of the 'priv' variable wherever it is used to hold
a private key (byte-swapped initialized from ctx->private_key) in proper
byte order and uses ctx->private_key directly that is now initialized in
proper byte order.
Regards,
Stefan
Stefan Berger (2):
crypto: ecdh - Pass private key in proper byte order to check valid
key
crypto: ecdh & ecc - Initialize ctx->private_key in proper byte order
crypto/ecc.c | 29 ++++++++++-------------------
crypto/ecdh.c | 3 ++-
include/crypto/internal/ecc.h | 3 ++-
3 files changed, 14 insertions(+), 21 deletions(-)
--
2.43.0
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);
if (ecc_is_key_valid(ctx->curve_id, ctx->ndigits,
- ctx->private_key, params.key_size) < 0) {
+ priv, params.key_size) < 0) {
memzero_explicit(ctx->private_key, params.key_size);
return -EINVAL;
}
--
2.43.0
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?
Just a question to understand what is going on, not actual review
feedback.
>
> if (ecc_is_key_valid(ctx->curve_id, ctx->ndigits,
> - ctx->private_key, params.key_size) < 0) {
> + priv, params.key_size) < 0) {
> memzero_explicit(ctx->private_key, params.key_size);
> return -EINVAL;
> }
BR, Jarkko
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.
>
> Just a question to understand what is going on, not actual review
> feedback.
>
>>
>> if (ecc_is_key_valid(ctx->curve_id, ctx->ndigits,
>> - ctx->private_key, params.key_size) < 0) {
>> + priv, params.key_size) < 0) {
>> memzero_explicit(ctx->private_key, params.key_size);
>> return -EINVAL;
>> }
>
> BR, Jarkko
On 4/16/24 22:12, Joachim Vandersmissen wrote:
> 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?
Yes, fixed in v2.
Stefan
>
> 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
>>