tlsfuzzer / python-ecdsa

pure-python ECDSA signature/verification and ECDH key agreement
Other
915 stars 315 forks source link

OpenSSL signatures no longer compatible with ecdsa RAW format (README outdated) #67

Closed jrconlin closed 5 years ago

jrconlin commented 7 years ago

Hi,

While trying to convert from ecdsa to python cryptography (which wraps OpenSSL) I discovered that the current iteration of OpenSSL (libssl 1.0.2g+) returns a DER formatted signature value instead of a raw pair of 32octect numbers. (FWIW: the signature appears to be a Sequence of NamedTypes containing Integers)

If, say, a JWT that has a signature from a direct OpenSSL wrapper that is unaware of this is attempted to be run through ecdsa, it'll fail due to the signature length check*. Folks who wish to use this library should check signature length != 64 and perform whatever transmogrification required to get the raw pair of key values that ecdsa requires.

Also: might want to update the examples in the "OpenSSL Compatibility" section of the README to reflect this.

I'm going to try to follow up with other library owners in the chain to make sure that they're aware or at least comment about this problem lest others develop the same drinking habit I seem to have.

*really wish that some of these returned more meaningful errors than "AssertionError(71, 64)"

gsauthof commented 6 years ago

I can confirm this issue on Fedora 27. @jrconlin is right, with newer openssl versions, openssl writes the signature DER encoded. The following shows how to reproduce the issue and work around it:

OpenSSL part:

# create private key
openssl ecparam -name secp256k1 -genkey -noout -out secp256k1-key.pem
# create public key
openssl ec -in secp256k1-key.pem -pubout -noout -out secp256k1-pub-key.pem
# create signature of foo.txt
openssl dgst -sha256 -sign secp256k1-key.pem -out foo.text.sig foo.txt

How to make it work in Python ECDSA:

import ecdsa
import hashlib

key = ecdsa.VerifyingKey.from_pem(open('secp256k1-pub-key.pem').read())
msg = open('foo.txt', 'br').read()
sig = open('foo.txt.sig', 'rb').read()
if not sig[2*2]:
    s = sig[5:5+32]
else:
    s = sig[4:4+32]
r = sig[-32:]
sig = s + r
result = key.verify(sig, msg, hashlib.sha256)
print(result)

Note that the DER encoding adds 6 to 8 bytes overhead - depending on whether the highest bit of the most significant byte of each component is set or not. Since it's such a minimal DER file using a real DER-derserializer library would be overkill.

I agree, getting an error like AssertionError: (70, 64) instead of a proper exception that contains a message like 'signature size X does match the expected size Y' is a bit confusing.

Btw, the current README has a section on openssl compatibility, but the commands listed there don't work with current openssl versions.

tomato42 commented 6 years ago

the difference in expected sizes (70 vs 64) is likely from the fact that the signature is now ASN.1 encoded, if that's the case, it would make it a duplicate of #55

also, I agree that raising such unreadable AssertionError is bad idea – it should be python-ecdsa specific exception (that inherits from AssertionError for API compatibility reasons) and have a clear message

I'll definitely want to get it fixed, but unlikely it will happen in the next 2 months.

gsauthof commented 6 years ago

@tomato42 , yes this is what the OP and I wrote - DER is one of the ASN.1 encoding rules.

I wouldn't call this an exact duplicate, though. This issue also is about improving the error handling and the documentation (the README example).

Also, FWIW, this issue contains some self contained Python code for working around this issue.

tomato42 commented 5 years ago

Yes, the issue is a simple documentation problem; the exceptions raised got fixed with #115 (the code now will always raise BadSignatureError), the code already tests interoperability with never versions of openssl: https://github.com/warner/python-ecdsa/blob/bcf6afe422a2fb142433e5579908e9d662e28f5e/src/ecdsa/test_pyecdsa.py#L450-L458

https://github.com/warner/python-ecdsa/blob/bcf6afe422a2fb142433e5579908e9d662e28f5e/src/ecdsa/test_pyecdsa.py#L512-L523

(Note that it uses sigdecode=sigdecode_der and sigencode=sigencode_der)

but for new versions it uses different parameter than documented: https://github.com/warner/python-ecdsa/blob/bcf6afe422a2fb142433e5579908e9d662e28f5e/src/ecdsa/test_pyecdsa.py#L392-L400