From e61c49d5f06a8099e4bd8db25c8c014b11b5c655 Mon Sep 17 00:00:00 2001 From: Maxim Shishmarev Date: Wed, 15 Jun 2022 15:29:13 +1000 Subject: [PATCH 1/6] chore: move import inside macro --- engine/src/multisig/crypto/curve25519_ristretto.rs | 2 +- engine/src/multisig/crypto/helpers.rs | 3 ++- engine/src/multisig/crypto/secp255k1.rs | 1 - 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/engine/src/multisig/crypto/curve25519_ristretto.rs b/engine/src/multisig/crypto/curve25519_ristretto.rs index ea7dcecdb2f..57e01c8fa50 100644 --- a/engine/src/multisig/crypto/curve25519_ristretto.rs +++ b/engine/src/multisig/crypto/curve25519_ristretto.rs @@ -70,7 +70,7 @@ mod point_impls { mod scalar_impls { - use zeroize::{Zeroize, ZeroizeOnDrop}; + use zeroize::Zeroize; use super::*; diff --git a/engine/src/multisig/crypto/helpers.rs b/engine/src/multisig/crypto/helpers.rs index 8106ba17790..a290750a0de 100644 --- a/engine/src/multisig/crypto/helpers.rs +++ b/engine/src/multisig/crypto/helpers.rs @@ -8,11 +8,12 @@ macro_rules! derive_scalar_impls { impl Drop for $scalar { fn drop(&mut self) { + use zeroize::Zeroize; self.zeroize(); } } - impl ZeroizeOnDrop for $scalar {} + impl zeroize::ZeroizeOnDrop for $scalar {} impl std::ops::Add for $scalar { type Output = $scalar; diff --git a/engine/src/multisig/crypto/secp255k1.rs b/engine/src/multisig/crypto/secp255k1.rs index 10d1d5be26c..6f05a7cb30c 100644 --- a/engine/src/multisig/crypto/secp255k1.rs +++ b/engine/src/multisig/crypto/secp255k1.rs @@ -1,5 +1,4 @@ use generic_array::GenericArray; -use zeroize::{Zeroize, ZeroizeOnDrop}; use super::{ECPoint, ECScalar, Rng}; use serde::{Deserialize, Serialize}; From 44b0f44280432fe18acd03dbff30143b48ed27af Mon Sep 17 00:00:00 2001 From: Alastair Holmes Date: Wed, 22 Jun 2022 11:07:21 +0200 Subject: [PATCH 2/6] chore: remove curv dependency --- Cargo.lock | 270 ++-------------- engine/Cargo.toml | 7 +- .../multisig/client/keygen/keygen_frost.rs | 2 +- engine/src/multisig/client/signing/frost.rs | 2 +- engine/src/multisig/crypto.rs | 7 +- .../multisig/crypto/curve25519_ristretto.rs | 2 +- engine/src/multisig/crypto/eth.rs | 2 +- engine/src/multisig/crypto/secp255k1.rs | 300 +++++++++++++----- 8 files changed, 263 insertions(+), 329 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index d42b74652d2..0cb50c90b4f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -949,7 +949,6 @@ dependencies = [ "config", "crossbeam-channel", "csv", - "curv-kzen", "curve25519-dalek 2.1.3", "custom-rpc", "dyn-clone", @@ -977,6 +976,8 @@ dependencies = [ "log 0.4.17", "mockall", "multisig-p2p-transport", + "num-bigint 0.4.3", + "num-traits", "pallet-cf-auction", "pallet-cf-broadcast", "pallet-cf-environment", @@ -1197,12 +1198,6 @@ dependencies = [ "yaml-rust", ] -[[package]] -name = "const-oid" -version = "0.6.2" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "9d6f2aa4d0537bcc1c74df8755072bd31c1ef1a3a1b85a68e8404a8c353b7b8b" - [[package]] name = "const-oid" version = "0.7.1" @@ -1424,18 +1419,6 @@ version = "0.2.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7a81dae078cea95a014a339291cec439d2f232ebe854a9d672b796c6afafa9b7" -[[package]] -name = "crypto-bigint" -version = "0.2.5" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "8658c15c5d921ddf980f7fe25b1e82f4b7a4083b2c4985fea4922edb8e43e07d" -dependencies = [ - "generic-array 0.14.5", - "rand_core 0.6.3", - "subtle", - "zeroize", -] - [[package]] name = "crypto-bigint" version = "0.3.2" @@ -1478,12 +1461,6 @@ dependencies = [ "subtle", ] -[[package]] -name = "cryptoxide" -version = "0.1.3" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e35f15e1a0699dd988fed910dd78fdc6407f44654cd12589c91fa44ea67d9159" - [[package]] name = "csv" version = "1.1.6" @@ -1545,40 +1522,6 @@ dependencies = [ "rand 0.7.3", ] -[[package]] -name = "curv-kzen" -version = "0.9.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "ddc25c87ebf29b249e801de5eed820f0c9ba001054bf73008df884690a03e6eb" -dependencies = [ - "cryptoxide", - "curve25519-dalek 3.2.0", - "digest 0.9.0", - "ff-zeroize", - "generic-array 0.14.5", - "hex", - "hmac 0.11.0", - "lazy_static", - "merkle-cbt", - "num-integer", - "num-traits", - "p256", - "pairing-plus", - "rand 0.6.5", - "rand 0.7.3", - "rust-gmp-kzen", - "secp256k1 0.20.3", - "serde", - "serde_bytes", - "serde_derive", - "sha2 0.8.2", - "sha2 0.9.9", - "sha3 0.9.1", - "thiserror", - "typenum", - "zeroize", -] - [[package]] name = "curve25519-dalek" version = "2.1.3" @@ -1683,22 +1626,13 @@ dependencies = [ "syn", ] -[[package]] -name = "der" -version = "0.4.5" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "79b71cca7d95d7681a4b3b9cdf63c8dbc3730d0584c2c74e31416d64a90493f4" -dependencies = [ - "const-oid 0.6.2", -] - [[package]] name = "der" version = "0.5.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "6919815d73839e7ad218de758883aae3a257ba6759ce7a9992501efbb53d705c" dependencies = [ - "const-oid 0.7.1", + "const-oid", ] [[package]] @@ -1899,26 +1833,14 @@ version = "1.0.5" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "21e50f3adc76d6a43f5ed73b698a87d0760ca74617f60f7c3b879003536fdd28" -[[package]] -name = "ecdsa" -version = "0.12.4" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "43ee23aa5b4f68c7a092b5c3beb25f50c406adc75e2363634f242f28ab255372" -dependencies = [ - "der 0.4.5", - "elliptic-curve 0.10.4", - "hmac 0.11.0", - "signature", -] - [[package]] name = "ecdsa" version = "0.13.4" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "d0d69ae62e0ce582d56380743515fefaf1a8c70cec685d9677636d7e30ae9dc9" dependencies = [ - "der 0.5.1", - "elliptic-curve 0.11.12", + "der", + "elliptic-curve", "rfc6979", "signature", ] @@ -1952,22 +1874,6 @@ version = "1.6.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "e78d4f1cc4ae33bbfc157ed5d5a5ef3bc29227303d595861deb238fcec4e9457" -[[package]] -name = "elliptic-curve" -version = "0.10.4" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "83e5c176479da93a0983f0a6fdc3c1b8e7d5be0d7fe3fe05a99f15b96582b9a8" -dependencies = [ - "crypto-bigint 0.2.5", - "ff 0.10.1", - "generic-array 0.14.5", - "group 0.10.0", - "pkcs8", - "rand_core 0.6.3", - "subtle", - "zeroize", -] - [[package]] name = "elliptic-curve" version = "0.11.12" @@ -1975,11 +1881,11 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "25b477563c2bfed38a3b7a60964c49e058b2510ad3f12ba3483fd8f62c2306d6" dependencies = [ "base16ct", - "crypto-bigint 0.3.2", - "der 0.5.1", - "ff 0.11.1", + "crypto-bigint", + "der", + "ff", "generic-array 0.14.5", - "group 0.11.0", + "group", "rand_core 0.6.3", "sec1", "subtle", @@ -2181,16 +2087,6 @@ dependencies = [ "libc", ] -[[package]] -name = "ff" -version = "0.10.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "d0f40b2dcd8bc322217a5f6559ae5f9e9d1de202a2ecee2e9eafcbece7562a4f" -dependencies = [ - "rand_core 0.6.3", - "subtle", -] - [[package]] name = "ff" version = "0.11.1" @@ -2201,32 +2097,6 @@ dependencies = [ "subtle", ] -[[package]] -name = "ff-zeroize" -version = "0.6.3" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "c02169a2e8515aa316ce516eaaf6318a76617839fbf904073284bc2576b029ee" -dependencies = [ - "byteorder", - "ff_derive-zeroize", - "rand_core 0.5.1", - "zeroize", -] - -[[package]] -name = "ff_derive-zeroize" -version = "0.6.2" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b24d4059bc0d0a0bf26b740aa21af1f96a984f0ab7a21356d00b32475388b53a" -dependencies = [ - "num-bigint", - "num-integer", - "num-traits", - "proc-macro2", - "quote", - "syn", -] - [[package]] name = "file-per-thread-logger" version = "0.1.5" @@ -2856,24 +2726,13 @@ dependencies = [ "wasm-bindgen", ] -[[package]] -name = "group" -version = "0.10.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1c363a5301b8f153d80747126a04b3c82073b9fe3130571a9d170cacdeaf7912" -dependencies = [ - "ff 0.10.1", - "rand_core 0.6.3", - "subtle", -] - [[package]] name = "group" version = "0.11.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "bc5ac374b108929de78460075f3dc439fa66df9d8fc77e8f12caa5165fcf0c89" dependencies = [ - "ff 0.11.1", + "ff", "rand_core 0.6.3", "subtle", ] @@ -2998,9 +2857,6 @@ name = "hex" version = "0.4.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7f24254aa9a54b5c858eaee2f5bccdb46aaf0e486a595ed5fd8f86ba55232a70" -dependencies = [ - "serde", -] [[package]] name = "hex-literal" @@ -3639,8 +3495,8 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "19c3a5e0a0b8450278feda242592512e09f61c72e018b8cd5c859482802daf2d" dependencies = [ "cfg-if 1.0.0", - "ecdsa 0.13.4", - "elliptic-curve 0.11.12", + "ecdsa", + "elliptic-curve", "sec1", ] @@ -4528,15 +4384,6 @@ version = "0.3.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "71d96e3f3c0b6325d8ccd83c33b28acb183edcb6c67938ba104ec546854b0882" -[[package]] -name = "merkle-cbt" -version = "0.3.2" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "171d2f700835121c3b04ccf0880882987a050fd5c7ae88148abf537d33dd3a56" -dependencies = [ - "cfg-if 1.0.0", -] - [[package]] name = "merlin" version = "2.0.1" @@ -4928,6 +4775,17 @@ dependencies = [ "num-traits", ] +[[package]] +name = "num-bigint" +version = "0.4.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f93ab6289c7b344a8a9f60f88d80aa20032336fe78da341afc91c8a2341fc75f" +dependencies = [ + "autocfg 1.1.0", + "num-integer", + "num-traits", +] + [[package]] name = "num-complex" version = "0.4.1" @@ -4964,7 +4822,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "5c000134b5dbf44adc5cb772486d335293351644b801551abe8f75c84cfa4aef" dependencies = [ "autocfg 1.1.0", - "num-bigint", + "num-bigint 0.2.6", "num-integer", "num-traits", ] @@ -5140,32 +4998,6 @@ dependencies = [ "stable_deref_trait", ] -[[package]] -name = "p256" -version = "0.9.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "d053368e1bae4c8a672953397bd1bd7183dde1c72b0b7612a15719173148d186" -dependencies = [ - "ecdsa 0.12.4", - "elliptic-curve 0.10.4", - "sha2 0.9.9", -] - -[[package]] -name = "pairing-plus" -version = "0.19.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "58cda4f22e8e6720f3c254049960c8cc4f93cb82b5ade43bddd2622b5f39ea62" -dependencies = [ - "byteorder", - "digest 0.8.1", - "ff-zeroize", - "rand 0.4.6", - "rand_core 0.5.1", - "rand_xorshift 0.2.0", - "zeroize", -] - [[package]] name = "pallet-aura" version = "4.0.0-dev" @@ -5936,16 +5768,6 @@ version = "0.1.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "8b870d8c151b6f2fb93e84a13146138f05d02ed11c7e7c54f8826aaaf7c9f184" -[[package]] -name = "pkcs8" -version = "0.7.6" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "ee3ef9b64d26bad0536099c816c6734379e45bbd5f14798def6809e5cc350447" -dependencies = [ - "der 0.4.5", - "spki", -] - [[package]] name = "pkg-config" version = "0.3.25" @@ -6311,7 +6133,7 @@ dependencies = [ "rand_jitter", "rand_os", "rand_pcg 0.1.2", - "rand_xorshift 0.1.1", + "rand_xorshift", "winapi 0.3.9", ] @@ -6502,15 +6324,6 @@ dependencies = [ "rand_core 0.3.1", ] -[[package]] -name = "rand_xorshift" -version = "0.2.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "77d416b86801d23dde1aa643023b775c3a462efc0ed96443add11546cdf1dca8" -dependencies = [ - "rand_core 0.5.1", -] - [[package]] name = "rawpointer" version = "0.2.1" @@ -6740,7 +6553,7 @@ version = "0.1.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "96ef608575f6392792f9ecf7890c00086591d29a83910939d430753f7c050525" dependencies = [ - "crypto-bigint 0.3.2", + "crypto-bigint", "hmac 0.11.0", "zeroize", ] @@ -6824,17 +6637,6 @@ dependencies = [ "crossbeam-utils 0.8.8", ] -[[package]] -name = "rust-gmp-kzen" -version = "0.5.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "0e654bb304958a567aefa09e83cc313251388202c40bfc245fac19a0e2dd8d08" -dependencies = [ - "libc", - "num-traits", - "serde", -] - [[package]] name = "rust-ini" version = "0.18.0" @@ -7927,7 +7729,7 @@ version = "0.2.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "08da66b8b0965a5555b6bd6639e68ccba85e1e2506f5fbb089e93f8a04e1a2d1" dependencies = [ - "der 0.5.1", + "der", "generic-array 0.14.5", "subtle", "zeroize", @@ -8054,15 +7856,6 @@ dependencies = [ "serde_derive", ] -[[package]] -name = "serde_bytes" -version = "0.11.6" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "212e73464ebcde48d723aa02eb270ba62eff38a9b732df31f33f1b4e145f3a54" -dependencies = [ - "serde", -] - [[package]] name = "serde_derive" version = "1.0.137" @@ -9016,15 +8809,6 @@ version = "0.5.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "6e63cff320ae2c57904679ba7cb63280a3dc4613885beafb148ee7bf9aa9042d" -[[package]] -name = "spki" -version = "0.4.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "5c01a0c15da1b0b0e1494112e7af814a678fec9bd157881b49beac661e9b6f32" -dependencies = [ - "der 0.4.5", -] - [[package]] name = "ss58-registry" version = "1.17.0" diff --git a/engine/Cargo.toml b/engine/Cargo.toml index ca2776712a1..9f0a63f2aa4 100644 --- a/engine/Cargo.toml +++ b/engine/Cargo.toml @@ -47,7 +47,7 @@ lazy_static = "1.4" parking_lot = "0.12" regex = "1" reqwest = {version = "0.11.4", features = ["json"]} -secp256k1 = "0.20" +secp256k1 = {version = "0.20", features = ["serde", "rand-std", "global-context"]} serde = {version = "1.0", features = ["derive", "rc"]} serde_json = "1.0" sha2 = "0.9.5" @@ -58,6 +58,8 @@ thiserror = "1.0.26" tokio = {version = "1.13.1", features = ["full"]} tokio-stream = "0.1.5" url = "1.7.2" +num-bigint = "0.4" +num-traits = "0.2" web3 = {git = 'https://github.com/tomusdrw/rust-web3.git', rev = '8796c88', features = ['signing']} zeroize = "1.5.4" libp2p = "0.40" @@ -149,9 +151,6 @@ tag = 'chainflip-monthly-2022-05' git = 'https://github.com/chainflip-io/substrate.git' tag = 'chainflip-monthly-2022-05' -[dependencies.curv-kzen] -version = "0.9" - [dev-dependencies] env_logger = "0.9.0" jsonrpc-pubsub = "18.0.0" diff --git a/engine/src/multisig/client/keygen/keygen_frost.rs b/engine/src/multisig/client/keygen/keygen_frost.rs index 56b824bfb3b..a943cfbcf4b 100644 --- a/engine/src/multisig/client/keygen/keygen_frost.rs +++ b/engine/src/multisig/client/keygen/keygen_frost.rs @@ -101,7 +101,7 @@ fn generate_dkg_challenge( let x: [u8; 32] = result.as_slice().try_into().expect("Invalid hash size"); - P::Scalar::from_bytes(&x) + P::Scalar::from_bytes_mod_order(&x) } /// Generate ZKP (zero-knowledge proof) of `secret` diff --git a/engine/src/multisig/client/signing/frost.rs b/engine/src/multisig/client/signing/frost.rs index 8e449d1dddf..ab107e1c5f8 100644 --- a/engine/src/multisig/client/signing/frost.rs +++ b/engine/src/multisig/client/signing/frost.rs @@ -200,7 +200,7 @@ fn gen_rho_i( let x: [u8; 32] = result.as_slice().try_into().expect("Invalid hash size"); - P::Scalar::from_bytes(&x) + P::Scalar::from_bytes_mod_order(&x) } type SigningResponse

= LocalSig3

; diff --git a/engine/src/multisig/crypto.rs b/engine/src/multisig/crypto.rs index 5b3c5990cde..c8eca0b541a 100644 --- a/engine/src/multisig/crypto.rs +++ b/engine/src/multisig/crypto.rs @@ -7,7 +7,6 @@ pub mod secp255k1; use generic_array::{typenum::Unsigned, ArrayLength}; -pub use curv::{arithmetic::traits::Converter as BigIntConverter, BigInt}; use zeroize::{DefaultIsZeroes, ZeroizeOnDrop}; use std::fmt::Debug; @@ -61,6 +60,10 @@ pub trait ECPoint: } // Only relevant for ETH contract keys + // TODO: this is a property of a signing scheme + // rather than the underlying curve, so this + // should be moved to `CryptoScheme` before we + // Bitcoin or any other secp256k1 scheme fn is_compatible(&self) -> bool { true } @@ -117,7 +120,7 @@ pub trait ECScalar: { fn random(rng: &mut Rng) -> Self; - fn from_bytes(x: &[u8; 32]) -> Self; + fn from_bytes_mod_order(x: &[u8; 32]) -> Self; fn zero() -> Self; diff --git a/engine/src/multisig/crypto/curve25519_ristretto.rs b/engine/src/multisig/crypto/curve25519_ristretto.rs index 57e01c8fa50..74010c2f1ad 100644 --- a/engine/src/multisig/crypto/curve25519_ristretto.rs +++ b/engine/src/multisig/crypto/curve25519_ristretto.rs @@ -88,7 +88,7 @@ mod scalar_impls { Scalar(SK::from_bytes_mod_order_wide(&scalar_bytes)) } - fn from_bytes(x: &[u8; 32]) -> Self { + fn from_bytes_mod_order(x: &[u8; 32]) -> Self { Scalar(SK::from_bytes_mod_order(*x)) } diff --git a/engine/src/multisig/crypto/eth.rs b/engine/src/multisig/crypto/eth.rs index bdc54d1ddba..c2f3a0c1081 100644 --- a/engine/src/multisig/crypto/eth.rs +++ b/engine/src/multisig/crypto/eth.rs @@ -56,7 +56,7 @@ impl CryptoScheme for EthSigning { &pubkey_to_eth_addr(nonce_commitment.get_element()), ); - Scalar::from_bytes(&e) + Scalar::from_bytes_mod_order(&e) } fn build_response( diff --git a/engine/src/multisig/crypto/secp255k1.rs b/engine/src/multisig/crypto/secp255k1.rs index 6f05a7cb30c..932cd09bba9 100644 --- a/engine/src/multisig/crypto/secp255k1.rs +++ b/engine/src/multisig/crypto/secp255k1.rs @@ -1,34 +1,58 @@ -use generic_array::GenericArray; - use super::{ECPoint, ECScalar, Rng}; +use num_bigint::BigUint; +use secp256k1::constants::{CURVE_ORDER, SECRET_KEY_SIZE}; use serde::{Deserialize, Serialize}; -use curv::{ - arithmetic::Converter, - elliptic::curves::{ - secp256_k1::{Secp256k1Point, Secp256k1Scalar}, - ECPoint as CurvECPoint, ECScalar as CurvECScalar, - }, - BigInt, -}; +type SK = secp256k1::SecretKey; +type PK = secp256k1::PublicKey; + +// Wrapping in `Option` to make it easier to keep track +// of "zero" scalars which often need special treatment +#[derive(Clone, Debug, PartialEq)] +pub struct Scalar(Option); +// None if it is a "point at infinity" #[derive(Clone, Copy, Debug, PartialEq)] -pub struct Point(Secp256k1Point); +pub struct Point(Option); -#[derive(Clone, Debug, PartialEq)] -pub struct Scalar(Secp256k1Scalar); +const GENERATOR_COMPRESSED: [u8; 33] = [ + 0x02, 0x79, 0xBE, 0x66, 0x7E, 0xF9, 0xDC, 0xBB, 0xAC, 0x55, 0xA0, 0x62, 0x95, 0xCE, 0x87, 0x0B, + 0x07, 0x02, 0x9B, 0xFC, 0xDB, 0x2D, 0xCE, 0x28, 0xD9, 0x59, 0xF2, 0x81, 0x5B, 0x16, 0xF8, 0x17, + 0x98, +]; + +lazy_static::lazy_static! { + static ref GENERATOR: Point = Point(Some(PK::from_slice(&GENERATOR_COMPRESSED).unwrap())); + static ref GROUP_ORDER_BIG_UINT: BigUint = BigUint::from_bytes_be(&CURVE_ORDER); +} mod point_impls { use super::*; + const POINT_AT_INFINITY_COMPRESSED: [u8; 33] = [0; 33]; + const POINT_AT_INFINITY_UNCOMPRESSED: [u8; 65] = [0; 65]; + derive_point_impls!(Point, Scalar); impl> std::ops::Mul for Point { type Output = Self; - fn mul(self, rhs: B) -> Self::Output { - Point(self.0.scalar_mul(&rhs.borrow().0)) + fn mul(self, scalar: B) -> Self::Output { + let inner = match (self.0, scalar.borrow().0) { + (None, _) | (_, None) => { + // multiplication by 0 creates a "point at infinity" + None + } + (Some(mut point), Some(scalar)) => { + point + .mul_assign(secp256k1::SECP256K1, scalar.as_ref()) + .expect("scalar must be valid and non-zero"); + Some(point) + } + }; + + Point(inner) } } @@ -36,32 +60,54 @@ mod point_impls { type Output = Self; fn add(self, rhs: Self) -> Self::Output { - Point(self.0.add_point(&rhs.0)) + let inner = match (self.0, rhs.0) { + (None, rhs) => rhs, + (lhs, None) => lhs, + (Some(lhs), Some(rhs)) => { + // this can only fail if the result is + // a point at infinity which we represent + // with `None` + lhs.combine(&rhs).ok() + } + }; + Point(inner) } } impl std::ops::Sub for Point { type Output = Self; - fn sub(self, rhs: Self) -> Self::Output { - Point(self.0.sub_point(&rhs.0)) + // Silence clippy as addition is here by design + // (note that we negate the right operand first) + #[allow(clippy::suspicious_arithmetic_impl)] + fn sub(self, mut rhs: Self) -> Self::Output { + // Only negate if non-zero + if let Some(rhs) = rhs.0.as_mut() { + rhs.negate_assign(secp256k1::SECP256K1) + } + + self + rhs } } impl ECPoint for Point { type Scalar = Scalar; - type CompressedPointLength = ::CompressedPointLength; + type CompressedPointLength = typenum::U33; - fn from_scalar(scalar: &Scalar) -> Self { - Point(Secp256k1Point::generator().scalar_mul(&scalar.0)) + fn from_scalar(scalar: &Self::Scalar) -> Self { + *Self::generator() * scalar } - fn as_bytes(&self) -> GenericArray { - self.0.serialize_compressed() + fn as_bytes(&self) -> generic_array::GenericArray { + match self.0 { + Some(pk) => pk.serialize(), + None => POINT_AT_INFINITY_COMPRESSED, + } + .into() } fn point_at_infinity() -> Self { - Self(Secp256k1Point::zero()) + Point(None) } fn is_compatible(&self) -> bool { @@ -70,8 +116,8 @@ mod point_impls { let pk = self.get_element(); let pubkey = cf_chains::eth::AggKey::from_pubkey_compressed(pk.serialize()); - let x = BigInt::from_bytes(&pubkey.pub_key_x); - let half_order = BigInt::from_bytes(&secp256k1::constants::CURVE_ORDER) / 2 + 1; + let x = BigUint::from_bytes_be(&pubkey.pub_key_x); + let half_order = BigUint::from_bytes_be(&CURVE_ORDER) / 2u32 + 1u32; x < half_order } @@ -82,7 +128,7 @@ mod point_impls { where S: serde::Serializer, { - serializer.serialize_bytes(self.0.serialize_compressed().as_ref()) + serializer.serialize_bytes(&self.as_bytes()) } } @@ -93,20 +139,32 @@ mod point_impls { { let bytes = Vec::deserialize(deserializer)?; - Secp256k1Point::deserialize(&bytes) - .map(Point) - .map_err(serde::de::Error::custom) + // Check both compressed and uncompressed + // representations of zero (even though we + // only use compressed) + if bytes == POINT_AT_INFINITY_COMPRESSED || bytes == POINT_AT_INFINITY_UNCOMPRESSED { + Ok(Point::point_at_infinity()) + } else { + PK::from_slice(&bytes) + .map(|pk| Point(Some(pk))) + .map_err(serde::de::Error::custom) + } } } impl Point { + fn generator() -> &'static Point { + &GENERATOR + } + pub fn get_element(&self) -> secp256k1::PublicKey { - // TODO: ensure that we don't create points at infinity - // (we might want to sanitize p2p data) - self.0 - .underlying_ref() - .expect("unexpected point at infinity") - .0 + // We can be reasonably sure that the point is + // valid (i.e. not a point at infinity) as the + // method is only called on aggregate values and + // cannot be controlled by any single party (the + // chance of getting an invalid point by chance + // is negligible) + self.0.expect("unexpected point at infinity") } } @@ -125,17 +183,29 @@ mod scalar_impls { derive_scalar_impls!(Scalar); impl Scalar { - #[cfg(test)] - pub fn from_hex(sk_hex: &str) -> Self { - let bytes = hex::decode(sk_hex).expect("input must be hex encoded"); - - Scalar(Secp256k1Scalar::deserialize(&bytes).expect("input must represent a scalar")) + /// Expects `x` to be within the group, i.e. + /// it is smaller than the group's order + fn from_reduced_bigint(x: &BigUint) -> Self { + use num_traits::identities::Zero; + + assert!(x < &GROUP_ORDER_BIG_UINT, "x not within the group"); + + if x.is_zero() { + Scalar(None) + } else { + let x_bytes = x.to_bytes_be(); + let mut array = [0u8; SECRET_KEY_SIZE]; + array[SECRET_KEY_SIZE - x_bytes.len()..].copy_from_slice(&x_bytes); + + // Safe because `x` is within the group + // and `array` is correct size by construction + Scalar(Some(SK::from_slice(&array).unwrap())) + } } - pub fn as_bytes(&self) -> &[u8; 32] { - match self.0.underlying_ref() { - Some(secret_key) => secret_key.as_ref(), - // None represents "zero" scalar in `curv` + pub fn as_bytes(&self) -> &[u8; SECRET_KEY_SIZE] { + match self.0.as_ref() { + Some(sk) => sk.as_ref(), None => &[0; 32], } } @@ -143,48 +213,95 @@ mod scalar_impls { impl ECScalar for Scalar { fn random(rng: &mut Rng) -> Self { - use curv::elliptic::curves::secp256_k1::SK; - - Scalar(Secp256k1Scalar::from_underlying(Some(SK( - secp256k1::SecretKey::new(rng), - )))) + let sk = SK::new(rng); + // The key is guaranteed to be non-zero by + // the implementation of SK::new + Scalar(Some(sk)) } - fn from_bytes(x: &[u8; 32]) -> Self { - Scalar(CurvECScalar::from_bigint(&BigInt::from_bytes(x))) + fn from_bytes_mod_order(x: &[u8; 32]) -> Self { + // reduce `x` to make it a valid element in the group + let x = { + let mut x = BigUint::from_bytes_be(x); + + // Because the source is only 32 bytes, we know that + // it must be smaller than twice secp256k1's order, + // so a single subtraction is sufficient here + if x >= *GROUP_ORDER_BIG_UINT { + x -= &*GROUP_ORDER_BIG_UINT; + } + x + }; + + Self::from_reduced_bigint(&x) } fn zero() -> Self { - Scalar(Secp256k1Scalar::zero()) + Scalar(None) } + // Note that we don't need this to be constant-time as we + // only invert public values. fn invert(&self) -> Option { - self.0.invert().map(Scalar) + self.0.map(|x| { + let x = BigUint::from_bytes_be(x.as_ref()); + + let order = BigUint::from_bytes_be(&CURVE_ORDER); + + // Modular multiplicative inverse is equivalent to raising + // to the power of `order - 2` (using Euler's theorem; also + // see libsecp256k1 which uses a somewhat similar implementation: + // https://docs.rs/libsecp256k1-core/0.3.0/src/libsecp256k1_core/field.rs.html#1546) + let inverse = x.modpow(&(&order - 2u32), &order); + + Self::from_reduced_bigint(&inverse) + }) } } impl zeroize::Zeroize for Scalar { fn zeroize(&mut self) { - // Secp256k1Scalar doesn't expose a way to "zeroize" it apart from dropping, so have - // to do it manually (I think assigning a different value would be sufficient to drop - // and zeroize the value, but we are not 100% sure that it won't get optimised away). use core::sync::atomic; - unsafe { std::ptr::write_volatile(&mut self.0, Secp256k1Scalar::zero()) }; + unsafe { std::ptr::write_volatile(self, Scalar::zero()) }; atomic::compiler_fence(atomic::Ordering::SeqCst); } } impl From for Scalar { fn from(x: u32) -> Self { - Scalar(CurvECScalar::from_bigint(&BigInt::from(x))) + if x == 0 { + Scalar(None) + } else { + let mut array = [0u8; 32]; + array[28..].copy_from_slice(&x.to_be_bytes()); + + // Since `x` is u32, we know it to be within + // the curve order, and the slice is 32 bytes + // by construction, so this cannot fail + Scalar(Some(SK::from_slice(&array).unwrap())) + } } } impl std::ops::Sub for &Scalar { type Output = Scalar; + // Silence clippy as addition is here by design + // (note that we negate the right operand first) + #[allow(clippy::suspicious_arithmetic_impl)] fn sub(self, rhs: Self) -> Self::Output { - Scalar(self.0.sub(&rhs.0)) + // according to https://github.com/bitcoin-core/secp256k1/blob/44c2452fd387f7ca604ab42d73746e7d3a44d8a2/include/secp256k1.h#L649 + // `negate_assign` expects a valid non-zero scalar + + match rhs.0 { + None => Scalar(None), + Some(mut x) => { + // it is safe to negate non-zero Scalar + x.negate_assign(); + + self + &Scalar(Some(x)) + } + } } } @@ -192,7 +309,17 @@ mod scalar_impls { type Output = Scalar; fn mul(self, rhs: Self) -> Self::Output { - Scalar(self.0.mul(&rhs.0)) + let inner = match (self.0, rhs.0) { + (None, _) | (_, None) => None, + (Some(mut lhs), Some(rhs)) => { + lhs.mul_assign(rhs.as_ref()) + .expect("can't fail if both operands are valid"); + // implementation of mul_assign never returns + // a zero scalar + Some(lhs) + } + }; + Scalar(inner) } } @@ -200,16 +327,35 @@ mod scalar_impls { type Output = Scalar; fn add(self, rhs: Self) -> Self::Output { - Scalar(self.0.add(&rhs.0)) + let inner = match (self.0, rhs.0) { + (None, None) => None, + (None, Some(rhs)) => Some(rhs), + (Some(lhs), None) => Some(lhs), + (Some(mut lhs), Some(rhs)) => { + // Both lhs and rhs are considered "valid" (i.e. + // non-zero and belong to the group). Further, + // the addition is done modulo group order, so + // this function can only fail if the result + // itself is zero + lhs.add_assign(rhs.as_ref()).ok().map(|_| lhs) + } + }; + + Scalar(inner) } } + const ZERO_SCALAR_BYTES: [u8; 32] = [0; 32]; + impl Serialize for Scalar { fn serialize(&self, serializer: S) -> Result where S: serde::Serializer, { - serializer.serialize_bytes(self.0.serialize().as_ref()) + match self.0 { + Some(x) => serializer.serialize_bytes(x.as_ref()), + None => serializer.serialize_bytes(&ZERO_SCALAR_BYTES), + } } } @@ -219,20 +365,22 @@ mod scalar_impls { D: serde::Deserializer<'de>, { let bytes = Vec::deserialize(deserializer)?; - - Secp256k1Scalar::deserialize(&bytes) - .map(Scalar) - .map_err(serde::de::Error::custom) + if bytes == ZERO_SCALAR_BYTES { + Ok(Scalar::zero()) + } else { + SK::from_slice(&bytes) + .map(|x| Scalar(Some(x))) + .map_err(serde::de::Error::custom) + } } } -} -#[test] -fn sanity_check_point_at_infinity() { - // Sanity check: point at infinity should correspond - // to "zero" on the elliptic curve - assert_eq!( - Point::point_at_infinity(), - Point::from_scalar(&Scalar::zero()) - ); + #[cfg(test)] + impl Scalar { + pub fn from_hex(sk_hex: &str) -> Self { + let bytes = hex::decode(sk_hex).expect("input must be hex encoded"); + // `from_slice` never returns 0 + Scalar(Some(SK::from_slice(&bytes).expect("invalid scalar"))) + } + } } From 1bba639f38f035799c787d58a3ac059e66876a5e Mon Sep 17 00:00:00 2001 From: Alastair Holmes Date: Wed, 22 Jun 2022 11:10:51 +0200 Subject: [PATCH 3/6] chore: correct file name --- engine/src/multisig/crypto.rs | 2 +- engine/src/multisig/crypto/eth.rs | 2 +- engine/src/multisig/crypto/{secp255k1.rs => secp256k1.rs} | 0 3 files changed, 2 insertions(+), 2 deletions(-) rename engine/src/multisig/crypto/{secp255k1.rs => secp256k1.rs} (100%) diff --git a/engine/src/multisig/crypto.rs b/engine/src/multisig/crypto.rs index c8eca0b541a..8d561800d09 100644 --- a/engine/src/multisig/crypto.rs +++ b/engine/src/multisig/crypto.rs @@ -3,7 +3,7 @@ mod helpers; pub mod curve25519_ristretto; pub mod eth; pub mod polkadot; -pub mod secp255k1; +pub mod secp256k1; use generic_array::{typenum::Unsigned, ArrayLength}; diff --git a/engine/src/multisig/crypto/eth.rs b/engine/src/multisig/crypto/eth.rs index c2f3a0c1081..a6cd8c48d03 100644 --- a/engine/src/multisig/crypto/eth.rs +++ b/engine/src/multisig/crypto/eth.rs @@ -6,7 +6,7 @@ use super::{CryptoScheme, ECPoint}; // clear that these a the primitives used by ethereum. // TODO: we probably want to change the "clients" to // solely use "CryptoScheme" as generic parameter instead. -pub use super::secp255k1::{Point, Scalar}; +pub use super::secp256k1::{Point, Scalar}; use serde::{Deserialize, Serialize}; #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] diff --git a/engine/src/multisig/crypto/secp255k1.rs b/engine/src/multisig/crypto/secp256k1.rs similarity index 100% rename from engine/src/multisig/crypto/secp255k1.rs rename to engine/src/multisig/crypto/secp256k1.rs From a2a6b96ed8fcc613c02df94fe7e92294bdd1ed32 Mon Sep 17 00:00:00 2001 From: Maxim Shishmarev Date: Fri, 24 Jun 2022 10:45:08 +1000 Subject: [PATCH 4/6] add missing word in comment --- engine/src/multisig/crypto.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/engine/src/multisig/crypto.rs b/engine/src/multisig/crypto.rs index 8d561800d09..8634202ce34 100644 --- a/engine/src/multisig/crypto.rs +++ b/engine/src/multisig/crypto.rs @@ -63,7 +63,7 @@ pub trait ECPoint: // TODO: this is a property of a signing scheme // rather than the underlying curve, so this // should be moved to `CryptoScheme` before we - // Bitcoin or any other secp256k1 scheme + // add Bitcoin or any other secp256k1 scheme fn is_compatible(&self) -> bool { true } From be6a8fae6753160b230e8af3aff16946dabc9520 Mon Sep 17 00:00:00 2001 From: Maxim Shishmarev Date: Fri, 24 Jun 2022 11:05:36 +1000 Subject: [PATCH 5/6] fix zero substraction --- engine/src/multisig/crypto/secp256k1.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/engine/src/multisig/crypto/secp256k1.rs b/engine/src/multisig/crypto/secp256k1.rs index 932cd09bba9..1ea9fe9226a 100644 --- a/engine/src/multisig/crypto/secp256k1.rs +++ b/engine/src/multisig/crypto/secp256k1.rs @@ -294,7 +294,7 @@ mod scalar_impls { // `negate_assign` expects a valid non-zero scalar match rhs.0 { - None => Scalar(None), + None => self.clone(), Some(mut x) => { // it is safe to negate non-zero Scalar x.negate_assign(); From d7e979d030c61cd3f1e4cc09021cd07f60ecae66 Mon Sep 17 00:00:00 2001 From: Maxim Shishmarev Date: Fri, 24 Jun 2022 13:32:01 +1000 Subject: [PATCH 6/6] address minor comments --- engine/src/multisig/crypto/secp256k1.rs | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/engine/src/multisig/crypto/secp256k1.rs b/engine/src/multisig/crypto/secp256k1.rs index 1ea9fe9226a..e7f60950ccf 100644 --- a/engine/src/multisig/crypto/secp256k1.rs +++ b/engine/src/multisig/crypto/secp256k1.rs @@ -206,7 +206,7 @@ mod scalar_impls { pub fn as_bytes(&self) -> &[u8; SECRET_KEY_SIZE] { match self.0.as_ref() { Some(sk) => sk.as_ref(), - None => &[0; 32], + None => &ZERO_SCALAR_BYTES, } } } @@ -249,7 +249,7 @@ mod scalar_impls { let order = BigUint::from_bytes_be(&CURVE_ORDER); // Modular multiplicative inverse is equivalent to raising - // to the power of `order - 2` (using Euler's theorem; also + // to the power of `order - 2` if the order is prime (using Euler's theorem; also // see libsecp256k1 which uses a somewhat similar implementation: // https://docs.rs/libsecp256k1-core/0.3.0/src/libsecp256k1_core/field.rs.html#1546) let inverse = x.modpow(&(&order - 2u32), &order); @@ -328,9 +328,8 @@ mod scalar_impls { fn add(self, rhs: Self) -> Self::Output { let inner = match (self.0, rhs.0) { - (None, None) => None, - (None, Some(rhs)) => Some(rhs), - (Some(lhs), None) => Some(lhs), + (None, rhs) => rhs, + (lhs, None) => lhs, (Some(mut lhs), Some(rhs)) => { // Both lhs and rhs are considered "valid" (i.e. // non-zero and belong to the group). Further,