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/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()

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();

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"] }

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 {

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 = <Secp256k1 as CurveArithmetic>::AffinePoint;
 
-pub trait ScalarExt {
-    fn from_bytes(bytes: &[u8]) -> Self;
+pub trait ScalarExt: Sized {
+    fn from_bytes(bytes: [u8; 32]) -> Option<Self>;
+    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<Self> {
+        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<Scalar> for SerializableScalar {
+    fn from(scalar: Scalar) -> Self {
+        SerializableScalar { scalar }
+    }
+}
+
 impl BorshSerialize for SerializableScalar {
     fn serialize<W: std::io::prelude::Write>(&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<R: std::io::prelude::Read>(reader: &mut R) -> std::io::Result<Self> {
         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() {

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,

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

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;