diff --git a/cmd/soroban-cli/src/signer/ledger.rs b/cmd/soroban-cli/src/signer/ledger.rs index 957716968..a07d3b1b6 100644 --- a/cmd/soroban-cli/src/signer/ledger.rs +++ b/cmd/soroban-cli/src/signer/ledger.rs @@ -16,6 +16,9 @@ pub enum Error { #[error(transparent)] Xdr(#[from] xdr::Error), + + #[error(transparent)] + Validation(#[from] crate::signer::validation::Error), } #[cfg(feature = "additional-libs")] @@ -47,13 +50,17 @@ mod ledger_impl { Some(pk) => pk, None => live.public_key().await?, }; + let sig_bytes = live + .signer + .sign_transaction_hash(live.index, &tx_hash) + .await?; + // Only the cached-pubkey path can drift; the cacheless path fetched the + // key live from the same device that just signed. + if self.public_key.is_some() { + crate::signer::validation::verify_signature(&key, &tx_hash, &sig_bytes)?; + } let hint = SignatureHint(key.0[28..].try_into()?); - let signature = Signature( - live.signer - .sign_transaction_hash(live.index, &tx_hash) - .await? - .try_into()?, - ); + let signature = Signature(sig_bytes.try_into()?); Ok(DecoratedSignature { hint, signature }) } @@ -63,6 +70,9 @@ mod ledger_impl { .signer .sign_transaction_hash(live.index, &payload) .await?; + if let Some(pk) = self.public_key { + crate::signer::validation::verify_signature(&pk, &payload, &bytes)?; + } Ok(Ed25519Signature::from_bytes(bytes.as_slice().try_into()?)) } } diff --git a/cmd/soroban-cli/src/signer/mod.rs b/cmd/soroban-cli/src/signer/mod.rs index 6b922c110..76f2e0db9 100644 --- a/cmd/soroban-cli/src/signer/mod.rs +++ b/cmd/soroban-cli/src/signer/mod.rs @@ -59,6 +59,8 @@ pub enum Error { Ledger(#[from] ledger::Error), #[error(transparent)] Decode(#[from] stellar_strkey::DecodeError), + #[error(transparent)] + Validation(#[from] validation::Error), } /// Sign all SorobanAuthorizationEntry's in the transaction with the given signers. Returns a new @@ -472,12 +474,19 @@ impl SecureStoreEntry { let signed_tx_hash = secure_store::sign_tx_data(&self.name, self.hd_path, &tx_hash)?; + if let Some(pk) = self.public_key { + validation::verify_signature(&pk, &tx_hash, &signed_tx_hash)?; + } + let signature = Signature(signed_tx_hash.clone().try_into()?); Ok(DecoratedSignature { hint, signature }) } pub fn sign_payload(&self, payload: [u8; 32]) -> Result { let signed_bytes = secure_store::sign_tx_data(&self.name, self.hd_path, &payload)?; + if let Some(pk) = self.public_key { + validation::verify_signature(&pk, &payload, &signed_bytes)?; + } let sig = Ed25519Signature::from_bytes(signed_bytes.as_slice().try_into()?); Ok(sig) } diff --git a/cmd/soroban-cli/src/signer/validation.rs b/cmd/soroban-cli/src/signer/validation.rs index 5a458a77d..3b0430ad7 100644 --- a/cmd/soroban-cli/src/signer/validation.rs +++ b/cmd/soroban-cli/src/signer/validation.rs @@ -1,5 +1,45 @@ use crate::xdr::{HostFunction, SorobanAuthorizedFunction, SorobanAuthorizedInvocation}; +#[derive(thiserror::Error, Debug)] +pub enum Error { + #[error( + "the produced signature does not match the cached public key {public_key}; \ +the cached key may be stale — check the correct device is connected / the alias's \ +hd-path is right, or re-add the key" + )] + SignatureMismatch { public_key: String }, + + #[error("the cached public key {public_key} is not a valid ed25519 public key")] + InvalidPublicKey { public_key: String }, + + #[error("the signer returned a malformed ed25519 signature ({len} bytes, expected 64)")] + InvalidSignature { len: usize }, +} + +/// Verify that `signature` over `message` was produced by the secret key +/// corresponding to `public_key`. Used as a post-sign drift guard for +/// cached-pubkey signers (Ledger / Secure Store): the signature is produced on a +/// live device/keychain while the hint and embedded public key are derived from a +/// cached value, so a stale cache yields a transaction the network silently +/// rejects. This catches that locally with a clear error. +pub fn verify_signature( + public_key: &stellar_strkey::ed25519::PublicKey, + message: &[u8; 32], + signature: &[u8], +) -> Result<(), Error> { + use ed25519_dalek::{Signature, VerifyingKey}; + let vk = VerifyingKey::from_bytes(&public_key.0).map_err(|_| Error::InvalidPublicKey { + public_key: format!("{public_key}"), + })?; + let sig = Signature::from_slice(signature).map_err(|_| Error::InvalidSignature { + len: signature.len(), + })?; + vk.verify_strict(message, &sig) + .map_err(|_| Error::SignatureMismatch { + public_key: format!("{public_key}"), + }) +} + /// Classification of an `Address`-credential auth entry's relationship to the /// transaction's host function. /// @@ -125,6 +165,46 @@ mod tests { } } + #[test] + fn test_verify_signature_roundtrip() { + use ed25519_dalek::{ed25519::signature::Signer as _, SigningKey}; + + let signing_key = SigningKey::from_bytes(&[7u8; 32]); + let public_key = + ed25519::PublicKey::from_payload(signing_key.verifying_key().as_bytes()).unwrap(); + let message = [42u8; 32]; + let signature = signing_key.sign(&message).to_bytes(); + + // Matching key + untampered signature verifies. + verify_signature(&public_key, &message, &signature).unwrap(); + + // A different public key is rejected. + let cached_pk = ed25519::PublicKey::from_payload( + SigningKey::from_bytes(&[9u8; 32]) + .verifying_key() + .as_bytes(), + ) + .unwrap(); + assert!(matches!( + verify_signature(&cached_pk, &message, &signature), + Err(Error::SignatureMismatch { .. }) + )); + + // A tampered signature is rejected. + let mut tampered = signature; + tampered[0] ^= 0xff; + assert!(matches!( + verify_signature(&public_key, &message, &tampered), + Err(Error::SignatureMismatch { .. }) + )); + + // A wrong-length signature is reported as malformed, not as a mismatch. + assert!(matches!( + verify_signature(&public_key, &message, &signature[..63]), + Err(Error::InvalidSignature { len: 63 }) + )); + } + #[test] fn test_matching_root_invocation_is_strict() { let contract = [1u8; 32];