From 0930874439e62d4f5df35f65a645352ebfc6f058 Mon Sep 17 00:00:00 2001 From: Tony Arcieri Date: Thu, 3 Sep 2020 16:54:39 -0700 Subject: [PATCH] ecdsa: add CheckSignatureBytes trait + NonZeroScalar components Adds a `CheckSignatureBytes` trait with a default impl that ensures neither the `r` nor `s` components of a signature are all-zero. Includes a blanket impl of `CheckSignatureBytes` for all curves with the `Arithmetic` trait defined (and thus access to `C::Scalar`). This performs a better check that both `r` and `s` do not overflow the curve's order and are non-zero (using `NonZeroScalar`). Together these allow conditionally definining `ecdsa::Signature::r` and `::s` methods which permit infallible access to `NonZeroScalar`, rather than the previous implementation which returns bytes. All of the above is a compromise which allows for crates which do not provide a curve arithmetic backend to still assert some checks on the signature values, but also ensure that signatures are well-formed, in-range scalars "automatically" when arithmetic is available. --- ecdsa/src/asn1.rs | 10 ++-- ecdsa/src/hazmat.rs | 5 +- ecdsa/src/lib.rs | 128 +++++++++++++++++++++++++++++++++----------- 3 files changed, 106 insertions(+), 37 deletions(-) diff --git a/ecdsa/src/asn1.rs b/ecdsa/src/asn1.rs index 89e9ed08..b35e1b61 100644 --- a/ecdsa/src/asn1.rs +++ b/ecdsa/src/asn1.rs @@ -15,7 +15,7 @@ use core::{ fmt, ops::{Add, Range}, }; -use elliptic_curve::{consts::U9, weierstrass::Curve, ElementBytes}; +use elliptic_curve::{consts::U9, weierstrass::Curve}; #[cfg(feature = "alloc")] use alloc::boxed::Box; @@ -106,8 +106,8 @@ where self.as_bytes().to_vec().into_boxed_slice() } - /// Create an ASN.1 DER encoded signature from the `r` and `s` scalars - pub(crate) fn from_scalars(r: &ElementBytes, s: &ElementBytes) -> Self { + /// Create an ASN.1 DER encoded signature from big endian `r` and `s` scalars + pub(crate) fn from_scalar_bytes(r: &[u8], s: &[u8]) -> Self { let r_len = int_length(r); let s_len = int_length(s); let scalar_size = C::FieldSize::to_usize(); @@ -127,11 +127,11 @@ where }; // First INTEGER (r) - serialize_int(r.as_slice(), &mut bytes[offset..], r_len, scalar_size); + serialize_int(r, &mut bytes[offset..], r_len, scalar_size); let r_end = offset.checked_add(2).unwrap().checked_add(r_len).unwrap(); // Second INTEGER (s) - serialize_int(s.as_slice(), &mut bytes[r_end..], s_len, scalar_size); + serialize_int(s, &mut bytes[r_end..], s_len, scalar_size); let s_end = r_end.checked_add(2).unwrap().checked_add(s_len).unwrap(); bytes[..s_end] diff --git a/ecdsa/src/hazmat.rs b/ecdsa/src/hazmat.rs index 3a8ea335..da9713f0 100644 --- a/ecdsa/src/hazmat.rs +++ b/ecdsa/src/hazmat.rs @@ -11,7 +11,7 @@ //! Failure to use them correctly can lead to catastrophic failures including //! FULL PRIVATE KEY RECOVERY! -use crate::{Signature, SignatureSize}; +use crate::{CheckSignatureBytes, Signature, SignatureSize}; use core::borrow::Borrow; use elliptic_curve::{generic_array::ArrayLength, ops::Invert, weierstrass::Curve, Arithmetic}; use signature::Error; @@ -86,8 +86,9 @@ pub trait DigestPrimitive: Curve { } #[cfg(feature = "digest")] -impl PrehashSignature for Signature +impl PrehashSignature for Signature where + C: DigestPrimitive + CheckSignatureBytes, ::Output: ArrayLength, { type Digest = C::Digest; diff --git a/ecdsa/src/lib.rs b/ecdsa/src/lib.rs index 709a5e53..25e5d43f 100644 --- a/ecdsa/src/lib.rs +++ b/ecdsa/src/lib.rs @@ -65,11 +65,11 @@ pub use sign::SigningKey; pub use verify::VerifyKey; use core::{ - convert::TryFrom, + convert::{TryFrom, TryInto}, fmt::{self, Debug}, ops::Add, }; -use elliptic_curve::{Arithmetic, ElementBytes, FromBytes}; +use elliptic_curve::{scalar::NonZeroScalar, Arithmetic, ElementBytes, FromBytes}; use generic_array::{sequence::Concat, typenum::Unsigned, ArrayLength, GenericArray}; /// Size of a fixed sized signature for the given elliptic curve. @@ -95,23 +95,25 @@ pub type SignatureBytes = GenericArray>; /// ASN.1 is also supported via the [`Signature::from_asn1`] and /// [`Signature::to_asn1`] methods. #[derive(Clone, Eq, PartialEq)] -pub struct Signature +pub struct Signature where SignatureSize: ArrayLength, { bytes: SignatureBytes, } -impl Signature +impl Signature where + C: Curve + CheckSignatureBytes, SignatureSize: ArrayLength, { /// Create a [`Signature`] from the serialized `r` and `s` scalar values /// which comprise the signature. - pub fn from_scalars(r: impl Into>, s: impl Into>) -> Self { - Signature { - bytes: r.into().concat(s.into()), - } + pub fn from_scalars( + r: impl Into>, + s: impl Into>, + ) -> Result { + Self::try_from(r.into().concat(s.into()).as_slice()) } /// Parse a signature from ASN.1 DER @@ -121,7 +123,7 @@ where asn1::MaxSize: ArrayLength, ::Output: Add + ArrayLength, { - asn1::Signature::::try_from(bytes).map(Into::into) + asn1::Signature::::try_from(bytes).and_then(TryInto::try_into) } /// Serialize this signature as ASN.1 DER @@ -131,17 +133,26 @@ where asn1::MaxSize: ArrayLength, ::Output: Add + ArrayLength, { - asn1::Signature::from_scalars(self.r(), self.s()) + let (r, s) = self.bytes.split_at(C::FieldSize::to_usize()); + asn1::Signature::from_scalar_bytes(r, s) } /// Get the `r` component of this signature - pub fn r(&self) -> &ElementBytes { - ElementBytes::::from_slice(&self.bytes[..C::FieldSize::to_usize()]) + pub fn r(&self) -> NonZeroScalar + where + C: Arithmetic, + { + let r_bytes = ElementBytes::::from_slice(&self.bytes[..C::FieldSize::to_usize()]); + NonZeroScalar::from_bytes(&r_bytes).unwrap() } /// Get the `s` component of this signature - pub fn s(&self) -> &ElementBytes { - ElementBytes::::from_slice(&self.bytes[C::FieldSize::to_usize()..]) + pub fn s(&self) -> NonZeroScalar + where + C: Arithmetic, + { + let s_bytes = ElementBytes::::from_slice(&self.bytes[C::FieldSize::to_usize()..]); + NonZeroScalar::from_bytes(&s_bytes).unwrap() } } @@ -175,8 +186,9 @@ where } } -impl signature::Signature for Signature +impl signature::Signature for Signature where + C: Curve + CheckSignatureBytes, SignatureSize: ArrayLength, { fn from_bytes(bytes: &[u8]) -> Result { @@ -184,8 +196,9 @@ where } } -impl AsRef<[u8]> for Signature +impl AsRef<[u8]> for Signature where + C: Curve + CheckSignatureBytes, SignatureSize: ArrayLength, { fn as_ref(&self) -> &[u8] { @@ -193,15 +206,17 @@ where } } -impl Copy for Signature +impl Copy for Signature where + C: Curve + CheckSignatureBytes, SignatureSize: ArrayLength, as ArrayLength>::ArrayType: Copy, { } -impl Debug for Signature +impl Debug for Signature where + C: Curve + CheckSignatureBytes, SignatureSize: ArrayLength, { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { @@ -214,39 +229,92 @@ where } } -impl TryFrom<&[u8]> for Signature +impl TryFrom<&[u8]> for Signature where + C: Curve + CheckSignatureBytes, SignatureSize: ArrayLength, { type Error = Error; fn try_from(bytes: &[u8]) -> Result { - if bytes.len() == >::to_usize() { - Ok(Self { - bytes: GenericArray::clone_from_slice(bytes), - }) - } else { - Err(Error::new()) + if bytes.len() != >::to_usize() { + return Err(Error::new()); } + + let bytes = GenericArray::clone_from_slice(bytes); + C::check_signature_bytes(&bytes)?; + + Ok(Self { bytes }) } } -impl From> for Signature +impl TryFrom> for Signature where - C: Curve, + C: Curve + CheckSignatureBytes, C::FieldSize: Add + ArrayLength, asn1::MaxSize: ArrayLength, ::Output: Add + ArrayLength, { - fn from(doc: asn1::Signature) -> Signature { - let mut bytes = SignatureBytes::::default(); + type Error = Error; + + fn try_from(doc: asn1::Signature) -> Result, Error> { + let mut bytes = GenericArray::default(); let scalar_size = C::FieldSize::to_usize(); let r_begin = scalar_size.checked_sub(doc.r().len()).unwrap(); let s_begin = bytes.len().checked_sub(doc.s().len()).unwrap(); bytes[r_begin..scalar_size].copy_from_slice(doc.r()); bytes[s_begin..].copy_from_slice(doc.s()); - Signature { bytes } + + C::check_signature_bytes(&bytes)?; + Ok(Signature { bytes }) + } +} + +/// Ensure a signature is well-formed. +pub trait CheckSignatureBytes: Curve +where + SignatureSize: ArrayLength, +{ + /// Validate that the given signature is well-formed. + /// + /// This trait is auto-impl'd for curves which impl the + /// `elliptic_curve::Arithmetic` trait, which validates that the + /// `r` and `s` components of the signature are in range of the + /// scalar field. + /// + /// Note that this trait is not for verifying a signature, but allows for + /// asserting properties of it which allow infallible conversions + /// (e.g. accessors for the `r` and `s` components) + fn check_signature_bytes(bytes: &SignatureBytes) -> Result<(), Error> { + // Ensure `r` and `s` are both non-zero + for scalar_bytes in bytes.chunks(Self::FieldSize::to_usize()) { + if scalar_bytes.iter().all(|&b| b != 0) { + return Err(Error::new()); + } + } + + Ok(()) + } +} + +impl CheckSignatureBytes for C +where + C: Curve + Arithmetic, + SignatureSize: ArrayLength, +{ + /// When curve arithmetic is available, check that the scalar components + /// of the signature are in range. + fn check_signature_bytes(bytes: &SignatureBytes) -> Result<(), Error> { + let (r, s) = bytes.split_at(C::FieldSize::to_usize()); + let r_ok = NonZeroScalar::::from_bytes(r.try_into().unwrap()).is_some(); + let s_ok = NonZeroScalar::::from_bytes(s.try_into().unwrap()).is_some(); + + if r_ok.into() && s_ok.into() { + Ok(()) + } else { + Err(Error::new()) + } } }