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@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, ¶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@xxxxxxxxxx>
BR, Jarkko
