diff --git a/ai-docs/ARCHITECTURE.md b/ai-docs/ARCHITECTURE.md index ea5899101..97734e693 100644 --- a/ai-docs/ARCHITECTURE.md +++ b/ai-docs/ARCHITECTURE.md @@ -603,8 +603,9 @@ What it verifies, and how failures are treated: | Check | On failure | |---|---| | Published keysets and CRSes are present, and their raw stored bytes hash to the digests in `KeyGenMetadata` / `CrsGenMetadata` | repaired from peers and verified again when possible (threshold only); otherwise boot fails | -| Current private keygen and CRS metadata with a stored domain reconstruct a valid EIP-712 signature from the node's signing key | boot fails | -| Every non-ECDSA entry of the per-scheme `signatures` in current private keygen and CRS metadata verifies, under the key the node derives for that scheme, over the rebuilt result payload prefixed by the schemes the stored entries name | boot fails | +| Current private keygen and CRS metadata with a stored domain pass `verify_response_signatures`, the check a client runs on a result, as if the client had requested the schemes the stored entries name and knew only the node's own keys | boot fails | +| Current private keygen and CRS metadata without a stored domain carry no entry beyond ECDSA, since they predate per-scheme signatures; nothing else of them can be checked | boot fails | +| Current private keygen and CRS metadata list their per-scheme `signatures` in canonical order without repeats, and carry an `external_signature` when they carry a domain | boot fails | | `VerfKey` and `VerfAddress` at `SIGNING_KEY_ID` match the key derived from the private `SigningKey` | boot fails | | Every entry in a `PubDataType` folder is accounted for by private storage or by a fixed-ID convention | error logged, boot continues | | Every top-level name in public storage is a `PubDataType` folder, and every folder can be listed | error logged, boot continues | @@ -648,8 +649,9 @@ Legacy metadata has no digest, so its public objects receive a raw presence chec `external_signature` and the ECDSA entry of `signatures` sign an EIP-712 hash built from an `Eip712Domain` that arrives from a gRPC request. At boot, current private keygen and CRS metadata with a stored domain reconstruct their signed Solidity payload and must recover the node's -signing address. Older metadata versions upgrade with no domain and stay unverifiable. The -entries of the other schemes sign the serialized result payload inside a composite preimage, +signing address, and `external_signature` must equal the ECDSA entry, as a client requires. +Older metadata versions upgrade with no domain and stay unverifiable. The +entries of the other schemes sign the serialized result payload inside a composite preimage, which binds the canonical scheme set and payload type. A node that cannot derive a scheme's key, because it holds no root seed, fails boot on such an entry rather than passing it over. diff --git a/core-client/src/decrypt.rs b/core-client/src/decrypt.rs index 8876a6794..c48a0e6af 100644 --- a/core-client/src/decrypt.rs +++ b/core-client/src/decrypt.rs @@ -2177,10 +2177,16 @@ fn verify_public_decrypt_responses( kms_addrs: &[alloy_primitives::Address], num_expected_responses: usize, ) -> anyhow::Result<()> { - // Resolve the verification material into (domain, external handles, extra_data) plus the - // optional original request used for the internal request-binding check. - let (domain, external_handles, extra_data, request) = match verification { + // Resolve the verification material into (domain, external handles, extra_data). + let (domain, external_handles, extra_data) = match verification { PubDecVerificationMaterial::Request(decryption_request) => { + // With the request at hand, the responses are also validated against it: + // bound to it, and accepted by majority. + internal_client.process_decryption_resp( + &decryption_request, + num_expected_responses as u32, + resp_response_vec, + )?; let domain_msg = decryption_request .domain .as_ref() @@ -2192,19 +2198,13 @@ fn verify_public_decrypt_responses( .iter() .map(|ct| ct.external_handle.clone()) .collect(); - let extra_data = decryption_request.extra_data.clone(); - ( - domain, - external_handles, - extra_data, - Some(decryption_request), - ) + (domain, external_handles, decryption_request.extra_data) } PubDecVerificationMaterial::External { domain, external_handles, extra_data, - } => (domain, external_handles, extra_data, None), + } => (domain, external_handles, extra_data), }; // If an expected answer is provided, use it; otherwise consider the first answer. @@ -2222,15 +2222,8 @@ fn verify_public_decrypt_responses( .clone(), }; - // check the internal signatures (verifies responses are signed by the trusted KMS keys; - // request-binding only applies for the `Request` variant) - internal_client.process_decryption_resp( - request, - num_expected_responses as u32, - resp_response_vec, - )?; - - // check the per-scheme signatures + // Check every response's signatures, signer and plaintext. For `External` material this + // is the only check, since without the request there is nothing to bind the responses to. check_external_decryption_signature( resp_response_vec, ptxt, diff --git a/core/service/src/client/client_non_wasm.rs b/core/service/src/client/client_non_wasm.rs index 04f763144..998b13ac1 100644 --- a/core/service/src/client/client_non_wasm.rs +++ b/core/service/src/client/client_non_wasm.rs @@ -155,7 +155,7 @@ impl Client { dsep, internal_bytes: &[], payload, - eip712_hash: Some(sol_type.eip712_signing_hash(domain)), + eip712_hash: sol_type.eip712_signing_hash(domain), }, &self.signing_schemes, &ExpectedSigner::Discover { @@ -650,5 +650,26 @@ mod tests { assert!(check(signature.clone()).is_err(), "{case}, legacy={legacy}"); } } + + // Beside a valid list entry, the legacy field has to be a copy of that entry. + let list = [TypedSignature { + scheme: SigningSchemeType::Ecdsa256k1.as_wire(), + signature: valid.clone(), + }]; + assert_eq!( + verify_with_legacy(&client, &list, &valid, &payload()) + .unwrap() + .0, + PARTY + ); + for (case, signature) in &invalid { + let err = verify_with_legacy(&client, &list, signature, &payload()) + .unwrap_err() + .to_string(); + assert!( + err.contains("differs from its ECDSA entry"), + "a bad {case} legacy field beside a valid list entry: {err}" + ); + } } } diff --git a/core/service/src/client/public_decryption.rs b/core/service/src/client/public_decryption.rs index f6dddd3ec..48a8a4c35 100644 --- a/core/service/src/client/public_decryption.rs +++ b/core/service/src/client/public_decryption.rs @@ -6,7 +6,7 @@ use alloy_sol_types::Eip712Domain; use kms_grpc::identifiers::ContextId; use kms_grpc::kms::v1::TypedPlaintext; use kms_grpc::kms::v1::{PublicDecryptionRequest, PublicDecryptionResponse, TypedCiphertext}; -use kms_grpc::rpc_types::{alloy_to_protobuf_domain, optional_protobuf_to_alloy_domain}; +use kms_grpc::rpc_types::alloy_to_protobuf_domain; use kms_grpc::{EpochId, RequestId}; impl Client { @@ -47,26 +47,16 @@ impl Client { } /// Validates the aggregated decryption response `agg_resp` against the - /// original `DecryptionRequest` `request`, and returns the decrypted - /// plaintext if valid and at least `min_agree_count` agree on the result. - /// - /// __NOTE__: If the original request is not provided, we can __not__ check - /// that the response correctly contains the digest of the request. + /// original `request`, and returns the decrypted plaintext if valid and at + /// least `min_agree_count` agree on the result. /// /// # Arguments /// /// All arguments except `agg_resp` are **trusted** (client-side state): /// /// * `request` — The original public decryption request constructed by this - /// client. Used to verify that the server responses match the request - /// (digest, ciphertext handles, domain). - /// - /// Passing `None` skips the request-level checks, and with them the EIP-712 - /// domain, so neither `external_signature` nor the ECDSA entry of - /// `signatures` can be checked. A response is then authenticated by the - /// deprecated internal `signature`, which covers the serialized payload and - /// needs no domain. That is enough for a caller that only wants to inspect a - /// result. + /// client. The responses are verified against its EIP-712 domain, ciphertext + /// handles, extra data and signing schemes, and bound to it. /// * `min_agree_count` — Minimum number of server responses that must agree /// on the same plaintext for the result to be accepted. /// @@ -76,30 +66,14 @@ impl Client { /// (signatures, digest matching, majority agreement) before use. pub fn process_decryption_resp( &self, - request: Option, + request: &PublicDecryptionRequest, min_agree_count: u32, agg_resp: &[PublicDecryptionResponse], ) -> anyhow::Result> { - let eip712_domain = match &request { - Some(req) => Some(optional_protobuf_to_alloy_domain(req.domain.as_ref())?), - None => None, - }; - let ext_handles_bytes: Vec> = match &request { - Some(req) => req - .ciphertexts - .iter() - .map(|c| c.external_handle.clone()) - .collect(), - None => vec![], - }; - let extra_data = request.as_ref().map(|req| req.extra_data.as_slice()); let trusted_ctx = PublicDecTrustedValidationContext::new( self.get_server_pks()?, &self.scheme_verf_keys, - eip712_domain.as_ref(), - &ext_handles_bytes, - extra_data, - request.as_ref(), + request, )?; // Partition the untrusted responses and enforce the majority threshold. Partitioning is diff --git a/core/service/src/client/tests/centralized/public_decryption_tests.rs b/core/service/src/client/tests/centralized/public_decryption_tests.rs index ce56ddc35..b03ef2dad 100644 --- a/core/service/src/client/tests/centralized/public_decryption_tests.rs +++ b/core/service/src/client/tests/centralized/public_decryption_tests.rs @@ -287,7 +287,7 @@ pub(crate) async fn run_decryption_centralized( assert_eq!(responses.len(), 1); let received_plaintexts = internal_client - .process_decryption_resp(Some(req.clone()), 1, &responses) + .process_decryption_resp(req, 1, &responses) .unwrap(); // we need 1 plaintext for each ciphertext in the batch diff --git a/core/service/src/client/tests/threshold/misc_tests.rs b/core/service/src/client/tests/threshold/misc_tests.rs index e34f9f451..6c967f54f 100644 --- a/core/service/src/client/tests/threshold/misc_tests.rs +++ b/core/service/src/client/tests/threshold/misc_tests.rs @@ -616,7 +616,7 @@ async fn test_complete_session_notification() -> Result<()> { let threshold = max_threshold(amount_parties); let min_count_agree = (threshold + 1) as u32; let received_plaintexts = internal_client - .process_decryption_resp(Some(req.clone()), min_count_agree, &responses) + .process_decryption_resp(&req, min_count_agree, &responses) .unwrap(); // check that the plaintexts are correct diff --git a/core/service/src/client/tests/threshold/public_decryption_tests.rs b/core/service/src/client/tests/threshold/public_decryption_tests.rs index 555461364..3cabb185d 100644 --- a/core/service/src/client/tests/threshold/public_decryption_tests.rs +++ b/core/service/src/client/tests/threshold/public_decryption_tests.rs @@ -447,7 +447,7 @@ pub async fn run_decryption_threshold_optionally_fail( let threshold = max_threshold(amount_parties); let min_count_agree = (threshold + 1) as u32; let received_plaintexts = internal_client - .process_decryption_resp(Some(req.clone()), min_count_agree, &responses) + .process_decryption_resp(req, min_count_agree, &responses) .unwrap(); // we need 1 plaintext for each ciphertext in the batch @@ -457,23 +457,5 @@ pub async fn run_decryption_threshold_optionally_fail( for (i, plaintext) in received_plaintexts.iter().enumerate() { crate::client::tests::common::assert_plaintext(&msgs[i], plaintext); } - - // A response without the deprecated internal signature must be tolerated, as long as - // enough of the remaining responses still carry a valid one. - // TODO(0.16) remove along with the deprecated fields. - if responses.len() > min_count_agree as usize { - let mut responses_wo_internal_sig = responses.clone(); - responses_wo_internal_sig[0].signature = vec![]; - assert_eq!( - internal_client - .process_decryption_resp( - Some(req.clone()), - min_count_agree, - &responses_wo_internal_sig - ) - .unwrap(), - received_plaintexts - ); - } } } diff --git a/core/service/src/client/user_decryption_wasm.rs b/core/service/src/client/user_decryption_wasm.rs index 678efd573..e86f4dcdf 100644 --- a/core/service/src/client/user_decryption_wasm.rs +++ b/core/service/src/client/user_decryption_wasm.rs @@ -4,16 +4,13 @@ use crate::cryptography::signatures::PrivateSigKey; use crate::cryptography::signcryption::insecure_decrypt_ignoring_signature; use crate::cryptography::{ encryption::{UnifiedPrivateEncKey, UnifiedPublicEncKey}, - signatures::PublicSigKey, signcryption::{UnifiedUnsigncryptionKey, UnsigncryptFHEPlaintext}, signing::SigningSchemeType, }; -use crate::engine::signed_payload::user_dec_payload; use crate::engine::validation::{ - DSEP_USER_DECRYPTION, ERR_VALIDATE_USER_DECRYPTION_MISMATCH_EXTRA_DATA, ExpectedSigner, - RejectedUserDecResponse, ResponseSignatures, SignedPayloads, UserDecRejectReason, - UserDecTrustedValidationContext, UserDecryptionInvariants, user_decrypt_eip712_hash, - validate_user_decrypt_responses, verify_response_signatures, + DSEP_USER_DECRYPTION, Eip712VerificationParams, RejectedUserDecResponse, UserDecRejectReason, + UserDecTrustedValidationContext, UserDecryptionInvariants, + authenticate_user_decrypt_and_check_meta_data, validate_user_decrypt_responses, }; use crate::{anyhow_error_and_log, some_or_err}; use algebra::error_correction::ReconstructionHints; @@ -237,62 +234,29 @@ impl Client { ))); } - let stored_server_addrs = &self.get_server_addrs(); + let stored_server_addrs = self.get_server_addrs(); if stored_server_addrs.len() != 1 { return Err(anyhow_error_and_log("incorrect length for addresses")); } - - let cur_verf_key: PublicSigKey = bc2wrap::deserialize_slice(&payload.verification_key)?; - - // NOTE: ID starts at 1 - let expected_server_addr = if let Some(server_addr) = stored_server_addrs.get(&1) { - if *server_addr != cur_verf_key.address() { - return Err(anyhow_error_and_log("server address is not consistent")); - } - server_addr - } else { - return Err(anyhow_error_and_log("missing server address at ID 1")); - }; - - // The response must echo the request's extra data whichever signature we go - // on to verify below. The EIP-712 signature covers `extraData`, but the raw - // ECDSA one does not, so this check has to happen outside the branch. - if resp.extra_data != request.extra_data() { - return Err(anyhow_error_and_log( - ERR_VALIDATE_USER_DECRYPTION_MISMATCH_EXTRA_DATA, - )); - } - - // A response has to carry at least one of the two deprecated fields until 0.16, - // so that a node from a release before `signatures` stays verifiable. - if resp.signature.is_empty() && resp.external_signature.is_empty() { - return Err(anyhow_error_and_log("empty signature")); - } - - let response_bytes = bc2wrap::serialize(&payload)?; - verify_response_signatures( - &ResponseSignatures { - internal: &resp.signature, - external: &resp.external_signature, - list: &resp.signatures, - }, - &SignedPayloads { - dsep: &DSEP_USER_DECRYPTION, - internal_bytes: &response_bytes, - payload: &user_dec_payload(&response_bytes, &resp.extra_data), - eip712_hash: Some(user_decrypt_eip712_hash(&payload, request, eip712_domain)?), - }, - request.signing_schemes(), - &ExpectedSigner::Known { - // This path handles a single response, whose address was looked up at - // party id 1 just above, so that is the party its keys live under too. - party_id: 1, - address: *expected_server_addr, - verf_key: &cur_verf_key, - }, + // A single server, so no other response can outvote it. + let trusted_ctx = UserDecTrustedValidationContext::new( + &stored_server_addrs, &self.scheme_verf_keys, - ) - .inspect_err(|e| tracing::warn!("signature on received response is not valid ({})", e))?; + request, + eip712_domain, + Some(0), + )?; + let (cur_verf_key, _role) = authenticate_user_decrypt_and_check_meta_data( + &trusted_ctx, + &payload, + &resp.signature, + &resp.signatures, + &Eip712VerificationParams { + response_external_signature: &resp.external_signature, + response_extra_data: &resp.extra_data, + trusted_eip712_domain: eip712_domain, + }, + )?; let receiver_id = self.client_address.to_vec(); let unsign_key = UnifiedUnsigncryptionKey::new( diff --git a/core/service/src/cryptography/signing/mod.rs b/core/service/src/cryptography/signing/mod.rs index ecb146a32..c37f15a4c 100644 --- a/core/service/src/cryptography/signing/mod.rs +++ b/core/service/src/cryptography/signing/mod.rs @@ -502,15 +502,6 @@ impl HasSigningScheme for UnifiedPublicSigKey { /// id, the set of keys that party has published. pub type SchemeVerfKeys = HashMap; -/// The verification key `party_id` published for `scheme`, if it published one. -pub fn verf_key_for( - keys: &SchemeVerfKeys, - party_id: u32, - scheme: SigningSchemeType, -) -> Option<&UnifiedPublicSigKey> { - keys.get(&party_id).and_then(|keys| keys.get(scheme)) -} - /// Sign `msg` (domain-separated by `dsep`) under the scheme of `sk`. #[cfg(feature = "non-wasm")] pub fn unified_sign( diff --git a/core/service/src/engine/storage_material_verification.rs b/core/service/src/engine/storage_material_verification.rs index 100828bcf..25d030e8c 100644 --- a/core/service/src/engine/storage_material_verification.rs +++ b/core/service/src/engine/storage_material_verification.rs @@ -36,12 +36,10 @@ use crate::backup::BACKUP_SIGNING_SCHEMES; use crate::backup::operator::RecoveryValidationMaterial; use crate::consts::{SIGNING_KEY_ID, signing_material_id}; -use crate::cryptography::signing::ecdsa::{ - PrivateSigKey, PublicSigKey, recover_address_from_eip712_hash, -}; +use crate::cryptography::signing::ecdsa::{PrivateSigKey, PublicSigKey}; use crate::cryptography::signing::identity::NodeSigningIdentity; use crate::cryptography::signing::{ - Signature, SigningSchemeType, StoredTypedSignature, VerfKeySet, unified_verify, + SchemeVerfKeys, SigningSchemeType, StoredTypedSignature, VerfKeySet, }; use crate::engine::base::{ CrsGenMetadata, CrsGenMetadataInner, CurrentPublicMaterialLayout, DSEP_PUBDATA_CRS, @@ -53,14 +51,18 @@ use crate::engine::material_integrity::{ verify_compressed_key_digest_from_bytes, verify_crs_digest_from_bytes, verify_public_key_digest_from_bytes, verify_server_key_digest_from_bytes, }; +use crate::engine::validation::{ + ExpectedSigner, ResponseSignatures, SignedPayloads, verify_response_signatures, +}; use crate::util::key_setup::{non_legacy_verf_material_slots, validate_slots}; use crate::vault::storage::{ StorageReader, StorageReaderExt, read_all_data_versioned, read_text_at_request_id, }; -use alloy_primitives::{Address, B256}; +use alloy_primitives::B256; use alloy_sol_types::{Eip712Domain, SolStruct}; use hashing::DomainSep; use kms_grpc::RequestId; +use kms_grpc::kms::v1::TypedSignature; use kms_grpc::rpc_types::{PrivDataType, PubDataType}; use kms_grpc::{ContextId, EpochId}; use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet}; @@ -515,14 +517,20 @@ fn verify_crs_metadata_signature( ) } -/// Verify the signatures one current metadata record carries against `identity`. +/// The party ID a node files its own keys and signature under when it checks its own +/// records. A record at boot has one signer, the node itself, so the number only shows +/// up in error messages. +const OWN_PARTY_ID: u32 = 1; + +/// Verify the signatures one current metadata record carries against `identity` and are in canonical order. +/// +/// The external_signature` has to equal the ECDSA entry beside it, and a record with no entries is checked by its +/// `external_signature` alone, as a client checks a node from before `signatures`. /// -/// Both ECDSA forms, the `external_signature` and the ECDSA entry of `signatures`, -/// recover their signer from `eip712_hash`, so they are skipped when no stored -/// domain yields one. Every other scheme signs `payload` under `dsep` and is -/// checked against the key `identity` derives for it. A scheme `identity` cannot -/// derive a key for is an error rather than a skip: an entry nobody can check must -/// not pass as one that was checked. +/// Both ECDSA forms recover their signer from `eip712_hash`, so without a stored domain +/// neither is checked. A scheme `identity` cannot derive +/// a key for is an error rather than a skip: an entry nobody can check must not pass as +/// one that was checked. #[expect(clippy::too_many_arguments)] fn verify_metadata_signatures( metadata_kind: &str, @@ -537,78 +545,76 @@ fn verify_metadata_signatures( where T: serde::Serialize + tfhe::Versionize + tfhe::named::Named, { - let expected_address = identity.verf_key().address(); - if let Some(hash) = eip712_hash { - verify_eip712_metadata_signature( - metadata_kind, - metadata_id, - hash, - external_signature, - expected_address, - )?; - } - - // Metadata that names no scheme has no per-scheme entry to check, and no - // scheme set to bind a preimage to. - if signatures.is_empty() { - return Ok(()); - } - - // The set is derived from the stored entries rather than requested from - // outside: at boot there is no request to measure against. let stored_schemes: Vec<_> = signatures.iter().map(|stored| stored.scheme).collect(); - let signed_bytes = - crate::cryptography::signing::composite::scheme_bound_preimage(&stored_schemes, payload)?; - - for stored in signatures { - match stored.scheme { - SigningSchemeType::Ecdsa256k1 => { - if let Some(hash) = eip712_hash { - verify_eip712_metadata_signature( - metadata_kind, - metadata_id, - hash, - &stored.signature, - expected_address, - )?; - } - } - scheme => { - let verf_key = identity.unified_verifying_key(scheme).map_err(|e| { - anyhow::anyhow!( - "Private {metadata_kind} metadata for id={metadata_id} carries a {scheme} signature, but this node cannot derive the {scheme} verification key to check it against: {e}" - ) - })?; - let signature = Signature::new(scheme, stored.signature.clone()); - unified_verify(dsep, &signed_bytes, &signature, &verf_key).map_err(|e| { - anyhow::anyhow!( - "Invalid {scheme} signature in private {metadata_kind} metadata for id={metadata_id}: {e}" - ) - })?; - } - } - } - Ok(()) -} - -fn verify_eip712_metadata_signature( - metadata_kind: &str, - metadata_id: &RequestId, - eip712_hash: &B256, - external_signature: &[u8], - expected_address: Address, -) -> anyhow::Result<()> { - let recovered_address = recover_address_from_eip712_hash(eip712_hash, external_signature) - .map_err(|e| { - anyhow::anyhow!( - "Invalid EIP-712 signature in private {metadata_kind} metadata for id={metadata_id}: {e}" - ) - })?; - if recovered_address != expected_address { - anyhow::bail!( - "Invalid EIP-712 signature in private {metadata_kind} metadata for id={metadata_id}: recovered signer {recovered_address}, expected {expected_address}" + let Some(eip712_hash) = eip712_hash else { + anyhow::ensure!( + stored_schemes + .iter() + .all(|scheme| *scheme == SigningSchemeType::Ecdsa256k1), + "Private {metadata_kind} metadata for id={metadata_id} carries {stored_schemes:?} \ + signatures but no EIP-712 domain" + ); + // Return OK when there is no domain and only EIP-712 signatures are present since we cannot verify anything then + return Ok(()); + }; + // Return an error when EIP-712 domain is present but no external signature is provided. + anyhow::ensure!( + !external_signature.is_empty(), + "Private {metadata_kind} metadata for id={metadata_id} has a stored EIP-712 domain but \ + no external signature" + ); + // Strictly increasing is canonical order without repeats. + anyhow::ensure!( + stored_schemes.windows(2).all(|pair| pair[0] < pair[1]), + "Private {metadata_kind} metadata for id={metadata_id} lists its signatures out of order \ + or repeats a scheme: {stored_schemes:?}" + ); + // Distinct from a record that carries only an ECDSA entry, which is the ordinary + // shape. Every record this release writes or upgrades carries at least that entry. + if signatures.is_empty() { + tracing::warn!( + "Current {metadata_kind} metadata for id={metadata_id} carries no signatures at all; \ + nothing but its EIP-712 signature stands behind it" ); } + + // A record that names no scheme reads as ECDSA, as a request that names none does. + let requested = SigningSchemeType::resolve(&stored_schemes); + // A key this node cannot derive is an error rather than a skip: an entry nobody can + // check must not pass as one that was checked. + let keys = VerfKeySet::from_identity(identity, &requested).map_err(|e| { + anyhow::anyhow!( + "Private {metadata_kind} metadata for id={metadata_id} carries {requested:?} \ + signatures, but this node cannot derive every verification key to check them: {e}" + ) + })?; + let list: Vec = signatures.iter().map(TypedSignature::from).collect(); + let verf_key = identity.verf_key(); + verify_response_signatures( + &ResponseSignatures { + internal: &[], + external: external_signature, + list: &list, + }, + &SignedPayloads { + dsep, + internal_bytes: &[], + payload, + eip712_hash: *eip712_hash, + }, + &requested, + &ExpectedSigner::Known { + party_id: OWN_PARTY_ID, + address: verf_key.address(), + verf_key: &verf_key, + }, + &SchemeVerfKeys::from([(OWN_PARTY_ID, keys)]), + ) + .map_err(|e| { + anyhow::anyhow!( + "Invalid signature in private {metadata_kind} metadata for id={metadata_id}: {e}" + ) + })?; Ok(()) } @@ -2183,6 +2189,82 @@ mod tests { .expect("a seedless node without recovery material must still boot"); } + /// Removing an entry from a multi-scheme record is caught whenever a scheme-bound + /// entry survives, because it attests to the set the removed one belonged to. + /// Reordering or repeating entries is caught too, so the list a record shows is + /// the list its signatures were made over. A record reduced to its ECDSA entry + /// alone is the known exception, since that entry binds no scheme set. + #[test] + fn private_keygen_metadata_rejects_a_tampered_scheme_list() { + let mut rng = AesRng::seed_from_u64(177); + let identity = seeded_identity(&mut rng); + let domain = crate::dummy_domain(); + let prep_id = RequestId::new_random(&mut rng); + let key_id = RequestId::new_random(&mut rng); + let schemes = [SigningSchemeType::Ecdsa256k1, SigningSchemeType::MlDsa65]; + + let metadata = crate::engine::base::compute_info_standard_keygen_from_digests( + &identity, + &schemes, + &prep_id, + &key_id, + vec![0x11; 32], + vec![0x22; 32], + &domain, + vec![0x03], + ) + .expect("standard keygen metadata construction must succeed"); + let KeyGenMetadata::Current(inner) = &metadata else { + panic!("metadata construction must produce current metadata"); + }; + verify_keygen_metadata_signature(&key_id, inner, &identity) + .expect("the record must verify as written"); + assert_eq!(inner.signatures.len(), 2, "both entries must be present"); + + // Dropping the post-quantum entry leaves only the ECDSA one, which signs + // the EIP-712 hash and therefore binds no scheme set. This is a residual + // gap on `verify_metadata_signatures`, and it still verifies. + let mut stripped = inner.clone(); + stripped + .signatures + .retain(|entry| entry.scheme == SigningSchemeType::Ecdsa256k1); + verify_keygen_metadata_signature(&key_id, &stripped, &identity) + .expect("known gap: a record reduced to its ECDSA entry alone still verifies"); + + // The strip the binding does catch: the entry that survives is a bound one, + // and it attests to a set that no longer matches what the record lists. + let mut ecdsa_dropped = inner.clone(); + ecdsa_dropped + .signatures + .retain(|entry| entry.scheme != SigningSchemeType::Ecdsa256k1); + let err = verify_keygen_metadata_signature(&key_id, &ecdsa_dropped, &identity) + .expect_err("a record stripped of its ECDSA entry must not verify as complete"); + assert!( + err.to_string().contains("did not verify"), + "the surviving MlDsa65 entry should fail against the reduced set, got: {err}" + ); + + // Repeating an entry changes the list without changing any signature. + let mut repeated = inner.clone(); + repeated.signatures.push(inner.signatures[0].clone()); + let err = verify_keygen_metadata_signature(&key_id, &repeated, &identity) + .expect_err("a repeated scheme must be rejected"); + assert!( + err.to_string().contains("repeats a scheme"), + "the error should name the repeated scheme, got: {err}" + ); + + // So does listing them out of canonical order. + let mut reordered = inner.clone(); + reordered.signatures.reverse(); + let err = verify_keygen_metadata_signature(&key_id, &reordered, &identity) + .expect_err("an out-of-order scheme list must be rejected"); + assert!( + err.to_string().contains("out of order"), + "the error should name the ordering, got: {err}" + ); + } + #[test] fn private_standard_keygen_metadata_signature_verifies_with_stored_domain() { let mut rng = AesRng::seed_from_u64(176); @@ -2225,7 +2307,9 @@ mod tests { ) .expect_err("a changed server-key digest must invalidate the signature"); assert!( - err.to_string().contains("Invalid EIP-712 signature"), + err.to_string() + .contains("Invalid signature in private keygen metadata") + && err.to_string().contains("recovered to"), "expected a signature verification error, got: {err}" ); } @@ -2274,7 +2358,9 @@ mod tests { ) .expect_err("a changed stored domain must invalidate the signature"); assert!( - err.to_string().contains("Invalid EIP-712 signature"), + err.to_string() + .contains("Invalid signature in private keygen metadata") + && err.to_string().contains("recovered to"), "expected a signature verification error, got: {err}" ); } @@ -2307,7 +2393,7 @@ mod tests { } /// The per-scheme `signatures` of stored metadata are checked against the keys the - /// node derives, with and without a stored domain, and a tampered entry is rejected. + /// node derives, and a tampered entry is rejected. #[test] fn private_metadata_scheme_signatures_are_verified() { let mut rng = AesRng::seed_from_u64(177); @@ -2358,48 +2444,54 @@ mod tests { verify_crs_metadata_signature(&crs_id, &crs_inner, &identity) .expect("every CRS signature must verify under the identity that made it"); - // Without a stored domain neither ECDSA form can be rebuilt, but the other schemes - // sign the payload and are still checked. - for with_domain in [true, false] { - let mut key_metadata = key_inner.clone(); - let mut crs_metadata = crs_inner.clone(); - if !with_domain { - key_metadata.eip712_domain = None; - crs_metadata.eip712_domain = None; - } - verify_keygen_metadata_signature(&key_id, &key_metadata, &identity) - .expect("intact keygen metadata must verify"); - verify_crs_metadata_signature(&crs_id, &crs_metadata, &identity) - .expect("intact CRS metadata must verify"); - - let tampered_entry = key_metadata - .signatures - .iter_mut() - .find(|stored| stored.scheme == SigningSchemeType::MlDsa65) - .expect("the metadata carries an MlDsa65 entry"); - tampered_entry.signature[0] ^= 1; - let err = verify_keygen_metadata_signature(&key_id, &key_metadata, &identity) - .expect_err("a tampered MlDsa65 entry must be rejected"); - assert!( - err.to_string().contains("Invalid MlDsa65 signature"), - "with_domain={with_domain}: expected an MlDsa65 signature error, got: {err}" - ); + // A tampered entry of any scheme is rejected. + let mut key_metadata = key_inner.clone(); + key_metadata + .signatures + .iter_mut() + .find(|stored| stored.scheme == SigningSchemeType::MlDsa65) + .expect("the metadata carries an MlDsa65 entry") + .signature[0] ^= 1; + let err = verify_keygen_metadata_signature(&key_id, &key_metadata, &identity) + .expect_err("a tampered MlDsa65 entry must be rejected"); + assert!( + err.to_string().contains("did not verify"), + "expected an MlDsa65 signature error, got: {err}" + ); + let mut crs_metadata = crs_inner.clone(); + crs_metadata + .signatures + .iter_mut() + .find(|stored| stored.scheme == SigningSchemeType::Ed25519) + .expect("the metadata carries an Ed25519 entry") + .signature[0] ^= 1; + let err = verify_crs_metadata_signature(&crs_id, &crs_metadata, &identity) + .expect_err("a tampered Ed25519 entry must be rejected"); + assert!( + err.to_string().contains("did not verify"), + "expected an Ed25519 signature error, got: {err}" + ); - let tampered_entry = crs_metadata - .signatures - .iter_mut() - .find(|stored| stored.scheme == SigningSchemeType::Ed25519) - .expect("the metadata carries an Ed25519 entry"); - tampered_entry.signature[0] ^= 1; - let err = verify_crs_metadata_signature(&crs_id, &crs_metadata, &identity) - .expect_err("a tampered Ed25519 entry must be rejected"); - assert!( - err.to_string().contains("Invalid Ed25519 signature"), - "with_domain={with_domain}: expected an Ed25519 signature error, got: {err}" - ); - } + // A record without a stored domain predates per-scheme signatures, so one that + // carries a post-quantum entry is rejected rather than half checked. + let mut domainless = key_inner.clone(); + domainless.eip712_domain = None; + let err = verify_keygen_metadata_signature(&key_id, &domainless, &identity) + .expect_err("a domainless record with post-quantum entries must be rejected"); + assert!( + err.to_string().contains("but no EIP-712 domain"), + "expected a missing domain error, got: {err}" + ); + // The shape the upgrade does produce, the ECDSA entry alone, cannot be checked + // without a domain and passes. + domainless + .signatures + .retain(|stored| stored.scheme == SigningSchemeType::Ecdsa256k1); + verify_keygen_metadata_signature(&key_id, &domainless, &identity) + .expect("a domainless record with only its ECDSA entry must pass"); - // The ECDSA entry of `signatures` is held to the same standard as `external_signature`. + // As for a client, `external_signature` has to equal the ECDSA entry of + // `signatures`, so corrupting either one is a rejection. let mut tampered = key_inner.clone(); tampered .signatures @@ -2410,8 +2502,26 @@ mod tests { let err = verify_keygen_metadata_signature(&key_id, &tampered, &identity) .expect_err("a corrupt ECDSA entry must be rejected"); assert!( - err.to_string().contains("Invalid EIP-712 signature"), - "expected an EIP-712 signature error, got: {err}" + err.to_string().contains("differs from its ECDSA entry"), + "expected the two ECDSA forms to disagree, got: {err}" + ); + let mut tampered = key_inner.clone(); + tampered.external_signature[0] ^= 1; + let err = verify_keygen_metadata_signature(&key_id, &tampered, &identity) + .expect_err("a corrupt external signature must be rejected"); + assert!( + err.to_string().contains("differs from its ECDSA entry"), + "expected the two ECDSA forms to disagree, got: {err}" + ); + + // A record with a stored domain carries an external signature. + let mut tampered = key_inner.clone(); + tampered.external_signature.clear(); + let err = verify_keygen_metadata_signature(&key_id, &tampered, &identity) + .expect_err("a record with a domain but no external signature must be rejected"); + assert!( + err.to_string().contains("no external signature"), + "expected a missing external signature error, got: {err}" ); // The same ECDSA key under another seed derives other keys, so the entries of the @@ -2421,7 +2531,7 @@ mod tests { let err = verify_keygen_metadata_signature(&key_id, &key_inner, &other_seed) .expect_err("metadata signed under another seed must be rejected"); assert!( - err.to_string().contains("Invalid Ed25519 signature"), + err.to_string().contains("did not verify"), "expected an Ed25519 signature error, got: {err}" ); @@ -2432,7 +2542,8 @@ mod tests { .expect_err("a seedless node must not accept an entry it cannot check"); assert!( err.to_string() - .contains("cannot derive the Ed25519 verification key"), + .contains("cannot derive every verification key") + && err.to_string().contains("no Ed25519 key can be derived"), "expected an error naming the missing key, got: {err}" ); } @@ -2476,7 +2587,7 @@ mod tests { .expect_err("metadata signed by another key must be rejected"); let msg = err.to_string(); assert!( - msg.contains("Invalid EIP-712 signature") + msg.contains("Invalid signature in private keygen metadata") && msg.contains(&other_pk.address().to_string()) && msg.contains(&PublicSigKey::from_sk(&sk).address().to_string()), "error should name both the recovered and the expected signer, got: {msg}" diff --git a/core/service/src/engine/validation_non_wasm.rs b/core/service/src/engine/validation_non_wasm.rs index b947ac44e..a13e56c04 100644 --- a/core/service/src/engine/validation_non_wasm.rs +++ b/core/service/src/engine/validation_non_wasm.rs @@ -53,20 +53,20 @@ pub(crate) struct PublicDecTrustedValidationContext<'a> { /// client built without storage access, such as the browser, supplies an /// empty map and can then only check ECDSA. scheme_verf_keys: &'a SchemeVerfKeys, - eip712_domain: Option<&'a Eip712Domain>, - ext_handles_bytes: &'a [Vec], - extra_data: Option<&'a [u8]>, - request: Option<&'a PublicDecryptionRequest>, + /// The request this client sent. The responses are bound to it, and verified + /// against its domain, ciphertext handles, extra data and signing schemes. + request: &'a PublicDecryptionRequest, + /// The EIP-712 domain of `request`. + eip712_domain: Eip712Domain, + /// The external handles of the ciphertexts of `request`. + ext_handles_bytes: Vec>, } impl<'a> PublicDecTrustedValidationContext<'a> { pub fn new( server_pks: &'a HashMap, scheme_verf_keys: &'a SchemeVerfKeys, - eip712_domain: Option<&'a Eip712Domain>, - ext_handles_bytes: &'a [Vec], - extra_data: Option<&'a [u8]>, - request: Option<&'a PublicDecryptionRequest>, + request: &'a PublicDecryptionRequest, ) -> anyhow::Result { // Sanity check uniqueness of server public keys. This is a trusted context, so if the server keys are not unique, it is a configuration error. let unique_keys: HashSet<&PublicSigKey> = server_pks.values().collect(); @@ -74,13 +74,18 @@ impl<'a> PublicDecTrustedValidationContext<'a> { anyhow::bail!("Duplicate server public keys found in trusted validation context"); } + let eip712_domain = optional_protobuf_to_alloy_domain(request.domain.as_ref())?; + let ext_handles_bytes = request + .ciphertexts + .iter() + .map(|ct| ct.external_handle.clone()) + .collect(); Ok(Self { server_pks, scheme_verf_keys, + request, eip712_domain, ext_handles_bytes, - extra_data, - request, }) } } @@ -97,8 +102,6 @@ const ERR_VALIDATE_PUBLIC_DECRYPTION_MISSING_REQ_ID: &str = "Request ID is not set in public decryption response"; const ERR_VALIDATE_PUBLIC_DECRYPTION_BAD_FHE_TYPE: &str = "Plaintext type mismatch in public decryption response"; -const ERR_VALIDATE_PUBLIC_DECRYPTION_EMPTY_REQUEST: &str = - "Public decryption request is None while validating public decryption responses"; const ERR_VALIDATE_USER_DECRYPTION_EMPTY_CTS: &str = "No ciphertexts in user decryption request"; @@ -421,50 +424,18 @@ pub(crate) fn verify_user_decrypt_eip712( Ok(domain) } -/// Verify every signature a public-decryption response carries, and check that -/// they belong to `party_id`. +/// Verify the signatures of a public-decryption response, namely the entries that meet +/// the schemes of `trusted_ctx.request` and the deprecated fields, and check that they +/// belong to `party_id`. /// -/// This function is **infallible with respect to the (untrusted) response -/// content**: any malformed field is treated exactly like a mismatch and yields -/// `false`, never an error. This is what lets -/// [`partition_public_decrypt_responses`] tolerate up to `t` Byzantine responses -/// without a single one being able to abort the whole validation. +/// A malformed or invalid field of the (untrusted) response is an `Err`. The caller +/// turns that `Err` into a rejection of this one response, so that +/// [`validate_public_decrypt_responses`] tolerates up to `t` Byzantine responses. /// /// # What makes a response authentic /// /// [`verify_response_signatures`] decides that, as it does for every other result kind. #[expect(clippy::too_many_arguments)] -fn verify_public_decrypt_signatures( - trusted_ctx: &PublicDecTrustedValidationContext, - response: &PublicDecryptionResponsePayload, - party_id: u32, - verification_key: &PublicSigKey, - signature: &[u8], - external_signature: &[u8], - signatures: &[TypedSignature], - response_extra_data: &[u8], -) -> bool { - match check_public_decrypt_signatures( - trusted_ctx, - response, - party_id, - verification_key, - signature, - external_signature, - signatures, - response_extra_data, - ) { - Ok(()) => true, - Err(e) => { - tracing::warn!("A public decryption response of party {party_id} is rejected: {e}"); - false - } - } -} - -/// The fallible body of [`verify_public_decrypt_signatures`], which turns every error -/// here into `false`. -#[expect(clippy::too_many_arguments)] fn check_public_decrypt_signatures( trusted_ctx: &PublicDecTrustedValidationContext, response: &PublicDecryptionResponsePayload, @@ -475,29 +446,19 @@ fn check_public_decrypt_signatures( signatures: &[TypedSignature], response_extra_data: &[u8], ) -> anyhow::Result<()> { - let requested = match trusted_ctx.request { - Some(request) => SigningSchemeType::resolve_requested(&request.signing_schemes)?, - None => vec![SigningSchemeType::Ecdsa256k1], - }; + let requested = SigningSchemeType::resolve_requested(&trusted_ctx.request.signing_schemes)?; // NOTE that we cannot use `BaseKmsStruct::verify_sig` // because `BaseKmsStruct` cannot be compiled for wasm (it has an async mutex). let response_bytes = bc2wrap::serialize(&response)?; let payload = public_dec_payload(&response_bytes, response_extra_data); - // Built only when a domain is available: without one no ECDSA signature of this - // response can be checked, and the message would be of no use. - let eip712_hash = match trusted_ctx.eip712_domain { - Some(domain) => Some( - compute_public_decryption_message( - trusted_ctx.ext_handles_bytes, - &response.plaintexts, - response_extra_data, - )? - .eip712_signing_hash(domain), - ), - None => None, - }; + let eip712_hash = compute_public_decryption_message( + &trusted_ctx.ext_handles_bytes, + &response.plaintexts, + response_extra_data, + )? + .eip712_signing_hash(&trusted_ctx.eip712_domain); verify_response_signatures( &ResponseSignatures { @@ -544,10 +505,7 @@ pub(crate) struct PublicDecryptionInvariants { impl PublicDecryptionInvariants { /// Sanity-check the invariants against the trusted context. fn sanity_check(&self, trusted_ctx: &PublicDecTrustedValidationContext) -> anyhow::Result<()> { - let Some(req) = trusted_ctx.request else { - tracing::warn!(ERR_VALIDATE_PUBLIC_DECRYPTION_EMPTY_REQUEST); - return Ok(()); - }; + let req = trusted_ctx.request; // The consensus plaintext count must match the number of ciphertexts the client asked to // decrypt. This is a property of the (majority-backed) consensus, so a mismatch is a genuine @@ -675,9 +633,7 @@ fn authenticate_public_decrypt_response( tracing::warn!("A request ID must be present!"); return Err(PublicRejectReason::MissingRequestId); } - if let Some(expected_extra_data) = trusted_ctx.extra_data - && cur_resp.extra_data != expected_extra_data - { + if cur_resp.extra_data != trusted_ctx.request.extra_data { tracing::warn!("Extra data mismatch in public decryption!"); return Err(PublicRejectReason::ExtraDataMismatch); } @@ -726,7 +682,7 @@ fn authenticate_public_decrypt_response( // The deprecated internal `signature` and `external_signature` fields are checked alongside // `signatures`, as user decryption checks them, so a response stays verifiable without an // EIP-712 domain. TODO(0.16): drop the two fields and their arguments. - if !verify_public_decrypt_signatures( + if let Err(e) = check_public_decrypt_signatures( trusted_ctx, cur_payload, signing_party, @@ -736,7 +692,7 @@ fn authenticate_public_decrypt_response( &cur_resp.signatures, &cur_resp.extra_data, ) { - tracing::warn!("Some server did not provide a properly signed response!"); + tracing::warn!("A public decryption response of party {signing_party} is rejected: {e}"); return Err(PublicRejectReason::SignatureMismatch); } @@ -755,13 +711,12 @@ fn authenticate_public_decrypt_response( /// get to vote at all. /// 3. Discard every authenticated payload that does not match the consensus invariants. /// -/// In addition, if the original request is provided (via `trusted_ctx.request`) -/// the response matches the original request +/// In addition, the agreed result has to match the original request (`trusted_ctx.request`). /// /// Infallible w.r.t. adversarial per-response content: any malformed/inconsistent response is /// placed in `rejected`, never propagated. Returns `Err` ONLY when the honest invariant set cannot /// be established at all: no responses, no configured servers, no pivot with ≥ `t + 1` agreement, -/// or — when a request is provided — the agreed result does not match the client's own request. +/// or the agreed result does not match the client's own request. /// With ≥ `2t + 1` honest responses present, none of those `Err` cases can be triggered by ≤ `t` /// adversaries. /// @@ -1216,8 +1171,7 @@ mod tests { cryptography::{ encryption::{Encryption, PkeScheme, PkeSchemeType, UnifiedPublicEncKey}, signatures::{ - NodeSigningIdentity, PrivateSigKey, PublicSigKey, compute_eip712_signature, - gen_sig_keys, internal_sign, + NodeSigningIdentity, PrivateSigKey, PublicSigKey, gen_sig_keys, internal_sign, }, signing::SigningSchemeType, }, @@ -1240,9 +1194,9 @@ mod tests { use super::{ ERR_VALIDATE_PUBLIC_DECRYPTION_BAD_FHE_TYPE, ERR_VALIDATE_PUBLIC_DECRYPTION_BAD_LINK, ERR_VALIDATE_PUBLIC_DECRYPTION_EMPTY_CTS, ERR_VALIDATE_USER_DECRYPTION_EMPTY_CTS, - PublicDecTrustedValidationContext, TypedSignature, compute_public_decryption_message, + PublicDecTrustedValidationContext, TypedSignature, check_public_decrypt_signatures, unpack_public_decrypt_req, unpack_user_decrypt_req, verify_max_num_bits, - verify_public_decrypt_signatures, verify_user_decrypt_eip712, + verify_user_decrypt_eip712, }; /// Sign a public decryption result the way the server does, under ECDSA only. @@ -1295,6 +1249,31 @@ mod tests { bc2wrap::deserialize_slice(&payload.verification_key).unwrap() } + /// A request under `domain` for one `Uint8` ciphertext per handle in `handles`, + /// asking for `signing_schemes`. An empty `signing_schemes` means ECDSA. + fn public_decrypt_request( + domain: &Eip712Domain, + handles: &[Vec], + extra_data: &[u8], + signing_schemes: Vec, + ) -> PublicDecryptionRequest { + PublicDecryptionRequest { + signing_schemes, + ciphertexts: handles + .iter() + .map(|handle| TypedCiphertext { + fhe_type: tfhe::FheTypes::Uint8 as i32, + external_handle: handle.clone(), + ciphertext_format: 1, + ..Default::default() + }) + .collect(), + domain: Some(alloy_to_protobuf_domain(domain).unwrap()), + extra_data: extra_data.to_vec(), + ..Default::default() + } + } + #[test] fn test_validate_public_decrypt_req() { // setup data we're going to use in this test @@ -1692,121 +1671,6 @@ mod tests { } } - #[test] - fn test_validate_public_decrypt_meta_response() { - let mut rng = AesRng::seed_from_u64(0); - let (vk0, sk0) = gen_sig_keys(&mut rng); - let (vk1, sk1) = gen_sig_keys(&mut rng); - let (vk2, _sk2) = gen_sig_keys(&mut rng); - - let pks: HashMap = HashMap::from_iter( - [vk0, vk1, vk2] - .into_iter() - .enumerate() - .map(|(i, k)| (i as u32 + 1, k)), - ); - - let request_id = Some( - derive_request_id("test_validate_public_decrypt_meta_response") - .unwrap() - .into(), - ); - let pivot = PublicDecryptionResponsePayload { - verification_key: bc2wrap::serialize(&pks[&1]).unwrap(), - plaintexts: vec![TypedPlaintext { - bytes: vec![1], - fhe_type: tfhe::FheTypes::Uint8 as i32, // Uint8, supported for ABI encoding - }], - request_id: request_id.clone(), - }; - - // `verify_public_decrypt_signatures` checks *authenticity* only (every entry of the - // response's `signatures` list); agreement with the consensus invariants is checked - // separately in the match pass of `partition_public_decrypt_responses` (exercised by - // `test_validate_public_decrypt_responses`). - let domain = dummy_domain(); - let server_pks = pks.clone(); - let scheme_verf_keys = HashMap::new(); - let ctx = PublicDecTrustedValidationContext::new( - &server_pks, - &scheme_verf_keys, - Some(&domain), - &[], - None, - None, - ) - .unwrap(); - // The ECDSA entry is the recoverable EIP-712 signature over the handles, the plaintexts - // and the extra data. - let ecdsa_entry = |sk: &PrivateSigKey, payload: &PublicDecryptionResponsePayload| { - let message = compute_public_decryption_message(&[], &payload.plaintexts, &[]).unwrap(); - kms_grpc::rpc_types::ecdsa_signatures( - compute_eip712_signature(sk, &message, &domain).unwrap(), - ) - }; - let verify = |payload: &PublicDecryptionResponsePayload, sigs: &[TypedSignature]| { - verify_public_decrypt_signatures(&ctx, payload, 1, &vk_of(payload), &[], &[], sigs, &[]) - }; - - // an empty list and no internal field either, so nothing can be authenticated - assert!(!verify(&pivot, &[])); - - // signed with the wrong private key - assert!(!verify(&pivot, &ecdsa_entry(&sk1, &pivot))); - - // a malformed signature - assert!(!verify( - &pivot, - &kms_grpc::rpc_types::ecdsa_signatures(vec![0u8; 65]) - )); - - // signing the wrong value: the plaintexts differ, and the EIP-712 message covers them. - // - // NOTE: `request_id` is deliberately not part of this message — see - // `PublicDecryptVerification`. The request-id linkage of a response is established by - // `PublicDecryptionInvariants::sanity_check` against the client's own request, not by - // this signature. - { - let other_value = PublicDecryptionResponsePayload { - verification_key: bc2wrap::serialize(&pks[&1]).unwrap(), - plaintexts: vec![TypedPlaintext { - bytes: vec![2], - fhe_type: tfhe::FheTypes::Uint8 as i32, - }], - request_id: request_id.clone(), - }; - assert!(!verify(&pivot, &ecdsa_entry(&sk0, &other_value))); - } - - // a response whose key did not sign it: the payload carries a fresh key, but the - // signature is by `sk0`, so the recovered address is not the one the payload claims - { - let (vk, _sk) = gen_sig_keys(&mut rng); - let bad_value = PublicDecryptionResponsePayload { - verification_key: bc2wrap::serialize(&vk).unwrap(), - plaintexts: vec![TypedPlaintext { - bytes: vec![1], - fhe_type: tfhe::FheTypes::Uint8 as i32, - }], - request_id, - }; - assert!(!verify(&bad_value, &ecdsa_entry(&sk0, &bad_value))); - } - - // an entry of some other scheme does not stand in for the ECDSA one this - // context asks for, whether or not it could have been checked - assert!(!verify( - &pivot, - &[TypedSignature { - scheme: kms_grpc::kms::v1::SigningSchemeType::Mldsa65 as i32, - signature: vec![0u8; 64], - }] - )); - - // happy path - assert!(verify(&pivot, &ecdsa_entry(&sk0, &pivot))); - } - #[test] fn test_validate_public_decrypt_responses() { let mut rng = AesRng::seed_from_u64(0); @@ -1836,14 +1700,13 @@ mod tests { fhe_type: tfhe::FheTypes::Uint8 as i32, // Uint8, supported for ABI encoding }]; - let trusted_ctx = PublicDecTrustedValidationContext { - server_pks: &pks, - eip712_domain: Some(&alloy_domain), - ext_handles_bytes: &ext_handles_bytes, - extra_data: Some(&extra_data_0), - request: None, - scheme_verf_keys: &HashMap::new(), + let request = PublicDecryptionRequest { + request_id: request_id.clone(), + ..public_decrypt_request(&alloy_domain, &ext_handles_bytes, &extra_data_0, vec![]) }; + let scheme_verf_keys = HashMap::new(); + let trusted_ctx = + PublicDecTrustedValidationContext::new(&pks, &scheme_verf_keys, &request).unwrap(); // NOTE: the pks map uses 1-based index while the others use 0-based index like sk0 let resp0 = signed_public_decrypt_response( @@ -2059,14 +1922,9 @@ mod tests { &alloy_domain, ); - let trusted_ctx = PublicDecTrustedValidationContext { - server_pks: &pks, - eip712_domain: Some(&alloy_domain), - ext_handles_bytes: &ext_handles_bytes, - extra_data: Some(&extra_data), - request: Some(&request), - scheme_verf_keys: &HashMap::new(), - }; + let scheme_verf_keys = HashMap::new(); + let trusted_ctx = + PublicDecTrustedValidationContext::new(&pks, &scheme_verf_keys, &request).unwrap(); // invalid aggregate response, e.g., when there are none { @@ -2112,14 +1970,9 @@ mod tests { context_id: None, epoch_id: None, }; - let bad_ctx = PublicDecTrustedValidationContext { - server_pks: &pks, - eip712_domain: Some(&alloy_domain), - ext_handles_bytes: &ext_handles_bytes, - extra_data: Some(&extra_data), - request: Some(&bad_request), - scheme_verf_keys: &HashMap::new(), - }; + let bad_ctx = + PublicDecTrustedValidationContext::new(&pks, &scheme_verf_keys, &bad_request) + .unwrap(); assert!( validate_public_decrypt_responses(&bad_ctx, 2, &agg_resp) .unwrap_err() @@ -2128,14 +1981,6 @@ mod tests { ); } - // bad external signature - { - let mut bad_resp = resp1.clone(); - bad_resp.external_signature[0] ^= 1; - let agg_resp = vec![resp0.clone(), bad_resp]; - validate_public_decrypt_responses(&trusted_ctx, 1, &agg_resp).unwrap(); - } - // request ID { let agg_resp = vec![resp0.clone(), resp1.clone()]; @@ -2163,14 +2008,9 @@ mod tests { context_id: None, epoch_id: None, }; - let bad_ctx = PublicDecTrustedValidationContext { - server_pks: &pks, - eip712_domain: Some(&alloy_domain), - ext_handles_bytes: &ext_handles_bytes, - extra_data: Some(&extra_data), - request: Some(&bad_request), - scheme_verf_keys: &HashMap::new(), - }; + let bad_ctx = + PublicDecTrustedValidationContext::new(&pks, &scheme_verf_keys, &bad_request) + .unwrap(); assert!( validate_public_decrypt_responses(&bad_ctx, 2, &agg_resp) .unwrap_err() @@ -2179,20 +2019,6 @@ mod tests { ); } - // request is empty, which should pass - { - let agg_resp = vec![resp0.clone(), resp1.clone()]; - let none_ctx = PublicDecTrustedValidationContext { - server_pks: &pks, - eip712_domain: None, - ext_handles_bytes: &[], - extra_data: None, - request: None, - scheme_verf_keys: &HashMap::new(), - }; - validate_public_decrypt_responses(&none_ctx, 2, &agg_resp).unwrap(); - } - // happy path { let agg_resp = vec![resp0.clone(), resp1.clone()]; @@ -2246,114 +2072,115 @@ mod tests { let server_pks = HashMap::from([(1u32, vk_of(&pivot))]); let scheme_verf_keys = HashMap::new(); - let ctx = |domain| { - PublicDecTrustedValidationContext::new( - &server_pks, - &scheme_verf_keys, - domain, - &ext_handles_bytes, - None, - None, - ) - .unwrap() - }; - let verify = |ctx: &PublicDecTrustedValidationContext, sigs: &[TypedSignature]| { - verify_public_decrypt_signatures( - ctx, + let request = + public_decrypt_request(&alloy_domain, &ext_handles_bytes, &extra_data, vec![]); + let ctx = PublicDecTrustedValidationContext::new(&server_pks, &scheme_verf_keys, &request) + .unwrap(); + let verify = |internal: &[u8], external: &[u8], list: &[TypedSignature]| { + check_public_decrypt_signatures( + &ctx, &pivot, 1, &vk_of(&pivot), - &[], - &[], - sigs, + internal, + external, + list, &extra_data, ) + .is_ok() }; // an empty list, with no internal field to fall back on - assert!(!verify(&ctx(Some(&alloy_domain)), &[])); + assert!(!verify(&[], &[], &[])); // a tampered ECDSA signature recovers to another address - assert!(!verify(&ctx(Some(&alloy_domain)), &tampered)); - - // without a domain the EIP-712 message cannot be rebuilt, so the only entry - // there is gets passed over and the requested ECDSA is left unverified - assert!(!verify(&ctx(None), &signatures)); + assert!(!verify(&[], &[], &tampered)); // happy path - assert!(verify(&ctx(Some(&alloy_domain)), &signatures)); - - // The deprecated internal fields authenticate the same response. The raw ECDSA - // signature needs no domain. - let domainless = ctx(None); - assert!(verify_public_decrypt_signatures( - &domainless, - &pivot, - 1, - &vk_of(&pivot), - &signed.signature, - &[], - &[], - &extra_data, - )); - // `external_signature` does the same, but only with a domain to rebuild the - // EIP-712 message from. - let with_domain = ctx(Some(&alloy_domain)); - assert!(verify_public_decrypt_signatures( - &with_domain, - &pivot, - 1, - &vk_of(&pivot), - &[], - &signed.external_signature, - &[], - &extra_data, - )); - assert!(!verify_public_decrypt_signatures( - &domainless, - &pivot, - 1, - &vk_of(&pivot), - &[], - &signed.external_signature, - &[], - &extra_data, - )); - // With a domain the internal signature does not stand in for the EIP-712 forms: - // it covers the payload alone, not the handles or the extra data the EIP-712 - // message binds, so the requested ECDSA is left unverified. - assert!(!verify_public_decrypt_signatures( - &with_domain, - &pivot, - 1, - &vk_of(&pivot), - &signed.signature, - &[], - &[], - &extra_data, - )); - // A corrupt internal signature is a rejection, not something the list can - // paper over. + assert!(verify(&[], &[], &signatures)); + + // the EIP-712 message covers the plaintexts, so an entry signed over other ones fails + let other_value = PublicDecryptionResponsePayload { + plaintexts: vec![TypedPlaintext { + bytes: vec![2], + fhe_type: tfhe::FheTypes::Uint8 as i32, + }], + ..pivot.clone() + }; + let other_signatures = sign_ecdsa_public_decrypt_result( + &sk0, + other_value, + &ext_handles_bytes, + extra_data.clone(), + &alloy_domain, + ) + .signatures; + assert!(!verify(&[], &[], &other_signatures)); + + // a response whose key did not sign it: the payload carries a fresh key, but the + // signature is by `sk0`, so the recovered address is not the one the payload claims + let (fresh_vk, _) = gen_sig_keys(&mut rng); + let bad_value = PublicDecryptionResponsePayload { + verification_key: bc2wrap::serialize(&fresh_vk).unwrap(), + ..pivot.clone() + }; + let bad_signatures = sign_ecdsa_public_decrypt_result( + &sk0, + bad_value.clone(), + &ext_handles_bytes, + extra_data.clone(), + &alloy_domain, + ) + .signatures; + assert!( + check_public_decrypt_signatures( + &ctx, + &bad_value, + 1, + &fresh_vk, + &[], + &[], + &bad_signatures, + &extra_data, + ) + .is_err() + ); + + // A node from before the list is authenticated by `external_signature`. + assert!(verify(&[], &signed.external_signature, &[])); + + // The internal signature does not stand in for the EIP-712 forms: it covers + // the payload alone, not the handles or the extra data the EIP-712 message + // binds, so it only confirms the party and the requested ECDSA is left + // unverified. + assert!(!verify(&signed.signature, &[], &[])); + + // A corrupt internal signature is a rejection, beside a valid + // `external_signature` and beside a valid list alike. let mut bad_internal = signed.signature.clone(); bad_internal[0] ^= 1; - assert!(!verify_public_decrypt_signatures( - &with_domain, - &pivot, - 1, - &vk_of(&pivot), - &bad_internal, - &[], - &signatures, - &extra_data, + assert!(!verify(&bad_internal, &signed.external_signature, &[])); + assert!(!verify(&bad_internal, &[], &signatures)); + + // Beside the ECDSA entry, `external_signature` has to be a copy of that entry. + assert!(verify(&[], &signed.external_signature, &signatures)); + let mut bad_external = signed.external_signature.clone(); + bad_external[0] ^= 1; + assert!(!verify(&[], &bad_external, &signatures)); + + // A response as the server sends it, with all three forms, verifies. + assert!(verify( + &signed.signature, + &signed.external_signature, + &signatures )); } - /// The EIP-712 domain is a precondition of the ECDSA entry alone, so a - /// request that asked only for a post-quantum scheme is verified without one, - /// while a request that also asked for ECDSA needs both the domain and the - /// ECDSA entry. + /// A request that asked only for a post-quantum scheme is satisfied by that entry + /// of the list, a request that also asked for ECDSA needs the ECDSA entry too, and + /// the list may be a superset of what was asked for. #[test] - fn test_public_decrypt_signatures_without_an_eip712_domain() { + fn test_public_decrypt_signatures_follow_the_requested_schemes() { use crate::cryptography::signatures::test_support::seeded_identity; let mut rng = AesRng::seed_from_u64(77); @@ -2391,31 +2218,22 @@ mod tests { .map(TypedSignature::from) .collect(); + let domain = dummy_domain(); let server_pks = HashMap::from([(1u32, vk.clone())]); let scheme_verf_keys = HashMap::from([( 1u32, VerfKeySet::from_identity(&identity, &[scheme]).unwrap(), )]); - let request_for = |schemes: Vec| PublicDecryptionRequest { - signing_schemes: schemes, - ..Default::default() - }; - // No domain in either context: what differs is only what was asked for. + let request_for = + |schemes: Vec| public_decrypt_request(&domain, &[], &extra_data, schemes); + // What differs between the contexts is only what was asked for. let ctx_for = |request| { - PublicDecTrustedValidationContext::new( - &server_pks, - &scheme_verf_keys, - None, - &[], - None, - request, - ) - .unwrap() + PublicDecTrustedValidationContext::new(&server_pks, &scheme_verf_keys, request).unwrap() }; // This response carries no deprecated internal field, so the post-quantum // entry of `signatures` is the only thing that can authenticate it. let verify = |ctx: &PublicDecTrustedValidationContext, response_extra_data: &[u8]| { - verify_public_decrypt_signatures( + check_public_decrypt_signatures( ctx, &payload, 1, @@ -2425,12 +2243,13 @@ mod tests { &signatures, response_extra_data, ) + .is_ok() }; let pq_only = request_for(vec![kms_grpc::kms::v1::SigningSchemeType::Mldsa65 as i32]); - let pq_ctx = ctx_for(Some(&pq_only)); + let pq_ctx = ctx_for(&pq_only); - // No domain, yet the post-quantum entry the request asked for verifies. + // The post-quantum entry the request asked for verifies on its own. assert!(verify(&pq_ctx, &extra_data)); // The extra data is part of what that entry covers. @@ -2442,14 +2261,14 @@ mod tests { kms_grpc::kms::v1::SigningSchemeType::Mldsa65 as i32, kms_grpc::kms::v1::SigningSchemeType::Ecdsa256k1 as i32, ]); - let composite_ctx = ctx_for(Some(&composite)); + let composite_ctx = ctx_for(&composite); assert!(!verify(&composite_ctx, &extra_data)); - // Neither is an absent request, which names nothing and so means ECDSA: - // the domain that entry needs is missing, and the list has no ECDSA entry - // to check in any case. - let no_request_ctx = ctx_for(None); - assert!(!verify(&no_request_ctx, &extra_data)); + // Neither is a request that names no scheme, which means ECDSA, for which + // the list has no entry. + let no_scheme = request_for(vec![]); + let no_scheme_ctx = ctx_for(&no_scheme); + assert!(!verify(&no_scheme_ctx, &extra_data)); // A response signed under a superset of the request is accepted: the // post-quantum entry is checked against the set the response presents, @@ -2467,7 +2286,8 @@ mod tests { .map(TypedSignature::from) .collect(); let verify_list = |ctx: &PublicDecTrustedValidationContext, list: &[TypedSignature]| { - verify_public_decrypt_signatures(ctx, &payload, 1, &vk, &[], &[], list, &extra_data) + check_public_decrypt_signatures(ctx, &payload, 1, &vk, &[], &[], list, &extra_data) + .is_ok() }; assert!(verify_list(&pq_ctx, &superset_signatures)); @@ -2481,15 +2301,9 @@ mod tests { kms_grpc::kms::v1::SigningSchemeType::Ed25519 as i32, kms_grpc::kms::v1::SigningSchemeType::Mldsa65 as i32, ]); - let superset_ctx = PublicDecTrustedValidationContext::new( - &server_pks, - &superset_keys, - None, - &[], - None, - Some(&superset_request), - ) - .unwrap(); + let superset_ctx = + PublicDecTrustedValidationContext::new(&server_pks, &superset_keys, &superset_request) + .unwrap(); assert!(verify_list(&superset_ctx, &superset_signatures)); // Dropping the unrequested entry changes the presented set, which the @@ -2551,31 +2365,55 @@ mod tests { #[test] fn test_validate_new_mpc_epoch_request() { - // When previous_epoch is set but domain is None, optional_protobuf_to_alloy_domain - // should fail, and validate_new_mpc_epoch_request should surface InvalidArgument. + let epoch_id = derive_request_id("test_validate_new_mpc_epoch_request").unwrap(); + // Every case but the first carries a valid epoch ID, so that each can only be + // rejected for the reason it names. + let resharing_req = || NewMpcEpochRequest { + signing_schemes: vec![kms_grpc::kms::v1::SigningSchemeType::Ecdsa256k1 as i32], + epoch_id: Some(epoch_id.into()), + previous_epoch: Some(PreviousEpochInfo::default()), + domain: Some(alloy_to_protobuf_domain(&dummy_domain()).unwrap()), + ..Default::default() + }; + + // A request without an epoch ID is rejected. { let req = NewMpcEpochRequest { - signing_schemes: vec![kms_grpc::kms::v1::SigningSchemeType::Ecdsa256k1 as i32], - previous_epoch: Some(PreviousEpochInfo::default()), - domain: None, - ..Default::default() + epoch_id: None, + ..resharing_req() }; let err = validate_new_mpc_epoch_request(req) - .expect_err("request without domain must be rejected"); + .expect_err("request without epoch ID must be rejected"); assert_eq!(err.code(), tonic::Code::InvalidArgument); } - // Happy path + // A resharing request has to carry the EIP-712 domain its results are signed under. { let req = NewMpcEpochRequest { - signing_schemes: vec![kms_grpc::kms::v1::SigningSchemeType::Ecdsa256k1 as i32], - previous_epoch: Some(PreviousEpochInfo::default()), - domain: Some(alloy_to_protobuf_domain(&dummy_domain()).unwrap()), - ..Default::default() + domain: None, + ..resharing_req() }; let err = validate_new_mpc_epoch_request(req) - .expect_err("request without domain must be rejected"); + .expect_err("resharing request without domain must be rejected"); assert_eq!(err.code(), tonic::Code::InvalidArgument); } + // A resharing request with a domain is accepted, and keeps that domain. + { + let verified = validate_new_mpc_epoch_request(resharing_req()).unwrap(); + let resharing = verified + .resharing + .expect("a request with a previous epoch is a resharing"); + assert_eq!(resharing.signing_domain, dummy_domain()); + } + // Without a previous epoch nothing is reshared, so no domain is needed. + { + let req = NewMpcEpochRequest { + previous_epoch: None, + domain: None, + ..resharing_req() + }; + let verified = validate_new_mpc_epoch_request(req).unwrap(); + assert!(verified.resharing.is_none()); + } } #[test] @@ -2719,22 +2557,14 @@ mod tests { let (vk0, _sk0) = gen_sig_keys(&mut rng); let (vk1, _sk1) = gen_sig_keys(&mut rng); let server_pks = HashMap::from([(1, vk0.clone()), (2, vk1.clone())]); + let request = public_decrypt_request(&dummy_domain(), &[], &[], vec![]); - PublicDecTrustedValidationContext::new(&server_pks, &HashMap::new(), None, &[], None, None) - .unwrap(); + PublicDecTrustedValidationContext::new(&server_pks, &HashMap::new(), &request).unwrap(); // Error if the server_pks has duplicate keys let server_pks = HashMap::from([(1, vk1.clone()), (2, vk1)]); assert!( - PublicDecTrustedValidationContext::new( - &server_pks, - &HashMap::new(), - None, - &[], - None, - None - ) - .is_err() + PublicDecTrustedValidationContext::new(&server_pks, &HashMap::new(), &request).is_err() ); } diff --git a/core/service/src/engine/validation_wasm.rs b/core/service/src/engine/validation_wasm.rs index 4ed8daef4..793cedcc9 100644 --- a/core/service/src/engine/validation_wasm.rs +++ b/core/service/src/engine/validation_wasm.rs @@ -158,8 +158,6 @@ pub(crate) struct Eip712VerificationParams<'a> { pub trusted_eip712_domain: &'a Eip712Domain, } -const ERR_VALIDATE_USER_DECRYPTION_MISSING_SIGNATURE: &str = - "Missing signature in user decryption response"; const ERR_VALIDATE_USER_DECRYPTION_ID_NOT_FOUND: &str = "ID claimed in payload not found"; const ERR_VALIDATE_USER_DECRYPTION_WRONG_ADDRESS: &str = "ID or address claimed in payload is incorrect"; @@ -278,9 +276,9 @@ pub(crate) struct SignedPayloads<'a, T> { pub internal_bytes: &'a [u8], /// The payload every non-ECDSA scheme covers. pub payload: &'a T, - /// The EIP-712 signing hash both ECDSA signatures recover from, or `None` when no - /// domain is available and neither can therefore be checked. - pub eip712_hash: Option, + /// The EIP-712 signing hash both ECDSA signatures recover from. Every result is + /// signed under a domain, so the verifier always has one. + pub eip712_hash: B256, } /// The party a response has to belong to. @@ -407,11 +405,11 @@ fn attribute_scheme_entry( /// - First, before any cryptography, that `list` has an entry for every requested /// scheme. Only when `list` is empty may a deprecated field meet a requested ECDSA /// instead. +/// - Beside an ECDSA entry of `list`, that a non-empty `external_signature` equals it. /// - The deprecated internal `signature`, when the result kind carries one. It covers the -/// payload alone, so it satisfies a requested ECDSA only when no EIP-712 domain is -/// available. -/// - The deprecated `external_signature`, whenever an EIP-712 domain is available. It is -/// checked *in addition to* the internal one, not instead of it. +/// payload alone, so it never satisfies a requested ECDSA. +/// - The deprecated `external_signature`, whenever it is present. It is checked *in +/// addition to* the internal one, not instead of it. /// - Every entry of `list` for a requested scheme. An entry for a scheme nobody asked /// for is not checked, and neither is an entry of a scheme this release does not /// know, so a response signed under a superset of the request is accepted. @@ -472,6 +470,25 @@ where wire_scheme_bound_preimage(&presented, payloads.payload) .map_err(|e| anyhow_tracked(format!("could not build the signed payload: {e}")))? }; + // A server signs `external_signature` and the requested ECDSA entry of `list` over + // the same EIP-712 hash with deterministic ECDSA, so the two are byte-identical. + let ecdsa = SigningSchemeType::Ecdsa256k1.as_wire(); + let ecdsa_entries: Vec<&[u8]> = sigs + .list + .iter() + .filter(|typed| typed.scheme == ecdsa) + .map(|typed| typed.signature.as_slice()) + .collect(); + if requested.contains(&SigningSchemeType::Ecdsa256k1) + && !sigs.external.is_empty() + && !ecdsa_entries.is_empty() + && !ecdsa_entries.contains(&sigs.external) + { + return Err(anyhow_tracked( + "the deprecated external signature of the response differs from its ECDSA entry" + .to_string(), + )); + } let mut verified: Vec = Vec::with_capacity(sigs.list.len() + 1); let mut signer: Option<(u32, Address)> = None; @@ -506,29 +523,19 @@ where "the deprecated internal signature of party {party_id} did not verify: {e}" )) })?; - signer = Some((*party_id, *address)); // The internal signature covers the response payload alone, not the fields the - // EIP-712 message binds, such as the handles and the extra data. So it meets a - // requested ECDSA only when no EIP-712 form can be checked; with a domain, one of - // the two EIP-712 forms has to verify. - if payloads.eip712_hash.is_none() { - push_once(&mut verified, SigningSchemeType::Ecdsa256k1); - } + // EIP-712 message binds, such as the handles and the extra data. So it never meets + // a requested ECDSA; one of the two EIP-712 forms has to. + signer = Some((*party_id, *address)); } if !sigs.external.is_empty() { - match payloads.eip712_hash.as_ref() { - Some(hash) => { - let found = - expected.attribute(recover_address_from_eip712_hash(hash, sigs.external)?)?; - signer = Some(agree(signer, found)?); - push_once(&mut verified, SigningSchemeType::Ecdsa256k1); - } - None => tracing::warn!( - "No EIP-712 domain is available, so the deprecated external signature of a \ - response cannot be checked" - ), - } + let found = expected.attribute(recover_address_from_eip712_hash( + &payloads.eip712_hash, + sigs.external, + )?)?; + signer = Some(agree(signer, found)?); + push_once(&mut verified, SigningSchemeType::Ecdsa256k1); } for typed in sigs.list { @@ -548,15 +555,10 @@ where continue; } let found = if scheme == SigningSchemeType::Ecdsa256k1 { - // Only this entry is an EIP-712 signature, so only it needs the domain. - let Some(hash) = payloads.eip712_hash.as_ref() else { - tracing::warn!( - "No EIP-712 domain is available, so the ECDSA entry of a response cannot \ - be checked" - ); - continue; - }; - expected.attribute(recover_address_from_eip712_hash(hash, &typed.signature)?)? + expected.attribute(recover_address_from_eip712_hash( + &payloads.eip712_hash, + &typed.signature, + )?)? } else { attribute_scheme_entry( &typed.signature, @@ -590,7 +592,7 @@ where /// /// Returns the (verified) role and verification key it deserialized, so the caller can reuse it without /// parsing the raw bytes a second time. -fn authenticate_user_decrypt_and_check_meta_data( +pub(crate) fn authenticate_user_decrypt_and_check_meta_data( trusted_ctx: &UserDecTrustedValidationContext, response: &UserDecryptionResponsePayload, signature: &[u8], @@ -619,14 +621,6 @@ fn authenticate_user_decrypt_and_check_meta_data( )); } - // A response has to carry at least one of the two deprecated fields until 0.16, so - // that a node from a release before `signatures` stays verifiable. - if signature.is_empty() && eip712_params.response_external_signature.is_empty() { - return Err(anyhow_error_and_log( - ERR_VALIDATE_USER_DECRYPTION_MISSING_SIGNATURE, - )); - } - let response_bytes = bc2wrap::serialize(&response)?; verify_response_signatures( &ResponseSignatures { @@ -638,11 +632,11 @@ fn authenticate_user_decrypt_and_check_meta_data( dsep: &DSEP_USER_DECRYPTION, internal_bytes: &response_bytes, payload: &user_dec_payload(&response_bytes, eip712_params.response_extra_data), - eip712_hash: Some(user_decrypt_eip712_hash( + eip712_hash: user_decrypt_eip712_hash( response, trusted_ctx.client_request, eip712_params.trusted_eip712_domain, - )?), + )?, }, trusted_ctx.client_request.signing_schemes(), &ExpectedSigner::Known { @@ -978,8 +972,7 @@ mod tests { cryptography::{ encryption::{Encryption, PkeScheme, PkeSchemeType}, signatures::{ - ERR_EXT_USER_DECRYPTION_SIG_BAD_LENGTH, NodeSigningIdentity, PrivateSigKey, - PublicSigKey, gen_sig_keys, internal_sign, + NodeSigningIdentity, PrivateSigKey, PublicSigKey, gen_sig_keys, internal_sign, }, signing::SigningSchemeType, }, @@ -996,11 +989,9 @@ mod tests { }; use super::{ - DSEP_USER_DECRYPTION, ERR_VALIDATE_USER_DECRYPTION_MISSING_SIGNATURE, - ERR_VALIDATE_USER_DECRYPTION_NOT_ENOUGH_RESP, Eip712VerificationParams, ExpectedSigner, - ResponseSignatures, SignedPayloads, UserDecTrustedValidationContext, - UserDecryptionInvariants, user_decrypt_eip712_hash, validate_user_decrypt_responses, - verify_response_signatures, + DSEP_USER_DECRYPTION, ERR_VALIDATE_USER_DECRYPTION_NOT_ENOUGH_RESP, + Eip712VerificationParams, UserDecTrustedValidationContext, UserDecryptionInvariants, + validate_user_decrypt_responses, }; /// Asking for no scheme at all is a rejection, whatever the response verified @@ -1046,154 +1037,6 @@ mod tests { .external_signature) } - #[test] - fn test_verify_response_signatures_external_user_decryption() { - let mut rng = AesRng::seed_from_u64(0); - let (vk0, sk0) = gen_sig_keys(&mut rng); - let (vk1, _sk1) = gen_sig_keys(&mut rng); - let (vk2, _sk2) = gen_sig_keys(&mut rng); - let pks: HashMap = HashMap::from_iter( - [vk0, vk1, vk2] - .into_iter() - .enumerate() - .map(|(i, k)| (i as u32 + 1, k)), - ); - let kms_addrs = pks - .iter() - .map(|(i, pk)| (*i, pk.address())) - .collect::>(); - - let mut encryption = Encryption::new(PkeSchemeType::MlKem512, &mut rng); - let (_eph_client_sk, eph_client_pk) = encryption.keygen().unwrap(); - let (client_vk, _client_sk) = gen_sig_keys(&mut rng); - - let ciphertext_handle = vec![5, 6, 7, 8]; - - let mut enc_key_buf = Vec::new(); - tfhe::safe_serialization::safe_serialize( - &eph_client_pk, - &mut enc_key_buf, - crate::consts::SAFE_SER_SIZE_LIMIT, - ) - .unwrap(); - - let domain = dummy_domain(); - let extra_data = vec![1, 2, 3, 4]; - let request = ParsedUserDecryptionRequest::new( - None, // No signature is needed - client_vk.address(), - enc_key_buf, - vec![CiphertextHandle::new(ciphertext_handle.clone())], - domain.verifying_contract.unwrap(), - vec![SigningSchemeType::Ecdsa256k1], - extra_data, - ); - - let payload = UserDecryptionResponsePayload { - verification_key: bc2wrap::serialize(&pks[&1]).unwrap(), - digest: vec![1, 2, 3, 4], - signcrypted_ciphertexts: vec![TypedSigncryptedCiphertext { - fhe_type: tfhe::FheTypes::Uint4 as i32, - signcrypted_ciphertext: vec![1, 2, 3, 4], - external_handle: ciphertext_handle.clone(), - packing_factor: 1, - }], - party_id: 1, - degree: 1, - }; - let external_sig = compute_external_user_decrypt_signature( - &sk0, - &payload, - &domain, - request.enc_key(), - request.extra_data(), - ) - .unwrap(); - - let verify = |external: &[u8], - response: &UserDecryptionResponsePayload, - eip712_domain: &Eip712Domain| { - let response_bytes = bc2wrap::serialize(response).unwrap(); - verify_response_signatures( - &ResponseSignatures { - internal: &[], - external, - list: &[], - }, - &SignedPayloads { - dsep: &DSEP_USER_DECRYPTION, - internal_bytes: &response_bytes, - payload: &super::user_dec_payload(&response_bytes, request.extra_data()), - eip712_hash: Some( - user_decrypt_eip712_hash(response, &request, eip712_domain).unwrap(), - ), - }, - request.signing_schemes(), - &ExpectedSigner::Known { - party_id: 1, - address: kms_addrs[&1], - verf_key: &pks[&1], - }, - &HashMap::new(), - ) - }; - - // incorrect external signature length - { - assert!( - verify(&external_sig[0..64], &payload, &domain) - .unwrap_err() - .to_string() - .contains(ERR_EXT_USER_DECRYPTION_SIG_BAD_LENGTH) - ); - } - - // bad signature due to bad signing key - { - let (_vk_bad, sk_bad) = gen_sig_keys(&mut rng); - let bad_external_sig = compute_external_user_decrypt_signature( - &sk_bad, - &payload, - &domain, - request.enc_key(), - request.extra_data(), - ) - .unwrap(); - assert!(verify(&bad_external_sig, &payload, &domain).is_err()); - } - - // bad signature due to bad domain - { - let bad_domain = alloy_sol_types::eip712_domain!( - name: "Authorization token", - version: "1", - chain_id: 1234, // incorrect chain ID - verifying_contract: alloy_primitives::address!("66f9664f97F2b50F62D13eA064982f936dE76657"), - ); - assert!(verify(&external_sig, &payload, &bad_domain).is_err()); - } - - // check that we detect the error if payload is modified - { - let mut bad_payload = payload.clone(); - bad_payload.party_id = 2; // modify ID - assert!( - verify(&external_sig, &bad_payload, &domain) - .unwrap_err() - .to_string() - .contains("an ECDSA signature of party 1 recovered to") - ); - } - - // happy path - { - assert_eq!( - verify(&external_sig, &payload, &domain).unwrap(), - (1, kms_addrs[&1]) - ); - } - } - #[test] fn test_validate_user_decrypt_meta_data_and_signature() { let mut rng = AesRng::seed_from_u64(0); @@ -1272,27 +1115,6 @@ mod tests { // done here — they are a single `UserDecryptionInvariants` equality in // `classify_user_decrypt_response`, exercised via `test_validate_user_decrypt_responses`. - // no signatures are provided - { - let params = Eip712VerificationParams { - response_external_signature: &[], - response_extra_data: &extra_data, - trusted_eip712_domain: &dummy_domain, - }; - assert!( - authenticate_user_decrypt_and_check_meta_data( - &trusted_ctx, - &pivot_resp, - &[], // the ECDSA signature may be empty, thus we check the external one - &[], - ¶ms, - ) - .unwrap_err() - .to_string() - .contains(ERR_VALIDATE_USER_DECRYPTION_MISSING_SIGNATURE) - ); - } - // if the ID is changed to something that does not exist, return error { let mut other_resp = pivot_resp.clone(); @@ -1316,27 +1138,6 @@ mod tests { ); } - // no signatures are provided - { - let params = Eip712VerificationParams { - response_external_signature: &[], - response_extra_data: &extra_data, - trusted_eip712_domain: &dummy_domain, - }; - assert!( - authenticate_user_decrypt_and_check_meta_data( - &trusted_ctx, - &pivot_resp, - &[], // the ECDSA signature may be empty, thus we check the external one - &[], - ¶ms, - ) - .unwrap_err() - .to_string() - .contains(ERR_VALIDATE_USER_DECRYPTION_MISSING_SIGNATURE) - ); - } - // if the ID is changed to something that does not exist, return error { let mut other_resp = pivot_resp.clone(); @@ -1360,7 +1161,7 @@ mod tests { ); } - // Signature failures are covered by `test_verify_response_signatures_external_user_decryption`. + // The response has to echo the request's extra data. { let pivot_buf = bc2wrap::serialize(&pivot_resp).unwrap(); let signature_buf = internal_sign(&DSEP_USER_DECRYPTION, &pivot_buf, &sk0) @@ -1402,6 +1203,54 @@ mod tests { .unwrap(); } + // The EIP-712 message covers the whole payload and is bound to the domain, so the + // same external signature fails for a changed payload or under another domain. + { + let params = Eip712VerificationParams { + response_external_signature: &external_signature, + response_extra_data: &extra_data, + trusted_eip712_domain: &dummy_domain, + }; + let changed = UserDecryptionResponsePayload { + degree: 2, + ..pivot_resp.clone() + }; + let err = authenticate_user_decrypt_and_check_meta_data( + &trusted_ctx, + &changed, + &[], + &[], + ¶ms, + ) + .unwrap_err() + .to_string(); + assert!( + err.contains("an ECDSA signature of party 1 recovered to"), + "{err}" + ); + + let other_domain = alloy_sol_types::eip712_domain!( + name: "Authorization token", + version: "1", + chain_id: 1234, // incorrect chain ID + verifying_contract: alloy_primitives::address!("66f9664f97F2b50F62D13eA064982f936dE76657"), + ); + let params = Eip712VerificationParams { + trusted_eip712_domain: &other_domain, + ..params + }; + assert!( + authenticate_user_decrypt_and_check_meta_data( + &trusted_ctx, + &pivot_resp, + &[], + &[], + ¶ms, + ) + .is_err() + ); + } + // The internal signature alone is not enough: a domain is always available for user // decryption, and the internal signature covers the payload only, so the requested // ECDSA has to be met by an EIP-712 form.