RustCrypto / elliptic-curves

Collection of pure Rust elliptic curve implementations: NIST P-224, P-256, P-384, P-521, secp256k1, SM2
682 stars 191 forks source link

p521: field arithmetic implementation not working correctly #947

Closed tarcieri closed 1 year ago

tarcieri commented 1 year ago

This is a tracking issue for promoting p521's wip-arithmetic-do-not-use feature to a working arithmetic feature.

Notably the base field implementation in p521 uses code generated by fiat-crypto specific to Solinas primes, which is different from the Montgomery representation which is easily supported using macros in the primeorder crate.

945 fixed an issue where fiat_p521_tight_field_element uses a different limb representation than U576, however this was not sufficient to make the basic field arithmetic tests work: #946

MasterAwesome commented 1 year ago

What's remaining in terms of tests before promoting this into a sem-ver feature arithmetic?

tarcieri commented 1 year ago

When #951 lands we should basically be there, although I'd also like to wire up an ECDSA feature and add the Wycheproof test vectors for it

MasterAwesome commented 1 year ago

When #951 lands we should basically be there, although I'd also like to wire up an ECDSA feature and add the Wycheproof test vectors for it

I see, are you working on this or should I create a pull request?

tarcieri commented 1 year ago

I'm working on it. It's somewhat involved since the ecdsa crate assumes that the hash function output size is the same as the size of a serialized field element (it gets particularly hairy in the current RFC6979 implementation).

Fixing that will involve breaking changes to the ecdsa crate, but in the meantime I'm adding some newtypes to work around it.

MasterAwesome commented 1 year ago

I'm working on it. It's somewhat involved since the ecdsa crate assumes that the hash function output size is the same as the size of a serialized field element (it gets particularly hairy in the current RFC6979 implementation).

Fixing that will involve breaking changes to the ecdsa crate, but in the meantime I'm adding some newtypes to work around it.

Newtype for the curve's default hashing function that returns field element sized outputsize?

tarcieri commented 1 year ago

Newtypes for ecdsa::{SigningKey, VerifyingKey}

MasterAwesome commented 1 year ago

Newtypes for ecdsa::{SigningKey, VerifyingKey}

Ah I see, so this eventually will require a breaking change in ecdsa crate.

tarcieri commented 10 months ago

@MasterAwesome I was able to make the breaking changes in the ecdsa and rfc6979 crates:

However, the rfc6979 crate still can't correctly generate a P-521 test vector for whatever reason: