From baa5c2922bd96e1e0a8073845fdba5e2973c3ee7 Mon Sep 17 00:00:00 2001 From: DavidM-D Date: Fri, 19 Jul 2024 21:34:38 +0200 Subject: [PATCH] Fail on bad scalar conversions (#703) * Fail on bad scalar conversions Previously we'd allow bad scalars to be converted too and from bytes. This should be impossible since all conversions are done internally, but it's good to check. * Remove TODO * Made to_bytes return an option * Fixed test * Updated Cargo.lock --- chain-signatures/Cargo.lock | 2 +- chain-signatures/contract/src/lib.rs | 18 +++++-- chain-signatures/contract/tests/tests.rs | 9 ++-- chain-signatures/crypto-shared/Cargo.toml | 1 + chain-signatures/crypto-shared/src/kdf.rs | 3 +- chain-signatures/crypto-shared/src/types.rs | 47 +++++++++++++++---- 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 | 5 +- .../chain-signatures/tests/actions/mod.rs | 18 +++---- .../chain-signatures/tests/cases/mod.rs | 2 +- 12 files changed, 103 insertions(+), 42 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/contract/tests/tests.rs b/chain-signatures/contract/tests/tests.rs index 187c42a0d..49a10fe0e 100644 --- a/chain-signatures/contract/tests/tests.rs +++ b/chain-signatures/contract/tests/tests.rs @@ -1,12 +1,13 @@ use crypto_shared::kdf::{check_ec_signature, derive_secret_key}; use crypto_shared::{ - derive_epsilon, derive_key, SerializableAffinePoint, SerializableScalar, SignatureResponse, + derive_epsilon, derive_key, ScalarExt as _, SerializableAffinePoint, SerializableScalar, + SignatureResponse, }; use ecdsa::signature::Verifier; use k256::elliptic_curve::ops::Reduce; use k256::elliptic_curve::point::DecompressPoint; use k256::elliptic_curve::sec1::ToEncodedPoint; -use k256::{AffinePoint, FieldBytes, Secp256k1}; +use k256::{AffinePoint, FieldBytes, Scalar, Secp256k1}; use mpc_contract::primitives::{CandidateInfo, ParticipantInfo, SignRequest}; use mpc_contract::SignatureRequest; use near_sdk::NearToken; @@ -135,8 +136,8 @@ async fn create_response( let s = signature.s(); let (r_bytes, _s_bytes) = signature.split_bytes(); - - let respond_req = SignatureRequest::new(payload_hash, predecessor_id, path); + let payload_hash_s = Scalar::from_bytes(payload_hash).unwrap(); + let respond_req = SignatureRequest::new(payload_hash_s, predecessor_id, path); let big_r = AffinePoint::decompress(&r_bytes, k256::elliptic_curve::subtle::Choice::from(0)).unwrap(); let s: k256::Scalar = *s.as_ref(); 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 d4898eb47..a05b96e5d 100644 --- a/chain-signatures/crypto-shared/src/types.rs +++ b/chain-signatures/crypto-shared/src/types.rs @@ -1,29 +1,57 @@ use borsh::{BorshDeserialize, BorshSerialize}; use k256::{ - elliptic_curve::{scalar::FromUintUnchecked, CurveArithmetic}, + elliptic_curve::{bigint::ArrayEncoding, CurveArithmetic, PrimeField}, AffinePoint, Scalar, Secp256k1, U256, }; 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; } -// TODO prevent bad scalars from beind sent impl ScalarExt for Scalar { - fn from_bytes(bytes: &[u8]) -> Self { - Scalar::from_uint_unchecked(U256::from_be_slice(bytes)) + /// 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..be47ad50c 100644 --- a/integration-tests/chain-signatures/Cargo.lock +++ b/integration-tests/chain-signatures/Cargo.lock @@ -1838,6 +1838,7 @@ dependencies = [ "serde", "serde_json", "sha3", + "subtle", ] [[package]] @@ -7688,9 +7689,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();