| Message ID | 20240415003026.2661270-2-stefanb@linux.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | crypto: ecdh & ecc: Fix private key byte ordering issues | expand |
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 <ardb@kernel.org> > Cc: Salvatore Benedetto <salvatore.benedetto@intel.com> > Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> > --- > 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 <ardb@kernel.org> >> Cc: Salvatore Benedetto <salvatore.benedetto@intel.com> >> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> >> --- >> 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 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 <ardb@kernel.org> > >> Cc: Salvatore Benedetto <salvatore.benedetto@intel.com> > >> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> > >> --- > >> 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 <jarkko@kernel.org> 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 <ardb@kernel.org> >>>> Cc: Salvatore Benedetto <salvatore.benedetto@intel.com> >>>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> >>>> --- >>>> 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 <jarkko@kernel.org> > > 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 <ardb@kernel.org> >>>>> Cc: Salvatore Benedetto <salvatore.benedetto@intel.com> >>>>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> >>>>> --- >>>>> 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 <jarkko@kernel.org> >> >> BR, Jarkko >>
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; }
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 <ardb@kernel.org> Cc: Salvatore Benedetto <salvatore.benedetto@intel.com> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> --- crypto/ecdh.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)