From 9fb5edb39e55e576a54d9804ddae2fa8009f582a Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Thu, 22 Aug 2024 18:14:41 +1000 Subject: [PATCH] ecdsa: Improve error types There are a couple of issues around the ECDSA signature decoding / parsing code. We have duplicate code in `from_str` and `from_slice` and both use the same error type even though it is impossible to get a hex error in `from_slice`. Create two errors: - A `DecodeError` returned by `from_slice` - A `ParseSignatureError` that has a decode variant and a hex variant Call through to `from_slice` after parsing hex into a byte vector. Removes an instance of `unreachable!`. Fix: #1193 --- bitcoin/src/crypto/ecdsa.rs | 78 ++++++++++++++++++++++++----------- bitcoin/src/psbt/error.rs | 2 +- bitcoin/src/psbt/serialize.rs | 7 ++-- 3 files changed, 59 insertions(+), 28 deletions(-) diff --git a/bitcoin/src/crypto/ecdsa.rs b/bitcoin/src/crypto/ecdsa.rs index cd5e8e110..9fa2d734c 100644 --- a/bitcoin/src/crypto/ecdsa.rs +++ b/bitcoin/src/crypto/ecdsa.rs @@ -36,10 +36,11 @@ impl Signature { } /// Deserializes from slice following the standardness rules for [`EcdsaSighashType`]. - pub fn from_slice(sl: &[u8]) -> Result { - let (sighash_type, sig) = sl.split_last().ok_or(Error::EmptySignature)?; + pub fn from_slice(sl: &[u8]) -> Result { + let (sighash_type, sig) = sl.split_last().ok_or(DecodeError::EmptySignature)?; let sighash_type = EcdsaSighashType::from_standard(*sighash_type as u32)?; - let signature = secp256k1::ecdsa::Signature::from_der(sig).map_err(Error::Secp256k1)?; + let signature = + secp256k1::ecdsa::Signature::from_der(sig).map_err(DecodeError::Secp256k1)?; Ok(Signature { signature, sighash_type }) } @@ -87,15 +88,11 @@ impl fmt::Display for Signature { } impl FromStr for Signature { - type Err = Error; + type Err = ParseSignatureError; fn from_str(s: &str) -> Result { let bytes = Vec::from_hex(s)?; - let (sighash_byte, signature) = bytes.split_last().ok_or(Error::EmptySignature)?; - Ok(Signature { - signature: secp256k1::ecdsa::Signature::from_der(signature)?, - sighash_type: EcdsaSighashType::from_standard(*sighash_byte as u32)?, - }) + Ok(Self::from_slice(&bytes)?) } } @@ -204,12 +201,10 @@ impl<'a> IntoIterator for &'a SerializedSignature { fn into_iter(self) -> Self::IntoIter { (*self).iter() } } -/// An ECDSA signature-related error. +/// Error encountered while parsing an ECDSA signature from a byte slice. #[derive(Debug, Clone, PartialEq, Eq)] #[non_exhaustive] -pub enum Error { - /// Hex decoding error. - Hex(hex::HexToBytesError), +pub enum DecodeError { /// Non-standard sighash type. SighashType(NonStandardSighashTypeError), /// Signature was empty. @@ -218,14 +213,13 @@ pub enum Error { Secp256k1(secp256k1::Error), } -internals::impl_from_infallible!(Error); +internals::impl_from_infallible!(DecodeError); -impl fmt::Display for Error { +impl fmt::Display for DecodeError { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - use Error::*; + use DecodeError::*; match *self { - Hex(ref e) => write_err!(f, "signature hex decoding error"; e), SighashType(ref e) => write_err!(f, "non-standard signature hash type"; e), EmptySignature => write!(f, "empty ECDSA signature"), Secp256k1(ref e) => write_err!(f, "secp256k1"; e), @@ -234,12 +228,11 @@ impl fmt::Display for Error { } #[cfg(feature = "std")] -impl std::error::Error for Error { +impl std::error::Error for DecodeError { fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { - use Error::*; + use DecodeError::*; match *self { - Hex(ref e) => Some(e), Secp256k1(ref e) => Some(e), SighashType(ref e) => Some(e), EmptySignature => None, @@ -247,18 +240,57 @@ impl std::error::Error for Error { } } -impl From for Error { +impl From for DecodeError { fn from(e: secp256k1::Error) -> Self { Self::Secp256k1(e) } } -impl From for Error { +impl From for DecodeError { fn from(e: NonStandardSighashTypeError) -> Self { Self::SighashType(e) } } -impl From for Error { +/// Error encountered while parsing an ECDSA signature from a string. +#[derive(Debug, Clone, PartialEq, Eq)] +#[non_exhaustive] +pub enum ParseSignatureError { + /// Hex string decoding error. + Hex(hex::HexToBytesError), + /// Signature byte slice decoding error. + Decode(DecodeError), +} + +internals::impl_from_infallible!(ParseSignatureError); + +impl fmt::Display for ParseSignatureError { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + use ParseSignatureError::*; + + match *self { + Hex(ref e) => write_err!(f, "signature hex decoding error"; e), + Decode(ref e) => write_err!(f, "signature byte slice decoding error"; e), + } + } +} + +#[cfg(feature = "std")] +impl std::error::Error for ParseSignatureError { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + use ParseSignatureError::*; + + match *self { + Hex(ref e) => Some(e), + Decode(ref e) => Some(e), + } + } +} + +impl From for ParseSignatureError { fn from(e: hex::HexToBytesError) -> Self { Self::Hex(e) } } +impl From for ParseSignatureError { + fn from(e: DecodeError) -> Self { Self::Decode(e) } +} + #[cfg(test)] mod tests { use super::*; diff --git a/bitcoin/src/psbt/error.rs b/bitcoin/src/psbt/error.rs index 142b2396e..9a2fdf5f4 100644 --- a/bitcoin/src/psbt/error.rs +++ b/bitcoin/src/psbt/error.rs @@ -82,7 +82,7 @@ pub enum Error { /// Parsing error indicating invalid xonly public keys InvalidXOnlyPublicKey, /// Parsing error indicating invalid ECDSA signatures - InvalidEcdsaSignature(crate::crypto::ecdsa::Error), + InvalidEcdsaSignature(crate::crypto::ecdsa::DecodeError), /// Parsing error indicating invalid Taproot signatures InvalidTaprootSignature(crate::crypto::taproot::SigFromSliceError), /// Parsing error indicating invalid control block diff --git a/bitcoin/src/psbt/serialize.rs b/bitcoin/src/psbt/serialize.rs index 6523e0c29..a836f1e5f 100644 --- a/bitcoin/src/psbt/serialize.rs +++ b/bitcoin/src/psbt/serialize.rs @@ -172,10 +172,9 @@ impl Deserialize for ecdsa::Signature { // 0x05, the sighash message would have the last field as 0x05u32 while, the verification // would use check the signature assuming sighash_u32 as `0x01`. ecdsa::Signature::from_slice(bytes).map_err(|e| match e { - ecdsa::Error::EmptySignature => Error::InvalidEcdsaSignature(e), - ecdsa::Error::SighashType(err) => Error::NonStandardSighashType(err.0), - ecdsa::Error::Secp256k1(..) => Error::InvalidEcdsaSignature(e), - ecdsa::Error::Hex(..) => unreachable!("decoding from slice, not hex"), + ecdsa::DecodeError::EmptySignature => Error::InvalidEcdsaSignature(e), + ecdsa::DecodeError::SighashType(err) => Error::NonStandardSighashType(err.0), + ecdsa::DecodeError::Secp256k1(..) => Error::InvalidEcdsaSignature(e), }) } }