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

[Lukas Wunner <lukas@wunner.de>]

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!

Next, I'm asking myself if -diff can ever be greater than keylen.
I don't think it can.  For that to be true, vlen would have to be
negative.  But vlen is of unsigned type size_t!

I just wanted to double-check with you whether ...

+	if (-diff >= keylen)
+		return -EINVAL;

... is indeed superfluous as I suspect or whether I'm missing
something.  I'm guessing that the check might be some kind of
safety net to avoid an out-of-bounds access in the memset()
and memcpy() calls that follow further down in the function,
in case sig->curve->g.ndigits was neglected to be set by the
programmer.  But there doesn't seem to be any real need for
the check, so I'm leaning towards not carrying it over to the
x962 template.

The check was already present in v1 of your ecdsa patch set:

https://lore.kernel.org/all/20210126170359.363969-2-stefanb@linux.vnet.ibm.com/

Thanks,

Lukas