Add Wycheproof ECDSA p256 test vectors

[daviddrysdale]

Created from:
https://github.com/google/wycheproof/blob/2196000605e45d91097147c9c71f26b72af58003/testvectors/ecdsa_secp256r1_sha256_test.json

[daviddrysdale]

Currently only works in --release mode because of a debug_assert_eq! in ecdsa/src/der.rs

[tarcieri]

That debug_assert_eq should likely be removed, especially since it duplicates a check immediately below it. Offhand I don't remember why I added it in the first place but it seems rather vestigial.

[tarcieri]

I removed the debug_assert_eq in RustCrypto/signatures#258

[daviddrysdale]

I removed the debug_assert_eq in #258

Thanks! How do the inter-repo/inter-crate dependencies work, BTW? Does there need to be a new release of ecdsa before the change is visible here?

[tarcieri]

You can add the following to this repo's Cargo.toml as part of this PR to source ecdsa via git:

[patch.crates-io]
ecdsa = { git = "https://github.com/RustCrypto/signatures.git" }

[tarcieri]

Looks like it's passing now 🎉

[codecov-io]

Codecov Report

Merging #313 (b250cb9) into master (77ca1ff) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #313   +/-   ##
=======================================
  Coverage   58.60%   58.60%           
=======================================
  Files          35       35           
  Lines        3860     3860           
=======================================
  Hits         2262     2262           
  Misses       1598     1598           

Impacted Files	Coverage Δ
p256/src/ecdsa.rs	90.47% <ø> (ø)
p384/src/lib.rs	0.00% <ø> (ø)
bp256/src/r1.rs	0.00% <0.00%> (ø)
bp256/src/t1.rs	0.00% <0.00%> (ø)
bp384/src/r1.rs	0.00% <0.00%> (ø)
bp384/src/t1.rs	0.00% <0.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 481c617...b250cb9. Read the comment docs.

[tarcieri]

Unfortunately cross doesn't seem to work with those [patch.crates-io] directives.

I can either cut some prerelease crates or you can temporarily comment out the cross tests in .github/workflows.

[daviddrysdale]

I don't think there's any rush – I can just pause this until the next time there's an ecdsa update.

[tarcieri]

Cool, we'll hopefully have one out after RustCrypto/traits#392 and RustCrypto/traits#478

[tarcieri]

@daviddrysdale for unrelated reasons I cut another release of the ecdsa crate which incorporates your changes.

If you rebase and remove the git-based dependencies, I think it might work?

[tarcieri]

Sweet, looks like the tests are passing.

@daviddrysdale let me know if there are any other changes you want to make, or otherwise I'd say this is ready to merge

[daviddrysdale]

Thanks, I think this is ready.

[tarcieri]

Thank you!

[tarcieri]

@daviddrysdale how do I use the conversion tool to generate a .blb file of the secp256k1 test vectors (see #232)

[daviddrysdale]

I tweaked RustCrypto/utils#280 to use curve names (rather than just ECDSA), so:

wycheproof2blb /path/to/wycheproof secp256k1 0 secp256k1.blb secp256k1.txt

should work.

However, when I add a test to k256 (with ecdsa_core::new_wycheproof_test!(wycheproof, "wycheproof", Secp256k1);) there are a lot of failures that I've not dug into yet. I wonder if there's maybe some unsupported format problem (but OTOH there are some signatures in the test data that do verify correctly, so it's not a ubiquitous problem).

[tarcieri]

I wonder if there's maybe some unsupported format problem (but OTOH there are some signatures in the test data that do verify correctly, so it's not a ubiquitous problem).

It may be that there are test vectors that aren't low-S normalized. k256 rejects these as secp256k1 is often used in consensus-critical applications, and this normalization prevents malleability (but at the same time rejects signatures that would verify in other ECDSA implementations).

[newpavlov]

@tarcieri
Maybe we should introduce a separate method for non-normalized signatures?

[tarcieri]

The NormalizeLow::normalize_low method can be used to low-S normalize a signature in advance.

[newpavlov]

It could be a bit hard to discover, no? Also I couldn't find any mention of the normalization requirement in the docs (at least on the verification methods). This behavior may be quite surprising for users not familiar with malleability. Even worse, it may result in rejection of signatures valid in some areas (e.g. DSA for electronic document management).

[tarcieri]

It should probably be mentioned in the documentation.

However, it doesn't really come up in practice, because secp256k1 is used almost exclusively for cryptocurrency applications and low-S normalization is ubiquitous in those applications.

[newpavlov]

Ah, so this behavior is defined on the elliptic curve crate level? (I thought ecdsa provides a generic implementation over EC) I am not sure if such silent and subtle difference between behavior of EC crates is ideal. Having separate methods for malleable and non-malleable signatures would have been probably better, but since signature is already released under v1.0, I am not sure if it will be an easy change.

[tarcieri]

It's really a property of the secp256k1 ecosystem specifically in relation to signatures being consensus critical and therefore needing to be non-malleable.

Pretty much every other library that specializes in ECDSA/secp256k1 has adopted the same rules for this reason. It would be odd for an ECDSA/secp256k1 library to do anything else, IMO (e.g. OpenSSL does not enforce these rules, and for that reason practically nothing uses OpenSSL in cryptocurrency applications anymore).

[tarcieri]

...since signature is already released under v1.0, I am not sure if it will be an easy change.

There are a few other applications for other signature algorithms where it might be interesting to have a sort of separate set of "consensus critical validation" rules, such as the ZIP-215 rules for Ed25519 verification.

But really it doesn't make sense for an ECDSA/secp256k1 library to make it more difficult to discover how to perform non-malleable verification, IMO. I'd worry about it subtly introducing malleability bugs if a user has to go out of their way to discover them.

[newpavlov]

I'd worry about it subtly introducing malleability bugs if a user has to go out of their way to discover them.

Exactly. IIUC with current approach users can introduce malleability bugs by using curves which are less traditional for consensus critical applications. And having different curves behave differently in this regard, only makes the matter worse. By having two separate methods, they would at least learn about malleability and think a bit about which variant do they need in their application.

[tarcieri]

Exactly. IIUC with current approach users can introduce malleability bugs by using curves which are less traditional for consensus critical applications.

That's something that really needs to get spec'd and added to the verification rules for a particular application, IMO.

As it were though, it's a good point: I did think of an application where someone is trying to do this with P-256 and they didn't incorporate such rules.

[daviddrysdale]

It may be that there are test vectors that aren't low-S normalized.

Yep, looks that way.

The NormalizeLow::normalize_low method can be used to low-S normalize a signature in advance.

Adding that leads to all cases passing except two – rough code at #384