| Message ID | 20240418152445.2773042-2-stefanb@linux.ibm.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | crypto: ecdh & ecc: Fix private key byte ordering issues | expand |
On 4/18/24 11:24, 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. > Fixes: 3c4b23901a0c ("crypto: ecdh - Add ECDH software support") > Cc: Ard Biesheuvel <ardb@kernel.org> > Cc: Salvatore Benedetto <salvatore.benedetto@intel.com> > Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> > Acked-by: Jarkko Sakkinen <jarkko@kernel.org> > --- > crypto/ecdh.c | 11 ++++++++--- > 1 file changed, 8 insertions(+), 3 deletions(-) > > diff --git a/crypto/ecdh.c b/crypto/ecdh.c > index 3049f147e011..c02c9a2b9682 100644 > --- a/crypto/ecdh.c > +++ b/crypto/ecdh.c > @@ -27,7 +27,9 @@ 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; > + int ret = 0; > > if (crypto_ecdh_decode_key(buf, len, ¶ms) < 0 || > params.key_size > sizeof(u64) * ctx->ndigits) > @@ -40,13 +42,16 @@ 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; > + ret = -EINVAL; > } > - return 0; > + memzero_explicit(priv, sizeof(priv)); > + > + return ret; > } > > static int ecdh_compute_value(struct kpp_request *req)
On Thu, Apr 18, 2024 at 11:24:44AM -0400, 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> > Acked-by: Jarkko Sakkinen <jarkko@kernel.org> > --- > crypto/ecdh.c | 11 ++++++++--- > 1 file changed, 8 insertions(+), 3 deletions(-) > > diff --git a/crypto/ecdh.c b/crypto/ecdh.c > index 3049f147e011..c02c9a2b9682 100644 > --- a/crypto/ecdh.c > +++ b/crypto/ecdh.c > @@ -27,7 +27,9 @@ 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; > + int ret = 0; > > if (crypto_ecdh_decode_key(buf, len, ¶ms) < 0 || > params.key_size > sizeof(u64) * ctx->ndigits) > @@ -40,13 +42,16 @@ 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); These functions need to use our sparse marking mechanism correctly to prevent future occurences of such errors. They should not be passing around void * pointers or worse, treating u64 * arrays as big-endian. Cheers,
diff --git a/crypto/ecdh.c b/crypto/ecdh.c index 3049f147e011..c02c9a2b9682 100644 --- a/crypto/ecdh.c +++ b/crypto/ecdh.c @@ -27,7 +27,9 @@ 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; + int ret = 0; if (crypto_ecdh_decode_key(buf, len, ¶ms) < 0 || params.key_size > sizeof(u64) * ctx->ndigits) @@ -40,13 +42,16 @@ 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; + ret = -EINVAL; } - return 0; + memzero_explicit(priv, sizeof(priv)); + + return ret; } static int ecdh_compute_value(struct kpp_request *req)