AntonKueltz / fastecdsa

Python library for fast elliptic curve crypto
https://pypi.python.org/pypi/fastecdsa
The Unlicense
264 stars 78 forks source link

Key Error when decoding DER signatures on P521 curve #61

Closed SeanVisser1998 closed 3 years ago

SeanVisser1998 commented 3 years ago

Hello,

There is a key error when decoding the encoded signature using the P521-curve.

'''
    Generating Keys
'''

from fastecdsa import keys, curve, ecdsa

priv_key,pub_key = keys.gen_keypair(curve.P521)

'''
    Signing and verifyng
'''
m = "Hello World"

r,s = ecdsa.sign(m,priv_key,curve=curve.P521)
valid = ecdsa.verify((r,s),m,pub_key,curve=curve.P521)

'''
    Encoding signatures
'''

from fastecdsa.encoding.der import DEREncoder

encoded = DEREncoder.encode_signature(r,s)
decoded_r, decoded_s = DEREncoder.decode_signature(encoded)

print(decoded_r)
AntonKueltz commented 3 years ago

Could you provide the error output associated with the code snippet above? Thanks.

SeanVisser1998 commented 3 years ago

Could you provide the error output associated with the code snippet above? Thanks.

`--------------------------------------------------------------------------- KeyError Traceback (most recent call last)

in () 22 23 encoded = DEREncoder.encode_signature(r,s) ---> 24 decoded_r, decoded_s = DEREncoder.decode_signature(encoded) 25 26 print(decoded_r) /home//.local/lib/python3.6/site-packages/fastecdsa/encoding/der.py in decode_signature(sig) 55 56 try: ---> 57 seqlen, sequence, leftover = parse_asn1_length(sig[1:]) 58 except ASN1EncodingError as asn1_error: 59 raise InvalidDerSignature(asn1_error) /home//.local/lib/python3.6/site-packages/fastecdsa/encoding/asn1.py in parse_asn1_length(data) 96 fmt = {1: '=B', 2: '=H', 4: '=L'} 97 ---> 98 (length,) = unpack(fmt[count], data[:count]) 99 data = data[count:] 100 KeyError: 7`
pcece commented 3 years ago

Hi all,

I struggled a little bit also on this point, but discovered that the problem is on the DEREncoding structure. As far as I know this coding is false. For instance, I got the following r,s values : r = 0152a036cd84ccf8caada25deb6bad215b33908bba22cf9aa335257b55ab72004061303ea0a8681a3d8545f25fbc45ec66d7364585ac55a6b1ada1ff38215249deb1 s = 01883a2eeb0948c1d972704721b884c613aa334b050110f2bfe92130c8ff8d246c36879ca7af86a650bc53b3ca448b6464fe3eaf1c79db9744bdefc26639ad30f87e

And received the following DerEncodigValues = 308802420152a036cd84ccf8caada25deb6bad215b33908bba22cf9aa335257b55ab72004061303ea0a8681a3d8545f25fbc45ec66d7364585ac55a6b1ada1ff38215249deb1024201883a2eeb0948c1d972704721b884c613aa334b050110f2bfe92130c8ff8d246c36879ca7af86a650bc53b3ca448b6464fe3eaf1c79db9744bdefc26639ad30f87e

But the length is false. It shouldn't be 88 but 81.

You could use this to correct :

`from asn1crypto import csr, core class eccSignatureClass(core.Sequence): _fields = [ ('r', core.Integer), ('s', core.Integer) ]

signature = eccSignatureClass()
signature['r'] = r
signature['s'] = s

byteValues = signature.dump()

And to check eccSignature = eccSignatureClass().load(byteValues)

`

I hope it will help

pcece commented 3 years ago

I forgot to stress that I have tested the signature computation ECDSA NIST-P521 SHA2-512 with FIPS-186-4 test vectors and everything was OK. So thank you very much for this great library and this great work.

AntonKueltz commented 3 years ago

Just getting to this now, been a busy year 😓 . Breaking down the example above I see -

30 88 (SEQUENCE LEN 0x88) 
    02 42 (INTEGER LEN 0x42)
        0152a036cd84ccf8 caada25deb6bad21 5b33908bba22cf9a a335257b55ab7200 
        4061303ea0a8681a 3d8545f25fbc45ec 66d7364585ac55a6 b1ada1ff38215249
        deb1 (INTEGER R)
    02 42 (INTEGER LEN 0x42)
        01883a2eeb0948c1 d972704721b884c6 13aa334b050110f2 bfe92130c8ff8d24
        6c36879ca7af86a6 50bc53b3ca448b64 64fe3eaf1c79db97 44bdefc26639ad30
        f87e (INTEGER S)

That looks right to me, the sequence is 0x88=136 octets long, 0x42=66 octets per integer, plus 2 octets to encode the type and length per integer, so 2 * (68) = 136 octets.

Edit: Ah right, ASN.1/DER has the edge case of of anything > 0x80 needing a multi byte encoding, so this should be a len encoding of 0x8188. The ASN.1 length encoding was already correct in the asn1 module, but unfortunately the DER code wasn't using it. 🙁

AntonKueltz commented 3 years ago

Fixed in 7fd41568eacc745811801396157b5672eb5789a4, will be in release v2.1.1.

   ...: '''
   ...:     Signing and verifyng
   ...: '''
   ...: m = "Hello World"
   ...:
   ...: r,s = ecdsa.sign(m,priv_key,curve=curve.P521)
   ...: valid = ecdsa.verify((r,s),m,pub_key,curve=curve.P521)
   ...:
   ...: '''
   ...:     Encoding signatures
   ...: '''
   ...:
   ...: from fastecdsa.encoding.der import DEREncoder
   ...:
   ...: encoded = DEREncoder.encode_signature(r,s)
   ...: decoded_r, decoded_s = DEREncoder.decode_signature(encoded)
   ...:
   ...: print(r == decoded_r, s == decoded_s)
True True