linux-kernel - Re: [PATCH v12 02/10] crypto: Add support for ECDSA signature verification

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <6eee0c55-40cd-4e7b-8819-1a4c9596062a@linux.ibm.com>
Date: Mon, 22 Jul 2024 08:19:41 -0400
From: Stefan Berger <stefanb@...ux.ibm.com>
To: Lukas Wunner <lukas@...ner.de>
Cc: keyrings@...r.kernel.org, linux-crypto@...r.kernel.org,
        davem@...emloft.net, herbert@...dor.apana.org.au, dhowells@...hat.com,
        zohar@...ux.ibm.com, jarkko@...nel.org,
        linux-integrity@...r.kernel.org, linux-security-module@...r.kernel.org,
        linux-kernel@...r.kernel.org, patrick@...terwijk.org
Subject: Re: [PATCH v12 02/10] crypto: Add support for ECDSA signature
 verification



On 7/17/24 12:17, Lukas Wunner wrote:
> Hi Stefan,
> 
> On Tue, Mar 16, 2021 at 05:07:32PM -0400, Stefan Berger wrote:
>> +/*
>> + * Get the r and s components of a signature from the X509 certificate.
>> + */
>> +static int ecdsa_get_signature_rs(u64 *dest, size_t hdrlen, unsigned char tag,
>> +				  const void *value, size_t vlen, unsigned int ndigits)
>> +{
>> +	size_t keylen = ndigits * sizeof(u64);
>> +	ssize_t diff = vlen - keylen;
>> +	const char *d = value;
>> +	u8 rs[ECC_MAX_BYTES];
>> +
>> +	if (!value || !vlen)
>> +		return -EINVAL;
>> +
>> +	/* diff = 0: 'value' has exacly the right size
>> +	 * diff > 0: 'value' has too many bytes; one leading zero is allowed that
>> +	 *           makes the value a positive integer; error on more
>> +	 * diff < 0: 'value' is missing leading zeros, which we add
>> +	 */
>> +	if (diff > 0) {
>> +		/* skip over leading zeros that make 'value' a positive int */
>> +		if (*d == 0) {
>> +			vlen -= 1;
>> +			diff--;
>> +			d++;
>> +		}
>> +		if (diff)
>> +			return -EINVAL;
>> +	}
>> +	if (-diff >= keylen)
>> +		return -EINVAL;
> 
> I'm in the process of creating a crypto_template for decoding an x962
> signature as requested by Herbert:
> 
> https://lore.kernel.org/all/ZoHXyGwRzVvYkcTP@gondor.apana.org.au/
> 
> I intend to move the above code to the template and to do so I'm
> trying to understand what it's doing.
> 
> There's an oddity in the above-quoted function.  The check ...
> 
> +	if (-diff >= keylen)
> +		return -EINVAL;
> 
> ... seems superfluous. diff is assigned the following value at the
> top of the function:
> 
> +	ssize_t diff = vlen - keylen;
> 
> This means that:  -diff == keylen - vlen.
> 
> Now, if vlen is zero, -diff would equal keylen and then the
> "-diff >= keylen" check would be true.  However at the top of
> the function, there's already a !vlen check.  No need to check
> the same thing again!

You're right, this check is not necessary.

    Stefan