Re: [PATCH 1/2] crypto: ecdh - Pass private key in proper byte order to check valid key

[Stefan Berger]

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@xxxxxxxxxx>
> > > > > Cc: Salvatore Benedetto <salvatore.benedetto@xxxxxxxxx>
> > > > > Signed-off-by: Stefan Berger <stefanb@xxxxxxxxxxxxx>
> > > > > ---
> > > > >    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, &params) < 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@xxxxxxxxxx>
> >
> > BR, Jarkko