diff --git a/packages/rs-dpp/src/address_funds/platform_address.rs b/packages/rs-dpp/src/address_funds/platform_address.rs index 248eb209588..e1cb3540a7d 100644 --- a/packages/rs-dpp/src/address_funds/platform_address.rs +++ b/packages/rs-dpp/src/address_funds/platform_address.rs @@ -12,12 +12,11 @@ use bincode::{Decode, DecodeUntrusted, Encode}; use dashcore::address::Payload; use dashcore::blockdata::script::ScriptBuf; use dashcore::hashes::sha256d; -use dashcore::{ - sign_message, signer, Address, Network, PrivateKey, PubkeyHash, PublicKey, ScriptHash, -}; +use dashcore::{sign_message, Address, Network, PrivateKey, PubkeyHash, PublicKey, ScriptHash}; use platform_serialization_derive::{ PlatformDeserializeTrusted, PlatformDeserializeUntrusted, PlatformSerialize, }; +use platform_version::version::PlatformVersion; #[cfg(feature = "serde-conversion")] use serde::{Deserialize, Serialize}; use std::convert::TryFrom; @@ -473,14 +472,19 @@ impl PlatformAddress { /// # Arguments /// * `witness` - The witness containing signature(s) and either a public key (P2PKH) or redeem script (P2SH) /// * `signable_bytes` - The data that was signed (will be double-SHA256 hashed internally) + /// * `platform_version` - Selects `verify_hash_signature` for a P2PKH witness: from protocol + /// version 15 a high-S signature and a header naming an uncompressed key are refused. /// /// # Returns /// * `Ok(AddressWitnessVerificationOperations)` - Operations performed if verification succeeds - /// * `Err(ProtocolError)` if verification fails + /// * `Err(ProtocolError::UnknownVersionMismatch)` if this build does not know the selected + /// `verify_hash_signature` version + /// * `Err(ProtocolError)` of another kind if verification fails pub fn verify_bytes_against_witness( &self, witness: &AddressWitness, signable_bytes: &[u8], + platform_version: &PlatformVersion, ) -> Result { match (self, witness) { (PlatformAddress::P2pkh(pubkey_hash), AddressWitness::P2pkh { signature }) => { @@ -492,13 +496,18 @@ impl PlatformAddress { // This saves 33 bytes per witness (no need to include pubkey) // at a ~4% CPU cost increase (recovery vs verify). let data_hash = dashcore::signer::double_sha(signable_bytes); - signer::verify_hash_signature(&data_hash, signature.as_slice(), pubkey_hash) - .map_err(|e| { - ProtocolError::AddressWitnessError(format!( - "P2PKH signature verification failed: {}", - e - )) - })?; + ecdsa::verify_hash_signature( + &data_hash, + signature.as_slice(), + pubkey_hash, + platform_version, + )? + .map_err(|e| { + ProtocolError::AddressWitnessError(format!( + "P2PKH signature verification failed: {}", + e + )) + })?; Ok(AddressWitnessVerificationOperations::for_p2pkh( signable_bytes.len(), @@ -896,7 +905,11 @@ mod tests { }; // Verify should succeed - let result = address.verify_bytes_against_witness(&witness, signable_bytes); + let result = address.verify_bytes_against_witness( + &witness, + signable_bytes, + PlatformVersion::latest(), + ); assert!( result.is_ok(), "P2PKH verification should succeed: {:?}", @@ -925,7 +938,8 @@ mod tests { }; // Verify should fail (recovered pubkey won't match because message differs) - let result = address.verify_bytes_against_witness(&witness, verify_bytes); + let result = + address.verify_bytes_against_witness(&witness, verify_bytes, PlatformVersion::latest()); assert!( result.is_err(), "P2PKH verification should fail with wrong data" @@ -954,7 +968,11 @@ mod tests { }; // Verify should fail (recovered pubkey hash won't match address) - let result = address.verify_bytes_against_witness(&witness, signable_bytes); + let result = address.verify_bytes_against_witness( + &witness, + signable_bytes, + PlatformVersion::latest(), + ); assert!( result.is_err(), "P2PKH verification should fail when signed with wrong key" @@ -995,7 +1013,11 @@ mod tests { }; // Verify should succeed - let result = address.verify_bytes_against_witness(&witness, signable_bytes); + let result = address.verify_bytes_against_witness( + &witness, + signable_bytes, + PlatformVersion::latest(), + ); assert!( result.is_ok(), "P2SH 2-of-3 multisig verification should succeed: {:?}", @@ -1032,7 +1054,11 @@ mod tests { }; // Verify should succeed - let result = address.verify_bytes_against_witness(&witness, signable_bytes); + let result = address.verify_bytes_against_witness( + &witness, + signable_bytes, + PlatformVersion::latest(), + ); assert!( result.is_ok(), "P2SH 2-of-3 multisig with keys 1 and 2 should succeed: {:?}", @@ -1068,7 +1094,11 @@ mod tests { }; // Verify should fail - let result = address.verify_bytes_against_witness(&witness, signable_bytes); + let result = address.verify_bytes_against_witness( + &witness, + signable_bytes, + PlatformVersion::latest(), + ); assert!( result.is_err(), "P2SH should fail with only 1 signature when 2 required" @@ -1107,7 +1137,11 @@ mod tests { }; // Verify should fail (script doesn't hash to address) - let result = address.verify_bytes_against_witness(&witness, signable_bytes); + let result = address.verify_bytes_against_witness( + &witness, + signable_bytes, + PlatformVersion::latest(), + ); assert!( result.is_err(), "P2SH should fail when script hash doesn't match address" @@ -1148,8 +1182,11 @@ mod tests { let p2pkh_witness = AddressWitness::P2pkh { signature: BinaryData::new(p2pkh_sig), }; - let p2pkh_result = - p2pkh_address.verify_bytes_against_witness(&p2pkh_witness, signable_bytes); + let p2pkh_result = p2pkh_address.verify_bytes_against_witness( + &p2pkh_witness, + signable_bytes, + PlatformVersion::latest(), + ); assert!( p2pkh_result.is_ok(), "P2PKH redemption should succeed: {:?}", @@ -1163,7 +1200,11 @@ mod tests { signatures: vec![BinaryData::new(p2sh_sig0), BinaryData::new(p2sh_sig2)], redeem_script: BinaryData::new(redeem_script), }; - let p2sh_result = p2sh_address.verify_bytes_against_witness(&p2sh_witness, signable_bytes); + let p2sh_result = p2sh_address.verify_bytes_against_witness( + &p2sh_witness, + signable_bytes, + PlatformVersion::latest(), + ); assert!( p2sh_result.is_ok(), "P2SH redemption should succeed: {:?}", @@ -1192,7 +1233,11 @@ mod tests { signatures: vec![BinaryData::new(vec![0x30, 0x44])], redeem_script: BinaryData::new(vec![0x52]), }; - let result = p2pkh_address.verify_bytes_against_witness(&p2sh_witness, signable_bytes); + let result = p2pkh_address.verify_bytes_against_witness( + &p2sh_witness, + signable_bytes, + PlatformVersion::latest(), + ); assert!(result.is_err()); assert!(result .unwrap_err() @@ -1203,7 +1248,11 @@ mod tests { let p2pkh_witness = AddressWitness::P2pkh { signature: BinaryData::new(vec![0x30, 0x44]), }; - let result = p2sh_address.verify_bytes_against_witness(&p2pkh_witness, signable_bytes); + let result = p2sh_address.verify_bytes_against_witness( + &p2pkh_witness, + signable_bytes, + PlatformVersion::latest(), + ); assert!(result.is_err()); assert!(result .unwrap_err() diff --git a/packages/rs-dpp/src/state_transition/mod.rs b/packages/rs-dpp/src/state_transition/mod.rs index ce7e77c4195..7c0b0bdf293 100644 --- a/packages/rs-dpp/src/state_transition/mod.rs +++ b/packages/rs-dpp/src/state_transition/mod.rs @@ -2404,6 +2404,7 @@ impl StateTransition { fn verify_ecdsa_hash_160_signature_by_public_key_hash( &self, public_key_hash: &[u8], + platform_version: &PlatformVersion, ) -> Result<(), ProtocolError> { let Some(signature) = self.signature() else { return Err(ProtocolError::InvalidVerificationWrongNumberOfElements { @@ -2419,15 +2420,19 @@ impl StateTransition { } let data = self.signable_bytes()?; let data_hash = double_sha(data); - signer::verify_hash_signature(&data_hash, signature.as_slice(), public_key_hash).map_err( - |e| { - ProtocolError::from(ConsensusError::SignatureError( - SignatureError::InvalidStateTransitionSignatureError( - InvalidStateTransitionSignatureError::new(e.to_string()), - ), - )) - }, - ) + ecdsa::verify_hash_signature( + &data_hash, + signature.as_slice(), + public_key_hash, + platform_version, + )? + .map_err(|e| { + ProtocolError::from(ConsensusError::SignatureError( + SignatureError::InvalidStateTransitionSignatureError( + InvalidStateTransitionSignatureError::new(e.to_string()), + ), + )) + }) } #[cfg(feature = "state-transition-validation")] @@ -3428,6 +3433,91 @@ mod tests { } } + // An ECDSA_HASH160 key is checked by recovering the public key, which takes any S and either + // header form. From protocol version 15 (`verify_hash_signature` 1) the two twins a third + // party can derive from a valid signature, the high-S one and the one whose header names an + // uncompressed key, are refused; protocol version 14 keeps accepting them for replay. + // Masternode votes, signed by an ECDSA_HASH160 VOTING key, take the same path. + #[cfg(feature = "state-transition-signing")] + #[test] + fn should_refuse_an_ecdsa_hash160_signature_twin_from_protocol_version_15() { + use crate::identity::identity_public_key::v0::IdentityPublicKeyV0; + use crate::tests::utils::{high_s_twin, uncompressed_header_twin}; + use secp256k1::{PublicKey as SecpPublicKey, SecretKey}; + + let bls = crate::bls::native_bls::NativeBlsModule; + let private_key = [0x21; 32]; + let public_key = SecpPublicKey::from_secret_key( + &SecretKey::from_secret_bytes(private_key).expect("a valid ECDSA private key"), + ); + let key_hash = ripemd160_sha256(&public_key.serialize()); + + let latest = PlatformVersion::latest(); + let last_without_the_check = PlatformVersion::get(14).expect("protocol version 14"); + + for (transition, key_id, purpose, label) in [ + (sample_transfer_st(), 11, Purpose::TRANSFER, "transfer"), + ( + sample_masternode_vote_st(), + 5, + Purpose::VOTING, + "masternode vote", + ), + ] { + let key: IdentityPublicKey = IdentityPublicKeyV0 { + id: key_id, + purpose, + security_level: SecurityLevel::CRITICAL, + key_type: KeyType::ECDSA_HASH160, + data: key_hash.to_vec().into(), + ..Default::default() + } + .into(); + + let mut signed = transition; + signed + .sign_by_private_key(&private_key, KeyType::ECDSA_HASH160, &bls) + .expect("the key signs"); + let signature = signed.signature().expect("signed").to_vec(); + + for platform_version in [latest, last_without_the_check] { + signed + .verify_identity_signed_signature(&key, &bls, platform_version) + .unwrap_or_else(|e| { + panic!( + "{label}: the low-S signature verifies at protocol version {}: {e:?}", + platform_version.protocol_version + ) + }); + } + for (twin, twin_label) in [ + (high_s_twin(&signature), "high-S twin"), + ( + uncompressed_header_twin(&signature), + "uncompressed header twin", + ), + ] { + let mut malleated = signed.clone(); + malleated.set_signature(BinaryData::new(twin)); + malleated + .verify_identity_signed_signature(&key, &bls, last_without_the_check) + .unwrap_or_else(|e| { + panic!("{label}: protocol version 14 accepts the {twin_label}: {e:?}") + }); + let error = malleated + .verify_identity_signed_signature(&key, &bls, latest) + .expect_err("the twin is refused"); + assert!( + matches!(&error, ProtocolError::ConsensusError(consensus_error) + if matches!(&**consensus_error, ConsensusError::SignatureError( + SignatureError::InvalidStateTransitionSignatureError(_) + ))), + "{label}, {twin_label}: {error:?}" + ); + } + } + } + fn sample_batch_st_with_delete() -> StateTransition { let base = DocumentBaseTransition::V0(DocumentBaseTransitionV0 { id: Identifier::from([1u8; 32]), diff --git a/packages/rs-dpp/src/state_transition/state_transitions/address_funds/address_funding_from_asset_lock_transition/signing_tests.rs b/packages/rs-dpp/src/state_transition/state_transitions/address_funds/address_funding_from_asset_lock_transition/signing_tests.rs index 94b90c44637..55959db9056 100644 --- a/packages/rs-dpp/src/state_transition/state_transitions/address_funds/address_funding_from_asset_lock_transition/signing_tests.rs +++ b/packages/rs-dpp/src/state_transition/state_transitions/address_funds/address_funding_from_asset_lock_transition/signing_tests.rs @@ -145,7 +145,7 @@ async fn try_from_asset_lock_with_signer_and_private_key_signs_single_p2pkh_inpu .expect("signable_bytes"); let input_addr = v0.inputs.keys().next().expect("one input"); input_addr - .verify_bytes_against_witness(&v0.input_witnesses[0], &signable) + .verify_bytes_against_witness(&v0.input_witnesses[0], &signable, PlatformVersion::latest()) .expect("witness should verify against signable bytes"); } @@ -187,7 +187,7 @@ async fn try_from_asset_lock_with_signer_and_private_key_signs_multiple_inputs() .signable_bytes() .expect("signable_bytes"); for (addr, witness) in v0.inputs.keys().zip(v0.input_witnesses.iter()) { - addr.verify_bytes_against_witness(witness, &signable) + addr.verify_bytes_against_witness(witness, &signable, PlatformVersion::latest()) .expect("witness should verify against signable bytes"); } } @@ -299,6 +299,6 @@ async fn try_from_asset_lock_with_signers_produces_matching_signature() { .expect("signable_bytes"); let input_addr = v0.inputs.keys().next().expect("one input"); input_addr - .verify_bytes_against_witness(&v0.input_witnesses[0], &signable) + .verify_bytes_against_witness(&v0.input_witnesses[0], &signable, PlatformVersion::latest()) .expect("witness should verify against signable bytes"); } diff --git a/packages/rs-dpp/src/state_transition/state_transitions/address_funds/address_funds_transfer_transition/signing_tests.rs b/packages/rs-dpp/src/state_transition/state_transitions/address_funds/address_funds_transfer_transition/signing_tests.rs index 680010c9051..ce1b814137e 100644 --- a/packages/rs-dpp/src/state_transition/state_transitions/address_funds/address_funds_transfer_transition/signing_tests.rs +++ b/packages/rs-dpp/src/state_transition/state_transitions/address_funds/address_funds_transfer_transition/signing_tests.rs @@ -241,7 +241,7 @@ fn verify_transition_signatures( .enumerate() { let _operations = address - .verify_bytes_against_witness(witness, &signable_bytes) + .verify_bytes_against_witness(witness, &signable_bytes, PlatformVersion::latest()) .map_err(|e| { ProtocolError::Generic(format!("Witness {} verification failed: {}", i, e)) })?; diff --git a/packages/rs-dpp/src/state_transition/traits/state_transition_witness_validation.rs b/packages/rs-dpp/src/state_transition/traits/state_transition_witness_validation.rs index ec9d56ce610..132e1c97ae9 100644 --- a/packages/rs-dpp/src/state_transition/traits/state_transition_witness_validation.rs +++ b/packages/rs-dpp/src/state_transition/traits/state_transition_witness_validation.rs @@ -3,6 +3,8 @@ use crate::consensus::signature::InvalidStateTransitionSignatureError; use crate::serialization::Signable; use crate::state_transition::StateTransitionWitnessSigned; use crate::validation::SimpleConsensusValidationResult; +use crate::ProtocolError; +use platform_version::version::PlatformVersion; /// Result of witness validation, containing both the validation result and the operations performed. pub struct WitnessValidationResult { @@ -46,45 +48,61 @@ pub trait StateTransitionWitnessValidation: StateTransitionWitnessSigned + Signa /// /// # Arguments /// * `signable_bytes` - The bytes that were signed (typically from `state_transition.signable_bytes()`) + /// * `platform_version` - Selects `verify_hash_signature` for P2PKH witnesses /// /// # Returns - /// * `WitnessValidationResult` - Contains validation result and operations performed - fn validate_witnesses(&self, signable_bytes: &[u8]) -> WitnessValidationResult { + /// * `Ok(WitnessValidationResult)` - Contains validation result and operations performed + /// * `Err(ProtocolError::UnknownVersionMismatch)` - This build does not know the selected + /// `verify_hash_signature` version + fn validate_witnesses( + &self, + signable_bytes: &[u8], + platform_version: &PlatformVersion, + ) -> Result { let inputs = self.inputs(); let witnesses = self.witnesses(); // Verify witness count matches input count before zipping if inputs.len() != witnesses.len() { - return WitnessValidationResult::new_with_error( + return Ok(WitnessValidationResult::new_with_error( InvalidStateTransitionSignatureError::new(format!( "Number of witnesses ({}) does not match number of inputs ({})", witnesses.len(), inputs.len() )) .into(), - ); + )); } let mut total_operations = AddressWitnessVerificationOperations::new(); // Validate each witness against its corresponding input address for (i, (address, witness)) in inputs.keys().zip(witnesses.iter()).enumerate() { - match address.verify_bytes_against_witness(witness, signable_bytes) { + match address.verify_bytes_against_witness(witness, signable_bytes, platform_version) { Ok(operations) => { total_operations.combine(&operations); } + // A version this build does not know is an internal error, not a witness that + // does not verify. `verify_bytes_against_witness` returns it only from the + // `verify_hash_signature` dispatcher, and every protocol version selects a known + // `verify_hash_signature`, so no witness that maps to a consensus error below is + // turned into this one. + Err(e @ ProtocolError::UnknownVersionMismatch { .. }) => return Err(e), Err(e) => { - return WitnessValidationResult::new_with_error( + return Ok(WitnessValidationResult::new_with_error( InvalidStateTransitionSignatureError::new(format!( "Witness {} verification failed: {}", i, e )) .into(), - ); + )); } } } - WitnessValidationResult::new(SimpleConsensusValidationResult::new(), total_operations) + Ok(WitnessValidationResult::new( + SimpleConsensusValidationResult::new(), + total_operations, + )) } } diff --git a/packages/rs-dpp/src/state_transition/verify_identity_signed_signature/mod.rs b/packages/rs-dpp/src/state_transition/verify_identity_signed_signature/mod.rs index 15b12d00b71..d014e12a81a 100644 --- a/packages/rs-dpp/src/state_transition/verify_identity_signed_signature/mod.rs +++ b/packages/rs-dpp/src/state_transition/verify_identity_signed_signature/mod.rs @@ -12,6 +12,10 @@ impl StateTransition { /// Generation 0 (protocol versions 1 to 13) accepts a BLS12_381 signature that is well /// formed but does not verify: it fails only when the BLS module errors, never on its /// `Ok(false)`. Generation 1 refuses that signature too. + /// + /// An ECDSA_HASH160 key is checked by recovering the public key, through + /// `verify_hash_signature`, which `platform_version` selects too: from protocol version 15 it + /// refuses a high-S signature and a header naming an uncompressed key. pub fn verify_identity_signed_signature( &self, public_key: &IdentityPublicKey, @@ -23,8 +27,8 @@ impl StateTransition { .state_transition_method_versions .verify_identity_signed_signature { - 0 => self.verify_identity_signed_signature_v0(public_key, bls), - 1 => self.verify_identity_signed_signature_v1(public_key, bls), + 0 => self.verify_identity_signed_signature_v0(public_key, bls, platform_version), + 1 => self.verify_identity_signed_signature_v1(public_key, bls, platform_version), version => Err(ProtocolError::UnknownVersionMismatch { method: "StateTransition::verify_identity_signed_signature".to_string(), known_versions: vec![0, 1], diff --git a/packages/rs-dpp/src/state_transition/verify_identity_signed_signature/v0/mod.rs b/packages/rs-dpp/src/state_transition/verify_identity_signed_signature/v0/mod.rs index 6c99ca6a32d..6a29799acf8 100644 --- a/packages/rs-dpp/src/state_transition/verify_identity_signed_signature/v0/mod.rs +++ b/packages/rs-dpp/src/state_transition/verify_identity_signed_signature/v0/mod.rs @@ -4,6 +4,7 @@ use crate::identity::{IdentityPublicKey, KeyType}; use crate::state_transition::errors::{PublicKeyMismatchError, StateTransitionIsNotSignedError}; use crate::state_transition::StateTransition; use crate::{BlsModule, ProtocolError}; +use platform_version::version::PlatformVersion; impl StateTransition { /// Kept as is for replay: the BLS verdict is dropped, so a BLS12_381 signature that does @@ -13,18 +14,24 @@ impl StateTransition { &self, public_key: &IdentityPublicKey, bls: &impl BlsModule, + platform_version: &PlatformVersion, ) -> Result<(), ProtocolError> { - self.identity_signed_signature_verdict_v0(public_key, bls) + self.identity_signed_signature_verdict_v0(public_key, bls, platform_version) .map(|_| ()) } /// The checks of generation 0, returning whether the signature verifies. Today only the /// BLS12_381 arm returns `false`, for a signature that is well formed but does not verify; /// the ECDSA arms report a failed check as an error. Generation 1 runs the same checks. + /// + /// The ECDSA_HASH160 arm reads `platform_version` only to select `verify_hash_signature`: + /// 0, the check this arm made before, at protocol versions 1 to 14, and 1, which also refuses + /// a high-S signature and a header naming an uncompressed key, from 15. pub(super) fn identity_signed_signature_verdict_v0( &self, public_key: &IdentityPublicKey, bls: &impl BlsModule, + platform_version: &PlatformVersion, ) -> Result { // self.verify_public_key_level_and_purpose(public_key)?; if public_key.disabled_at().is_some() { @@ -51,7 +58,10 @@ impl StateTransition { let public_key_bytes = public_key.data().as_slice(); match public_key.key_type() { KeyType::ECDSA_HASH160 => self - .verify_ecdsa_hash_160_signature_by_public_key_hash(public_key_bytes) + .verify_ecdsa_hash_160_signature_by_public_key_hash( + public_key_bytes, + platform_version, + ) .map(|()| true), KeyType::ECDSA_SECP256K1 => self diff --git a/packages/rs-dpp/src/state_transition/verify_identity_signed_signature/v1/mod.rs b/packages/rs-dpp/src/state_transition/verify_identity_signed_signature/v1/mod.rs index 5d4e245656f..48433d7ba7a 100644 --- a/packages/rs-dpp/src/state_transition/verify_identity_signed_signature/v1/mod.rs +++ b/packages/rs-dpp/src/state_transition/verify_identity_signed_signature/v1/mod.rs @@ -4,6 +4,7 @@ use crate::identity::identity_public_key::accessors::v0::IdentityPublicKeyGetter use crate::identity::IdentityPublicKey; use crate::state_transition::StateTransition; use crate::{BlsModule, ProtocolError}; +use platform_version::version::PlatformVersion; impl StateTransition { /// Runs the checks of generation 0 and also refuses a signature they report does not @@ -13,8 +14,9 @@ impl StateTransition { &self, public_key: &IdentityPublicKey, bls: &impl BlsModule, + platform_version: &PlatformVersion, ) -> Result<(), ProtocolError> { - if self.identity_signed_signature_verdict_v0(public_key, bls)? { + if self.identity_signed_signature_verdict_v0(public_key, bls, platform_version)? { Ok(()) } else { Err(ProtocolError::from(ConsensusError::SignatureError( diff --git a/packages/rs-dpp/src/tests/utils/mod.rs b/packages/rs-dpp/src/tests/utils/mod.rs index 30661cd5785..b7343bdd363 100644 --- a/packages/rs-dpp/src/tests/utils/mod.rs +++ b/packages/rs-dpp/src/tests/utils/mod.rs @@ -284,3 +284,35 @@ pub fn new_block_header(timestamp_secs: Option) -> Header { time: timestamp_secs.unwrap_or_default(), } } + +/// `signature`, a 65-byte compact recoverable ECDSA signature, with S replaced by `n - S` and the +/// recovery id's parity flipped: the high-S twin a third party can derive from a valid low-S +/// signature. Key recovery accepts it like the original. +pub fn high_s_twin(signature: &[u8]) -> Vec { + let mut twin = signature.to_vec(); + let s: [u8; 32] = twin[33..].try_into().expect("a 65-byte signature"); + let negated = secp256k1::SecretKey::from_secret_bytes(s) + .expect("S is a valid scalar") + .negate(); + twin[33..].copy_from_slice(&negated.to_secret_bytes()); + // The header is 27 + recovery id, plus 4 for a compressed key. + let base = if twin[0] >= 31 { 31 } else { 27 }; + twin[0] = base + ((twin[0] - base) ^ 1); + twin +} + +/// `signature`, a 65-byte compact recoverable ECDSA signature made by a compressed key, with its +/// header rewritten from `31 + id` to `27 + id`, the header of an uncompressed key: a twin a +/// third party can derive from a valid signature without touching R or S. Key recovery accepts +/// it like the original, because it hashes the recovered key in its compressed form whatever +/// the header says. +pub fn uncompressed_header_twin(signature: &[u8]) -> Vec { + assert_eq!(signature.len(), 65, "a 65-byte compact signature"); + assert!( + (31..=34).contains(&signature[0]), + "the header of a compressed key" + ); + let mut twin = signature.to_vec(); + twin[0] -= 4; + twin +} diff --git a/packages/rs-dpp/src/util/ecdsa.rs b/packages/rs-dpp/src/util/ecdsa/mod.rs similarity index 79% rename from packages/rs-dpp/src/util/ecdsa.rs rename to packages/rs-dpp/src/util/ecdsa/mod.rs index 8145af267f2..7331f5c5e4b 100644 --- a/packages/rs-dpp/src/util/ecdsa.rs +++ b/packages/rs-dpp/src/util/ecdsa/mod.rs @@ -3,10 +3,19 @@ //! low-S twin, the same signature with S negated, so Platform checks S itself, at every protocol //! version. Accepting it would fork from nodes that refuse it and change the replay of chain //! history. +//! +//! A signature checked by recovering its public key ([`verify_hash_signature`]) is the +//! exception: every released Platform accepts a high-S one there, and one whose header names an +//! uncompressed key, so refusing them is a rule of protocol version 15, selected by +//! `verify_hash_signature`. use dashcore::crypto::ecdsa::{EcdsaError, EcdsaPublicKey, EcdsaSignature}; use dashcore::{sign_message, signer}; +mod verify_hash_signature; + +pub use verify_hash_signature::verify_hash_signature; + /// [`signer::verify_data_signature`], refusing a high-S signature with the error of a signature /// that does not verify. Every other input gets the result and the error `dashcore` gives. pub fn verify_data_signature( diff --git a/packages/rs-dpp/src/util/ecdsa/verify_hash_signature/mod.rs b/packages/rs-dpp/src/util/ecdsa/verify_hash_signature/mod.rs new file mode 100644 index 00000000000..99f0d027d06 --- /dev/null +++ b/packages/rs-dpp/src/util/ecdsa/verify_hash_signature/mod.rs @@ -0,0 +1,268 @@ +use crate::util::ecdsa::verify_hash_signature::v0::verify_hash_signature_v0; +use crate::util::ecdsa::verify_hash_signature::v1::verify_hash_signature_v1; +use crate::ProtocolError; +use platform_version::version::PlatformVersion; + +mod v0; +mod v1; + +/// Checks that `signature`, a 65-byte compact recoverable ECDSA signature over `data_hash`, was +/// made by the key whose HASH160 is `public_key_hash`, by recovering the key: an ECDSA_HASH160 +/// identity key, a P2PKH address witness, an asset lock's one-time key. +/// +/// Recovery alone leaves a third party two ways to turn a valid signature into another valid +/// one, and so to change the hash of the transition carrying it: +/// +/// - it takes any S, so the signature stays valid with S replaced by `n - S` and the recovery +/// id's parity flipped; +/// - it hashes the recovered key in its compressed form whatever the header says, so the +/// signature stays valid with its header rewritten from `31 + id` (a compressed key) to +/// `27 + id` (an uncompressed one). +/// +/// # Parameters +/// +/// - `data_hash`: the 32-byte digest that was signed. +/// - `signature`: the compact recoverable signature. +/// - `public_key_hash`: the HASH160 of the compressed public key expected to have signed. +/// - `platform_version`: selects the rule through +/// `dpp.state_transition_method_versions.verify_hash_signature`. 0 is +/// `dashcore::signer::verify_hash_signature`, which accepts both rewrites (protocol versions +/// 1 to 14); 1 also refuses a high-S signature and a header that does not name a compressed +/// key. +/// +/// # Returns +/// +/// `Ok(Ok(()))` when the signature is valid, `Ok(Err(_))` with the reason when it is not, and +/// `Err(ProtocolError::UnknownVersionMismatch)` for a version this build does not know. +pub fn verify_hash_signature( + data_hash: &[u8], + signature: &[u8], + public_key_hash: &[u8], + platform_version: &PlatformVersion, +) -> Result, ProtocolError> { + match platform_version + .dpp + .state_transition_method_versions + .verify_hash_signature + { + 0 => Ok(verify_hash_signature_v0( + data_hash, + signature, + public_key_hash, + )), + 1 => Ok(verify_hash_signature_v1( + data_hash, + signature, + public_key_hash, + )), + version => Err(ProtocolError::UnknownVersionMismatch { + method: "verify_hash_signature".to_string(), + known_versions: vec![0, 1], + received: version, + }), + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::tests::utils::{high_s_twin, uncompressed_header_twin}; + use dashcore::crypto::ecdsa::{EcdsaError, EcdsaSignature}; + use dashcore::signer::{double_sha, sign_hash}; + use dashcore::{sign_message, signer, PrivateKey}; + + /// Half the curve order, `n / 2`, the largest low S. + const HALF_ORDER: [u8; 32] = [ + 0x7f, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, + 0xff, 0x5d, 0x57, 0x6e, 0x73, 0x57, 0xa4, 0x50, 0x1d, 0xdf, 0xe9, 0x2f, 0x46, 0x68, 0x1b, + 0x20, 0xa0, + ]; + /// The curve order `n`. + const ORDER: [u8; 32] = [ + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, + 0xfe, 0xba, 0xae, 0xdc, 0xe6, 0xaf, 0x48, 0xa0, 0x3b, 0xbf, 0xd2, 0x5e, 0x8c, 0xd0, 0x36, + 0x41, 0x41, + ]; + + fn signature_s_is_low(signature: &[u8]) -> bool { + sign_message::from_slice(signature) + .expect("a compact recoverable signature") + .signature() + .expect("a valid signature") + .is_low_s() + } + + /// A low-S signature over a digest, the digest, and the signing key's HASH160. + fn fixture() -> (Vec, Vec, Vec) { + let secret = [0x11; 32]; + let key_hash = PrivateKey::from_byte_array(&secret, dashcore::Network::Testnet) + .expect("a valid key") + .public_key() + .pubkey_hash(); + let digest = double_sha(b"a message"); + let signature = sign_hash(&digest, &secret).expect("signs").to_vec(); + (digest, signature, key_hash.to_byte_array().to_vec()) + } + + /// `signature` with its S replaced by `s`. + fn with_s(signature: &[u8], s: [u8; 32]) -> Vec { + let mut changed = signature.to_vec(); + changed[33..].copy_from_slice(&s); + changed + } + + /// `signature` with its header byte replaced by `header`. + fn with_header(signature: &[u8], header: u8) -> Vec { + let mut changed = signature.to_vec(); + changed[0] = header; + changed + } + + fn protocol_version(version: u32) -> &'static PlatformVersion { + PlatformVersion::get(version).expect("a known protocol version") + } + + #[test] + fn should_hold_s_equal_to_half_the_order_low_and_one_more_high() { + let mut compact = [1u8; 64]; + compact[32..].copy_from_slice(&HALF_ORDER); + assert!(EcdsaSignature::from_bytes(&compact) + .expect("a valid signature") + .is_low_s()); + compact[63] += 1; + assert!(!EcdsaSignature::from_bytes(&compact) + .expect("a valid signature") + .is_low_s()); + } + + #[test] + fn should_build_a_high_s_twin_that_recovery_alone_accepts() { + let (digest, signature, key_hash) = fixture(); + let twin = high_s_twin(&signature); + assert!(signature_s_is_low(&signature)); + assert!(!signature_s_is_low(&twin)); + signer::verify_hash_signature(&digest, &twin, &key_hash) + .expect("recovery alone accepts the twin"); + } + + #[test] + fn should_build_an_uncompressed_header_twin_that_recovery_alone_accepts() { + let (digest, signature, key_hash) = fixture(); + let twin = uncompressed_header_twin(&signature); + assert_eq!(twin[0] + 4, signature[0]); + assert_eq!(twin[1..], signature[1..]); + assert!(signature_s_is_low(&twin)); + signer::verify_hash_signature(&digest, &twin, &key_hash) + .expect("recovery alone accepts the twin"); + } + + #[test] + fn should_accept_the_high_s_twin_up_to_protocol_version_14() { + let (digest, signature, key_hash) = fixture(); + let twin = high_s_twin(&signature); + for version in [1, 13, 14] { + let platform_version = protocol_version(version); + verify_hash_signature(&digest, &signature, &key_hash, platform_version) + .expect("a known version") + .expect("the low-S signature is valid"); + verify_hash_signature(&digest, &twin, &key_hash, platform_version) + .expect("a known version") + .expect("protocol version 14 and earlier accept the high-S twin"); + } + } + + #[test] + fn should_refuse_the_high_s_twin_at_the_latest_protocol_version() { + let (digest, signature, key_hash) = fixture(); + let platform_version = PlatformVersion::latest(); + verify_hash_signature(&digest, &signature, &key_hash, platform_version) + .expect("a known version") + .expect("the low-S signature is valid"); + let error = verify_hash_signature( + &digest, + &high_s_twin(&signature), + &key_hash, + platform_version, + ) + .expect("a known version") + .expect_err("the high-S twin is refused"); + assert_eq!(error.to_string(), EcdsaError::VerifyFailed.to_string()); + } + + #[test] + fn should_accept_the_uncompressed_header_twin_up_to_protocol_version_14() { + let (digest, signature, key_hash) = fixture(); + let twin = uncompressed_header_twin(&signature); + for version in [1, 13, 14] { + verify_hash_signature(&digest, &twin, &key_hash, protocol_version(version)) + .expect("a known version") + .expect("protocol version 14 and earlier accept the uncompressed header"); + } + } + + #[test] + fn should_refuse_the_uncompressed_header_twin_at_the_latest_protocol_version() { + let (digest, signature, key_hash) = fixture(); + let platform_version = PlatformVersion::latest(); + // The twin, and the twin of the high-S twin: neither rewrite hides the other. + for twin in [ + uncompressed_header_twin(&signature), + uncompressed_header_twin(&high_s_twin(&signature)), + ] { + let error = verify_hash_signature(&digest, &twin, &key_hash, platform_version) + .expect("a known version") + .expect_err("the uncompressed header is refused"); + assert_eq!(error.to_string(), EcdsaError::VerifyFailed.to_string()); + } + } + + /// Of the eight headers recovery parses, 27 to 34, the latest protocol version accepts one: + /// the compressed-key header carrying the recovery id that recovers the signing key. + #[test] + fn should_accept_a_single_header_at_the_latest_protocol_version() { + let (digest, signature, key_hash) = fixture(); + let accepted: Vec = (27..=34) + .filter(|header| { + let mut candidate = signature.clone(); + candidate[0] = *header; + verify_hash_signature(&digest, &candidate, &key_hash, PlatformVersion::latest()) + .expect("a known version") + .is_ok() + }) + .collect(); + assert_eq!(accepted, vec![signature[0]]); + } + + /// A signature recovery already refuses (another key, a wrong length, S zero or not below the + /// curve order, a header outside 27 to 34) fails with the library's own error at both + /// versions. + #[test] + fn should_fail_an_invalid_signature_with_the_same_error_at_both_versions() { + let (digest, signature, key_hash) = fixture(); + let wrong_key_hash = [7u8; 20]; + let cases = [ + (signature.clone(), wrong_key_hash.to_vec(), "another key"), + (signature[..64].to_vec(), key_hash.clone(), "a wrong length"), + (with_s(&signature, [0; 32]), key_hash.clone(), "S zero"), + (with_s(&signature, ORDER), key_hash.clone(), "S equal to n"), + ( + with_s(&signature, [0xff; 32]), + key_hash.clone(), + "S above n", + ), + (with_header(&signature, 26), key_hash.clone(), "header 26"), + (with_header(&signature, 35), key_hash.clone(), "header 35"), + ]; + for (signature, key_hash, label) in cases { + let legacy = + verify_hash_signature(&digest, &signature, &key_hash, protocol_version(14)) + .expect("a known version") + .expect_err(label); + let latest = + verify_hash_signature(&digest, &signature, &key_hash, PlatformVersion::latest()) + .expect("a known version") + .expect_err(label); + assert_eq!(legacy.to_string(), latest.to_string(), "{label}"); + } + } +} diff --git a/packages/rs-dpp/src/util/ecdsa/verify_hash_signature/v0/mod.rs b/packages/rs-dpp/src/util/ecdsa/verify_hash_signature/v0/mod.rs new file mode 100644 index 00000000000..aa189caa20d --- /dev/null +++ b/packages/rs-dpp/src/util/ecdsa/verify_hash_signature/v0/mod.rs @@ -0,0 +1,12 @@ +use dashcore::signer; + +/// Generation 0, protocol versions 1 to 14: `dashcore`'s recovery check and nothing else. It +/// accepts a high-S signature and a header naming an uncompressed key. Kept as is for replay. +#[inline(always)] +pub(super) fn verify_hash_signature_v0( + data_hash: &[u8], + signature: &[u8], + public_key_hash: &[u8], +) -> Result<(), anyhow::Error> { + signer::verify_hash_signature(data_hash, signature, public_key_hash) +} diff --git a/packages/rs-dpp/src/util/ecdsa/verify_hash_signature/v1/mod.rs b/packages/rs-dpp/src/util/ecdsa/verify_hash_signature/v1/mod.rs new file mode 100644 index 00000000000..9d66cfd7250 --- /dev/null +++ b/packages/rs-dpp/src/util/ecdsa/verify_hash_signature/v1/mod.rs @@ -0,0 +1,25 @@ +use anyhow::ensure; +use dashcore::crypto::ecdsa::EcdsaError; +use dashcore::{sign_message, signer}; + +/// Generation 1, from protocol version 15: `dashcore`'s recovery check, then the signature must +/// be the one form a signer produces. A header naming an uncompressed key (`27 + id` in place +/// of `31 + id`) and an S in the upper half of the curve order are refused with +/// [`EcdsaError::VerifyFailed`], the error of a signature that does not verify. +/// +/// The library checks first, so every other input fails exactly as it does in generation 0, +/// with the library's own error. +#[inline(always)] +pub(super) fn verify_hash_signature_v1( + data_hash: &[u8], + signature: &[u8], + public_key_hash: &[u8], +) -> Result<(), anyhow::Error> { + signer::verify_hash_signature(data_hash, signature, public_key_hash)?; + // `dashcore` parsed the signature to recover its key, so parsing it again succeeds. + let signature = sign_message::from_slice(signature)?; + // The key hash compared above is the compressed key's, whatever the header says. + ensure!(signature.is_compressed()?, EcdsaError::VerifyFailed); + ensure!(signature.signature()?.is_low_s(), EcdsaError::VerifyFailed); + Ok(()) +} diff --git a/packages/rs-dpp/tests/ecdsa_compatibility.rs b/packages/rs-dpp/tests/ecdsa_compatibility.rs index a5885ffe1ca..936d0404c35 100644 --- a/packages/rs-dpp/tests/ecdsa_compatibility.rs +++ b/packages/rs-dpp/tests/ecdsa_compatibility.rs @@ -19,6 +19,7 @@ use dpp::identity::{IdentityPublicKey, KeyType, Purpose, SecurityLevel}; use dpp::secp256k1::{ecdsa::RecoverableSignature, ecdsa::RecoveryId, Message}; use dpp::serialization::PlatformMessageSignable; use dpp::util::ecdsa::verify_data_signature; +use dpp::version::PlatformVersion; use dpp::ProtocolError; use vectors::{ECDSA_PUBLIC_KEY, TRANSFER_ECDSA_SIGNATURE, TRANSFER_HASH, TRANSFER_SIGNABLE}; @@ -76,7 +77,7 @@ fn should_preserve_witness_fee_operations_for_recovery_and_multisig_search() { }; assert_eq!( address - .verify_bytes_against_witness(&witness, &data) + .verify_bytes_against_witness(&witness, &data, PlatformVersion::latest()) .unwrap(), AddressWitnessVerificationOperations::for_p2pkh(data.len()) ); @@ -95,12 +96,12 @@ fn should_preserve_witness_fee_operations_for_recovery_and_multisig_search() { }; assert_eq!( address - .verify_bytes_against_witness(&witness, &data) + .verify_bytes_against_witness(&witness, &data, PlatformVersion::latest()) .unwrap(), AddressWitnessVerificationOperations::for_p2sh_multisig(2, data.len()) ); assert!(address - .verify_bytes_against_witness(&witness, b"different message") + .verify_bytes_against_witness(&witness, b"different message", PlatformVersion::latest()) .is_err()); } @@ -275,6 +276,61 @@ fn should_reject_high_s_verification_but_accept_high_s_recovery() { .unwrap(); } +/// The historical signature with its header rewritten from that of a compressed key to that of +/// an uncompressed one: R, S and the recovery id are untouched. Key recovery hashes the recovered +/// key in its compressed form either way. +fn uncompressed_header_twin() -> Vec { + let (_, mut signature, _) = fixture(); + assert_eq!(signature[0], 31); + signature[0] = 27; + signature +} + +/// A P2PKH witness is checked by recovering the key, which takes any S and either header form. +/// Protocol version 14 accepts the two twins a third party can derive from the historical +/// signature, the high-S one and the one whose header names an uncompressed key, for replay; +/// from protocol version 15 (`verify_hash_signature` 1) each is refused as a signature that does +/// not verify. The signature itself is accepted at both. +#[test] +fn should_refuse_a_p2pkh_witness_twin_from_protocol_version_15() { + let (data, signature, public_key) = fixture(); + let address = PlatformAddress::P2pkh(hash160::Hash::hash(&public_key).to_byte_array()); + let witness = |signature: Vec| AddressWitness::P2pkh { + signature: signature.into(), + }; + let last_without_the_check = PlatformVersion::get(14).unwrap(); + let latest = PlatformVersion::latest(); + for platform_version in [last_without_the_check, latest] { + assert_eq!( + address + .verify_bytes_against_witness(&witness(signature.clone()), &data, platform_version) + .unwrap(), + AddressWitnessVerificationOperations::for_p2pkh(data.len()) + ); + } + for twin in [high_s_twin(), uncompressed_header_twin()] { + assert_eq!( + address + .verify_bytes_against_witness(&witness(twin.clone()), &data, last_without_the_check) + .unwrap(), + AddressWitnessVerificationOperations::for_p2pkh(data.len()) + ); + match address + .verify_bytes_against_witness(&witness(twin), &data, latest) + .unwrap_err() + { + ProtocolError::AddressWitnessError(message) => assert_eq!( + message, + format!( + "P2PKH signature verification failed: {}", + EcdsaError::VerifyFailed + ) + ), + error => panic!("unexpected error: {error}"), + } + } +} + #[test] fn should_reject_a_high_s_signature_in_a_multisig_witness() { let (data, signature, public_key) = fixture(); @@ -287,10 +343,10 @@ fn should_reject_a_high_s_signature_in_a_multisig_witness() { redeem_script: script.clone().into(), }; address - .verify_bytes_against_witness(&witness(signature), &data) + .verify_bytes_against_witness(&witness(signature), &data, PlatformVersion::latest()) .unwrap(); match address - .verify_bytes_against_witness(&witness(high_s_twin()), &data) + .verify_bytes_against_witness(&witness(high_s_twin()), &data, PlatformVersion::latest()) .unwrap_err() { ProtocolError::AddressWitnessError(message) => { @@ -319,7 +375,7 @@ fn should_charge_every_verification_a_refused_high_s_twin_takes_in_a_multisig_wi }; assert_eq!( address - .verify_bytes_against_witness(&witness, &data) + .verify_bytes_against_witness(&witness, &data, PlatformVersion::latest()) .unwrap(), AddressWitnessVerificationOperations::for_p2sh_multisig(3, data.len()) ); @@ -367,7 +423,7 @@ fn should_preserve_multisig_malformed_signature_error() { redeem_script: script.into(), }; let error = address - .verify_bytes_against_witness(&witness, &data) + .verify_bytes_against_witness(&witness, &data, PlatformVersion::latest()) .unwrap_err(); match error { ProtocolError::AddressWitnessError(message) => { @@ -389,7 +445,7 @@ fn should_preserve_multisig_malformed_public_key_error() { redeem_script: script.into(), }; let error = address - .verify_bytes_against_witness(&witness, &data) + .verify_bytes_against_witness(&witness, &data, PlatformVersion::latest()) .unwrap_err(); match error { ProtocolError::AddressWitnessError(message) => { diff --git a/packages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/address_witnesses.rs b/packages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/address_witnesses.rs index c3dc54460ae..b3b4066a628 100644 --- a/packages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/address_witnesses.rs +++ b/packages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/address_witnesses.rs @@ -78,21 +78,23 @@ impl StateTransitionAddressWitnessValidationV0 for StateTransition { let witness_result = match self { StateTransition::AddressFundsTransfer(st) => { - st.validate_witnesses(&signable_bytes) + st.validate_witnesses(&signable_bytes, platform_version) } StateTransition::IdentityCreateFromAddresses(st) => { - st.validate_witnesses(&signable_bytes) + st.validate_witnesses(&signable_bytes, platform_version) } StateTransition::IdentityTopUpFromAddresses(st) => { - st.validate_witnesses(&signable_bytes) + st.validate_witnesses(&signable_bytes, platform_version) } StateTransition::AddressCreditWithdrawal(st) => { - st.validate_witnesses(&signable_bytes) + st.validate_witnesses(&signable_bytes, platform_version) } StateTransition::AddressFundingFromAssetLock(st) => { - st.validate_witnesses(&signable_bytes) + st.validate_witnesses(&signable_bytes, platform_version) + } + StateTransition::Shield(st) => { + st.validate_witnesses(&signable_bytes, platform_version) } - StateTransition::Shield(st) => st.validate_witnesses(&signable_bytes), // These state transitions don't have address witness validation StateTransition::DataContractCreate(_) | StateTransition::DataContractUpdate(_) @@ -120,7 +122,7 @@ impl StateTransitionAddressWitnessValidationV0 for StateTransition { | StateTransition::IdentityCreateFromShieldedPool(_) => { return Ok(SimpleConsensusValidationResult::new()); } - }; + }?; // Add operations to execution context for fee calculation add_witness_verification_operations_to_context( @@ -511,5 +513,27 @@ mod tests { assert_eq!(signature_verifications(&exec_ctx), 16); } } + + /// A `verify_hash_signature` version this build does not know is an internal error, not + /// a witness that does not verify: it must not become an unpaid consensus refusal. + #[tokio::test] + async fn should_return_an_internal_error_for_an_unknown_hash_signature_version() { + let mut platform_version = PlatformVersion::latest().clone(); + platform_version + .dpp + .state_transition_method_versions + .verify_hash_signature = 99; + let st = signed_transfer(1, PlatformVersion::latest()).await; + let mut exec_ctx = + StateTransitionExecutionContext::default_for_platform_version(&platform_version) + .unwrap(); + let result = st.validate_address_witnesses(&mut exec_ctx, &platform_version); + assert_matches!( + result, + Err(Error::Protocol( + dpp::ProtocolError::UnknownVersionMismatch { received: 99, .. } + )) + ); + } } } diff --git a/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/address_funding_from_asset_lock/tests.rs b/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/address_funding_from_asset_lock/tests.rs index 130ccb4fd33..4ca6a8350e4 100644 --- a/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/address_funding_from_asset_lock/tests.rs +++ b/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/address_funding_from_asset_lock/tests.rs @@ -1200,6 +1200,99 @@ mod tests { ); } + /// The asset lock's one-time key is checked by recovering the public key, which takes + /// any S and either header form. Protocol version 14 accepts the high-S twin of the + /// signature and the twin whose header names an uncompressed key, for replay; from + /// protocol version 15 a block refuses each unpaid, with the error of a signature that + /// does not verify, and leaves the asset lock unused. The signature the key made is + /// accepted at both. + #[tokio::test] + async fn should_refuse_an_asset_lock_signature_twin_from_protocol_version_15() { + use crate::execution::validation::state_transition::state_transitions::test_helpers::process_transition_and_commit; + use dpp::consensus::signature::SignatureError; + use dpp::tests::utils::{high_s_twin, uncompressed_header_twin}; + + let latest = PlatformVersion::latest().protocol_version; + let twins: [(&str, fn(&[u8]) -> Vec); 2] = [ + ("high-S twin", high_s_twin), + ("uncompressed header twin", uncompressed_header_twin), + ]; + // (protocol version, the twins submitted before the signature itself). The asset + // lock is consumed by the first transition a block accepts, so at 14 each twin gets + // its own platform, as does the signature. + for (protocol_version, submitted) in [ + (14, &twins[..0]), + (14, &twins[..1]), + (14, &twins[1..]), + (latest, &twins[..]), + ] { + let platform_version = + PlatformVersion::get(protocol_version).expect("known protocol version"); + let platform = TestPlatformBuilder::new() + .with_config(PlatformConfig { + testing_configs: PlatformTestConfig { + disable_instant_lock_signature_verification: true, + ..Default::default() + }, + ..Default::default() + }) + .with_initial_protocol_version(protocol_version) + .build_with_mock_rpc() + .set_genesis_state(); + + let signer = TestAddressSigner::new(); + let mut rng = StdRng::seed_from_u64(567); + let (asset_lock_proof, asset_lock_pk) = create_asset_lock_proof_with_key(&mut rng); + let mut outputs = BTreeMap::new(); + outputs.insert(create_platform_address(1), Some(dash_to_credits!(0.5))); + outputs.insert(create_platform_address(2), None); + let signed = create_signed_address_funding_from_asset_lock_transition( + asset_lock_proof, + &asset_lock_pk, + &signer, + BTreeMap::new(), + outputs, + vec![AddressFundsFeeStrategyStep::ReduceOutput(0)], + ) + .await; + let signature = signed.signature().expect("signed").to_vec(); + + let mut asset_lock_used = false; + for (twin_label, twin) in submitted { + let mut malleated = signed.clone(); + malleated.set_signature(twin(&signature).into()); + let result = + process_transition_and_commit(&platform, malleated, platform_version); + if protocol_version == 14 { + assert_matches!( + result.execution_results().as_slice(), + [StateTransitionExecutionResult::SuccessfulExecution { .. }], + "protocol version 14 accepts the {twin_label}" + ); + asset_lock_used = true; + } else { + assert_matches!( + result.execution_results().as_slice(), + [StateTransitionExecutionResult::UnpaidConsensusError( + ConsensusError::SignatureError(SignatureError::BasicECDSAError(_)) + )], + "protocol version {protocol_version} refuses the {twin_label}" + ); + } + } + if !asset_lock_used { + // No twin was accepted, so the asset lock is unused and the signature the + // key made still funds. + let result = process_transition_and_commit(&platform, signed, platform_version); + assert_matches!( + result.execution_results().as_slice(), + [StateTransitionExecutionResult::SuccessfulExecution { .. }], + "protocol version {protocol_version} accepts the signature" + ); + } + } + } + #[tokio::test] async fn test_asset_lock_funding_to_multiple_addresses() { let platform_version = PlatformVersion::latest(); diff --git a/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/address_funding_from_asset_lock/transform_into_action/v0/mod.rs b/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/address_funding_from_asset_lock/transform_into_action/v0/mod.rs index d1c45b073c3..837251d90c4 100644 --- a/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/address_funding_from_asset_lock/transform_into_action/v0/mod.rs +++ b/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/address_funding_from_asset_lock/transform_into_action/v0/mod.rs @@ -6,10 +6,10 @@ use dpp::balances::credits::CREDITS_PER_DUFF; use dpp::consensus::basic::identity::IdentityAssetLockTransactionOutPointNotEnoughBalanceError; use dpp::consensus::signature::{BasicECDSAError, SignatureError}; -use dpp::dashcore::signer; use dpp::dashcore::{ScriptBuf, Txid}; use dpp::identity::state_transition::AssetLockProved; use dpp::identity::KeyType; +use dpp::util::ecdsa::verify_hash_signature; use dpp::prelude::ConsensusValidationResult; @@ -175,11 +175,15 @@ impl AddressFundingFromAssetLockStateTransitionTransformIntoActionValidationV0 SignatureVerificationOperation::new(KeyType::ECDSA_HASH160), )); - if let Err(e) = signer::verify_hash_signature( + // `verify_hash_signature` 0 at protocol versions 14 and earlier is the + // `dashcore::signer::verify_hash_signature` call this site made before; 1 also + // refuses a high-S signature and a header naming an uncompressed key. + if let Err(e) = verify_hash_signature( signable_bytes_hasher.hash_bytes().as_slice(), self.signature().as_slice(), public_key_hash, - ) { + platform_version, + )? { return Ok(ConsensusValidationResult::new_with_error( SignatureError::BasicECDSAError(BasicECDSAError::new(e.to_string())).into(), )); diff --git a/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/address_funds_transfer/tests.rs b/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/address_funds_transfer/tests.rs index 7d1b89af779..3b41ad601e0 100644 --- a/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/address_funds_transfer/tests.rs +++ b/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/address_funds_transfer/tests.rs @@ -9256,4 +9256,128 @@ mod tests { } } } + + // ========================================== + // P2PKH WITNESS TWINS + // ========================================== + + /// A P2PKH witness is checked by recovering the public key, which takes any S and either + /// header form. A third party can replace a witness's signature with its high-S twin, or with + /// the twin whose header names an uncompressed key, and change the transition's hash. + /// Protocol version 14 accepts each twin, for replay; from protocol version 15 a block + /// refuses each unpaid, with the error of a witness that does not verify. The witness the + /// key made is accepted at both. + mod witness_twins { + use super::*; + use dpp::tests::utils::{high_s_twin, uncompressed_header_twin}; + + #[tokio::test] + async fn should_refuse_a_p2pkh_witness_twin_from_protocol_version_15() { + for platform_version in [ + PlatformVersion::get(14).expect("expected protocol version 14"), + PlatformVersion::latest(), + ] { + let protocol_version = platform_version.protocol_version; + let mut platform = TestPlatformBuilder::new() + .with_config(PlatformConfig::default()) + .with_initial_protocol_version(protocol_version) + .build_with_mock_rpc() + .set_genesis_state(); + + let mut signer = TestAddressSigner::new(); + let input_address = signer.add_p2pkh([1u8; 32]); + setup_address_with_balance(&mut platform, input_address, 0, dash_to_credits!(1.0)); + let mut inputs = BTreeMap::new(); + inputs.insert(input_address, (1, dash_to_credits!(0.1))); + let mut outputs = BTreeMap::new(); + outputs.insert(create_platform_address(2), dash_to_credits!(0.1)); + let signed = AddressFundsTransferTransitionV0::try_from_inputs_with_signer( + inputs, + outputs, + vec![AddressFundsFeeStrategyStep::DeductFromInput(0)], + &signer, + 0, + platform_version, + ) + .await + .expect("should create signed transition"); + + let StateTransition::AddressFundsTransfer(AddressFundsTransferTransition::V0( + signed_v0, + )) = &signed + else { + panic!("expected an address funds transfer"); + }; + let AddressWitness::P2pkh { signature } = &signed_v0.input_witnesses[0] else { + panic!("expected a P2PKH witness"); + }; + let signature = signature.to_vec(); + let with_witness_signature = |signature: Vec| { + let mut malleated = signed.clone(); + let StateTransition::AddressFundsTransfer(AddressFundsTransferTransition::V0( + v0, + )) = &mut malleated + else { + panic!("expected an address funds transfer"); + }; + v0.input_witnesses[0] = AddressWitness::P2pkh { + signature: BinaryData::new(signature), + }; + malleated + }; + + // Each transition runs in its own uncommitted block transaction, so all spend + // the same nonce. + let process = |transition: &StateTransition| { + let platform_state = platform.state.load(); + let transaction = platform.drive.grove.start_transaction(); + platform + .platform + .process_raw_state_transitions( + &[transition.serialize_to_bytes().expect("serializes")], + &platform_state, + &BlockInfo::default(), + &transaction, + platform_version, + false, + None, + ) + .expect("expected to process state transition") + .into_execution_results() + }; + + assert_matches!( + process(&signed).as_slice(), + [StateTransitionExecutionResult::SuccessfulExecution { .. }], + "protocol version {protocol_version} accepts the witness the key made" + ); + for (twin_label, twin) in [ + ("high-S twin", high_s_twin(&signature)), + ( + "uncompressed header twin", + uncompressed_header_twin(&signature), + ), + ] { + let malleated_results = process(&with_witness_signature(twin)); + if protocol_version == 14 { + assert_matches!( + malleated_results.as_slice(), + [StateTransitionExecutionResult::SuccessfulExecution { .. }], + "protocol version 14 accepts the {twin_label}" + ); + } else { + assert_matches!( + malleated_results.as_slice(), + [StateTransitionExecutionResult::UnpaidConsensusError( + ConsensusError::SignatureError( + SignatureError::InvalidStateTransitionSignatureError(_) + ) + )], + "protocol version {protocol_version} refuses the {twin_label}" + ); + } + } + } + } + } } diff --git a/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_create/mod.rs b/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_create/mod.rs index 9c07ac33abd..5abad522ab1 100644 --- a/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_create/mod.rs +++ b/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_create/mod.rs @@ -508,6 +508,149 @@ mod tests { assert_eq!(identity_balance, expected_identity_balance); } + /// The asset lock's one-time key is checked by recovering the public key, which takes any S + /// and either header form. A third party can replace an identity create's signature with its + /// high-S twin, or with the twin whose header names an uncompressed key, and change the + /// transition's hash. Protocol version 14 accepts each twin, for replay; from protocol + /// version 15 a block refuses each unpaid, with the error of a signature that does not + /// verify, and leaves the asset lock unused. The signature the key made is accepted at both. + #[tokio::test] + async fn should_refuse_an_asset_lock_signature_twin_from_protocol_version_15() { + use crate::platform_types::state_transitions_processing_result::StateTransitionExecutionResult; + use dpp::consensus::signature::SignatureError; + use dpp::tests::utils::{high_s_twin, uncompressed_header_twin}; + + let latest = PlatformVersion::latest().protocol_version; + let twins: [(&str, fn(&[u8]) -> Vec); 2] = [ + ("high-S twin", high_s_twin), + ("uncompressed header twin", uncompressed_header_twin), + ]; + // (protocol version, the twins submitted before the signature itself). The asset lock is + // consumed by the first transition a block accepts, so at 14 each twin gets its own + // platform, as does the signature. + for (protocol_version, submitted) in [ + (14, &twins[..0]), + (14, &twins[..1]), + (14, &twins[1..]), + (latest, &twins[..]), + ] { + let platform_version = PlatformVersion::get(protocol_version) + .expect("expected platform version for the requested protocol_version"); + let platform = TestPlatformBuilder::new() + .with_initial_protocol_version(protocol_version) + .with_config(PlatformConfig { + testing_configs: PlatformTestConfig { + disable_instant_lock_signature_verification: true, + ..Default::default() + }, + ..Default::default() + }) + .build_with_mock_rpc() + .set_initial_state_structure(); + let platform_state = platform.state.load(); + let mut signer = SimpleSigner::default(); + let mut rng = StdRng::seed_from_u64(567); + + let (master_key, master_private_key) = + IdentityPublicKey::random_ecdsa_master_authentication_key( + 0, + Some(58), + platform_version, + ) + .expect("expected to get key pair"); + signer.add_identity_public_key(master_key.clone(), master_private_key); + + let (_, pk) = ECDSA_SECP256K1 + .random_public_and_private_key_data(&mut rng, platform_version) + .unwrap(); + let asset_lock_proof = instant_asset_lock_proof_fixture( + Some(PrivateKey::from_byte_array(&pk, Network::Testnet).unwrap()), + None, + ); + let identity: Identity = IdentityV0 { + id: asset_lock_proof + .create_identifier() + .expect("expected an identifier"), + public_keys: BTreeMap::from([(0, master_key)]), + balance: 1000000000, + revision: 0, + } + .into(); + + let signed: StateTransition = + IdentityCreateTransition::try_from_identity_with_signer_and_private_key( + &identity, + asset_lock_proof, + pk.as_slice(), + &signer, + &NativeBlsModule, + 0, + platform_version, + ) + .await + .expect("expected an identity create transition"); + let signature = signed.signature().expect("signed").to_vec(); + + let process = |transition: &StateTransition| { + let transaction = platform.drive.grove.start_transaction(); + let result = platform + .platform + .process_raw_state_transitions( + &[transition.serialize_to_bytes().expect("serializes")], + &platform_state, + &BlockInfo::default(), + &transaction, + platform_version, + false, + None, + ) + .expect("expected to process state transition"); + platform + .drive + .grove + .commit_transaction(transaction) + .unwrap() + .expect("expected to commit"); + result + }; + + let mut asset_lock_used = false; + for (twin_label, twin) in submitted { + let mut malleated = signed.clone(); + malleated.set_signature(twin(&signature).into()); + let result = process(&malleated); + if protocol_version == 14 { + assert_eq!( + result.valid_count(), + 1, + "protocol version 14 accepts the {twin_label}: {:?}", + result.execution_results() + ); + asset_lock_used = true; + } else { + assert_matches!( + result.execution_results().as_slice(), + [StateTransitionExecutionResult::UnpaidConsensusError( + ConsensusError::SignatureError(SignatureError::BasicECDSAError(_)) + )], + "protocol version {protocol_version} refuses the {twin_label}" + ); + } + } + if !asset_lock_used { + // No twin was accepted, so the asset lock is unused and the signature the key + // made still creates. + let result = process(&signed); + assert_eq!( + result.valid_count(), + 1, + "protocol version {protocol_version} accepts the signature: {:?}", + result.execution_results() + ); + } + } + } + /// Neither the InstantSend lock nor the transition's signature covers an instant proof's /// asset-lock payload length prefix, and every node has always read the payload whatever /// length the prefix announces. Anyone relaying a pending IdentityCreate can change the diff --git a/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_create/state/v0/mod.rs b/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_create/state/v0/mod.rs index b688f19d77f..ffa60170fbb 100644 --- a/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_create/state/v0/mod.rs +++ b/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_create/state/v0/mod.rs @@ -7,9 +7,9 @@ use dpp::consensus::basic::identity::IdentityAssetLockTransactionOutPointNotEnou use dpp::consensus::signature::{BasicECDSAError, SignatureError}; use dpp::consensus::state::identity::IdentityAlreadyExistsError; -use dpp::dashcore::signer; use dpp::dashcore::{ScriptBuf, Txid}; use dpp::identity::KeyType; +use dpp::util::ecdsa::verify_hash_signature; use dpp::identity::state_transition::AssetLockProved; use dpp::prelude::ConsensusValidationResult; @@ -234,11 +234,15 @@ impl IdentityCreateStateTransitionStateValidationV0 for IdentityCreateTransition SignatureVerificationOperation::new(KeyType::ECDSA_HASH160), )); - if let Err(e) = signer::verify_hash_signature( + // `verify_hash_signature` 0 at protocol versions 14 and earlier is the + // `dashcore::signer::verify_hash_signature` call this site made before; 1 also + // refuses a high-S signature and a header naming an uncompressed key. + if let Err(e) = verify_hash_signature( signable_bytes_hasher.hash_bytes().as_slice(), self.signature().as_slice(), public_key_hash, - ) { + platform_version, + )? { return Ok(ConsensusValidationResult::new_with_error( SignatureError::BasicECDSAError(BasicECDSAError::new(e.to_string())).into(), )); diff --git a/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_top_up/mod.rs b/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_top_up/mod.rs index d6a54ae5397..283de584131 100644 --- a/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_top_up/mod.rs +++ b/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_top_up/mod.rs @@ -416,6 +416,154 @@ mod tests { assert_eq!(identity_balance, expected_identity_balance); // about 0.5 Dash starting balance + 1 Dash asset lock top up } + /// The asset lock's one-time key is checked by recovering the public key, which takes any S + /// and either header form. A third party can replace a top-up's signature with its + /// high-S twin, or with the twin whose header names an uncompressed key, and change the + /// transition's hash. Protocol version 14 accepts each twin, for replay; from protocol + /// version 15 a block refuses each unpaid, with the error of a signature that does not + /// verify, and leaves the asset lock unused. The signature the key made is accepted at both. + #[test] + fn should_refuse_an_asset_lock_signature_twin_from_protocol_version_15() { + use crate::platform_types::state_transitions_processing_result::StateTransitionExecutionResult; + use assert_matches::assert_matches; + use dpp::consensus::signature::SignatureError; + use dpp::consensus::ConsensusError; + use dpp::tests::utils::{high_s_twin, uncompressed_header_twin}; + + let latest = PlatformVersion::latest().protocol_version; + let twins: [(&str, fn(&[u8]) -> Vec); 2] = [ + ("high-S twin", high_s_twin), + ("uncompressed header twin", uncompressed_header_twin), + ]; + // (protocol version, the twins submitted before the signature itself). The asset lock is + // consumed by the first transition a block accepts, so at 14 each twin gets its own + // platform, as does the signature. + for (protocol_version, submitted) in [ + (14, &twins[..0]), + (14, &twins[..1]), + (14, &twins[1..]), + (latest, &twins[..]), + ] { + let platform_version = PlatformVersion::get(protocol_version) + .expect("expected platform version for the requested protocol_version"); + let platform = TestPlatformBuilder::new() + .with_initial_protocol_version(protocol_version) + .with_config(PlatformConfig { + testing_configs: PlatformTestConfig { + disable_instant_lock_signature_verification: true, + ..Default::default() + }, + ..Default::default() + }) + .build_with_mock_rpc() + .set_initial_state_structure(); + let platform_state = platform.state.load(); + let mut rng = StdRng::seed_from_u64(567); + + let (master_key, _) = IdentityPublicKey::random_ecdsa_master_authentication_key( + 0, + Some(58), + platform_version, + ) + .expect("expected to get key pair"); + let identity: Identity = IdentityV0 { + id: Identifier::random_with_rng(&mut rng), + public_keys: BTreeMap::from([(0, master_key)]), + balance: 50000000000, + revision: 0, + } + .into(); + platform + .drive + .add_new_identity( + identity.clone(), + false, + &BlockInfo::default(), + true, + None, + platform_version, + ) + .expect("expected to add a new identity"); + + let (_, pk) = ECDSA_SECP256K1 + .random_public_and_private_key_data(&mut rng, platform_version) + .unwrap(); + let asset_lock_proof = instant_asset_lock_proof_fixture( + Some(PrivateKey::from_byte_array(&pk, Network::Testnet).unwrap()), + None, + ); + let signed: StateTransition = + IdentityTopUpTransition::try_from_identity_with_private_key( + &identity, + asset_lock_proof, + pk.as_slice(), + 0, + platform_version, + None, + ) + .expect("expected an identity top up transition"); + let signature = signed.signature().expect("signed").to_vec(); + + let process = |transition: &StateTransition| { + let transaction = platform.drive.grove.start_transaction(); + let result = platform + .platform + .process_raw_state_transitions( + &[transition.serialize_to_bytes().expect("serializes")], + &platform_state, + &BlockInfo::default(), + &transaction, + platform_version, + false, + None, + ) + .expect("expected to process state transition"); + platform + .drive + .grove + .commit_transaction(transaction) + .unwrap() + .expect("expected to commit"); + result + }; + + let mut asset_lock_used = false; + for (twin_label, twin) in submitted { + let mut malleated = signed.clone(); + malleated.set_signature(twin(&signature).into()); + let result = process(&malleated); + if protocol_version == 14 { + assert_eq!( + result.valid_count(), + 1, + "protocol version 14 accepts the {twin_label}: {:?}", + result.execution_results() + ); + asset_lock_used = true; + } else { + assert_matches!( + result.execution_results().as_slice(), + [StateTransitionExecutionResult::UnpaidConsensusError( + ConsensusError::SignatureError(SignatureError::BasicECDSAError(_)) + )], + "protocol version {protocol_version} refuses the {twin_label}" + ); + } + } + if !asset_lock_used { + // No twin was accepted, so the asset lock is unused and the signature the key + // made still tops up. + let result = process(&signed); + assert_eq!( + result.valid_count(), + 1, + "protocol version {protocol_version} accepts the signature: {:?}", + result.execution_results() + ); + } + } + } + /// Regression for the proposer-side mint accounting: a minting transition (an asset-lock /// top up) that executes but is then dropped from the proposal (`InternalError` -> /// `TxAction::Removed`) has its GroveDB writes rolled back to the savepoint — its mint diff --git a/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_top_up/transform_into_action/v0/mod.rs b/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_top_up/transform_into_action/v0/mod.rs index 174391c0975..3b2098c7058 100644 --- a/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_top_up/transform_into_action/v0/mod.rs +++ b/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_top_up/transform_into_action/v0/mod.rs @@ -6,10 +6,10 @@ use dpp::balances::credits::CREDITS_PER_DUFF; use dpp::consensus::basic::identity::IdentityAssetLockTransactionOutPointNotEnoughBalanceError; use dpp::consensus::signature::{BasicECDSAError, SignatureError}; -use dpp::dashcore::signer; use dpp::dashcore::{ScriptBuf, Txid}; use dpp::identity::state_transition::AssetLockProved; use dpp::identity::KeyType; +use dpp::util::ecdsa::verify_hash_signature; use dpp::prelude::ConsensusValidationResult; @@ -166,11 +166,15 @@ impl IdentityTopUpStateTransitionStateValidationV0 for IdentityTopUpTransition { SignatureVerificationOperation::new(KeyType::ECDSA_HASH160), )); - if let Err(e) = signer::verify_hash_signature( + // `verify_hash_signature` 0 at protocol versions 14 and earlier is the + // `dashcore::signer::verify_hash_signature` call this site made before; 1 also + // refuses a high-S signature and a header naming an uncompressed key. + if let Err(e) = verify_hash_signature( signable_bytes_hasher.hash_bytes().as_slice(), self.signature().as_slice(), public_key_hash, - ) { + platform_version, + )? { return Ok(ConsensusValidationResult::new_with_error( SignatureError::BasicECDSAError(BasicECDSAError::new(e.to_string())).into(), )); diff --git a/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shield_from_asset_lock/tests.rs b/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shield_from_asset_lock/tests.rs index b0eb7784452..511bff9996c 100644 --- a/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shield_from_asset_lock/tests.rs +++ b/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shield_from_asset_lock/tests.rs @@ -572,6 +572,12 @@ mod tests { mod signature_validation { use super::*; + use crate::execution::validation::state_transition::state_transitions::test_helpers::{ + build_outputs_only_bundle_bound, process_transition_and_commit, + setup_platform_at_protocol_version, OutputsOnlyBundle, + }; + use dpp::tests::utils::{high_s_twin, uncompressed_header_twin}; + use std::collections::BTreeMap; #[test] fn test_wrong_ecdsa_signature_returns_error() { @@ -642,11 +648,96 @@ mod tests { )] ); } - } - // ========================================== - // ZK PROOF VERIFICATION TESTS - // ========================================== + /// The asset lock's one-time key is checked by recovering the public key, which takes + /// any S and either header form. A third party can replace a shield's signature with its + /// high-S twin, or with the twin whose header names an uncompressed key, and change the + /// transition's hash. Protocol version 14 accepts each twin, for replay; from protocol + /// version 15 a block refuses each unpaid, with the error of a signature that does not + /// verify, and leaves the asset lock unused. The signature the key made is accepted at + /// both. + /// + /// The bundle is proved for real, so a transition that passes the signature check is + /// executed. + #[test] + fn should_refuse_an_asset_lock_signature_twin_from_protocol_version_15() { + let latest = PlatformVersion::latest().protocol_version; + let twins: [(&str, fn(&[u8]) -> Vec); 2] = [ + ("high-S twin", high_s_twin), + ("uncompressed header twin", uncompressed_header_twin), + ]; + // (protocol version, the twins submitted before the signature itself). The asset + // lock is consumed by the first transition a block accepts, so at 14 each twin gets + // its own platform, as does the signature. + // Every platform below is funded by the same asset lock, so a bundle proved against + // it serves each platform whose protocol version binds the lock the same way. + let mut bundles: BTreeMap, OutputsOnlyBundle> = BTreeMap::new(); + for (protocol_version, submitted) in [ + (14, &twins[..0]), + (14, &twins[..1]), + (14, &twins[1..]), + (latest, &twins[..]), + ] { + let platform_version = + PlatformVersion::get(protocol_version).expect("known protocol version"); + let platform = setup_platform_at_protocol_version(protocol_version); + + let mut rng = StdRng::seed_from_u64(567); + let (asset_lock_proof, asset_lock_pk) = create_asset_lock_proof_with_key(&mut rng); + let extra_sighash_data = + shield_from_asset_lock_extra_sighash_data(&asset_lock_proof, platform_version) + .expect("the binding of the funding asset lock"); + let bundle = bundles + .entry(extra_sighash_data.clone()) + .or_insert_with(|| build_outputs_only_bundle_bound(5_000, &extra_sighash_data)) + .clone(); + let signed = create_signed_shield_from_asset_lock_transition( + asset_lock_proof, + &asset_lock_pk, + bundle.actions, + bundle.amount, + bundle.anchor, + bundle.proof, + bundle.binding_signature, + ); + let signature = signed.signature().expect("signed").to_vec(); + + let mut asset_lock_used = false; + for (twin_label, twin) in submitted { + let mut malleated = signed.clone(); + malleated.set_signature(twin(&signature).into()); + let result = + process_transition_and_commit(&platform, malleated, platform_version); + if protocol_version == 14 { + assert_matches!( + result.execution_results().as_slice(), + [StateTransitionExecutionResult::SuccessfulExecution { .. }], + "protocol version 14 accepts the {twin_label}" + ); + asset_lock_used = true; + } else { + assert_matches!( + result.execution_results().as_slice(), + [StateTransitionExecutionResult::UnpaidConsensusError( + ConsensusError::SignatureError(SignatureError::BasicECDSAError(_)) + )], + "protocol version {protocol_version} refuses the {twin_label}" + ); + } + } + if !asset_lock_used { + // No twin was accepted, so the asset lock is unused and the signature the + // key made still shields. + let result = process_transition_and_commit(&platform, signed, platform_version); + assert_matches!( + result.execution_results().as_slice(), + [StateTransitionExecutionResult::SuccessfulExecution { .. }], + "protocol version {protocol_version} accepts the signature" + ); + } + } + } + } mod proof_verification { use super::*; diff --git a/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shield_from_asset_lock/transform_into_action/v1/mod.rs b/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shield_from_asset_lock/transform_into_action/v1/mod.rs index 620dc964b83..38f2a9d8cc8 100644 --- a/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shield_from_asset_lock/transform_into_action/v1/mod.rs +++ b/packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/shield_from_asset_lock/transform_into_action/v1/mod.rs @@ -22,7 +22,7 @@ use dpp::consensus::basic::identity::IdentityAssetLockTransactionOutPointNotEnou use dpp::consensus::basic::state_transition::ShieldedImplicitFeeCapExceededError; use dpp::consensus::signature::{BasicECDSAError, SignatureError}; use dpp::consensus::state::state_error::StateError; -use dpp::dashcore::signer; +use dpp::util::ecdsa::verify_hash_signature; use dpp::dashcore::{ ScriptBuf, Txid}; use dpp::fee::Credits; use dpp::shielded::{compute_minimum_shielded_fee, shield_from_asset_lock_extra_sighash_data}; @@ -225,11 +225,15 @@ impl ShieldFromAssetLockStateTransitionTransformIntoActionValidationV1 SignatureVerificationOperation::new(KeyType::ECDSA_HASH160), )); - if let Err(e) = signer::verify_hash_signature( + // `verify_hash_signature` 0 at protocol versions 14 and earlier is the + // `dashcore::signer::verify_hash_signature` call this site made before; 1 also + // refuses a high-S signature and a header naming an uncompressed key. + if let Err(e) = verify_hash_signature( signable_bytes_hasher.hash_bytes().as_slice(), self.signature().as_slice(), public_key_hash, - ) { + platform_version, + )? { return Ok(ConsensusValidationResult::new_with_error( SignatureError::BasicECDSAError(BasicECDSAError::new(e.to_string())).into(), )); diff --git a/packages/rs-platform-version/src/version/dpp_versions/dpp_state_transition_method_versions/mod.rs b/packages/rs-platform-version/src/version/dpp_versions/dpp_state_transition_method_versions/mod.rs index 3564e202adc..56585545867 100644 --- a/packages/rs-platform-version/src/version/dpp_versions/dpp_state_transition_method_versions/mod.rs +++ b/packages/rs-platform-version/src/version/dpp_versions/dpp_state_transition_method_versions/mod.rs @@ -2,6 +2,7 @@ use versioned_feature_core::FeatureVersion; pub mod v1; pub mod v2; +pub mod v3; #[derive(Clone, Debug, Default)] pub struct DPPStateTransitionMethodVersions { @@ -9,6 +10,13 @@ pub struct DPPStateTransitionMethodVersions { /// `StateTransition::verify_identity_signed_signature`: 0 accepts a BLS12_381 signature /// that is well formed but does not verify, 1 refuses it. pub verify_identity_signed_signature: FeatureVersion, + /// `dpp::util::ecdsa::verify_hash_signature`, the check of a compact recoverable + /// ECDSA signature by recovering its public key and comparing the key's HASH160 (an + /// ECDSA_HASH160 identity key, a P2PKH address witness, an asset lock's one-time key): 0 + /// accepts a signature whose S is in the upper half of the curve order and one whose header + /// names an uncompressed key, each of which any third party can derive from a valid + /// signature, 1 refuses both. + pub verify_hash_signature: FeatureVersion, } #[derive(Clone, Debug, Default)] diff --git a/packages/rs-platform-version/src/version/dpp_versions/dpp_state_transition_method_versions/v1.rs b/packages/rs-platform-version/src/version/dpp_versions/dpp_state_transition_method_versions/v1.rs index 2d02c1c55a4..c0cc3a40698 100644 --- a/packages/rs-platform-version/src/version/dpp_versions/dpp_state_transition_method_versions/v1.rs +++ b/packages/rs-platform-version/src/version/dpp_versions/dpp_state_transition_method_versions/v1.rs @@ -13,4 +13,5 @@ pub const STATE_TRANSITION_METHOD_VERSIONS_V1: DPPStateTransitionMethodVersions validate_identity_public_keys_structure: 0, }, verify_identity_signed_signature: 0, + verify_hash_signature: 0, }; diff --git a/packages/rs-platform-version/src/version/dpp_versions/dpp_state_transition_method_versions/v2.rs b/packages/rs-platform-version/src/version/dpp_versions/dpp_state_transition_method_versions/v2.rs index f00e3e9b46d..a429a6e5d2b 100644 --- a/packages/rs-platform-version/src/version/dpp_versions/dpp_state_transition_method_versions/v2.rs +++ b/packages/rs-platform-version/src/version/dpp_versions/dpp_state_transition_method_versions/v2.rs @@ -18,4 +18,5 @@ pub const STATE_TRANSITION_METHOD_VERSIONS_V2: DPPStateTransitionMethodVersions validate_identity_public_keys_structure: 1, }, verify_identity_signed_signature: 1, + verify_hash_signature: 0, }; diff --git a/packages/rs-platform-version/src/version/dpp_versions/dpp_state_transition_method_versions/v3.rs b/packages/rs-platform-version/src/version/dpp_versions/dpp_state_transition_method_versions/v3.rs new file mode 100644 index 00000000000..88f6c2a47c1 --- /dev/null +++ b/packages/rs-platform-version/src/version/dpp_versions/dpp_state_transition_method_versions/v3.rs @@ -0,0 +1,13 @@ +use crate::version::dpp_versions::dpp_state_transition_method_versions::v2::STATE_TRANSITION_METHOD_VERSIONS_V2; +use crate::version::dpp_versions::dpp_state_transition_method_versions::DPPStateTransitionMethodVersions; + +/// V3 is protocol version 15's table. `verify_hash_signature` 0 -> 1: a signature checked by +/// recovering its public key (an ECDSA_HASH160 identity key, a P2PKH address witness, an asset +/// lock's one-time key) must have S in the lower half of the curve order and a header naming a +/// compressed key, so a third party can no longer turn a valid signature into a second one and +/// change the transition's hash. V2 stays as is for replay. +pub const STATE_TRANSITION_METHOD_VERSIONS_V3: DPPStateTransitionMethodVersions = + DPPStateTransitionMethodVersions { + verify_hash_signature: 1, + ..STATE_TRANSITION_METHOD_VERSIONS_V2 + }; diff --git a/packages/rs-platform-version/src/version/v15.rs b/packages/rs-platform-version/src/version/v15.rs index 2a3c212d2a9..6a0691f5044 100644 --- a/packages/rs-platform-version/src/version/v15.rs +++ b/packages/rs-platform-version/src/version/v15.rs @@ -7,7 +7,7 @@ use crate::version::dpp_versions::dpp_factory_versions::v1::DPP_FACTORY_VERSIONS use crate::version::dpp_versions::dpp_identity_versions::v1::IDENTITY_VERSIONS_V1; use crate::version::dpp_versions::dpp_method_versions::v3::DPP_METHOD_VERSIONS_V3; use crate::version::dpp_versions::dpp_state_transition_conversion_versions::v2::STATE_TRANSITION_CONVERSION_VERSIONS_V2; -use crate::version::dpp_versions::dpp_state_transition_method_versions::v2::STATE_TRANSITION_METHOD_VERSIONS_V2; +use crate::version::dpp_versions::dpp_state_transition_method_versions::v3::STATE_TRANSITION_METHOD_VERSIONS_V3; use crate::version::dpp_versions::dpp_state_transition_serialization_versions::v4::STATE_TRANSITION_SERIALIZATION_VERSIONS_V4; use crate::version::dpp_versions::dpp_state_transition_versions::v5::STATE_TRANSITION_VERSIONS_V5; use crate::version::dpp_versions::dpp_token_versions::v3::TOKEN_VERSIONS_V3; @@ -913,6 +913,22 @@ pub const PROTOCOL_VERSION_15: ProtocolVersion = 15; /// `validate_schema_patterns` and `registration_cost` 2, so a block accepts there what it /// accepted before, and a contract registered earlier is not checked again when it is /// read. +/// +/// 43. **One signature form where the key is recovered**: an ECDSA signature checked by +/// recovering its public key and comparing the key's HASH160 (an identity-signed +/// transition's ECDSA_HASH160 key, masternode votes included; a P2PKH address witness; +/// the one-time key of an asset lock funding an identity create, a top-up, an address +/// funding or a shield) stays valid with S replaced by `n - S` and the recovery id's +/// parity flipped, and with its header rewritten from that of a compressed key +/// (`31 + id`) to that of an uncompressed one (`27 + id`), so a third party could change +/// a transition's hash. `verify_hash_signature` 1 (`STATE_TRANSITION_METHOD_VERSIONS_V3`) +/// refuses a signature whose S is in the upper half of the curve order or whose header +/// does not name a compressed key, with the error each path gives a signature that does +/// not verify, unpaid as before. Signing produces low-S signatures under the compressed +/// key's header, so no honest signer is affected. The checks that verify against a +/// public key already refuse high-S. Protocol v14 and earlier keep +/// `verify_hash_signature` 0, so the blocks committed under them replay with the same +/// results. pub const PLATFORM_V15: PlatformVersion = PlatformVersion { protocol_version: PROTOCOL_VERSION_15, drive: DRIVE_VERSION_V10, // changed: calculate_fee 1 prices a refund at the storing epoch's rate (note 27); apply_contract_with_serialization 1 writes nothing for a contract whose format version 1 bytes match the stored one's (note 8); vote method versions v4: register_identity_vote 1 registers a yes/no vote, remove_all_votes_given_by_identities 1 also removes the yes/no votes of a masternode that left the list, every yes_no method and remove_vote_poll_end_date_query_operations Some(0) make yes/no polls active, add_vote_poll_end_date_query_operations 1 adds only the entry when the batch already queues the end date's tree with other storage flags, and verify methods v4 make the yes/no proof verifiers active (note 10) and verify_private_document_entries Some(0) a page of a private type's store (note 13); state transition method versions v5: document_create_transition 1 deletes a consumed document from what its lookup read (note 11); document method versions v5: insert_empty_private_document_store_operation Some(0) creates a private document type's store, and fetch_private_document_entries and prove_private_document_entries Some(0) read and prove a page of it (note 13); apply_drive_operations 2 resolves a batch's estimate, remove_reference_for_index_level_for_contract_operations 2, add_reference_for_index_level_for_contract_operations 1, stateless_delete_of_non_tree_for_costs 1 and for_total_system_credits_update 1 describe their layers as they are (note 14); create_initial_state_structure 5 adds the token marketplace escrow trees; calculate_total_tokens_balance 2 counts escrowed tokens; the escrow primitives; the identity balance, token balance, prefunded specialized balance and contract fee pot estimations (for_balances 1, for_token_balances 1, estimated_cost_for_prefunded_specialized_balance_update 2, add_estimation_costs_for_contract_fee_pot_update 1) keep a deeper layer description (note 17); fetch_core_fork_activation_height and store_core_fork_activation_height Some(0) keep the Core fork activations (note 18); document method versions v5: append_private_document_entries_operations Some(0) appends a private document create's entries (note 25); grove_version GROVE_V5 (note 26); verify methods v4: verify_proof_keep_serialized 1 verifies a document that may be contested (note 29); document method versions v5: single_document_path_query 1 selects the latest retained revision at or before a block time, and verify_proof_keep_serialized 1 verifies it (note 31); retained-history writers, readers, estimates and verifiers select the per-type revision layout (note 35); add_new_keys_to_masternode_identity 1 adds a masternode key whose hash the identity already holds a reference for (note 32); document method versions v5: insert.add_document_for_contract_operations 2, delete.delete_document_for_contract_operations 1, delete.remove_document_from_primary_storage 1 and its estimate 1, query.fetch_document_lifecycle Some(0), and the erase/lifecycle estimate and erase_refund_recipient_cost selectors Some(0); batch_operations.convert_drive_operations_to_grove_operations 2; state transition method versions v5: document_delete_transition 1 and document_erase_transition Some(0); verify methods v4: single_document.verify_proof 1 extend retained history (note 41); document method versions v5: add_indices_for_top_index_level_for_contract_operations 3 writes no entry in an expired time range bucket (note 36) @@ -929,7 +945,7 @@ pub const PLATFORM_V15: PlatformVersion = PlatformVersion { validation: DPP_VALIDATION_VERSIONS_V6, // changed: document type update 2 preserves erasure constraints (note 41) state_transition_serialization_versions: STATE_TRANSITION_SERIALIZATION_VERSIONS_V4, // changed: document erase bounds Some(0) admit the appended kind (note 41) state_transition_conversion_versions: STATE_TRANSITION_CONVERSION_VERSIONS_V2, - state_transition_method_versions: STATE_TRANSITION_METHOD_VERSIONS_V2, + state_transition_method_versions: STATE_TRANSITION_METHOD_VERSIONS_V3, // changed: verify_hash_signature 1 refuses a high-S signature and an uncompressed-key header where the key is recovered (note 43) state_transitions: STATE_TRANSITION_VERSIONS_V5, // changed: validate_asset_lock_transaction_structure 1 drops the input cap and validate_instant_asset_lock_proof_structure 1 applies it to instant proofs only, so a chain proof redeems a lock of more than 100 inputs (note 30) contract_versions: CONTRACT_VERSIONS_V7, // changed: parse_property_constraints 1 lets countOf and sumOf match byte array keys (note 4); validate_document 2 checks documents natively (note 6); check_document_type_schema 1 checks schemas natively at registration (note 7); contract_storage_serialization_version 2 stores contracts compiled (note 8); parse_private_document 0 parses private document types (note 13); apply_generated_from 1 admits sys.tokens.decimals (note 15); try_from_schema 4 parses lifecycle flags (note 41); validate_schema_patterns 0 bounds the patterns of a contract and registration_cost 3 estimates its fee without compiling one (note 42) document_versions: DOCUMENT_VERSIONS_V4, diff --git a/packages/rs-sdk/src/platform/transition/broadcast/retry_tests.rs b/packages/rs-sdk/src/platform/transition/broadcast/retry_tests.rs index b55a906f3f5..4fae33476d9 100644 --- a/packages/rs-sdk/src/platform/transition/broadcast/retry_tests.rs +++ b/packages/rs-sdk/src/platform/transition/broadcast/retry_tests.rs @@ -521,7 +521,7 @@ fn signed_address_transfer() -> StateTransition { signature: signer::sign(&bytes, &private_key).unwrap().to_vec().into(), }; input - .verify_bytes_against_witness(&witness, &bytes) + .verify_bytes_against_witness(&witness, &bytes, PlatformVersion::latest()) .unwrap(); transfer.input_witnesses.push(witness); let transition = transfer.into();