Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
jrconlin opened this issue Mar 27, 2017 · 4 comments · Fixed by #140
Closed

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

jrconlin opened this issue Mar 27, 2017 · 4 comments · Fixed by #140
Labels
bug unintended behaviour in ecdsa code help wanted
Milestone

Comments

@jrconlin
Copy link

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
Copy link

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 tomato42 added help wanted bug unintended behaviour in ecdsa code labels Sep 5, 2018
@tomato42 tomato42 added this to the v0.14 milestone Sep 5, 2018
@tomato42
Copy link
Member

tomato42 commented Sep 5, 2018

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
Copy link

@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
Copy link
Member

tomato42 commented Sep 29, 2019

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

@tomato42 tomato42 changed the title OpenSSL signatures no longer compatible with ecdsa RAW format OpenSSL signatures no longer compatible with ecdsa RAW format (README outdated) Sep 29, 2019
@tomato42 tomato42 mentioned this issue Oct 19, 2019
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug unintended behaviour in ecdsa code help wanted
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants