Re: [PATCH] crypto: ecdsa - Reject non-DER X9.62 signatures

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] crypto: ecdsa - keyctl_pkey_params_get_2() breaks
  ecdsa_x962_verify()
--

commit 507f70b0a1dc8642ced214dd9427e506b434b4b9
Author: Florian Bauckholt <florian.bauckholt@cispa.de>

crypto: ecdsa - Reject non-DER X9.62 signatures

This commit enforces strict DER encoding for X9.62 ECDSA signatures by
validating the INTEGER and SEQUENCE headers and rejecting trailing bytes,
along with adding Wycheproof test vectors for invalid BER encodings.

> diff --git a/crypto/ecdsa-x962.c b/crypto/ecdsa-x962.c
> index ee71594d10a06..69e932bbd5511 100644
> --- a/crypto/ecdsa-x962.c
> +++ b/crypto/ecdsa-x962.c
[ ... ]
> @@ -73,6 +88,24 @@ int ecdsa_get_signature_s(void *context, size_t hdrlen, unsigned char tag,
>  				      sig_ctx->ndigits);
>  }
>  
> +/*
> + * The BER decoder accepts a SEQUENCE tag without the constructed bit,
> + * long-form and indefinite lengths, and ignores data following the SEQUENCE.
> + * DER permits only the constructed tag, the shortest length encoding and
> + * nothing after the SEQUENCE.  The decoder has already checked that r and s
> + * fill a definite-length SEQUENCE exactly.
> + */
> +static bool ecdsa_x962_seq_is_der(const u8 *src, unsigned int slen)
> +{
> +	if (src[0] != (ASN1_CONS_BIT | ASN1_SEQ))
> +		return false;
> +
> +	if (src[1] < 0x80)
> +		return slen == 2 + src[1];
> +
> +	return src[1] == 0x81 && src[2] >= 0x80 && slen == 3 + src[2];
> +}
> +
>  static int ecdsa_x962_verify(struct crypto_sig *tfm,
>  			     const void *src, unsigned int slen,
>  			     const void *digest, unsigned int dlen)
> @@ -88,6 +121,9 @@ static int ecdsa_x962_verify(struct crypto_sig *tfm,
>  	if (err < 0)
>  		return err;
>  
> +	if (!ecdsa_x962_seq_is_der(src, slen))
> +		return -EINVAL;

[Severity: High]
Does requiring slen to match the exact DER SEQUENCE length break
keyctl(KEYCTL_PKEY_VERIFY) for valid X9.62 signatures in this tree?

In struct kernel_pkey_params, out_len and in2_len share an anonymous union.
Since commit cb481e59ea6c ("KEYS: fix overflow in
keyctl_pkey_params_get_2()"), keyctl_pkey_params_get_2() overwrites
params->in2_len with info.max_sig_size:

security/keys/keyctl_pkey.c:keyctl_pkey_params_get_2() {
    ...
	case KEYCTL_PKEY_VERIFY:
		if (uparams.in_len  > info.max_data_size ||
		    uparams.in2_len > info.max_sig_size)
			return -EINVAL;

		params->out_len = info.max_sig_size;
		break;
    ...
}

keyctl_pkey_verify() -> asymmetric_key_verify_signature() then passes
info.max_sig_size as slen to ecdsa_x962_verify().  As noted in the patch
commentary, until the separate keyctl fix is applied in this tree, any
valid DER-encoded X9.62 signature shorter than max_sig_size (for example, a
70- or 71-byte NIST P-256 signature where max_sig_size is 72) fails the
slen check in ecdsa_x962_seq_is_der() with -EINVAL.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112-ecdsa-x962-der-v1-1-36b24c5f5e11@cispa.de?part=1