| Message ID | 20240415003026.2661270-2-stefanb@linux.ibm.com |
|---|---|
| State | New |
| 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 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(-)