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

[Stefan Berger <stefanb@linux.ibm.com>]

On 11/12/26 12:06 PM, Stefan Berger wrote:
> 
> 
> On 11/12/26 5:47 AM, Florian Bauckholt via B4 Relay wrote:
>> From: Florian Bauckholt <florian.bauckholt@cispa.de>
>>
>> The x962(ecdsa-nist-*) template decodes Ecdsa-Sig-Value with the
>> generic BER decoder, and ecdsa_get_signature_rs() only strips one
>> leading zero byte without looking at the INTEGER's sign or encoding.
>> As a result the following encodings of a valid signature are accepted
>> as well:
>>
>>   - a SEQUENCE tag without the constructed bit (0x10 instead of 0x30),
>>     as the decoder ignores that bit when matching a constructed tag
>>   - non-minimal length encodings of the SEQUENCE, r or s (long form
>>     where the short form fits, or padded with zero bytes)
>>   - indefinite length for the SEQUENCE
>>   - data following the SEQUENCE (the decoder does not check that the
>>     whole buffer was consumed)
>>   - r or s with the most significant bit set but without the leading
>>     zero byte, i.e. a negative INTEGER read as positive
>>   - r or s with a superfluous leading zero byte
>>
> 
> I have a user-space test suite here:
> https://github.com/stefanberger/eckey-testing
> 
> It tries to load openssl-generated self-signed certs into the kernel and 
> this fails now:
> 
> $ ./test-ecc-kernel-keys.sh
> Testing with curves:  prime256v1 secp384r1 secp521r1
> Testing with hashes:  sha224 sha256 sha384 sha512 sha3-256 sha3-384 
> sha3-512
> Good: curve: prime256v1 hash:   sha224 keyid: 927415098 
> keyctl_pkey_verify: Invalid argument
> 
> 
> 
>> RFC 3279 section 2.2.3 and SEC 1 section C.5 require Ecdsa-Sig-Value to
>> be DER-encoded, so all of these are invalid and should be rejected.
>>
>> This is a behaviour change for every user of the x962 template (X.509

I would say a 'potential but unlikely behaviour change' because if you 
signed with a tool (e.g., OpenSSL) that created properly (DER-)formatted 
signatures in X.509 certificates, then those signatures still verify 
with your patch applied. I would assume that most people signed with 
such a tool. In your patch you seem to add more tests for proper DER 
formatting that all existing x.509 certs and signatures should meet, right?

>> certificates, module and IMA signatures, KEYCTL_PKEY_VERIFY): signatures
>> that are valid ECDSA signatures but are not DER-encoded used to verify
>> and are now rejected with -EINVAL. The p1363 template is not affected.

What breaks the signature verification test in my test suite currently 
was pointed out by AI. For signature verifications using the 
KEYCTL_PKEY_VERIFY interface, it currently would pass along an input 
size of 72 bytes for a secp256r1 signature, which is the max. size, but 
this does not always correspond to the application provided signature 
size, which may for example be just 71 bytes. Your patch now detects 
that commit cb481e59ea6ca extended the input buffer by a byte that 
shouldn't be there. So this fixes it for me and is an error introduced 
for signature verification in that patch. Bbefore that patch it used to 
set params->out_len = uparams.out_len which is equivalent to 
params->in2_len = uparams->in2_len per that union used in that structure.

index 15b6bf6399b5..d27f8b22224a 100644
--- a/security/keys/keyctl_pkey.c
+++ b/security/keys/keyctl_pkey.c
@@ -160,7 +160,7 @@ static int keyctl_pkey_params_get_2(const struct 
keyctl_pkey_params __user *_par
                     uparams.in2_len > info.max_sig_size)
                         return -EINVAL;

-               params->out_len = info.max_sig_size;
+               params->in2_len = uparams.in2_len;
                 break;
         default:
                 return -EOPNOTSUPP;