From 7f3aedd6ca1911b0fd720cea73e9d84f81e2e220 Mon Sep 17 00:00:00 2001 From: Tony Arcieri Date: Wed, 21 Apr 2021 11:30:29 -0700 Subject: [PATCH] ecdsa: replace CheckSignatureBytes with Order (#281) Uses a generic implementation for checking that the scalars in a signature are in range which doesn't require a curve arithmetic backend. --- Cargo.lock | 4 +-- ecdsa/Cargo.toml | 2 +- ecdsa/src/hazmat.rs | 7 ++-- ecdsa/src/lib.rs | 88 ++++++++++++--------------------------------- ecdsa/tests/lib.rs | 15 ++++++++ 5 files changed, 42 insertions(+), 74 deletions(-) create mode 100644 ecdsa/tests/lib.rs diff --git a/Cargo.lock b/Cargo.lock index 5ce4dd8..d452bad 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -152,9 +152,9 @@ dependencies = [ [[package]] name = "elliptic-curve" -version = "0.9.8" +version = "0.9.9" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "c1a9176ec191bd6a85fdb3018336d04e8a98e3ca8ae56951d9846b0da94f3dfb" +checksum = "409dd3e6ac38c26a53efe0e4eba2708fdc8d0a3a6469e035a9f2d2f6f0244a0b" dependencies = [ "bitvec", "ff", diff --git a/ecdsa/Cargo.toml b/ecdsa/Cargo.toml index 08134b1..09a35de 100644 --- a/ecdsa/Cargo.toml +++ b/ecdsa/Cargo.toml @@ -15,7 +15,7 @@ keywords = ["crypto", "ecc", "nist", "secp256k1", "signature"] [dependencies] der = { version = "0.3", optional = true, features = ["big-uint"] } -elliptic-curve = { version = "0.9.8", default-features = false } +elliptic-curve = { version = "0.9.9", default-features = false } hmac = { version = "0.10", optional = true, default-features = false } signature = { version = ">= 1.3.0, < 1.4.0", default-features = false, features = ["rand-preview"] } diff --git a/ecdsa/src/hazmat.rs b/ecdsa/src/hazmat.rs index 2218e9a..e40e455 100644 --- a/ecdsa/src/hazmat.rs +++ b/ecdsa/src/hazmat.rs @@ -22,10 +22,7 @@ use { }; #[cfg(feature = "digest")] -use crate::{ - signature::{digest::Digest, PrehashSignature}, - CheckSignatureBytes, -}; +use crate::signature::{digest::Digest, PrehashSignature}; #[cfg(any(feature = "arithmetic", feature = "digest"))] use crate::{ @@ -165,7 +162,7 @@ pub trait FromDigest { #[cfg(feature = "digest")] impl PrehashSignature for Signature where - C: DigestPrimitive + CheckSignatureBytes, + C: DigestPrimitive, ::Output: ArrayLength, { type Digest = C::Digest; diff --git a/ecdsa/src/lib.rs b/ecdsa/src/lib.rs index 72ba296..7653fd3 100644 --- a/ecdsa/src/lib.rs +++ b/ecdsa/src/lib.rs @@ -126,7 +126,7 @@ pub type SignatureBytes = GenericArray>; /// ASN.1 DER-encoded signatures also supported via the /// [`Signature::from_der`] and [`Signature::to_der`] methods. #[derive(Clone, Eq, PartialEq)] -pub struct Signature +pub struct Signature where SignatureSize: ArrayLength, { @@ -135,7 +135,7 @@ where impl Signature where - C: Curve + Order + CheckSignatureBytes, + C: Curve + Order, SignatureSize: ArrayLength, { /// Create a [`Signature`] from the serialized `r` and `s` scalar values @@ -221,7 +221,7 @@ where impl signature::Signature for Signature where - C: Curve + Order + CheckSignatureBytes, + C: Curve + Order, SignatureSize: ArrayLength, { fn from_bytes(bytes: &[u8]) -> Result { @@ -231,7 +231,7 @@ where impl AsRef<[u8]> for Signature where - C: Curve + Order + CheckSignatureBytes, + C: Curve + Order, SignatureSize: ArrayLength, { fn as_ref(&self) -> &[u8] { @@ -241,7 +241,7 @@ where impl Copy for Signature where - C: Curve + Order + CheckSignatureBytes, + C: Curve + Order, SignatureSize: ArrayLength, as ArrayLength>::ArrayType: Copy, { @@ -249,7 +249,7 @@ where impl Debug for Signature where - C: Curve + Order + CheckSignatureBytes, + C: Curve + Order, SignatureSize: ArrayLength, { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { @@ -264,7 +264,7 @@ where impl TryFrom<&[u8]> for Signature where - C: Curve + Order + CheckSignatureBytes, + C: Curve + Order, SignatureSize: ArrayLength, { type Error = Error; @@ -274,10 +274,19 @@ where return Err(Error::new()); } - let bytes = GenericArray::clone_from_slice(bytes); - C::check_signature_bytes(&bytes)?; + for scalar in bytes.chunks_exact(C::FieldSize::to_usize()) { + if scalar.iter().all(|&byte| byte == 0) { + return Err(Error::new()); + } - Ok(Self { bytes }) + if !bool::from(C::is_scalar_repr_in_range(GenericArray::from_slice(scalar))) { + return Err(Error::new()); + } + } + + Ok(Self { + bytes: GenericArray::clone_from_slice(bytes), + }) } } @@ -285,7 +294,7 @@ where #[cfg_attr(docsrs, doc(cfg(feature = "der")))] impl TryFrom> for Signature where - C: Curve + Order + CheckSignatureBytes, + C: Curve + Order, C::FieldSize: Add + ArrayLength + NonZero, der::MaxSize: ArrayLength, ::Output: Add + ArrayLength, @@ -293,67 +302,14 @@ where type Error = Error; fn try_from(doc: der::Signature) -> Result, Error> { - let mut bytes = GenericArray::default(); + let mut bytes = SignatureBytes::::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()); - - 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::ProjectiveArithmetic` 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 - // TODO(tarcieri): check that `r` and `s` are in range of the curve's order - for scalar_bytes in bytes.chunks(Self::FieldSize::to_usize()) { - if scalar_bytes.iter().all(|&b| b == 0) { - return Err(Error::new()); - } - } - - Ok(()) - } -} - -#[cfg(feature = "arithmetic")] -#[cfg_attr(docsrs, doc(cfg(feature = "arithmetic")))] -impl CheckSignatureBytes for C -where - C: Curve + ProjectiveArithmetic, - Scalar: PrimeField>, - 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_repr(GenericArray::clone_from_slice(r)).is_some(); - let s_ok = NonZeroScalar::::from_repr(GenericArray::clone_from_slice(s)).is_some(); - - if r_ok && s_ok { - Ok(()) - } else { - Err(Error::new()) - } + Self::try_from(bytes.as_slice()) } } diff --git a/ecdsa/tests/lib.rs b/ecdsa/tests/lib.rs new file mode 100644 index 0000000..cb84a4c --- /dev/null +++ b/ecdsa/tests/lib.rs @@ -0,0 +1,15 @@ +//! Smoke tests which use `MockCurve` + +#![cfg(feature = "dev")] + +use core::convert::TryFrom; +use elliptic_curve::dev::MockCurve; + +type Signature = ecdsa::Signature; +type SignatureBytes = ecdsa::SignatureBytes; + +#[test] +fn rejects_all_zero_signature() { + let all_zero_bytes = SignatureBytes::default(); + assert!(Signature::try_from(all_zero_bytes.as_ref()).is_err()); +}