From b74fb967e27e1a94893d76cac1d0514536159a74 Mon Sep 17 00:00:00 2001 From: Romain Ruetschi Date: Wed, 25 Aug 2021 13:40:01 +0200 Subject: [PATCH 1/7] Show underlying detail in some signature errors --- tendermint/src/error.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tendermint/src/error.rs b/tendermint/src/error.rs index 8f32ce97c..c09bf5377 100644 --- a/tendermint/src/error.rs +++ b/tendermint/src/error.rs @@ -45,7 +45,7 @@ define_error! { Protocol { detail: String } - |_| { format_args!("protocol error") }, + |e| { format_args!("protocol error: {}", e.detail) }, OutOfRange [ DisplayOnly ] @@ -53,7 +53,7 @@ define_error! { SignatureInvalid { detail: String } - |_| { format_args!("bad signature") }, + |e| { format_args!("bad signature: {}", e.detail) }, InvalidMessageType |_| { format_args!("invalid message type") }, From 64875698dad9098d29447986876bfa7ccf4a28c4 Mon Sep 17 00:00:00 2001 From: Romain Ruetschi Date: Thu, 26 Aug 2021 12:17:27 +0200 Subject: [PATCH 2/7] Add support for Secp256k1 signatures --- tendermint/Cargo.toml | 2 + tendermint/src/error.rs | 4 ++ tendermint/src/public_key.rs | 16 ++++-- tendermint/src/signature.rs | 97 ++++++++++++++++++++++++++++++++---- 4 files changed, 107 insertions(+), 12 deletions(-) diff --git a/tendermint/Cargo.toml b/tendermint/Cargo.toml index bbb4b5991..2e128d14c 100644 --- a/tendermint/Cargo.toml +++ b/tendermint/Cargo.toml @@ -72,4 +72,6 @@ std = [ [dev-dependencies] pretty_assertions = "0.7.2" proptest = "0.10.1" +rand = "0.8.4" +rand_chacha = "0.3.1" tendermint-pbt-gen = { path = "../pbt-gen" } diff --git a/tendermint/src/error.rs b/tendermint/src/error.rs index c09bf5377..b4769ad5b 100644 --- a/tendermint/src/error.rs +++ b/tendermint/src/error.rs @@ -55,6 +55,10 @@ define_error! { { detail: String } |e| { format_args!("bad signature: {}", e.detail) }, + SignatureMismatch + { detail: String } + |e| { format_args!("signature mismatch: {}", e.detail) }, + InvalidMessageType |_| { format_args!("invalid message type") }, diff --git a/tendermint/src/public_key.rs b/tendermint/src/public_key.rs index ab6b2068c..fa057aa8e 100644 --- a/tendermint/src/public_key.rs +++ b/tendermint/src/public_key.rs @@ -134,12 +134,22 @@ impl PublicKey { Signature::Ed25519(sig) => pk.verify(msg, sig).map_err(|_| { Error::signature_invalid("Ed25519 signature verification failed".to_string()) }), + #[cfg(feature = "secp256k1")] + Signature::Secp256k1(_) => Err(Error::signature_mismatch( + "Secp256k1 signature incompatible with Ed25519 public key".to_string(), + )), Signature::None => Err(Error::signature_invalid("missing signature".to_string())), }, #[cfg(feature = "secp256k1")] - PublicKey::Secp256k1(_) => Err(Error::invalid_key( - "unsupported signature algorithm (ECDSA/secp256k1)".to_string(), - )), + PublicKey::Secp256k1(pk) => match signature { + Signature::Secp256k1(sig) => pk.verify(msg, sig).map_err(|_| { + Error::signature_invalid("Secp256k1 signature verification failed".to_string()) + }), + Signature::Ed25519(_) => Err(Error::signature_invalid( + "Ed25519 signature incompatible with Secp256k1 public key".to_string(), + )), + Signature::None => Err(Error::signature_invalid("missing signature".to_string())), + }, } } diff --git a/tendermint/src/signature.rs b/tendermint/src/signature.rs index 90f4a752b..b191da15e 100644 --- a/tendermint/src/signature.rs +++ b/tendermint/src/signature.rs @@ -4,7 +4,7 @@ pub use ed25519::{Signature as Ed25519Signature, SIGNATURE_LENGTH as ED25519_SIG pub use signature::{Signer, Verifier}; #[cfg(feature = "secp256k1")] -pub use k256::ecdsa::Signature as Secp256k1; +pub use k256::ecdsa::Signature as Secp256k1Signature; use crate::error::Error; use std::convert::TryFrom; @@ -16,6 +16,12 @@ use tendermint_proto::Protobuf; pub enum Signature { /// Ed25519 block signature Ed25519(Ed25519Signature), + + /// Secp256k1 signature + #[cfg(feature = "secp256k1")] + #[cfg_attr(docsrs, doc(cfg(feature = "secp256k1")))] + Secp256k1(Secp256k1Signature), + /// No signature present None, /* This could have been implemented as an `Option<>` but then handling it would be * outside the scope of this enum. */ @@ -27,15 +33,26 @@ impl TryFrom> for Signature { type Error = Error; fn try_from(value: Vec) -> Result { + Self::try_from(value.as_ref()) + } +} + +impl TryFrom<&'_ [u8]> for Signature { + type Error = Error; + + fn try_from(value: &'_ [u8]) -> Result { if value.is_empty() { - return Ok(Self::default()); - } - if value.len() != ED25519_SIGNATURE_SIZE { - return Err(Error::invalid_signature_id_length()); + return Ok(Self::None); } - let mut slice: [u8; ED25519_SIGNATURE_SIZE] = [0; ED25519_SIGNATURE_SIZE]; - slice.copy_from_slice(&value[..]); - Ok(Signature::Ed25519(Ed25519Signature::new(slice))) + + let sig = Self::from_raw_ed25519(value); + + #[cfg(feature = "secp256k1")] + let sig = sig.or_else(|_| Self::from_raw_secp256k1(value)); + + sig.map_err(|_| { + Error::signature_invalid("malformed Ed25519 or Secp256k1 signature".to_string()) + }) } } @@ -52,10 +69,34 @@ impl Default for Signature { } impl Signature { + /// Parse an Ed25519 signature from raw bytes + fn from_raw_ed25519(value: &[u8]) -> Result { + if value.len() != ED25519_SIGNATURE_SIZE { + return Err(Error::invalid_signature_id_length()); + } + + let mut slice: [u8; ED25519_SIGNATURE_SIZE] = [0; ED25519_SIGNATURE_SIZE]; + slice.copy_from_slice(value); + + Ok(Self::Ed25519(Ed25519Signature::new(slice))) + } + + /// Parse a Secp256k1 signature from raw bytes + #[cfg(feature = "secp256k1")] + #[cfg_attr(docsrs, doc(cfg(feature = "secp256k1")))] + fn from_raw_secp256k1(value: &[u8]) -> Result { + let sig = Secp256k1Signature::try_from(value) + .map_err(|_| Error::signature_invalid("malformed Secp256k1 signature".to_string()))?; + + Ok(Self::Secp256k1(sig)) + } + /// Return the algorithm used to create this particular signature pub fn algorithm(&self) -> Algorithm { match self { Signature::Ed25519(_) => Algorithm::Ed25519, + #[cfg(feature = "secp256k1")] + Signature::Secp256k1(_) => Algorithm::EcdsaSecp256k1, Signature::None => Algorithm::Ed25519, /* It doesn't matter what algorithm an empty * signature has. */ } @@ -65,7 +106,17 @@ impl Signature { pub fn ed25519(self) -> Option { match self { Signature::Ed25519(sig) => Some(sig), - Signature::None => None, + _ => None, + } + } + + /// Get Secp256k1 signature + #[cfg(feature = "secp256k1")] + #[cfg_attr(docsrs, doc(cfg(feature = "secp256k1")))] + pub fn secp256k1(self) -> Option { + match self { + Signature::Secp256k1(sig) => Some(sig), + _ => None, } } @@ -84,6 +135,8 @@ impl AsRef<[u8]> for Signature { fn as_ref(&self) -> &[u8] { match self { Signature::Ed25519(sig) => sig.as_ref(), + #[cfg(feature = "secp256k1")] + Signature::Secp256k1(sig) => sig.as_ref(), Signature::None => &[], } } @@ -95,6 +148,13 @@ impl From for Signature { } } +#[cfg(feature = "secp256k1")] +impl From for Signature { + fn from(pk: Secp256k1Signature) -> Signature { + Signature::Secp256k1(pk) + } +} + /// Digital signature algorithms #[derive(Copy, Clone, Debug, Eq, PartialEq)] pub enum Algorithm { @@ -104,3 +164,22 @@ pub enum Algorithm { /// EdDSA over Curve25519 Ed25519, } + +#[cfg(test)] +mod tests { + + #[test] + #[cfg(feature = "secp256k1")] + fn parse_secp256k1() { + use super::*; + use rand::SeedableRng; + + let rng = rand_chacha::ChaCha8Rng::seed_from_u64(42); + let signing_key = k256::ecdsa::SigningKey::random(rng); + let sig: Secp256k1Signature = signing_key.sign(&[1, 2, 3, 4, 5]); + let sig_bytes = sig.as_ref(); + let sig = Signature::from_raw_secp256k1(sig_bytes).unwrap(); + dbg!(&sig); + assert!(matches!(sig, Signature::Secp256k1(_))); + } +} From 989cfedd97d750f0250d14f84a8ad3edc005c4d7 Mon Sep 17 00:00:00 2001 From: Romain Ruetschi Date: Fri, 27 Aug 2021 14:39:52 +0200 Subject: [PATCH 3/7] Represent Signature as a wrapper over a byte array --- light-client/src/operations/voting_power.rs | 13 +- light-client/src/predicates/errors.rs | 7 +- tendermint/src/block/commit_sig.rs | 23 ++- tendermint/src/error.rs | 3 + tendermint/src/proposal.rs | 18 ++- tendermint/src/public_key.rs | 46 +++--- tendermint/src/signature.rs | 153 +++----------------- tendermint/src/vote.rs | 45 ++++-- tendermint/src/vote/sign_vote.rs | 18 +-- testgen/src/commit.rs | 4 +- testgen/src/helpers.rs | 16 +- testgen/src/vote.rs | 15 +- 12 files changed, 141 insertions(+), 220 deletions(-) diff --git a/light-client/src/operations/voting_power.rs b/light-client/src/operations/voting_power.rs index adf854dee..bb7b19b2b 100644 --- a/light-client/src/operations/voting_power.rs +++ b/light-client/src/operations/voting_power.rs @@ -138,12 +138,9 @@ impl VotingPowerCalculator for ProdVotingPowerCalculator { None => continue, // Cannot find matching validator, so we skip the vote }; - let signed_vote = SignedVote::new( - vote.clone(), - signed_header.header.chain_id.clone(), - vote.validator_address, - vote.signature, - ); + let signed_vote = + SignedVote::from_vote(vote.clone(), signed_header.header.chain_id.clone()) + .ok_or_else(VerificationError::missing_signature)?; // Check vote is valid let sign_bytes = signed_vote.sign_bytes(); @@ -152,7 +149,7 @@ impl VotingPowerCalculator for ProdVotingPowerCalculator { .is_err() { return Err(VerificationError::invalid_signature( - signed_vote.signature().to_bytes(), + signed_vote.signature().as_bytes().to_vec(), Box::new(validator), sign_bytes, )); @@ -212,7 +209,7 @@ fn non_absent_vote( timestamp: Some(timestamp), validator_address, validator_index, - signature: *signature, + signature: signature.clone(), }) } diff --git a/light-client/src/predicates/errors.rs b/light-client/src/predicates/errors.rs index 9ab9112fa..57afb44c9 100644 --- a/light-client/src/predicates/errors.rs +++ b/light-client/src/predicates/errors.rs @@ -58,6 +58,11 @@ define_error! { e.address) }, + MissingSignature + | _ | { + format_args!("missing signature") + }, + InvalidSignature { signature: Vec, @@ -65,7 +70,7 @@ define_error! { sign_bytes: Vec, } | e | { - format_args!("Couldn't verify signature `{:?}` with validator `{:?}` on sign_bytes `{:?}`", + format_args!("failed to verify signature `{:?}` with validator `{:?}` on sign_bytes `{:?}`", e.signature, e.validator, e.sign_bytes) }, diff --git a/tendermint/src/block/commit_sig.rs b/tendermint/src/block/commit_sig.rs index e5b2a35c2..24592c7f8 100644 --- a/tendermint/src/block/commit_sig.rs +++ b/tendermint/src/block/commit_sig.rs @@ -20,7 +20,7 @@ pub enum CommitSig { /// Timestamp of vote timestamp: Time, /// Signature of vote - signature: Signature, + signature: Option, }, /// voted for nil. BlockIdFlagNil { @@ -29,7 +29,7 @@ pub enum CommitSig { /// Timestamp of vote timestamp: Time, /// Signature of vote - signature: Signature, + signature: Option, }, } @@ -79,26 +79,33 @@ impl TryFrom for CommitSig { )); } } + if !value.signature.is_empty() { - return Err(Error::invalid_signature("empty signature".to_string())); + return Err(Error::invalid_signature( + "expected empty signature for absent commitsig".to_string(), + )); } + return Ok(CommitSig::BlockIdFlagAbsent); } + if value.block_id_flag == BlockIdFlag::Commit.to_i32().unwrap() { if value.signature.is_empty() { return Err(Error::invalid_signature( - "regular commitsig has no signature".to_string(), + "expected non-empty signature for regular commitsig".to_string(), )); } + if value.validator_address.is_empty() { return Err(Error::invalid_validator_address()); } + let timestamp = value.timestamp.ok_or_else(Error::missing_timestamp)?.into(); return Ok(CommitSig::BlockIdFlagCommit { validator_address: value.validator_address.try_into()?, timestamp, - signature: value.signature.try_into()?, + signature: Signature::new(value.signature), }); } if value.block_id_flag == BlockIdFlag::Nil.to_i32().unwrap() { @@ -113,7 +120,7 @@ impl TryFrom for CommitSig { return Ok(CommitSig::BlockIdFlagNil { validator_address: value.validator_address.try_into()?, timestamp: value.timestamp.ok_or_else(Error::missing_timestamp)?.into(), - signature: value.signature.try_into()?, + signature: Signature::new(value.signature), }); } Err(Error::block_id_flag()) @@ -137,7 +144,7 @@ impl From for RawCommitSig { block_id_flag: BlockIdFlag::Nil.to_i32().unwrap(), validator_address: validator_address.into(), timestamp: Some(timestamp.into()), - signature: signature.into(), + signature: signature.map(|s| s.to_bytes()).unwrap_or_default(), }, CommitSig::BlockIdFlagCommit { validator_address, @@ -147,7 +154,7 @@ impl From for RawCommitSig { block_id_flag: BlockIdFlag::Commit.to_i32().unwrap(), validator_address: validator_address.into(), timestamp: Some(timestamp.into()), - signature: signature.into(), + signature: signature.map(|s| s.to_bytes()).unwrap_or_default(), }, } } diff --git a/tendermint/src/error.rs b/tendermint/src/error.rs index b4769ad5b..ebb734801 100644 --- a/tendermint/src/error.rs +++ b/tendermint/src/error.rs @@ -51,6 +51,9 @@ define_error! { [ DisplayOnly ] |_| { format_args!("value out of range") }, + EmptySignature + |_| { format_args!("empty signature") }, + SignatureInvalid { detail: String } |e| { format_args!("bad signature: {}", e.detail) }, diff --git a/tendermint/src/proposal.rs b/tendermint/src/proposal.rs index f2c508f74..807866043 100644 --- a/tendermint/src/proposal.rs +++ b/tendermint/src/proposal.rs @@ -35,7 +35,7 @@ pub struct Proposal { /// Timestamp pub timestamp: Option