From e4a081a90c8f432e8819691ea17e0bc725e78794 Mon Sep 17 00:00:00 2001 From: David Millar-Durrant Date: Fri, 19 Jul 2024 18:44:43 +0200 Subject: [PATCH] Made to_bytes return an option --- chain-signatures/Cargo.lock | 2 +- chain-signatures/contract/src/lib.rs | 18 +++++--- chain-signatures/crypto-shared/Cargo.toml | 1 + chain-signatures/crypto-shared/src/kdf.rs | 3 +- chain-signatures/crypto-shared/src/types.rs | 45 ++++++++++++++++--- chain-signatures/node/src/indexer.rs | 28 +++++++++--- chain-signatures/node/src/kdf.rs | 2 +- .../node/src/protocol/signature.rs | 10 ++--- integration-tests/chain-signatures/Cargo.lock | 6 +-- .../chain-signatures/tests/actions/mod.rs | 18 ++++---- .../chain-signatures/tests/cases/mod.rs | 2 +- 11 files changed, 97 insertions(+), 38 deletions(-) diff --git a/chain-signatures/Cargo.lock b/chain-signatures/Cargo.lock index 2717cadc3..74c78e429 100644 --- a/chain-signatures/Cargo.lock +++ b/chain-signatures/Cargo.lock @@ -1753,7 +1753,7 @@ dependencies = [ "near-sdk", "serde", "serde_json", - "sha3", + "subtle", ] [[package]] diff --git a/chain-signatures/contract/src/lib.rs b/chain-signatures/contract/src/lib.rs index f71cc5b6f..69a647fb1 100644 --- a/chain-signatures/contract/src/lib.rs +++ b/chain-signatures/contract/src/lib.rs @@ -4,6 +4,7 @@ use crypto_shared::{ derive_epsilon, derive_key, kdf::check_ec_signature, near_public_key_to_affine_point, types::SignatureResponse, ScalarExt as _, SerializableScalar, }; +use k256::Scalar; use near_sdk::borsh::{self, BorshDeserialize, BorshSerialize}; use near_sdk::collections::LookupMap; use near_sdk::serde::{Deserialize, Serialize}; @@ -98,13 +99,16 @@ pub struct YieldIndex { #[borsh(crate = "near_sdk::borsh")] pub struct SignatureRequest { pub epsilon: SerializableScalar, - pub payload_hash: [u8; 32], + pub payload_hash: SerializableScalar, } impl SignatureRequest { - pub fn new(payload_hash: [u8; 32], predecessor_id: &AccountId, path: &str) -> Self { - let scalar = derive_epsilon(predecessor_id, path); - let epsilon = SerializableScalar { scalar }; + pub fn new(payload_hash: Scalar, predecessor_id: &AccountId, path: &str) -> Self { + let epsilon = derive_epsilon(predecessor_id, path); + let epsilon = SerializableScalar { scalar: epsilon }; + let payload_hash = SerializableScalar { + scalar: payload_hash, + }; SignatureRequest { epsilon, payload_hash, @@ -167,6 +171,9 @@ impl VersionedMpcContract { key_version, } = request; let latest_key_version: u32 = self.latest_key_version(); + // It's important we fail here because the MPC nodes will fail in an identical way. + // This allows users to get the error message + let payload = Scalar::from_bytes(payload).expect("Payload hash is bad"); assert!( key_version <= latest_key_version, "This version of the signer contract doesn't support versions greater than {}", @@ -280,6 +287,7 @@ impl VersionedMpcContract { pub fn respond(&mut self, request: SignatureRequest, response: SignatureResponse) { let protocol_state = self.mutable_state(); + if let ProtocolContractState::Running(_) = protocol_state { let signer = env::signer_account_id(); log!( @@ -301,7 +309,7 @@ impl VersionedMpcContract { &expected_public_key, &response.big_r.affine_point, &response.s.scalar, - k256::Scalar::from_bytes(&request.payload_hash[..]), + request.payload_hash.scalar, response.recovery_id, ) .is_err() diff --git a/chain-signatures/crypto-shared/Cargo.toml b/chain-signatures/crypto-shared/Cargo.toml index 4891be579..60017ff0c 100644 --- a/chain-signatures/crypto-shared/Cargo.toml +++ b/chain-signatures/crypto-shared/Cargo.toml @@ -18,6 +18,7 @@ near-account-id = "1" serde_json = "1" near-sdk = { version = "5.2.1", features = ["unstable"] } sha3 = "0.10.8" +subtle = "2.6.1" [target.'cfg(target_arch = "wasm32")'.dependencies] getrandom = { version = "0.2.12", features = ["custom"] } diff --git a/chain-signatures/crypto-shared/src/kdf.rs b/chain-signatures/crypto-shared/src/kdf.rs index 27590b3bb..67e015388 100644 --- a/chain-signatures/crypto-shared/src/kdf.rs +++ b/chain-signatures/crypto-shared/src/kdf.rs @@ -22,7 +22,8 @@ pub fn derive_epsilon(predecessor_id: &AccountId, path: &str) -> Scalar { let derivation_path = format!("{EPSILON_DERIVATION_PREFIX}{},{}", predecessor_id, path); let mut hasher = Sha3_256::new(); hasher.update(derivation_path); - Scalar::from_bytes(&hasher.finalize()) + let hash: [u8; 32] = hasher.finalize().into(); + Scalar::from_non_biased(hash) } pub fn derive_key(public_key: PublicKey, epsilon: Scalar) -> PublicKey { diff --git a/chain-signatures/crypto-shared/src/types.rs b/chain-signatures/crypto-shared/src/types.rs index 3b930b7e2..a05b96e5d 100644 --- a/chain-signatures/crypto-shared/src/types.rs +++ b/chain-signatures/crypto-shared/src/types.rs @@ -7,23 +7,51 @@ use serde::{Deserialize, Serialize}; pub type PublicKey = ::AffinePoint; -pub trait ScalarExt { - fn from_bytes(bytes: &[u8]) -> Self; +pub trait ScalarExt: Sized { + fn from_bytes(bytes: [u8; 32]) -> Option; + fn from_non_biased(bytes: [u8; 32]) -> Self; } impl ScalarExt for Scalar { - fn from_bytes(bytes: &[u8]) -> Self { - let bytes = U256::from_be_slice(bytes); - Scalar::from_repr(bytes.to_be_byte_array()).expect("Byte conversion to scalar failed") + /// Returns nothing if the bytes are greater than the field size of Secp256k1. + /// This will be very rare with random bytes as the field size is 2^256 - 2^32 - 2^9 - 2^8 - 2^7 - 2^6 - 2^4 - 1 + fn from_bytes(bytes: [u8; 32]) -> Option { + let bytes = U256::from_be_slice(bytes.as_slice()); + Scalar::from_repr(bytes.to_be_byte_array()).into_option() + } + + /// When the user can't directly select the value, this will always work + /// Use cases are things that we know have been hashed + fn from_non_biased(hash: [u8; 32]) -> Self { + // This should never happen. + // The space of inputs is 2^256, the space of the field is ~2^256 - 2^32. + // This mean that you'd have to run 2^224 hashes to find a value that causes this to fail. + Scalar::from_bytes(hash).expect("Derived epsilon value falls outside of the field") } } +#[test] +fn scalar_fails_as_expected() { + let too_high = [0xFF; 32]; + assert!(Scalar::from_bytes(too_high).is_none()); + + let mut not_too_high = [0xFF; 32]; + not_too_high[27] = 0xFD; + assert!(Scalar::from_bytes(not_too_high).is_some()); +} + // Is there a better way to force a borsh serialization? #[derive(Debug, PartialEq, Eq, Serialize, Deserialize, Clone, Copy)] pub struct SerializableScalar { pub scalar: Scalar, } +impl From for SerializableScalar { + fn from(scalar: Scalar) -> Self { + SerializableScalar { scalar } + } +} + impl BorshSerialize for SerializableScalar { fn serialize(&self, writer: &mut W) -> std::io::Result<()> { let to_ser: [u8; 32] = self.scalar.to_bytes().into(); @@ -34,7 +62,10 @@ impl BorshSerialize for SerializableScalar { impl BorshDeserialize for SerializableScalar { fn deserialize_reader(reader: &mut R) -> std::io::Result { let from_ser: [u8; 32] = BorshDeserialize::deserialize_reader(reader)?; - let scalar = Scalar::from_bytes(&from_ser[..]); + let scalar = Scalar::from_bytes(from_ser).ok_or(std::io::Error::new( + std::io::ErrorKind::InvalidData, + "Scalar bytes are not in the k256 field", + ))?; Ok(SerializableScalar { scalar }) } } @@ -67,7 +98,7 @@ fn serializeable_scalar_roundtrip() { Scalar::ZERO, Scalar::ONE, Scalar::from_u128(u128::MAX), - Scalar::from_bytes(&[3; 32]), + Scalar::from_bytes([3; 32]).unwrap(), ]; for scalar in test_vec.into_iter() { diff --git a/chain-signatures/node/src/indexer.rs b/chain-signatures/node/src/indexer.rs index bb5db968d..82128a442 100644 --- a/chain-signatures/node/src/indexer.rs +++ b/chain-signatures/node/src/indexer.rs @@ -2,7 +2,9 @@ use crate::gcp::GcpService; use crate::kdf; use crate::protocol::{SignQueue, SignRequest}; use crate::types::LatestBlockHeight; -use crypto_shared::derive_epsilon; +use anyhow::Context as _; +use crypto_shared::{derive_epsilon, ScalarExt}; +use k256::Scalar; use near_account_id::AccountId; use near_lake_framework::{LakeBuilder, LakeContext}; use near_lake_primitives::actions::ActionMetaDataExt; @@ -67,17 +69,26 @@ impl Options { } #[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Eq)] -pub struct SignArguments { - pub request: ContractSignRequest, +struct SignArguments { + request: UnvalidatedContractSignRequest, } +/// What is recieved when sign is called #[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Eq)] -pub struct ContractSignRequest { +struct UnvalidatedContractSignRequest { pub payload: [u8; 32], pub path: String, pub key_version: u32, } +/// A validated version of the sign request +#[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Eq)] +pub struct ContractSignRequest { + pub payload: Scalar, + pub path: String, + pub key_version: u32, +} + #[derive(LakeContext)] struct Context { mpc_contract_id: AccountId, @@ -113,6 +124,8 @@ async fn handle_block( tracing::warn!("`sign` did not produce entropy"); continue; } + let payload = Scalar::from_bytes(arguments.request.payload) + .context("Payload cannot be converted to scalar, not in k256 field")?; let Ok(entropy) = serde_json::from_str::<'_, [u8; 32]>(&receipt.logs()[1]) else { tracing::warn!( @@ -133,9 +146,14 @@ async fn handle_block( entropy = hex::encode(entropy), "indexed new `sign` function call" ); + let request = ContractSignRequest { + payload, + path: arguments.request.path, + key_version: arguments.request.key_version, + }; pending_requests.push(SignRequest { receipt_id, - request: arguments.request, + request, epsilon, delta, entropy, diff --git a/chain-signatures/node/src/kdf.rs b/chain-signatures/node/src/kdf.rs index 421651b40..84a9f5064 100644 --- a/chain-signatures/node/src/kdf.rs +++ b/chain-signatures/node/src/kdf.rs @@ -13,7 +13,7 @@ pub fn derive_delta(receipt_id: CryptoHash, entropy: [u8; 32]) -> Scalar { let info = format!("{DELTA_DERIVATION_PREFIX}:{}", receipt_id); let mut okm = [0u8; 32]; hk.expand(info.as_bytes(), &mut okm).unwrap(); - Scalar::from_bytes(&okm) + Scalar::from_non_biased(okm) } // Constant prefix that ensures delta derivation values are used specifically for diff --git a/chain-signatures/node/src/protocol/signature.rs b/chain-signatures/node/src/protocol/signature.rs index 91b1f6944..c0f09df1b 100644 --- a/chain-signatures/node/src/protocol/signature.rs +++ b/chain-signatures/node/src/protocol/signature.rs @@ -9,8 +9,8 @@ use crate::util::AffinePointExt; use cait_sith::protocol::{Action, InitializationError, Participant, ProtocolError}; use cait_sith::{FullSignature, PresignOutput}; use chrono::Utc; +use crypto_shared::SerializableScalar; use crypto_shared::{derive_key, PublicKey}; -use crypto_shared::{ScalarExt, SerializableScalar}; use k256::{Scalar, Secp256k1}; use mpc_contract::SignatureRequest; use rand::rngs::StdRng; @@ -58,7 +58,7 @@ impl SignQueue { pub fn add(&mut self, request: SignRequest) { tracing::info!( receipt_id = %request.receipt_id, - payload = hex::encode(request.request.payload), + payload = hex::encode(request.request.payload.to_bytes()), entropy = hex::encode(request.entropy), "new sign request" ); @@ -263,7 +263,7 @@ impl SignatureManager { me, derive_key(public_key, epsilon), output, - Scalar::from_bytes(&request.payload), + request.payload, )?); Ok(SignatureGenerator::new( protocol, @@ -470,7 +470,7 @@ impl SignatureManager { self.completed.insert(generator.presignature_id, Instant::now()); let request = SignatureRequest { epsilon: SerializableScalar {scalar: generator.epsilon}, - payload_hash: generator.request.payload, + payload_hash: generator.request.payload.into(), }; if generator.proposer == self.me { self.signatures @@ -585,7 +585,7 @@ impl SignatureManager { &expected_public_key, &signature.big_r, &signature.s, - Scalar::from_bytes(&request.payload_hash), + request.payload_hash.scalar, ) else { tracing::error!(%receipt_id, "Failed to generate a recovery ID"); continue; diff --git a/integration-tests/chain-signatures/Cargo.lock b/integration-tests/chain-signatures/Cargo.lock index 3dbbbdb4d..ed62c1e6f 100644 --- a/integration-tests/chain-signatures/Cargo.lock +++ b/integration-tests/chain-signatures/Cargo.lock @@ -1837,7 +1837,7 @@ dependencies = [ "near-sdk", "serde", "serde_json", - "sha3", + "subtle", ] [[package]] @@ -7688,9 +7688,9 @@ dependencies = [ [[package]] name = "subtle" -version = "2.5.0" +version = "2.6.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "81cdd64d312baedb58e21336b31bc043b77e01cc99033ce76ef539f78e965ebc" +checksum = "13c2bddecc57b384dee18652358fb23172facb8a2c51ccc10d74c157bdea3292" [[package]] name = "symbolic-common" diff --git a/integration-tests/chain-signatures/tests/actions/mod.rs b/integration-tests/chain-signatures/tests/actions/mod.rs index 9a0ee4545..8087e1274 100644 --- a/integration-tests/chain-signatures/tests/actions/mod.rs +++ b/integration-tests/chain-signatures/tests/actions/mod.rs @@ -88,14 +88,14 @@ pub async fn request_sign( pub async fn assert_signature( account_id: &near_workspaces::AccountId, mpc_pk_bytes: &[u8], - payload: &[u8; 32], + payload: [u8; 32], signature: &FullSignature, ) { let mpc_point = EncodedPoint::from_bytes(mpc_pk_bytes).unwrap(); let mpc_pk = AffinePoint::from_encoded_point(&mpc_point).unwrap(); let epsilon = derive_epsilon(account_id, "test"); let user_pk = derive_key(mpc_pk, epsilon); - assert!(signature.verify(&user_pk, &Scalar::from_bytes(payload),)); + assert!(signature.verify(&user_pk, &Scalar::from_bytes(payload).unwrap(),)); } // A normal signature, but we try to insert a bad response which fails and the signature is generated @@ -129,7 +129,7 @@ pub async fn single_signature_rogue_responder( // hex::encode(&payload_hash), // account.id(), // ); - assert_signature(account.id(), &mpc_pk_bytes, &payload_hash, &signature).await; + assert_signature(account.id(), &mpc_pk_bytes, payload_hash, &signature).await; Ok(()) } @@ -143,7 +143,7 @@ pub async fn single_signature_production( let mut mpc_pk_bytes = vec![0x04]; mpc_pk_bytes.extend_from_slice(&state.public_key.as_bytes()[1..]); - assert_signature(account.id(), &mpc_pk_bytes, &payload_hash, &signature).await; + assert_signature(account.id(), &mpc_pk_bytes, payload_hash, &signature).await; Ok(()) } @@ -169,7 +169,7 @@ pub async fn rogue_respond( let epsilon = derive_epsilon(predecessor, path); let request = SignatureRequest { - payload_hash, + payload_hash: Scalar::from_bytes(payload_hash).unwrap().into(), epsilon: SerializableScalar { scalar: epsilon }, }; @@ -294,7 +294,7 @@ pub async fn single_payload_signature_production( assert_signature( account.clone().id(), &mpc_pk_bytes, - &payload_hash, + payload_hash, &signature, ) .await; @@ -370,7 +370,7 @@ async fn signatures_havent_changed() { .try_into() .unwrap(); - let payload_hash_scalar = Scalar::from_bytes(&payload_hash); + let payload_hash_scalar = Scalar::from_bytes(payload_hash).unwrap(); // Derive and convert user pk let mpc_pk = hex::decode(mpc_key).unwrap(); @@ -398,8 +398,8 @@ async fn signatures_havent_changed() { let big_r_y_parity = big_r.y_is_odd().unwrap_u8() as i32; assert!(big_r_y_parity == 0 || big_r_y_parity == 1); - let s = hex::decode(s).unwrap(); - let s = k256::Scalar::from_bytes(s.as_slice()); + let s = hex::decode(s).unwrap().try_into().unwrap(); + let s = k256::Scalar::from_bytes(s).unwrap(); let r = x_coordinate::(&big_r); let signature = cait_sith::FullSignature:: { big_r, s }; diff --git a/integration-tests/chain-signatures/tests/cases/mod.rs b/integration-tests/chain-signatures/tests/cases/mod.rs index 280890aed..917c6cae0 100644 --- a/integration-tests/chain-signatures/tests/cases/mod.rs +++ b/integration-tests/chain-signatures/tests/cases/mod.rs @@ -124,7 +124,7 @@ async fn test_key_derivation() -> anyhow::Result<()> { &user_pk, &sig.big_r, &sig.s, - k256::Scalar::from_bytes(&payload_hashed), + k256::Scalar::from_bytes(payload_hashed).unwrap(), ) .unwrap();