Add Secp256r1 #240

jonas-lj · 2022-11-21T10:10:31Z

Fixes #217 and paves the way for MystenLabs/sui#6235

kchalkias · 2022-11-22T01:24:34Z

fastcrypto/src/secp256r1.rs

+}
+
+impl Secp256r1Signature {
+    /// Recover public key from signature


Can you refer to the original impl this was based, just to ensure no malleability and consistency with the secpK1 lib. Ideally we should copy paste that logic (if not).
@joyqvq please have a look as well

I will. It is mostly based on the specs (section 4.1.6 in https://www.secg.org/sec1-v2.pdf) and also on an implementation in the k256 crate. I know it's not optimal to do this on our own, but I couldn't find code I could copy-paste directly.

I have two somewhat related questions regarding this:

I'm assuming we wan't to recover and check public keys because of the attack you found recently, but not both verify and ecrecover. Which should we do on default?

Should we add a recovery id byte to signatures as it is done in secp256k1? Right now, we're recovering up to four potential keys, but since the end goal is interoperability with hardware wallets, the answer to this depends on how the hardware wallets are handling this. The documentation doesn't mention it, but the examples I have found looks like signatures are export as ASN.1 in DER formet without a recovery id byte.

kchalkias · 2022-11-22T01:26:06Z

fastcrypto/Cargo.toml

@@ -37,6 +37,7 @@ typenum = "1.15.0"
 # TODO: switch to https://github.com/celo-org/celo-bls-snark-rs
 # when https://github.com/celo-org/celo-bls-snark-rs/issues/228 is solved
 celo-bls = { git = "https://github.com/huitseeker/celo-bls-snark-rs", branch = "updates-2-with-parallelism-toggle", package = "bls-crypto", default-features = false }
+p256 = { version = "0.11.1", features = ["ecdsa"] }


Indeed, this is unfortunately the lib to use (secp256r1 is not very popular in the chain space, and probably nobody worked on ecrecover and micro-optimizations :) ), unless @huitseeker has seen any fork of the one we use secpk1.

There wasn't any ecrecover, and I'll check regarding benchmarks, but if we only consider the usage in Sui, where it's only going to be used for wallets, I'm assuming the performance is not that critical?

Ecrecover helps on compressing txs, but we know it's more expensive than regular verification. Atm we try to be similar to Ethereum that expects ecrecover and we should use exactly the same logic, with the v byte flag that denotes which public key to pick when recovering.

We also want both ECDSAs to behave similarly.

We will also need the regular verification in both ECDSAs as i/ requires for our fair benchmarks + to switch if in the future we decide that cost is more important than size.

You could try the arithmetic generated by fiat-crypto, which is usually pretty darn fast:
https://github.com/mit-plv/fiat-crypto/tree/master/fiat-rust/src
I haven't run benches to compare to k256/p256, though.

With the most recent commits, we are reimplementing ECDSA anyway, so using a different lib could make sense. We should run some benchmarks first to see if it's worth it though.

kchalkias

Clarifications re ecrecover and v flag to be 100% compatible with the other ECDSA.

Note that wallets that sign with libraries that do not support outputs with ecrecover, we're going to provide converters.

We should also normalise signatures as we do for k1 curve, by reducing s value.

joyqvq · 2022-11-22T14:45:46Z

fastcrypto/src/secp256r1.rs

+}
+
+impl Verifier<Secp256r1Signature> for Secp256r1PublicKey {
+    fn verify(&self, msg: &[u8], signature: &Secp256r1Signature) -> Result<(), signature::Error> {


for secp256k1, we have the recovery id appended to the 64 bytes signature. perhaps we can do the same for the r1 signature as well?

joyqvq · 2022-11-22T14:53:35Z

fastcrypto/src/tests/secp256r1_tests.rs

+// }
+
+#[test]
+fn wycheproof_test() {


joyqvq · 2022-11-22T15:13:50Z

fastcrypto/src/secp256r1.rs

+        // This implementation is based on recover_verifying_key_from_digest_bytes in the p256 crate,
+        // but also handles the case the the x-coordinate is larger than the group order.
+
+        let (r, s) = self.sig.split_scalars();


for secp256k1, we reject signature with s in the lower half of the field. perhaps to maintain the same behavior here.

To align the schemes, that certainly makes sense to do during signing. It's what Ethereum does, right, but I don't think it's part of the standard? The wycheproof tests for p256 fails if signatures with s > n/2 signatures are rejected, so we'll have to get around that if we wan't to enforce this on verification.

jonas-lj · 2022-11-22T15:48:16Z

Clarifications re ecrecover and v flag to be 100% compatible with the other ECDSA.

Note that wallets that sign with libraries that do not support outputs with ecrecover, we're going to provide converters.

We should also normalise signatures as we do for k1 curve, by reducing s value.

I'll add v flag and integrate your and Joys comments such that this implementation is 100% compatible with the k256 impl. And then I suggest that we for both ECDSAs implement both verify and ecrecover, perhaps with two different signature types. But I think we should do this in another PR.

kchalkias · 2022-11-22T15:56:33Z

Clarifications re ecrecover and v flag to be 100% compatible with the other ECDSA.

Note that wallets that sign with libraries that do not support outputs with ecrecover, we're going to provide converters.

We should also normalise signatures as we do for k1 curve, by reducing s value.

I'll add v flag and integrate your and Joys comments such that this implementation is 100% compatible with the k256 impl. And then I suggest that we for both ECDSAs implement both verify and ecrecover, perhaps with two different signature types. But I think we should do this in another PR.

Yeah in another PR

huitseeker

Thanks a bunch for the rapid turnaround!

As much as I love the awesome folks at RustCrypto, I'd urge to read their scary warning on this crate:

⚠️ Security Warning

The elliptic curve arithmetic contained in this crate has never been independently audited! (...)

I think we have several options here, which all improve security:

we liaise with the RustCrypto folks and get this crate audited,
we use the point and field arithmetic from fiat-crypto
we proptest all the operations we use from p256, comparing the outputs to those of an audited Rust crypto lib such as ring
we use symbolic testing with crux-mir
more conventional fuzz-testing than proptest
...

I don't think we need to use all of those methods in this PR, of course, but I do think we need to think about employing at least a couple of these. Until then, I would make the secp256r1 module pub(crate)

huitseeker · 2022-11-22T20:03:43Z

fastcrypto/Cargo.toml

@@ -37,6 +37,7 @@ typenum = "1.15.0"
 # TODO: switch to https://github.com/celo-org/celo-bls-snark-rs
 # when https://github.com/celo-org/celo-bls-snark-rs/issues/228 is solved
 celo-bls = { git = "https://github.com/huitseeker/celo-bls-snark-rs", branch = "updates-2-with-parallelism-toggle", package = "bls-crypto", default-features = false }
+p256 = { version = "0.11.1", features = ["ecdsa"] }


You could try the arithmetic generated by fiat-crypto, which is usually pretty darn fast:
https://github.com/mit-plv/fiat-crypto/tree/master/fiat-rust/src
I haven't run benches to compare to k256/p256, though.

fastcrypto/src/secp256r1.rs

huitseeker · 2022-11-22T20:16:24Z

fastcrypto/src/tests/secp256r1_tests.rs

+#[test]
+fn serialize_deserialize() {
+    let kpref = keys().pop().unwrap();
+    let public_key = kpref.public();
+
+    let bytes = bincode::serialize(&public_key).unwrap();
+    let pk2 = bincode::deserialize::<Secp256r1PublicKey>(&bytes).unwrap();
+    assert_eq!(public_key.as_ref(), pk2.as_ref());
+
+    let private_key = kpref.private();
+    let bytes = bincode::serialize(&private_key).unwrap();
+    let privkey = bincode::deserialize::<Secp256r1PrivateKey>(&bytes).unwrap();
+    let bytes2 = bincode::serialize(&privkey).unwrap();
+    assert_eq!(bytes, bytes2);
+
+    let signature = Secp256r1Signature::default();
+    let bytes = bincode::serialize(&signature).unwrap();
+    let sig = bincode::deserialize::<Secp256r1Signature>(&bytes).unwrap();
+    let bytes2 = bincode::serialize(&sig).unwrap();
+    assert_eq!(bytes, bytes2);
+
+    // test serde_json serialization
+    let serialized = serde_json::to_string(&signature).unwrap();
+    println!("{:?}", serialized);
+    let deserialized: Secp256r1Signature = serde_json::from_str(&serialized).unwrap();
+    assert_eq!(deserialized.as_ref(), signature.as_ref());
+}
+
+#[test]
+fn import_export_public_key() {
+    let kpref = keys().pop().unwrap();
+    let public_key = kpref.public();
+    let export = public_key.encode_base64();
+    let import = Secp256r1PublicKey::decode_base64(&export);
+    assert!(import.is_ok());
+    assert_eq!(import.unwrap().as_ref(), public_key.as_ref());
+}
+
+#[test]
+fn test_public_key_bytes_conversion() {
+    let kp = keys().pop().unwrap();
+    let pk_bytes: Secp256r1PublicKeyBytes = kp.public().into();
+    let rebuilt_pk: Secp256r1PublicKey = pk_bytes.try_into().unwrap();
+    assert_eq!(kp.public().as_bytes(), rebuilt_pk.as_bytes());
+}
+
+#[test]
+fn test_public_key_recovery() {
+    let kp = keys().pop().unwrap();
+    let message: &[u8] = b"Hello, world!";
+    let signature: Secp256r1Signature = kp.sign(message);
+    let recovered_keys = signature.recover(message).unwrap();
+    assert!(recovered_keys.contains(kp.public()));
+}


Nit: Those roundtrip tests are ideal candidates for a proptest conversion, specially if we implement the Arbitrary trait.

fastcrypto/src/secp256r1.rs

huitseeker · 2022-11-22T20:35:33Z

fastcrypto/src/secp256r1.rs

+
+impl ToFromBytes for Secp256r1PrivateKey {
+    fn from_bytes(bytes: &[u8]) -> Result<Self, FastCryptoError> {
+        match ExternalSecretKey::try_from(bytes) {


I'd love if we could make sure (either through unit testing here, or figuring out a wycheproof test that checks this) that non-canonical scalars are handled properly.

There doesn't seem to be a wycheproof test for this. I'll add an issue.

fastcrypto/src/tests/secp256r1_tests.rs

jonas-lj · 2022-11-23T09:40:50Z

Thanks a bunch for the rapid turnaround!

As much as I love the awesome folks at RustCrypto, I'd urge to read their scary warning on this crate:

⚠️ Security Warning

The elliptic curve arithmetic contained in this crate has never been independently audited! (...)

I think we have several options here, which all improve security:

we liaise with the RustCrypto folks and get this crate audited,

we use the point and field arithmetic from fiat-crypto

we proptest all the operations we use from p256, comparing the outputs to those of an audited Rust crypto lib such as ring

we use symbolic testing with crux-mir

more conventional fuzz-testing than proptest

...

I don't think we need to use all of those methods in this PR, of course, but I do think we need to think about employing at least a couple of these. Until then, I would make the secp256r1 module pub(crate)

Thanks for the review!

The issue about lack of independent auditing is not specific for this PR since it's also the case for at least the k256 crate and perhaps others that do not specifically state, that they've been audited. So I suggest we discuss how we should handle this for all crates we depend on and use the ideas you mention as inputs to that discussion. We could make the module pub(crate), but should in principle do the same for k256 then :).

jonas-lj · 2022-11-25T13:00:51Z

Clarifications re ecrecover and v flag to be 100% compatible with the other ECDSA.

Note that wallets that sign with libraries that do not support outputs with ecrecover, we're going to provide converters.

We should also normalise signatures as we do for k1 curve, by reducing s value.

This is implemented now. Code used to sign, generate recovery id and public key recovery is copied from the k256 crate.

kchalkias

I think we're in a good state to merge, please resolve conflicts and will approve

jonas-lj · 2022-11-29T08:23:43Z

I think we're in a good state to merge, please resolve conflicts and will approve

It is done!

kchalkias · 2022-11-29T18:13:13Z

fastcrypto/src/secp256r1.rs

+
+impl Signer<Secp256r1Signature> for Secp256r1KeyPair {
+    fn try_sign(&self, msg: &[u8]) -> Result<Secp256r1Signature, signature::Error> {
+        // Code copied from Sign.rs in k256@0.11.6


kchalkias · 2022-11-29T18:13:49Z

fastcrypto/src/secp256r1.rs

+        // Code copied from Sign.rs in k256@0.11.6
+
+        // Hash message
+        let z = FieldBytes::from(Sha256::digest(msg).digest);


Interesting that internally the message is hashed with sha256.

Yes, I noticed that too. However, now that we have reproduced all the code, we could easily make the hash function generic.

kchalkias · 2022-11-29T18:14:44Z

fastcrypto/src/secp256r1.rs

+        let sig = ExternalSignature::from_scalars(r, s)?;
+
+        // Note: This line is introduced here because big_r.y is a private field.
+        let y: Scalar = Scalar::from_be_bytes_reduced(*big_r.to_encoded_point(false).y().unwrap());


Can this fail with a malformed R? maybe avoid unwrap just in case. ah it's signing, we don't really care, as it will never run on-chain + R is computed here.

The y() method only returns None if the encoded point is compressed or compact, but we specifically ask for an uncompressed encoding, so the unwrap will never fail in this case. I will make the code bit more clear.

kchalkias · 2022-11-29T18:16:41Z

fastcrypto/src/secp256r1.rs

+    /// An [FastCryptoError::GeneralError] is returned if no public keys can be recovered.
+    pub fn recover(&self, msg: &[u8]) -> Result<Secp256r1PublicKey, FastCryptoError> {
+        let (r, s) = self.sig.split_scalars();
+        let v = RecoveryId::from_byte(self.recovery_id).unwrap();


As this will run on blockchain, let's ensure it returns an error vs panic on fail, unless we know it will never fail, even under malicious signers.

That's true. The deserialization doesn't check the recovery id byte, so a malicious signer could just put anything there. It's fixed now.

kchalkias

Awesome!

kchalkias reviewed Nov 22, 2022

View reviewed changes

jonas-lj marked this pull request as ready for review November 22, 2022 13:20

jonas-lj requested review from huitseeker, joyqvq and benr-ml as code owners November 22, 2022 13:20

kchalkias reviewed Nov 22, 2022

View reviewed changes

joyqvq reviewed Nov 22, 2022

View reviewed changes

fastcrypto/src/tests/secp256r1_tests.rs

// }

#[test]

fn wycheproof_test() {

Copy link

Collaborator

joyqvq Nov 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!

joyqvq reviewed Nov 22, 2022

View reviewed changes

jonas-lj mentioned this pull request Nov 22, 2022

Use FastCryptoError for batched verification #243

Closed

huitseeker reviewed Nov 22, 2022

View reviewed changes

jonas-lj mentioned this pull request Nov 23, 2022

Fix default public key in secp256k1? #245

Closed

jonas-lj marked this pull request as draft November 24, 2022 12:05

jonas-lj mentioned this pull request Nov 25, 2022

Make sure that non-canonical scalars are handled properly #248

Closed

jonas-lj marked this pull request as ready for review November 25, 2022 12:56

kchalkias reviewed Nov 29, 2022

View reviewed changes

jonas-lj added 9 commits November 29, 2022 09:08

Add first working draft of ECDSA over secp256r1

d17feb9

Add some tests

956618b

Hash messages with sha256

76851fb

Add wycheproof tests

e32d6df

Remove unused imports

3e9db4f

Remove hashes tests and (almost) fix zeroization

f298143

Comment conditional line

fa208cd

Fix zeroization

5eaca88

Remove finished todo comments

08dbf74

jonas-lj added 19 commits November 29, 2022 09:08

Add pubkey recovery

4a6e391

Error handling for public key recovery

6f4dd7e

Fix public key recovery

307420b

Fail early if pubkey recover cannot find any valid keys

a2252cc

Add comment

198b857

Fix default public key

3e6fb87

Keep bytes in from_bytes around until next time to_bytes is called

e7b2ac5

Fix test

765f192

Use low lever function to get recovery id byte

1d2565a

Copy signing code from the ecdsa crate

4b9ab8c

Add recoveryId to signature struct

2e6c657

Compute recovery id

8bdf9ac

Pubkey recover copied from k256

3d3bcdf

Clean up code

6be3d2c

Update doc

b711ca2

Fix doc

a55beaa

Handle large x-coordinate for kG#

aaa70c0

Docs and reject upper s test

2c6d8f0

Add todo to proptest

940e474

jonas-lj force-pushed the secp256r1 branch from 21934ab to 940e474 Compare November 29, 2022 08:12

Revert changes to bls12381

f8ab4b7

jonas-lj requested a review from kchalkias November 29, 2022 11:46

kchalkias reviewed Nov 29, 2022

View reviewed changes

jonas-lj added 2 commits November 30, 2022 10:16

Write method to extract y-coordinate from affine pint

8f148d4

Handle error on invalid recovery id

63eab1c

kchalkias approved these changes Nov 30, 2022

View reviewed changes

jonas-lj merged commit 874bb52 into main Nov 30, 2022

jonas-lj deleted the secp256r1 branch November 30, 2022 19:33

huitseeker mentioned this pull request Dec 1, 2022

Add a secp256r1 paragraph to the README #267

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Secp256r1 #240

Add Secp256r1 #240

jonas-lj commented Nov 21, 2022 •

edited

Loading

kchalkias Nov 22, 2022

jonas-lj Nov 22, 2022 •

edited

Loading

kchalkias Nov 22, 2022 •

edited

Loading

jonas-lj Nov 22, 2022

kchalkias Nov 22, 2022

kchalkias Nov 22, 2022

huitseeker Nov 22, 2022

jonas-lj Nov 25, 2022 •

edited

Loading

kchalkias left a comment

joyqvq Nov 22, 2022

joyqvq Nov 22, 2022

joyqvq Nov 22, 2022

jonas-lj Nov 23, 2022 •

edited

Loading

jonas-lj commented Nov 22, 2022

kchalkias commented Nov 22, 2022

huitseeker left a comment •

edited

Loading

huitseeker Nov 22, 2022

huitseeker Nov 22, 2022

huitseeker Nov 22, 2022

jonas-lj Nov 25, 2022

jonas-lj commented Nov 23, 2022 •

edited

Loading

⚠️ Security Warning

jonas-lj commented Nov 25, 2022 •

edited

Loading

kchalkias left a comment

jonas-lj commented Nov 29, 2022

kchalkias Nov 29, 2022

kchalkias Nov 29, 2022

jonas-lj Nov 30, 2022

kchalkias Nov 29, 2022

jonas-lj Nov 30, 2022

kchalkias Nov 29, 2022

jonas-lj Nov 30, 2022

kchalkias left a comment

Add Secp256r1 #240

Add Secp256r1 #240

Conversation

jonas-lj commented Nov 21, 2022 • edited Loading

Choose a reason for hiding this comment

jonas-lj Nov 22, 2022 • edited Loading

Choose a reason for hiding this comment

kchalkias Nov 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonas-lj Nov 25, 2022 • edited Loading

Choose a reason for hiding this comment

kchalkias left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonas-lj Nov 23, 2022 • edited Loading

Choose a reason for hiding this comment

jonas-lj commented Nov 22, 2022

kchalkias commented Nov 22, 2022

huitseeker left a comment • edited Loading

Choose a reason for hiding this comment

⚠️ Security Warning

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonas-lj commented Nov 23, 2022 • edited Loading

⚠️ Security Warning

jonas-lj commented Nov 25, 2022 • edited Loading

kchalkias left a comment

Choose a reason for hiding this comment

jonas-lj commented Nov 29, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kchalkias left a comment

Choose a reason for hiding this comment

jonas-lj commented Nov 21, 2022 •

edited

Loading

jonas-lj Nov 22, 2022 •

edited

Loading

kchalkias Nov 22, 2022 •

edited

Loading

jonas-lj Nov 25, 2022 •

edited

Loading

jonas-lj Nov 23, 2022 •

edited

Loading

huitseeker left a comment •

edited

Loading

jonas-lj commented Nov 23, 2022 •

edited

Loading

jonas-lj commented Nov 25, 2022 •

edited

Loading