Received-SPF: pass (google.com: domain of linux-crypto+bounces-3592-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <1ec8e98f-6b65-49b4-8429-26d598fc305f@linux.ibm.com>
Date: Tue, 16 Apr 2024 22:31:44 -0400
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 1/2] crypto: ecdh - Pass private key in proper byte order
 to check valid key
Content-Language: en-US
To: Joachim Vandersmissen <git@jvdsn.com>
Cc: linux-kernel@vger.kernel.org, ardb@kernel.org,
        salvatore.benedetto@intel.com, davem@davemloft.net,
        Jarkko Sakkinen <jarkko@kernel.org>, linux-crypto@vger.kernel.org,
        herbert@gondor.apana.org.au
References: <20240415003026.2661270-1-stefanb@linux.ibm.com>
 <20240415003026.2661270-2-stefanb@linux.ibm.com>
 <D0KX9NQPXKO1.2RXZU000DD1BB@kernel.org>
 <6442f387-e45e-4019-8af0-6ca309e4ce4f@linux.ibm.com>
 <D0LM72MR4LDH.3QN2WEXU4QEEJ@kernel.org>
 <04f5265f-77e6-45cc-856c-45cd80c278c2@jvdsn.com>
From: Stefan Berger <stefanb@linux.ibm.com>
In-Reply-To: <04f5265f-77e6-45cc-856c-45cd80c278c2@jvdsn.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit


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, &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@kernel.org>
>>
>> BR, Jarkko
>>