search

ZenGo-X / curv

Rust language general purpose elliptic curve cryptography.
MIT License
264 stars 111 forks source link

Bugfix/inconsistent from bigint in curves #144

Closed Shalevos closed 2 years ago

Shalevos commented 2 years ago

Fixed an inconsistency bug, where converting from BigInt to a Scalar and back again resulted in a different number - namely, the number modulo the curve order, which led to generating sign-verify keypairs on the curves from a given secret that were incompatible with other crypto libs such as NaCl. The issue arose when generating an EdDSA keypair from a user-chosen seed. However, the same issue was found in the Ristretto curve also and fixed.

A new test for consistency on all curves has been added and currently passes. The test uses a hardcoded value such that it is larger than the curve modulo.

Shalevos commented 2 years ago

Also verified with the underlying implementations of scalar multiplication of both curves that this should not hinder performance.

omershlo commented 2 years ago

Thanks ! 1) I see you changed from_bigint for ristretto and ed25519 . Isn't this introduce now inconstancy with the rest of the curves? for example: https://github.com/ZenGo-X/curv/blob/master/src/elliptic/curves/secp256_k1.rs#L181 .

2) your change is in the function from_bigint. This function is supposed to be generating a field element (scalar) out of a the input. This is why we enforce a mod operation. Your change makes the output potentially be not a field element which will cause problems. Is there other way to solve the downstream issue of compatibility ?

Shalevos commented 2 years ago

That is a very valid point. I looked over NaCl's implementation for reference (where there is no scalar struct at all) and I agree that the situation here is different. At this point it might be best to close this PR completely and modify code that uses curv s.t. it only uses the ECScalar struct for computing arithmetic on a curve. In the documentation of ECScalar it is noted that a modulo operation is performed (as pointed out by @survived off-thread), so therefore any user of curv should be able to use ECScalar properly. Essentially, the bug referenced above in EdDSA is a misuse of curv.