From 21a8b726a575320044e235dbf9a434e083509932 Mon Sep 17 00:00:00 2001 From: Quantum Explorer Date: Fri, 13 Nov 2026 02:12:04 +0700 Subject: [PATCH 1/3] fix(platform)!: refuse high-S signatures on key-recovery paths from protocol version 15 A signature checked by recovering its public key and comparing HASH160 (ECDSA_HASH160 identity keys, masternode votes included; P2PKH address witnesses; asset-lock one-time keys) stays valid with S replaced by n - S and the recovery parity flipped, so a third party can change a transition's hash. Every released Platform accepts that twin there. `dpp::util::ecdsa::verify_hash_signature` dispatches on the new `verify_hash_signature` switch: 0 is dashcore's recovery check, 1 also refuses S above n/2 (`EcdsaSignature::is_low_s`) with VerifyFailed, after the library check so every other input keeps its error. STATE_TRANSITION_METHOD_VERSIONS_V3 selects 1, for PLATFORM_V15 only. Each path keeps its error and its unpaid class: InvalidStateTransition- SignatureError for identity signatures and witnesses, BasicECDSAError for asset locks. `validate_witnesses` and `verify_bytes_against_witness` take `platform_version`; only UnknownVersionMismatch propagates as an internal error, every other error maps to the consensus error as before. Ported onto v5.1-dev from #5670, whose base (#5637) was closed unmerged. Co-authored-by: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Co-Authored-By: Claude Fable 5.2 --- .../src/address_funds/platform_address.rs | 95 ++++++-- packages/rs-dpp/src/state_transition/mod.rs | 100 ++++++++- .../signing_tests.rs | 6 +- .../signing_tests.rs | 2 +- .../state_transition_witness_validation.rs | 34 ++- .../verify_identity_signed_signature/mod.rs | 8 +- .../v0/mod.rs | 14 +- .../v1/mod.rs | 4 +- packages/rs-dpp/src/tests/utils/mod.rs | 16 ++ packages/rs-dpp/src/util/ecdsa.rs | 211 ++++++++++++++++++ packages/rs-dpp/tests/ecdsa_compatibility.rs | 59 ++++- .../processor/traits/address_witnesses.rs | 38 +++- .../address_funding_from_asset_lock/tests.rs | 84 +++++++ .../transform_into_action/v0/mod.rs | 10 +- .../address_funds_transfer/tests.rs | 104 +++++++++ .../state_transitions/identity_create/mod.rs | 135 +++++++++++ .../identity_create/state/v0/mod.rs | 10 +- .../state_transitions/identity_top_up/mod.rs | 140 ++++++++++++ .../transform_into_action/v0/mod.rs | 10 +- .../shield_from_asset_lock/tests.rs | 83 ++++++- .../transform_into_action/v1/mod.rs | 10 +- .../mod.rs | 7 + .../v1.rs | 1 + .../v2.rs | 1 + .../v3.rs | 13 ++ .../rs-platform-version/src/version/v15.rs | 17 +- .../transition/broadcast/retry_tests.rs | 2 +- 27 files changed, 1131 insertions(+), 83 deletions(-) create mode 100644 packages/rs-platform-version/src/version/dpp_versions/dpp_state_transition_method_versions/v3.rs diff --git a/packages/rs-dpp/src/address_funds/platform_address.rs b/packages/rs-dpp/src/address_funds/platform_address.rs index 248eb209588..a4a238dcd85 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 is 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 df55059a7b5..3bd11b04039 100644 --- a/packages/rs-dpp/src/state_transition/mod.rs +++ b/packages/rs-dpp/src/state_transition/mod.rs @@ -2401,6 +2401,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 { @@ -2416,15 +2417,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")] @@ -3425,6 +3430,83 @@ mod tests { } } + // An ECDSA_HASH160 key is checked by recovering the public key, which takes any S. From + // protocol version 15 (`verify_hash_signature` 1) the high-S twin of a valid signature is + // refused; protocol version 14 keeps accepting it for replay. Masternode votes, signed by an + // ECDSA_HASH160 VOTING key, take the same path. + #[cfg(feature = "state-transition-signing")] + #[test] + fn should_refuse_a_high_s_ecdsa_hash160_signature_from_protocol_version_15() { + use crate::identity::identity_public_key::v0::IdentityPublicKeyV0; + use crate::tests::utils::high_s_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 mut malleated = signed.clone(); + malleated.set_signature(BinaryData::new(high_s_twin( + signed.signature().expect("signed").as_slice(), + ))); + + 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 + ) + }); + } + malleated + .verify_identity_signed_signature(&key, &bls, last_without_the_check) + .unwrap_or_else(|e| { + panic!("{label}: protocol version 14 accepts the high-S twin: {e:?}") + }); + let error = malleated + .verify_identity_signed_signature(&key, &bls, latest) + .expect_err("the high-S twin is refused"); + assert!( + matches!(&error, ProtocolError::ConsensusError(consensus_error) + if matches!(&**consensus_error, ConsensusError::SignatureError( + SignatureError::InvalidStateTransitionSignatureError(_) + ))), + "{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..25d860dcac9 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. 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..94c1b4136ff 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, 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..a4494693362 100644 --- a/packages/rs-dpp/src/tests/utils/mod.rs +++ b/packages/rs-dpp/src/tests/utils/mod.rs @@ -284,3 +284,19 @@ 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 +} diff --git a/packages/rs-dpp/src/util/ecdsa.rs b/packages/rs-dpp/src/util/ecdsa.rs index 8145af267f2..b783187fdf1 100644 --- a/packages/rs-dpp/src/util/ecdsa.rs +++ b/packages/rs-dpp/src/util/ecdsa.rs @@ -3,9 +3,16 @@ //! 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, so the refusal is a rule of +//! protocol version 15, selected by `verify_hash_signature`. +use crate::ProtocolError; +use anyhow::ensure; use dashcore::crypto::ecdsa::{EcdsaError, EcdsaPublicKey, EcdsaSignature}; use dashcore::{sign_message, signer}; +use platform_version::version::PlatformVersion; /// [`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. @@ -36,3 +43,207 @@ pub fn verify_digest( } public_key.verify(digest, signature) } + +/// 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 takes any S, so a signature stays valid with S replaced by `n - S` and the recovery +/// id's parity flipped. A third party can derive that twin from a valid signature and change the +/// hash of the transition carrying it. +/// +/// # 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 +/// [`signer::verify_hash_signature`], which accepts a high-S signature (protocol versions 1 to +/// 14); 1 also refuses it. +/// +/// # 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(signer::verify_hash_signature( + data_hash, + signature, + public_key_hash, + )), + 1 => Ok(verify_low_s_hash_signature( + data_hash, + signature, + public_key_hash, + )), + version => Err(ProtocolError::UnknownVersionMismatch { + method: "verify_hash_signature".to_string(), + known_versions: vec![0, 1], + received: version, + }), + } +} + +/// [`signer::verify_hash_signature`], refusing a high-S signature with +/// [`EcdsaError::VerifyFailed`]. The library checks first, so every other input fails exactly as +/// it does in version 0, with the library's own error. +fn verify_low_s_hash_signature( + 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)?.signature()?; + ensure!(signature.is_low_s(), EcdsaError::VerifyFailed); + Ok(()) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::tests::utils::high_s_twin; + use dashcore::signer::{double_sha, sign_hash}; + use dashcore::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 + } + + 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_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()); + } + + /// A signature recovery already refuses (another key, a wrong length, S zero or not below the + /// curve order) 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", + ), + ]; + 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/tests/ecdsa_compatibility.rs b/packages/rs-dpp/tests/ecdsa_compatibility.rs index a5885ffe1ca..8331a22e18f 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,48 @@ fn should_reject_high_s_verification_but_accept_high_s_recovery() { .unwrap(); } +/// A P2PKH witness is checked by recovering the key, which takes any S. Protocol version 14 +/// accepts the historical signature's high-S twin, for replay; from protocol version 15 +/// (`verify_hash_signature` 1) it is refused as a signature that does not verify. The low-S +/// signature is accepted at both. +#[test] +fn should_refuse_a_high_s_p2pkh_witness_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()) + ); + } + assert_eq!( + address + .verify_bytes_against_witness(&witness(high_s_twin()), &data, last_without_the_check) + .unwrap(), + AddressWitnessVerificationOperations::for_p2pkh(data.len()) + ); + match address + .verify_bytes_against_witness(&witness(high_s_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 +330,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 +362,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 +410,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 +432,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..b0db6ec279e 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,90 @@ mod tests { ); } + /// The asset lock's one-time key is checked by recovering the public key, which takes + /// any S. Protocol version 14 accepts the high-S twin of the signature, for replay; from + /// protocol version 15 a block refuses it unpaid, with the error of a signature that does + /// not verify, and leaves the asset lock unused. The low-S signature is accepted at both. + #[tokio::test] + async fn should_refuse_a_high_s_asset_lock_signature_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; + + let latest = PlatformVersion::latest().protocol_version; + // (protocol version, whether the twin is submitted). The asset lock is consumed by + // the first transition a block accepts, so the low-S signature at 14 gets its own + // platform. + for (protocol_version, submit_twin) in [(14, false), (14, true), (latest, true)] { + 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 mut malleated = signed.clone(); + malleated.set_signature( + high_s_twin(signed.signature().expect("signed").as_slice()).into(), + ); + + if !submit_twin { + let result = process_transition_and_commit(&platform, signed, platform_version); + assert_matches!( + result.execution_results().as_slice(), + [StateTransitionExecutionResult::SuccessfulExecution { .. }], + "the low-S signature is accepted" + ); + continue; + } + 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 high-S twin" + ); + } else { + assert_matches!( + result.execution_results().as_slice(), + [StateTransitionExecutionResult::UnpaidConsensusError( + ConsensusError::SignatureError(SignatureError::BasicECDSAError(_)) + )], + "protocol version {protocol_version} refuses the high-S twin" + ); + // The asset lock is left unused, so 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 { .. }], + "the low-S signature is accepted" + ); + } + } + } + #[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..fe10c2f0866 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. + 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..8dba793b023 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,108 @@ mod tests { } } } + + // ========================================== + // HIGH-S P2PKH WITNESS + // ========================================== + + /// A P2PKH witness is checked by recovering the public key, which takes any S. A third party + /// can replace a witness's signature with its high-S twin and change the transition's hash. + /// Protocol version 14 accepts the twin, for replay; from protocol version 15 a block refuses + /// it unpaid, with the error of a witness that does not verify. The low-S witness is accepted + /// at both. + mod high_s_witness { + use super::*; + use dpp::tests::utils::high_s_twin; + + #[tokio::test] + async fn should_refuse_a_high_s_p2pkh_witness_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 mut malleated = signed.clone(); + let StateTransition::AddressFundsTransfer(AddressFundsTransferTransition::V0(v0)) = + &mut malleated + else { + panic!("expected an address funds transfer"); + }; + let AddressWitness::P2pkh { signature } = &v0.input_witnesses[0] else { + panic!("expected a P2PKH witness"); + }; + v0.input_witnesses[0] = AddressWitness::P2pkh { + signature: BinaryData::new(high_s_twin(signature.as_slice())), + }; + + // Each transition runs in its own uncommitted block transaction, so both 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 low-S witness" + ); + let malleated_results = process(&malleated); + if protocol_version == 14 { + assert_matches!( + malleated_results.as_slice(), + [StateTransitionExecutionResult::SuccessfulExecution { .. }], + "protocol version 14 accepts the high-S twin" + ); + } else { + assert_matches!( + malleated_results.as_slice(), + [StateTransitionExecutionResult::UnpaidConsensusError( + ConsensusError::SignatureError( + SignatureError::InvalidStateTransitionSignatureError(_) + ) + )], + "protocol version {protocol_version} refuses the high-S twin" + ); + } + } + } + } } 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..4b907a431a5 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,141 @@ 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. + /// A third party can replace an identity create's signature with its high-S twin and change + /// the transition's hash. Protocol version 14 accepts the twin, for replay; from protocol + /// version 15 a block refuses it unpaid, with the error of a signature that does not verify, + /// and leaves the asset lock unused. The low-S signature is accepted at both. + #[tokio::test] + async fn should_refuse_a_high_s_asset_lock_signature_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; + + let latest = PlatformVersion::latest().protocol_version; + // (protocol version, whether the twin is submitted). The asset lock is consumed by the + // first transition a block accepts, so the low-S signature at 14 gets its own platform. + for (protocol_version, submit_twin) in [(14, false), (14, true), (latest, true)] { + 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 mut malleated = signed.clone(); + malleated + .set_signature(high_s_twin(signed.signature().expect("signed").as_slice()).into()); + + 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 + }; + + if !submit_twin { + let result = process(&signed); + assert_eq!( + result.valid_count(), + 1, + "the low-S signature is accepted: {:?}", + result.execution_results() + ); + continue; + } + let result = process(&malleated); + if protocol_version == 14 { + assert_eq!( + result.valid_count(), + 1, + "protocol version 14 accepts the high-S twin: {:?}", + result.execution_results() + ); + } else { + assert_matches!( + result.execution_results().as_slice(), + [StateTransitionExecutionResult::UnpaidConsensusError( + ConsensusError::SignatureError(SignatureError::BasicECDSAError(_)) + )], + "protocol version {protocol_version} refuses the high-S twin" + ); + // The asset lock is left unused, so the signature the key made still creates. + let result = process(&signed); + assert_eq!( + result.valid_count(), + 1, + "the low-S signature is accepted: {:?}", + 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..af832005160 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. + 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..d0d418fabb9 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,146 @@ 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. + /// A third party can replace a top-up's signature with its high-S twin and change the + /// transition's hash. Protocol version 14 accepts the twin, for replay; from protocol version + /// 15 a block refuses it unpaid, with the error of a signature that does not verify, and + /// leaves the asset lock unused. The low-S signature the key made is accepted at both. + #[test] + fn should_refuse_a_high_s_asset_lock_signature_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; + + let latest = PlatformVersion::latest().protocol_version; + // (protocol version, whether the twin is submitted). The asset lock is consumed by the + // first transition a block accepts, so the low-S signature at 14 gets its own platform. + for (protocol_version, submit_twin) in [(14, false), (14, true), (latest, true)] { + 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 mut malleated = signed.clone(); + malleated + .set_signature(high_s_twin(signed.signature().expect("signed").as_slice()).into()); + + 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 + }; + + if !submit_twin { + let signed_result = process(&signed); + assert_eq!( + signed_result.valid_count(), + 1, + "the low-S signature is accepted: {:?}", + signed_result.execution_results() + ); + continue; + } + let twin_result = process(&malleated); + if protocol_version == 14 { + assert_eq!( + twin_result.valid_count(), + 1, + "protocol version 14 accepts the high-S twin: {:?}", + twin_result.execution_results() + ); + } else { + assert_matches!( + twin_result.execution_results().as_slice(), + [StateTransitionExecutionResult::UnpaidConsensusError( + ConsensusError::SignatureError(SignatureError::BasicECDSAError(_)) + )], + "protocol version {protocol_version} refuses the high-S twin" + ); + // The asset lock is left unused, so the signature the key made still tops up. + let signed_result = process(&signed); + assert_eq!( + signed_result.valid_count(), + 1, + "the low-S signature is accepted: {:?}", + signed_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..778729a5ecf 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. + 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..f774d4633a6 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,11 @@ 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, + }; + use dpp::tests::utils::high_s_twin; #[test] fn test_wrong_ecdsa_signature_returns_error() { @@ -642,11 +647,81 @@ mod tests { )] ); } - } - // ========================================== - // ZK PROOF VERIFICATION TESTS - // ========================================== + /// The asset lock's one-time key is checked by recovering the public key, which takes + /// any S. A third party can replace a shield's signature with its high-S twin and change + /// the transition's hash. Protocol version 14 accepts the twin, for replay; from protocol + /// version 15 a block refuses it unpaid, with the error of a signature that does not + /// verify, and leaves the asset lock unused. The low-S signature is accepted at both. + /// + /// The bundle is proved for real, so a transition that passes the signature check is + /// executed. + #[test] + fn should_refuse_a_high_s_asset_lock_signature_from_protocol_version_15() { + let latest = PlatformVersion::latest().protocol_version; + // (protocol version, whether the twin is submitted). The asset lock is consumed by + // the first transition a block accepts, so the low-S signature at 14 gets its own + // platform. + for (protocol_version, submit_twin) in [(14, false), (14, true), (latest, true)] { + 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 = build_outputs_only_bundle_bound(5_000, &extra_sighash_data); + 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 mut malleated = signed.clone(); + malleated.set_signature( + high_s_twin(signed.signature().expect("signed").as_slice()).into(), + ); + + if !submit_twin { + let result = process_transition_and_commit(&platform, signed, platform_version); + assert_matches!( + result.execution_results().as_slice(), + [StateTransitionExecutionResult::SuccessfulExecution { .. }], + "the low-S signature is accepted" + ); + continue; + } + 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 high-S twin" + ); + } else { + assert_matches!( + result.execution_results().as_slice(), + [StateTransitionExecutionResult::UnpaidConsensusError( + ConsensusError::SignatureError(SignatureError::BasicECDSAError(_)) + )], + "protocol version {protocol_version} refuses the high-S twin" + ); + // The asset lock is left unused, so 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 { .. }], + "the low-S signature is accepted" + ); + } + } + } + } 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..bf62a47f7ca 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. + 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..97a2535bf25 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,12 @@ 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, which any third party + /// can derive from a valid one, 1 refuses it. + 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..4f595b1e4f8 --- /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, 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 d64120c5e2b..7c5177035d7 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::v3::STATE_TRANSITION_SERIALIZATION_VERSIONS_V3; 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; @@ -866,6 +866,19 @@ pub const PROTOCOL_VERSION_15: ProtocolVersion = 15; /// names with the one its document type expects by contract, document type and index /// name, where generation 1, which protocol v14 keeps, compares them whole. Protocol /// v14 keeps generation 0 of `canonical_contested_index_values`. +/// +/// 40. **Low-S signatures 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, 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 with the error each path gives a +/// signature that does not verify, unpaid as before. Signing produces low-S signatures, +/// 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: add_indices_for_top_index_level_for_contract_operations 3 writes no entry in an expired time range bucket (note 36) @@ -882,7 +895,7 @@ pub const PLATFORM_V15: PlatformVersion = PlatformVersion { validation: DPP_VALIDATION_VERSIONS_V5, state_transition_serialization_versions: STATE_TRANSITION_SERIALIZATION_VERSIONS_V3, 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 checked by key recovery (note 40) 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) 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(); From 04ddb77cb728ccd0b36e071299042c2a6855a05e Mon Sep 17 00:00:00 2001 From: Quantum Explorer Date: Fri, 13 Nov 2026 06:16:31 +0700 Subject: [PATCH 2/3] fix(dpp)!: require the compressed-key header on key-recovery paths from protocol version 15 Key recovery hashes the recovered key in its compressed form whatever the compact signature's header says, so a valid signature stays valid with its header rewritten from 31 + id to 27 + id: a second twin, beside the high-S one, that changes a transition's hash without the signer's key. Generation 1 of `verify_hash_signature` (protocol version 15 only) now also refuses a header that does not name a compressed key, with VerifyFailed, the error of a signature that does not verify. Generation 0 is unchanged. The dispatcher moves to its own directory, `util/ecdsa/verify_hash_signature`, with one module per generation (`v0`, `v1`), as the versioning convention asks. Tests build the header twin with `uncompressed_header_twin` and run it beside the high-S twin through the dispatcher, the dpp paths and the five drive-abci paths: accepted at protocol version 14, refused at the latest. Co-Authored-By: Claude Fable 5.2 --- packages/rs-dpp/src/util/{ecdsa.rs => ecdsa/mod.rs} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename packages/rs-dpp/src/util/{ecdsa.rs => ecdsa/mod.rs} (100%) diff --git a/packages/rs-dpp/src/util/ecdsa.rs b/packages/rs-dpp/src/util/ecdsa/mod.rs similarity index 100% rename from packages/rs-dpp/src/util/ecdsa.rs rename to packages/rs-dpp/src/util/ecdsa/mod.rs From 953ad278f566e3d11191cf4f2c4c46ba62d8492b Mon Sep 17 00:00:00 2001 From: Quantum Explorer Date: Fri, 13 Nov 2026 06:17:01 +0700 Subject: [PATCH 3/3] fix(dpp)!: refuse an uncompressed-key header in generation 1 of verify_hash_signature The previous commit (04ddb77cb7) carries only the move of `util/ecdsa.rs` to `util/ecdsa/mod.rs`: its other paths were left unstaged by mistake. This commit holds the change its message describes: - generation 1 of `verify_hash_signature` (protocol version 15 only) refuses a compact signature whose header does not name a compressed key, beside the existing low-S check, with VerifyFailed; generation 0 is unchanged; - the dispatcher lives in `util/ecdsa/verify_hash_signature` with one module per generation (`v0`, `v1`); - `uncompressed_header_twin` and the header-twin cases in the dispatcher, dpp path and drive-abci path tests; - the doc comments and the protocol version 15 note (41) name both rules. Co-Authored-By: Claude Fable 5.2 --- .../src/address_funds/platform_address.rs | 2 +- packages/rs-dpp/src/state_transition/mod.rs | 58 ++-- .../verify_identity_signed_signature/mod.rs | 2 +- .../v0/mod.rs | 2 +- packages/rs-dpp/src/tests/utils/mod.rs | 16 ++ packages/rs-dpp/src/util/ecdsa/mod.rs | 216 +------------- .../util/ecdsa/verify_hash_signature/mod.rs | 268 ++++++++++++++++++ .../ecdsa/verify_hash_signature/v0/mod.rs | 12 + .../ecdsa/verify_hash_signature/v1/mod.rs | 25 ++ packages/rs-dpp/tests/ecdsa_compatibility.rs | 59 ++-- .../address_funding_from_asset_lock/tests.rs | 87 +++--- .../transform_into_action/v0/mod.rs | 2 +- .../address_funds_transfer/tests.rs | 88 +++--- .../state_transitions/identity_create/mod.rs | 88 +++--- .../identity_create/state/v0/mod.rs | 2 +- .../state_transitions/identity_top_up/mod.rs | 94 +++--- .../transform_into_action/v0/mod.rs | 2 +- .../shield_from_asset_lock/tests.rs | 100 ++++--- .../transform_into_action/v1/mod.rs | 2 +- .../mod.rs | 5 +- .../v3.rs | 6 +- .../rs-platform-version/src/version/v15.rs | 21 +- 22 files changed, 681 insertions(+), 476 deletions(-) create mode 100644 packages/rs-dpp/src/util/ecdsa/verify_hash_signature/mod.rs create mode 100644 packages/rs-dpp/src/util/ecdsa/verify_hash_signature/v0/mod.rs create mode 100644 packages/rs-dpp/src/util/ecdsa/verify_hash_signature/v1/mod.rs diff --git a/packages/rs-dpp/src/address_funds/platform_address.rs b/packages/rs-dpp/src/address_funds/platform_address.rs index a4a238dcd85..e1cb3540a7d 100644 --- a/packages/rs-dpp/src/address_funds/platform_address.rs +++ b/packages/rs-dpp/src/address_funds/platform_address.rs @@ -473,7 +473,7 @@ impl PlatformAddress { /// * `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 is refused. + /// version 15 a high-S signature and a header naming an uncompressed key are refused. /// /// # Returns /// * `Ok(AddressWitnessVerificationOperations)` - Operations performed if verification succeeds diff --git a/packages/rs-dpp/src/state_transition/mod.rs b/packages/rs-dpp/src/state_transition/mod.rs index 3bd11b04039..6763b30ca3d 100644 --- a/packages/rs-dpp/src/state_transition/mod.rs +++ b/packages/rs-dpp/src/state_transition/mod.rs @@ -3430,15 +3430,16 @@ mod tests { } } - // An ECDSA_HASH160 key is checked by recovering the public key, which takes any S. From - // protocol version 15 (`verify_hash_signature` 1) the high-S twin of a valid signature is - // refused; protocol version 14 keeps accepting it for replay. Masternode votes, signed by an - // ECDSA_HASH160 VOTING key, take the same path. + // 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_a_high_s_ecdsa_hash160_signature_from_protocol_version_15() { + 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; + use crate::tests::utils::{high_s_twin, uncompressed_header_twin}; use secp256k1::{PublicKey as SecpPublicKey, SecretKey}; let bls = crate::bls::native_bls::NativeBlsModule; @@ -3474,10 +3475,7 @@ mod tests { signed .sign_by_private_key(&private_key, KeyType::ECDSA_HASH160, &bls) .expect("the key signs"); - let mut malleated = signed.clone(); - malleated.set_signature(BinaryData::new(high_s_twin( - signed.signature().expect("signed").as_slice(), - ))); + let signature = signed.signature().expect("signed").to_vec(); for platform_version in [latest, last_without_the_check] { signed @@ -3489,21 +3487,31 @@ mod tests { ) }); } - malleated - .verify_identity_signed_signature(&key, &bls, last_without_the_check) - .unwrap_or_else(|e| { - panic!("{label}: protocol version 14 accepts the high-S twin: {e:?}") - }); - let error = malleated - .verify_identity_signed_signature(&key, &bls, latest) - .expect_err("the high-S twin is refused"); - assert!( - matches!(&error, ProtocolError::ConsensusError(consensus_error) - if matches!(&**consensus_error, ConsensusError::SignatureError( - SignatureError::InvalidStateTransitionSignatureError(_) - ))), - "{label}: {error:?}" - ); + 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:?}" + ); + } } } 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 25d860dcac9..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 @@ -15,7 +15,7 @@ impl StateTransition { /// /// 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. + /// refuses a high-S signature and a header naming an uncompressed key. pub fn verify_identity_signed_signature( &self, public_key: &IdentityPublicKey, 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 94c1b4136ff..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 @@ -26,7 +26,7 @@ impl StateTransition { /// /// 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, from 15. + /// 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, diff --git a/packages/rs-dpp/src/tests/utils/mod.rs b/packages/rs-dpp/src/tests/utils/mod.rs index a4494693362..b7343bdd363 100644 --- a/packages/rs-dpp/src/tests/utils/mod.rs +++ b/packages/rs-dpp/src/tests/utils/mod.rs @@ -300,3 +300,19 @@ pub fn high_s_twin(signature: &[u8]) -> Vec { 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/mod.rs b/packages/rs-dpp/src/util/ecdsa/mod.rs index b783187fdf1..7331f5c5e4b 100644 --- a/packages/rs-dpp/src/util/ecdsa/mod.rs +++ b/packages/rs-dpp/src/util/ecdsa/mod.rs @@ -5,14 +5,16 @@ //! history. //! //! A signature checked by recovering its public key ([`verify_hash_signature`]) is the -//! exception: every released Platform accepts a high-S one there, so the refusal is a rule of -//! protocol version 15, selected by `verify_hash_signature`. +//! 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 crate::ProtocolError; -use anyhow::ensure; use dashcore::crypto::ecdsa::{EcdsaError, EcdsaPublicKey, EcdsaSignature}; use dashcore::{sign_message, signer}; -use platform_version::version::PlatformVersion; + +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. @@ -43,207 +45,3 @@ pub fn verify_digest( } public_key.verify(digest, signature) } - -/// 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 takes any S, so a signature stays valid with S replaced by `n - S` and the recovery -/// id's parity flipped. A third party can derive that twin from a valid signature and change the -/// hash of the transition carrying it. -/// -/// # 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 -/// [`signer::verify_hash_signature`], which accepts a high-S signature (protocol versions 1 to -/// 14); 1 also refuses it. -/// -/// # 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(signer::verify_hash_signature( - data_hash, - signature, - public_key_hash, - )), - 1 => Ok(verify_low_s_hash_signature( - data_hash, - signature, - public_key_hash, - )), - version => Err(ProtocolError::UnknownVersionMismatch { - method: "verify_hash_signature".to_string(), - known_versions: vec![0, 1], - received: version, - }), - } -} - -/// [`signer::verify_hash_signature`], refusing a high-S signature with -/// [`EcdsaError::VerifyFailed`]. The library checks first, so every other input fails exactly as -/// it does in version 0, with the library's own error. -fn verify_low_s_hash_signature( - 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)?.signature()?; - ensure!(signature.is_low_s(), EcdsaError::VerifyFailed); - Ok(()) -} - -#[cfg(test)] -mod tests { - use super::*; - use crate::tests::utils::high_s_twin; - use dashcore::signer::{double_sha, sign_hash}; - use dashcore::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 - } - - 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_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()); - } - - /// A signature recovery already refuses (another key, a wrong length, S zero or not below the - /// curve order) 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", - ), - ]; - 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/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 8331a22e18f..936d0404c35 100644 --- a/packages/rs-dpp/tests/ecdsa_compatibility.rs +++ b/packages/rs-dpp/tests/ecdsa_compatibility.rs @@ -276,12 +276,23 @@ fn should_reject_high_s_verification_but_accept_high_s_recovery() { .unwrap(); } -/// A P2PKH witness is checked by recovering the key, which takes any S. Protocol version 14 -/// accepts the historical signature's high-S twin, for replay; from protocol version 15 -/// (`verify_hash_signature` 1) it is refused as a signature that does not verify. The low-S -/// signature is accepted at both. +/// 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_high_s_p2pkh_witness_from_protocol_version_15() { +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 { @@ -297,24 +308,26 @@ fn should_refuse_a_high_s_p2pkh_witness_from_protocol_version_15() { AddressWitnessVerificationOperations::for_p2pkh(data.len()) ); } - assert_eq!( - address - .verify_bytes_against_witness(&witness(high_s_twin()), &data, last_without_the_check) - .unwrap(), - AddressWitnessVerificationOperations::for_p2pkh(data.len()) - ); - match address - .verify_bytes_against_witness(&witness(high_s_twin()), &data, latest) - .unwrap_err() - { - ProtocolError::AddressWitnessError(message) => assert_eq!( - message, - format!( - "P2PKH signature verification failed: {}", - EcdsaError::VerifyFailed - ) - ), - error => panic!("unexpected error: {error}"), + 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}"), + } } } 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 b0db6ec279e..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 @@ -1201,20 +1201,31 @@ mod tests { } /// The asset lock's one-time key is checked by recovering the public key, which takes - /// any S. Protocol version 14 accepts the high-S twin of the signature, for replay; from - /// protocol version 15 a block refuses it unpaid, with the error of a signature that does - /// not verify, and leaves the asset lock unused. The low-S signature is accepted at both. + /// 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_a_high_s_asset_lock_signature_from_protocol_version_15() { + 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; + use dpp::tests::utils::{high_s_twin, uncompressed_header_twin}; let latest = PlatformVersion::latest().protocol_version; - // (protocol version, whether the twin is submitted). The asset lock is consumed by - // the first transition a block accepts, so the low-S signature at 14 gets its own - // platform. - for (protocol_version, submit_twin) in [(14, false), (14, true), (latest, true)] { + 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() @@ -1244,41 +1255,39 @@ mod tests { vec![AddressFundsFeeStrategyStep::ReduceOutput(0)], ) .await; - let mut malleated = signed.clone(); - malleated.set_signature( - high_s_twin(signed.signature().expect("signed").as_slice()).into(), - ); - - if !submit_twin { - let result = process_transition_and_commit(&platform, signed, platform_version); - assert_matches!( - result.execution_results().as_slice(), - [StateTransitionExecutionResult::SuccessfulExecution { .. }], - "the low-S signature is accepted" - ); - continue; + 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}" + ); + } } - 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 high-S twin" - ); - } else { - assert_matches!( - result.execution_results().as_slice(), - [StateTransitionExecutionResult::UnpaidConsensusError( - ConsensusError::SignatureError(SignatureError::BasicECDSAError(_)) - )], - "protocol version {protocol_version} refuses the high-S twin" - ); - // The asset lock is left unused, so the signature the key made still funds. + 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 { .. }], - "the low-S signature is accepted" + "protocol version {protocol_version} accepts the signature" ); } } 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 fe10c2f0866..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 @@ -177,7 +177,7 @@ impl AddressFundingFromAssetLockStateTransitionTransformIntoActionValidationV0 // `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. + // 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(), 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 8dba793b023..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 @@ -9258,20 +9258,21 @@ mod tests { } // ========================================== - // HIGH-S P2PKH WITNESS + // P2PKH WITNESS TWINS // ========================================== - /// A P2PKH witness is checked by recovering the public key, which takes any S. A third party - /// can replace a witness's signature with its high-S twin and change the transition's hash. - /// Protocol version 14 accepts the twin, for replay; from protocol version 15 a block refuses - /// it unpaid, with the error of a witness that does not verify. The low-S witness is accepted - /// at both. - mod high_s_witness { + /// 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; + use dpp::tests::utils::{high_s_twin, uncompressed_header_twin}; #[tokio::test] - async fn should_refuse_a_high_s_p2pkh_witness_from_protocol_version_15() { + 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(), @@ -9301,20 +9302,31 @@ mod tests { .await .expect("should create signed transition"); - let mut malleated = signed.clone(); - let StateTransition::AddressFundsTransfer(AddressFundsTransferTransition::V0(v0)) = - &mut malleated + let StateTransition::AddressFundsTransfer(AddressFundsTransferTransition::V0( + signed_v0, + )) = &signed else { panic!("expected an address funds transfer"); }; - let AddressWitness::P2pkh { signature } = &v0.input_witnesses[0] else { + let AddressWitness::P2pkh { signature } = &signed_v0.input_witnesses[0] else { panic!("expected a P2PKH witness"); }; - v0.input_witnesses[0] = AddressWitness::P2pkh { - signature: BinaryData::new(high_s_twin(signature.as_slice())), + 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 both spend + // 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(); @@ -9337,25 +9349,33 @@ mod tests { assert_matches!( process(&signed).as_slice(), [StateTransitionExecutionResult::SuccessfulExecution { .. }], - "protocol version {protocol_version} accepts the low-S witness" + "protocol version {protocol_version} accepts the witness the key made" ); - let malleated_results = process(&malleated); - if protocol_version == 14 { - assert_matches!( - malleated_results.as_slice(), - [StateTransitionExecutionResult::SuccessfulExecution { .. }], - "protocol version 14 accepts the high-S twin" - ); - } else { - assert_matches!( - malleated_results.as_slice(), - [StateTransitionExecutionResult::UnpaidConsensusError( - ConsensusError::SignatureError( - SignatureError::InvalidStateTransitionSignatureError(_) - ) - )], - "protocol version {protocol_version} refuses the high-S twin" - ); + 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 4b907a431a5..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,21 +508,32 @@ 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. - /// A third party can replace an identity create's signature with its high-S twin and change - /// the transition's hash. Protocol version 14 accepts the twin, for replay; from protocol - /// version 15 a block refuses it unpaid, with the error of a signature that does not verify, - /// and leaves the asset lock unused. The low-S signature is accepted at both. + /// 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_a_high_s_asset_lock_signature_from_protocol_version_15() { + 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; + use dpp::tests::utils::{high_s_twin, uncompressed_header_twin}; let latest = PlatformVersion::latest().protocol_version; - // (protocol version, whether the twin is submitted). The asset lock is consumed by the - // first transition a block accepts, so the low-S signature at 14 gets its own platform. - for (protocol_version, submit_twin) in [(14, false), (14, true), (latest, true)] { + 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() @@ -578,9 +589,7 @@ mod tests { ) .await .expect("expected an identity create transition"); - let mut malleated = signed.clone(); - malleated - .set_signature(high_s_twin(signed.signature().expect("signed").as_slice()).into()); + let signature = signed.signature().expect("signed").to_vec(); let process = |transition: &StateTransition| { let transaction = platform.drive.grove.start_transaction(); @@ -605,38 +614,37 @@ mod tests { result }; - if !submit_twin { - let result = process(&signed); - assert_eq!( - result.valid_count(), - 1, - "the low-S signature is accepted: {:?}", - result.execution_results() - ); - continue; + 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}" + ); + } } - let result = process(&malleated); - if protocol_version == 14 { - assert_eq!( - result.valid_count(), - 1, - "protocol version 14 accepts the high-S twin: {:?}", - result.execution_results() - ); - } else { - assert_matches!( - result.execution_results().as_slice(), - [StateTransitionExecutionResult::UnpaidConsensusError( - ConsensusError::SignatureError(SignatureError::BasicECDSAError(_)) - )], - "protocol version {protocol_version} refuses the high-S twin" - ); - // The asset lock is left unused, so the signature the key made still creates. + 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, - "the low-S signature is accepted: {:?}", + "protocol version {protocol_version} accepts the signature: {:?}", result.execution_results() ); } 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 af832005160..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 @@ -236,7 +236,7 @@ impl IdentityCreateStateTransitionStateValidationV0 for IdentityCreateTransition // `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. + // 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(), 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 d0d418fabb9..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,23 +416,34 @@ 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. - /// A third party can replace a top-up's signature with its high-S twin and change the - /// transition's hash. Protocol version 14 accepts the twin, for replay; from protocol version - /// 15 a block refuses it unpaid, with the error of a signature that does not verify, and - /// leaves the asset lock unused. The low-S signature the key made is accepted at both. + /// 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_a_high_s_asset_lock_signature_from_protocol_version_15() { + 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; + use dpp::tests::utils::{high_s_twin, uncompressed_header_twin}; let latest = PlatformVersion::latest().protocol_version; - // (protocol version, whether the twin is submitted). The asset lock is consumed by the - // first transition a block accepts, so the low-S signature at 14 gets its own platform. - for (protocol_version, submit_twin) in [(14, false), (14, true), (latest, true)] { + 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() @@ -491,9 +502,7 @@ mod tests { None, ) .expect("expected an identity top up transition"); - let mut malleated = signed.clone(); - malleated - .set_signature(high_s_twin(signed.signature().expect("signed").as_slice()).into()); + let signature = signed.signature().expect("signed").to_vec(); let process = |transition: &StateTransition| { let transaction = platform.drive.grove.start_transaction(); @@ -518,39 +527,38 @@ mod tests { result }; - if !submit_twin { - let signed_result = process(&signed); - assert_eq!( - signed_result.valid_count(), - 1, - "the low-S signature is accepted: {:?}", - signed_result.execution_results() - ); - continue; + 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}" + ); + } } - let twin_result = process(&malleated); - if protocol_version == 14 { - assert_eq!( - twin_result.valid_count(), - 1, - "protocol version 14 accepts the high-S twin: {:?}", - twin_result.execution_results() - ); - } else { - assert_matches!( - twin_result.execution_results().as_slice(), - [StateTransitionExecutionResult::UnpaidConsensusError( - ConsensusError::SignatureError(SignatureError::BasicECDSAError(_)) - )], - "protocol version {protocol_version} refuses the high-S twin" - ); - // The asset lock is left unused, so the signature the key made still tops up. - let signed_result = process(&signed); + 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!( - signed_result.valid_count(), + result.valid_count(), 1, - "the low-S signature is accepted: {:?}", - signed_result.execution_results() + "protocol version {protocol_version} accepts the signature: {:?}", + result.execution_results() ); } } 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 778729a5ecf..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 @@ -168,7 +168,7 @@ impl IdentityTopUpStateTransitionStateValidationV0 for IdentityTopUpTransition { // `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. + // 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(), 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 f774d4633a6..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 @@ -574,9 +574,10 @@ mod tests { 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, + setup_platform_at_protocol_version, OutputsOnlyBundle, }; - use dpp::tests::utils::high_s_twin; + use dpp::tests::utils::{high_s_twin, uncompressed_header_twin}; + use std::collections::BTreeMap; #[test] fn test_wrong_ecdsa_signature_returns_error() { @@ -649,20 +650,34 @@ mod tests { } /// The asset lock's one-time key is checked by recovering the public key, which takes - /// any S. A third party can replace a shield's signature with its high-S twin and change - /// the transition's hash. Protocol version 14 accepts the twin, for replay; from protocol - /// version 15 a block refuses it unpaid, with the error of a signature that does not - /// verify, and leaves the asset lock unused. The low-S signature is accepted at both. + /// 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_a_high_s_asset_lock_signature_from_protocol_version_15() { + fn should_refuse_an_asset_lock_signature_twin_from_protocol_version_15() { let latest = PlatformVersion::latest().protocol_version; - // (protocol version, whether the twin is submitted). The asset lock is consumed by - // the first transition a block accepts, so the low-S signature at 14 gets its own - // platform. - for (protocol_version, submit_twin) in [(14, false), (14, true), (latest, true)] { + 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); @@ -672,7 +687,10 @@ mod tests { 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 = build_outputs_only_bundle_bound(5_000, &extra_sighash_data); + 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, @@ -682,41 +700,39 @@ mod tests { bundle.proof, bundle.binding_signature, ); - let mut malleated = signed.clone(); - malleated.set_signature( - high_s_twin(signed.signature().expect("signed").as_slice()).into(), - ); - - if !submit_twin { - let result = process_transition_and_commit(&platform, signed, platform_version); - assert_matches!( - result.execution_results().as_slice(), - [StateTransitionExecutionResult::SuccessfulExecution { .. }], - "the low-S signature is accepted" - ); - continue; + 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}" + ); + } } - 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 high-S twin" - ); - } else { - assert_matches!( - result.execution_results().as_slice(), - [StateTransitionExecutionResult::UnpaidConsensusError( - ConsensusError::SignatureError(SignatureError::BasicECDSAError(_)) - )], - "protocol version {protocol_version} refuses the high-S twin" - ); - // The asset lock is left unused, so the signature the key made still shields. + 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 { .. }], - "the low-S signature is accepted" + "protocol version {protocol_version} accepts the signature" ); } } 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 bf62a47f7ca..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 @@ -227,7 +227,7 @@ impl ShieldFromAssetLockStateTransitionTransformIntoActionValidationV1 // `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. + // 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(), 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 97a2535bf25..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 @@ -13,8 +13,9 @@ pub struct DPPStateTransitionMethodVersions { /// `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, which any third party - /// can derive from a valid one, 1 refuses it. + /// 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, } 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 index 4f595b1e4f8..88f6c2a47c1 100644 --- 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 @@ -3,9 +3,9 @@ use crate::version::dpp_versions::dpp_state_transition_method_versions::DPPState /// 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, 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. +/// 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, diff --git a/packages/rs-platform-version/src/version/v15.rs b/packages/rs-platform-version/src/version/v15.rs index 85e7b7ffc39..18386c9e9ec 100644 --- a/packages/rs-platform-version/src/version/v15.rs +++ b/packages/rs-platform-version/src/version/v15.rs @@ -879,18 +879,21 @@ pub const PROTOCOL_VERSION_15: ProtocolVersion = 15; /// (`IdentityInsufficientBalanceError`). The fee schedules of protocol v14 and earlier /// carry 0, and the contract does not exist there, so nothing is charged. /// -/// 41. **Low-S signatures where the key is recovered**: an ECDSA signature checked by +/// 41. **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, 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 with the error each path gives a -/// signature that does not verify, unpaid as before. Signing produces low-S signatures, -/// 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. +/// 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: add_indices_for_top_index_level_for_contract_operations 3 writes no entry in an expired time range bucket (note 36) @@ -907,7 +910,7 @@ pub const PLATFORM_V15: PlatformVersion = PlatformVersion { validation: DPP_VALIDATION_VERSIONS_V5, state_transition_serialization_versions: STATE_TRANSITION_SERIALIZATION_VERSIONS_V3, state_transition_conversion_versions: STATE_TRANSITION_CONVERSION_VERSIONS_V2, - state_transition_method_versions: STATE_TRANSITION_METHOD_VERSIONS_V3, // changed: verify_hash_signature 1 refuses a high-S signature checked by key recovery (note 41) + 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 41) 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) document_versions: DOCUMENT_VERSIONS_V4,