purplesyringa / sslcrypto

Simple ECIES, ECDSA and AES library for Python, supporting OpenSSL and pure-Python environments
Other
27 stars 6 forks source link

Parse elliptic curve "nid" from secp256k1 public key. #21

Open georgepadayatti opened 2 years ago

georgepadayatti commented 2 years ago

Here there is a check to make sure the public key curve "nid" is matching - https://github.com/georgepadayatti/sslcrypto/blob/22c3bb311c291b2d1398b0b68d9e638f2ba81f59/sslcrypto/_ecc.py#L359

Question, is there a public documentation for why this is done so ? Wanted to check if there is a compatibility issue between sslcrypto and eccrypto (https://github.com/bitchan/eccrypto).

purplesyringa commented 2 years ago

Here there is a check to make sure the public key curve "nid" is matching

This seems like an important check. Otherwise, a maliciously crafted public key could lead to the usage of an insecure verification protocol or something. I'm not sure if there's any actual vulnerability, but I wouldn't count on it either way.

Wanted to check if there is a compatibility issue between sslcrypto and eccrypto

Why do you think there is an incompatibility? Given that sslcrypto often uses OpenSSL underhood, was tested on actual data generated and accepted by OpenSSL, and was cross-checked with some other cryptography libraries (though those checks aren't in the source tree sadly), I think it should be compatible.

georgepadayatti commented 2 years ago

Thanks for the reply. "nid" value returned for a secp256k1 public key came as "1158" where as it should have been "714" and therefore resulted in wrong curve exception. I will check on my end, if this is an issue with the implementation of secp256k1 public keys that I received from a 3rd party.

georgepadayatti commented 2 years ago

@imachug

I have recreated the issue using following snippets.

Node.js code snippet to generate secp256k1 (public, private) key pair:

var eccrypto = require("eccrypto");
var EC = require("elliptic").ec;
var ec = new EC("secp256k1");

var privateKeyA = eccrypto.generatePrivate();

var pk = ec.keyFromPrivate(privateKeyA).getPublic("hex")

console.log(pk)

Output:

~/nodejs-projects/eccrypto-snippets node index.js

04f8830f92f0eb103137f18109c92da663fb38309ae0f9046a0b62fcae04d74a2dc51ecf2e5eb0c3f8ad0bf3554268f744dcc6840042088cdf108364ed73fc9803

Python code snippet to calculate nid of the public key:

from coincurve import PublicKey
import struct

pk_hex = "04f8830f92f0eb103137f18109c92da663fb38309ae0f9046a0b62fcae04d74a2dc51ecf2e5eb0c3f8ad0bf3554268f744dcc6840042088cdf108364ed73fc9803"
pk_bytes = bytes.fromhex(pk_hex)

pk = PublicKey(pk_bytes)

nid, = struct.unpack("!H", pk_bytes[0:2])

print(nid)

Output:

(venv) ➜  ~/PythonProjects/sslcrypto-snippets python main.py 

1272

The nid is not equal to 714.

Any ideas ?

georgepadayatti commented 2 years ago

Raised the same in eccrypto repository as well. https://github.com/bitchan/eccrypto/issues/88

purplesyringa commented 2 years ago

Wait, actually, the problem is different.

Basically, there are two common formats of public keys.

The first format is ASN. This thing includes NID as well as everything else required to specify the curve and the public key.

The second format is a format which probably doesn't have a name, is used when the exact curve is already known, and is mostly common in secp256k1 world. It's a single byte 0x04 followed by the X and Y coordinates taking 32 bytes each, thus occupying 65 bytes in total. Or occasionally it's 0x02 or 0x03 followed by the X coordinate, so 33 bytes in total.

The problem is that the public key you're using is in the second format (so pk_bytes[0:2] is not NID but 0x04 followed by the first byte of the X coordinate), and sslcrypto is trying to interpret it as if it was in ASN format. So there's no problem regarding NIDs in other libraries, there must be another reason.

According to the code, the only place where _decode_public_key_openssl is used (this method assumes ASN) is when decrypting ECIES-encoded data. It would be very useful to know which library was used to encrypt the data in the first place. If it is indeed confirmed that the format of the ciphertext is <iv><raw ephemeral public key><encrypted data> rather than <iv><ASN ephemeral public key><encrypted data>, I'll be able to add support for that in a minute.